linux-kernel - 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: <54BB0DB525AEF5A4+20250824034629.GA2000422@nic-Precision-5820-Tower>
Date: Sun, 24 Aug 2025 11:46:29 +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 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@...se.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?
> 

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.