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:

[...]
> +/**
> + * 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);

it's unclear to me, what part of the code is managing values of cookie
structure? I didn't get the reason why are you putting the address of
cookie structure into request which is then directly passed to the FW.
Is the FW supposed to change values in cookie?

> +
> +	if (!err)
> +		err = -ETIMEDOUT;
> +	else
> +		err = 0;
> +	if (!err && cookie->errcode)
> +		err = cookie->errcode;
> +out:
> +	mutex_unlock(&hw->mbx.lock);
> +	return err;
> +}

[...]

> +struct mbx_fw_cmd_req {
> +	__le16 flags;
> +	__le16 opcode;
> +	__le16 datalen;
> +	__le16 ret_value;
> +	union {
> +		struct {
> +			__le32 cookie_lo;
> +			__le32 cookie_hi;
> +		};
> +
> +		void *cookie;
> +	};
> +	__le32 reply_lo;
> +	__le32 reply_hi;

what do these 2 fields mean? are you going to provide reply's buffer
address directly to FW?

> +	union {
> +		u8 data[32];
> +		struct {
> +			__le32 version;
> +			__le32 status;
> +		} ifinsmod;
> +		struct {
> +			__le32 port_mask;
> +			__le32 pfvf_num;
> +		} get_mac_addr;
> +	};
> +} __packed;
> +
> +struct mbx_fw_cmd_reply {
> +	__le16 flags;
> +	__le16 opcode;
> +	__le16 error_code;
> +	__le16 datalen;
> +	union {
> +		struct {
> +			__le32 cookie_lo;
> +			__le32 cookie_hi;
> +		};
> +		void *cookie;
> +	};

This part looks like the request, apart from datalen and error_code are
swapped in the header. And it actually means that the FW will put back
the address of provided cookie into reply, right? If yes, then it
doesn't look correct at all...

> +	union {
> +		u8 data[40];
> +		struct mac_addr {
> +			__le32 ports;
> +			struct _addr {
> +				/* for macaddr:01:02:03:04:05:06
> +				 * mac-hi=0x01020304 mac-lo=0x05060000
> +				 */
> +				u8 mac[8];
> +			} addrs[4];
> +		} mac_addr;
> +		struct hw_abilities hw_abilities;
> +	};
> +} __packed;