On Sat, Aug 23, 2025 at 04:02:29PM +0100, Vadim Fedorenko wrote:
> On 22/08/2025 03:34, Dong Yibo wrote:
> > Initialize basic mbx_fw ops, such as get_capability, reset phy
> > and so on.
> >
> > Signed-off-by: Dong Yibo <dong100@xxxxxxxxx>
>
> [...]
>
> > +/**
> > + * mucse_mbx_fw_post_req - Posts a mbx req to firmware and wait reply
> > + * @hw: pointer to the HW structure
> > + * @req: pointer to the cmd req structure
> > + * @cookie: pointer to the req cookie
> > + *
> > + * mucse_mbx_fw_post_req posts a mbx req to firmware and wait for the
> > + * reply. cookie->wait will be set in irq handler.
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +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_pf(hw, (u32 *)req, len);
> > + if (err)
> > + goto out;
> > + /* if write succeeds, we must wait for firmware response or
> > + * timeout to avoid using the already freed cookie->wait
> > + */
> > + 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;
>
> can cookie->errcode be non 0 if FW times out?
>
cookie is alloced by kzalloc, if fw timeout, nochange for it.
So cookie->errcode is 0 if FW times out.
>
> looks like this can be simplified to
>
> if(!wait_event_timeout())
> err = -ETIMEDOUT
> else
> err = cookie->errcode
>
Got it, I will update it.
> > +out:
> > + mutex_unlock(&hw->mbx.lock);
> > + return err;
> > +}
> > +
> > +/**
> > + * build_ifinsmod - build req with insmod opcode
> > + * @req: pointer to the cmd req structure
> > + * @status: true for insmod, false for rmmod
>
> naming is misleading here, I believe.. no strong feeling, but
> is_insmod might be better
>
I see, I will fix it.
> > + **/
> > +static void build_ifinsmod(struct mbx_fw_cmd_req *req,
> > + int status)
> > +{
> > + req->flags = 0;
> > + req->opcode = cpu_to_le16(DRIVER_INSMOD);
> > + req->datalen = cpu_to_le16(sizeof(req->ifinsmod) +
> > + MBX_REQ_HDR_LEN);
> > + req->cookie = NULL;
> > + req->reply_lo = 0;
> > + req->reply_hi = 0;
> > +#define FIXED_VERSION 0xFFFFFFFF
> > + req->ifinsmod.version = cpu_to_le32(FIXED_VERSION);
> > + req->ifinsmod.status = cpu_to_le32(status);
> > +}
> > +
> > +/**
> > + * mucse_mbx_ifinsmod - Echo driver insmod status to hw
> > + * @hw: pointer to the HW structure
> > + * @status: true for insmod, false for rmmod
>
> here as well
>
Got it.
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +int mucse_mbx_ifinsmod(struct mucse_hw *hw, int status)
> > +{
> > + struct mbx_fw_cmd_req req = {};
> > + int len;
> > + int err;
> > +
> > + build_ifinsmod(&req, status);
> > + len = le16_to_cpu(req.datalen);
> > + err = mutex_lock_interruptible(&hw->mbx.lock);
> > + if (err)
> > + return err;
> > +
> > + if (status) {
> > + err = mucse_write_posted_mbx(hw, (u32 *)&req,
> > + len);
> > + } else {
> > + err = mucse_write_mbx_pf(hw, (u32 *)&req,
> > + len);
> > + }
> > +
> > + mutex_unlock(&hw->mbx.lock);
> > + return err;
> > +}
>
Thanks for your feedback.
