netdev - Re: [PATCH v5 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: <f0c9aee0-0e57-429e-8918-d91bf307018e@lunn.ch>
Date: Thu, 21 Aug 2025 05:06:50 +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,
	netdev@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/5] net: rnpgbe: Add basic mbx ops support

On Thu, Aug 21, 2025 at 09:44:11AM +0800, Yibo Dong wrote:
> On Wed, Aug 20, 2025 at 10:23:44PM +0200, Andrew Lunn wrote:
> > > +/**
> > > + * mucse_mbx_get_ack - Read ack from reg
> > > + * @mbx: pointer to the MBX structure
> > > + * @reg: register to read
> > > + *
> > > + * @return: the ack value
> > > + **/
> > > +static u16 mucse_mbx_get_ack(struct mucse_mbx_info *mbx, int reg)
> > > +{
> > > +	return (mbx_data_rd32(mbx, reg) >> 16);
> > > +}
> > 
> > > +static int mucse_check_for_ack_pf(struct mucse_hw *hw)
> > > +{
> > > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > > +	u16 hw_fw_ack;
> > > +
> > > +	hw_fw_ack = mucse_mbx_get_ack(mbx, MBX_FW2PF_COUNTER);
> > 
> > > +int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
> > > +{
> > > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > > +	int size_inwords = size / 4;
> > > +	u32 ctrl_reg;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	ctrl_reg = PF2FW_MBOX_CTRL(mbx);
> > > +	ret = mucse_obtain_mbx_lock_pf(hw);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	for (i = 0; i < size_inwords; i++)
> > > +		mbx_data_wr32(mbx, MBX_FW_PF_SHM_DATA + i * 4, msg[i]);
> > > +
> > > +	/* flush msg and acks as we are overwriting the message buffer */
> > > +	hw->mbx.fw_ack = mucse_mbx_get_ack(mbx, MBX_FW2PF_COUNTER);
> > 
> > It seems like the ACK is always at MBX_FW2PF_COUNTER. So why pass it
> > to mucse_mbx_get_ack()? Please look at your other getters and setters.
> > 
> 
> 'mucse_mbx_get_ack' is always at MBX_FW2PF_COUNTER now, just for pf-fw mbx. 
> But, in the future, there will be pf-vf mbx with different input.
> Should I move 'MBX_FW2PF_COUNTER' to function 'mucse_mbx_get_ack', and
> update the function when I add vf relative code in the future?

Maybe add mucse_mbx_get_pf_ack() so you can later add
mucse_mbx_get_vf_ack()?

The problem is, our crystal ball about what will come next is not very
good. So we review the code we see now, and make comments about it
now. You can add comments explaining why something is the way it is
because in the future it needs to be more generic to handle additional
use cases, etc. Or explain in the commit message.

	Andrew