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

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

On 26/08/2025 02:31, Yibo Dong wrote:
> On Mon, Aug 25, 2025 at 05:37:27PM +0100, Vadim Fedorenko wrote:
>> 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?
>>
> 
> cookie will be used in an irq-handler. like this:
> static int rnpgbe_mbx_fw_reply_handler(struct mucse *mucse,
>                                         struct mbx_fw_cmd_reply *reply)
> {
>          struct mbx_req_cookie *cookie;
> 
>          cookie = reply->cookie;
> 
>          if (cookie->priv_len > 0)
>                  memcpy(cookie->priv, reply->data, cookie->priv_len);
>          cookie->done = 1;
>          if (le16_to_cpu(reply->flags) & FLAGS_ERR)
>                  cookie->errcode = -EIO;
>          else
>                  cookie->errcode = 0;
>          wake_up(&cookie->wait);
>          return 0;
> }
> That is why we must wait for firmware response.
> But irq is not added in this patch series. Maybe I should move all
> cookie relative codes to the patch will add irq?

well, yes, in general it's better to introduce the code as a solid
solution. this way it's much easier to review

> 
>>> +
>>> +	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?
>>
> 
> No, this is defined by fw. Some fw can access physical address.
> But I don't use it in this driver.

FW can access physical address without previously configuring IOMMU?
How can that be?

> 
>>> +	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...
>>
> 
> It is yes. cookie is used in irq handler as show above.
> Sorry, I didn't understand 'the not correct' point?

The example above showed that the irq handler uses some value received
from the device as a pointer to kernel memory. That's not safe, you
cannot be sure that provided value is valid pointer, and that it points
to previously allocated cookie structure. It is a clear way to corrupt
memory.

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