netdev - 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: <d61dd41c-5700-483f-847a-a92000b8a925@lunn.ch>
Date: Thu, 28 Aug 2025 15:09:51 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Dong Yibo <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

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

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

Please try to ensure comment say why you are doing something, not what
you are doing.


    Andrew

---
pw-bot: cr