netdev - Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <82E3BE49DB4195F0+20250826013113.GA6582@nic-Precision-5820-Tower>
Date: Tue, 26 Aug 2025 09:31:13 +0800
From: Yibo Dong <dong100@...se.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
	corbet@....net, gur.stavi@...wei.com, maddy@...ux.ibm.com,
	mpe@...erman.id.au, danishanwar@...com, lee@...ger.us,
	gongfan1@...wei.com, lorenzo@...nel.org, geert+renesas@...der.be,
	Parthiban.Veerasooran@...rochip.com, lukas.bulwahn@...hat.com,
	alexanderduyck@...com, richardcochran@...il.com, kees@...nel.org,
	gustavoars@...nel.org, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support

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?

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

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

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