Re: [PATCH v5 4/5] net: rnpgbe: Add basic mbx_fw support
From: Yibo Dong <dong100@mucse.com>
Date: 2025-08-21 02:50:22
Also in:
linux-doc, lkml
On Wed, Aug 20, 2025 at 10:37:01PM +0200, Andrew Lunn wrote:
quoted
+static int mucse_mbx_fw_post_req(struct mucse_hw *hw, + struct mbx_fw_cmd_req *req, + struct mbx_req_cookie *cookie) +{ + int len = le16_to_cpu(req->datalen); + int err; + + cookie->errcode = 0; + cookie->done = 0; + init_waitqueue_head(&cookie->wait); + err = mutex_lock_interruptible(&hw->mbx.lock); + if (err) + return err; + err = mucse_write_mbx(hw, (u32 *)req, len); + if (err) + goto out; + err = wait_event_timeout(cookie->wait, + cookie->done == 1, + cookie->timeout_jiffies); + + if (!err) + err = -ETIMEDOUT; + else + err = 0; + if (!err && cookie->errcode) + err = cookie->errcode; +out: + mutex_unlock(&hw->mbx.lock); + return err;What is your design with respect to mutex_lock_interruptible() and then calling wait_event_timeout() which will ignore signals? Is your intention that you can always ^C the driver, and it will clean up whatever it was doing and return -EINTR? Such unwinding can be tricky and needs careful review. Before i do that, i just want to make sure this is your intention, and you yourself have carefully reviewed the code. Andrew
'mucse_mbx_fw_post_req' is designed can be called by 'cat /sys/xxx', So I used xx_interruptible() before. The design sequence is: write_mbx with cookie ------> fw ----> dirver_irq_handler(call wake_up) | | V V wait_event_xxxx -------------------> free(cookie) But if ^C just after 'wait_event_interruptible_timeout', cookie will be free before fw really response, a crash will happen. cookie pointer is in mbx.req, and fw response with no change. write_mbx with cookie ------> fw ---------> dirver_irq_handler(call wake_up) | | V V wait_event_xxxx ---> free(cookie) crash with freed cookie | v ^C So I use goto retry if -ERESTARTSYS with wait_event_interruptible_timeout. And it is the same with wait_event_timeout. If ^C in mutex_lock_interruptible, it is safe return since no write to fw and no response from fw.