Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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@mucse.com>

[...]

> +/**
> + * 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?


looks like this can be simplified to

if(!wait_event_timeout())
   err = -ETIMEDOUT
else
   err = cookie->errcode

> +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

> + **/
> +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

> + *
> + * @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;
> +}