linux-kernel - Re: [PATCH net-next v9 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: <DB12A33105BC0233+20250829021254.GA904254@nic-Precision-5820-Tower>
Date: Fri, 29 Aug 2025 10:12:54 +0800
From: Yibo Dong <dong100@...se.com>
To: Andrew Lunn <andrew@...n.ch>
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, rdunlap@...radead.org,
	vadim.fedorenko@...ux.dev, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH net-next v9 4/5] net: rnpgbe: Add basic mbx_fw support

On Thu, Aug 28, 2025 at 03:09:51PM +0200, Andrew Lunn wrote:
> > +/**
> > + * mucse_mbx_get_capability - Get hw abilities from fw
> > + * @hw: pointer to the HW structure
> > + *
> > + * mucse_mbx_get_capability tries to get capabities from
> > + * hw. Many retrys will do if it is failed.
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + **/
> > +int mucse_mbx_get_capability(struct mucse_hw *hw)
> > +{
> > +	struct hw_abilities ability = {};
> > +	int try_cnt = 3;
> > +	int err;
> > +	/* It is called once in probe, if failed nothing
> > +	 * (register network) todo. Try more times to get driver
> > +	 * and firmware in sync.
> > +	 */
> > +	do {
> > +		err = mucse_fw_get_capability(hw, &ability);
> > +		if (err)
> > +			continue;
> > +		break;
> > +	} while (try_cnt--);
> > +
> > +	if (!err)
> > +		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> > +	return err;
> > +}
> 
> I still think this should be a dedicated function to get the MAC
> driver and firmware in sync, using a NOP or version request to the
> firmware. The name mucse_mbx_get_capability() does not indicate this
> function is special in any way, which is it.
> 

Maybe I should rename it like this?

/**
 * mucse_mbx_sync_fw_by_get_capability - Try to sync driver and fw
 * @hw: pointer to the HW structure
 *
 * mucse_mbx_sync_fw_by_get_capability tries to sync driver and fw
 * by get capabitiy mbx cmd. Many retrys will do if it is failed.
 *
 * Return: 0 on success, negative errno on failure
 **/
int mucse_mbx_sync_fw_by_get_capability(struct mucse_hw *hw)
{
	struct hw_abilities ability = {};
	int try_cnt = 3;
	int err;
	/* It is called once in probe, if failed nothing
	 * (register network) todo. Try more times to get driver
	 * and firmware in sync.
	 */
	do {
		err = mucse_fw_get_capability(hw, &ability);
		if (err)
			continue;
		break;
	} while (try_cnt--);

	if (!err)
		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
	return err;
}

> > +/**
> > + * build_ifinsmod - build req with insmod opcode
> > + * @req: pointer to the cmd req structure
> > + * @is_insmod: true for insmod, false for rmmod
> > + **/
> > +static void build_ifinsmod(struct mbx_fw_cmd_req *req,
> > +			   bool is_insmod)
> > +{
> > +	req->flags = 0;
> > +	req->opcode = cpu_to_le16(DRIVER_INSMOD);
> > +	req->datalen = cpu_to_le16(sizeof(req->ifinsmod) +
> > +				   MBX_REQ_HDR_LEN);
> > +	req->reply_lo = 0;
> > +	req->reply_hi = 0;
> > +#define FIXED_VERSION 0xFFFFFFFF
> > +	req->ifinsmod.version = cpu_to_le32(FIXED_VERSION);
> > +	if (is_insmod)
> > +		req->ifinsmod.status = cpu_to_le32(1);
> > +	else
> > +		req->ifinsmod.status = cpu_to_le32(0);
> > +}
> 
> Why does the firmware care? What does the firmware do when there is no
> kernel driver? How does it behaviour change when the driver loads?
> 

fw reduce working frequency to save power if no driver is probed to this
chip. And fw change frequency to normal after recieve insmod mbx cmd.

Maybe I should add the comment to func "mucse_mbx_ifinsmod"? 
/**
 * mucse_mbx_ifinsmod - Echo driver insmod status to fw
 * @hw: pointer to the HW structure
 * @is_insmod: true for insmod, false for rmmod
 *
 * mucse_mbx_ifinsmod echo driver insmod status to fw. fw changes working
 * frequency to normal after recieve insmod status, and reduce working
 * frequency if no driver is probed.
 *
 * Return: 0 on success, negative errno on failure
 **/
int mucse_mbx_ifinsmod(struct mucse_hw *hw, bool is_insmod)
{


}

> Please try to ensure comment say why you are doing something, not what
> you are doing.
> 
> 
>     Andrew
> 
> ---
> pw-bot: cr
> 

Thanks for your feedback.