linux-kernel - Re: [PATCH net-next v12 3/5] net: rnpgbe: Add basic mbx ops support

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: <5a942120-fe95-4d6a-a2cf-ef832f65343e@lunn.ch>
Date: Tue, 16 Sep 2025 21:33:36 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Jörg Sommer <joerg@...so.de>
Cc: Dong Yibo <dong100@...se.com>, 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 v12 3/5] net: rnpgbe: Add basic mbx ops support

> > +	if (fw_req != 0 && fw_req != hw->mbx.fw_req) {
> > +		hw->mbx.stats.reqs++;
> > +		return 0;
> > +	}
> > +
> > +	return -EIO;
> 
> Only a suggestion: Might it be clearer to flip the cases and handle the if
> as error case and continue with the success case?
> 
> if (fw_req == 0 || fw_req == hw->mbx.fw_req)
> 	return -EIO;
> 
> hw->mbx.stats.reqs++;
> return 0;

That would by the usual pattern in the kernel. It is good to follow
usual patterns.

> > +static void mucse_mbx_inc_pf_req(struct mucse_hw *hw)
> > +{
> > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > +	u16 req;
> > +	u32 val;
> > +
> > +	val = mbx_data_rd32(mbx, MUCSE_MBX_PF2FW_CNT);
> > +	req = FIELD_GET(GENMASK_U32(15, 0), val);
> 
> Why not assign the values in the declaration like done with mbx?

Reverse Christmas tree.

	struct mucse_mbx_info *mbx = &hw->mbx;
	u32 val = mbx_data_rd32(mbx, MUCSE_MBX_PF2FW_CNT);
	u16 req = FIELD_GET(GENMASK_U32(15, 0), val);

This is not reverse christmas tree. Sometimes you need to put
assignments into the body of the function.

	Andrew