linux-hardening - 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: <8a76222e-8da7-4499-981f-64660e377e1c@lunn.ch>
Date: Fri, 29 Aug 2025 21:48:12 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Yibo Dong <dong100@...se.com>
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

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

Why so much resistance to a NOP or firmware version, something which
is not that important? Why do you want to combine getting sync and
getting the capabilities?

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

So why is this called ifinsmod? Why not power save? If you had called
this power save, i would not of questioned what this does, it is
pretty obvious, and other drivers probably have something
similar. Some drivers probably have something like open/close, which
do similar things. Again, i would not of asked. By not following what
other drivers are doing, you just cause problems for everybody.

So please give this a new name. Not just the function, but also the
name of the firmware op and everything else to do with this. The
firmware does not care what the driver calls it, all it sees is a
binary message format, no names.

Please also go through your driver and look at all the other names. Do
they match what other drivers use. If not, you might want to rename
them, in order to get your code merged with a lot less back and forth
with reviewers.

	Andrew