linux-kernel - Re: [PATCH net-next v10 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: <C3FF2A9FD848166E+20250904023851.GB1015062@nic-Precision-5820-Tower>
Date: Thu, 4 Sep 2025 10:38:51 +0800
From: Yibo Dong <dong100@...se.com>
To: Simon Horman <horms@...nel.org>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, 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 v10 3/5] net: rnpgbe: Add basic mbx ops support

On Wed, Sep 03, 2025 at 05:44:31PM +0100, Simon Horman wrote:
> On Wed, Sep 03, 2025 at 10:54:28AM +0800, Dong Yibo wrote:
> > Initialize basic mbx function.
> > 
> > Signed-off-by: Dong Yibo <dong100@...se.com>
> 
> ...
> 
> > +/**
> > + * mucse_mbx_inc_pf_req - Increase req
> > + * @hw: pointer to the HW structure
> > + *
> > + * mucse_mbx_inc_pf_req read pf_req from hw, then write
> > + * new value back after increase
> > + **/
> > +static void mucse_mbx_inc_pf_req(struct mucse_hw *hw)
> > +{
> > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > +	u16 req;
> > +	u32 v;
> > +
> > +	v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
> > +	req = (v & GENMASK_U32(15, 0));
> 
> nit1: Unnecessary parentheses
> nit2: I would have used FIELD_GET here in conjunction with something like.
> 
>       #define MBX_PF2FW_COUNTER_MASK GENMASK_U32(15, 0)
> 

Got it, maybe update it like this?

req = FIELD_GET(GENMASK_U32(15, 0), v);

> > +	req++;
> > +	v &= GENMASK_U32(31, 16);
> > +	v |= req;
> 
>       And using FIELD_PREP is probably more succinct here.
> 
>       Likewise in the following function
> 

Got it, maybe update it like this?

req++;
v = (v & ~GENMASK_U32(15, 0)) | FIELD_PREP(GENMASK_U32(15, 0), req);

> > +	mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
> > +	hw->mbx.stats.msgs_tx++;
> > +}
> > +
> > +/**
> > + * mucse_mbx_inc_pf_ack - Increase ack
> > + * @hw: pointer to the HW structure
> > + *
> > + * mucse_mbx_inc_pf_ack read pf_ack from hw, then write
> > + * new value back after increase
> > + **/
> > +static void mucse_mbx_inc_pf_ack(struct mucse_hw *hw)
> > +{
> > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > +	u16 ack;
> > +	u32 v;
> > +
> > +	v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
> > +	ack = (v >> 16) & GENMASK_U32(15, 0);
> > +	ack++;
> > +	v &= GENMASK_U32(15, 0);
> > +	v |= (ack << 16);
> > +	mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
> > +	hw->mbx.stats.msgs_rx++;
> > +}
> 
> ...
> 

Maybe like this ?
...
v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
ack = FIELD_GET(GENMASK_U32(31, 16), v);
ack++;
v = (v & GENMASK_U32(15, 0)) | FIELD_PREP(GENMASK_U32(31, 16), ack);
mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
...

> > +/**
> > + * mucse_mbx_reset - Reset mbx info, sync info from regs
> > + * @hw: pointer to the HW structure
> > + *
> > + * This function reset all mbx variables to default.
> > + **/
> > +static void mucse_mbx_reset(struct mucse_hw *hw)
> > +{
> > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > +	u32 v;
> > +
> > +	v = mbx_data_rd32(mbx, MBX_FW2PF_COUNTER);
> > +	hw->mbx.fw_req = v & GENMASK_U32(15, 0);
> > +	hw->mbx.fw_ack = (v >> 16) & GENMASK_U32(15, 0);
> 
> I'd use FIELD_GET here too.
> 

Maybe like this?
hw->mbx.fw_req = FIELD_GET(GENMASK_U32(15, 0), v);
hw->mbx.fw_ack = FIELD_GET(GENMASK_U32(31, 16), v);

> > +	mbx_ctrl_wr32(mbx, PF2FW_MBOX_CTRL(mbx), 0);
> > +	mbx_ctrl_wr32(mbx, FW_PF_MBOX_MASK(mbx), GENMASK_U32(31, 16));
> > +}
> 
> ...
> 

Thanks for your feedback.