Re: [PATCH v2 03/15] net: rnpgbe: Add basic mbx ops support

[Yibo Dong]

On Mon, Jul 21, 2025 at 05:43:41PM +0200, Andrew Lunn wrote:
> >  #define MAX_VF_NUM (8)
> 
> > +	hw->max_vfs = 7;
> 
> ???

This is mistake, max vfs is 7. 8 is '7 vfs + 1 pf'.

> 
> 
> >  }
> >  
> >  /**
> > @@ -117,6 +119,7 @@ static void rnpgbe_get_invariants_n210(struct mucse_hw *hw)
> >  	/* update hw feature */
> >  	hw->feature_flags |= M_HW_FEATURE_EEE;
> >  	hw->usecstocount = 62;
> > +	hw->max_vfs_noari = 7;
> 
> ???

Bridge with no ari(Alternative Routing - ID Interpretation) function limits
8 function for one ep. This variable is used to limit vf numbers in no-ari
condition.
Of course, those not really used code should be removed in this patch.

> 
> > +int mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> > +		   enum MBX_ID mbx_id)
> > +{
> > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > +
> > +	/* limit read to size of mailbox */
> > +	if (size > mbx->size)
> > +		size = mbx->size;
> > +
> > +	if (!mbx->ops.read)
> > +		return -EIO;
> 
> How would that happen?
> 
> > +
> > +	return mbx->ops.read(hw, msg, size, mbx_id);
> 
> > +int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> > +		    enum MBX_ID mbx_id)
> > +{
> > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > +
> > +	if (size > mbx->size)
> > +		return -EINVAL;
> > +
> > +	if (!mbx->ops.write)
> > +		return -EIO;
> 
> How would either of these two conditions happen.
> 

Those are 'defensive code' which you point before. I should
remove those.

> > +static u16 mucse_mbx_get_req(struct mucse_hw *hw, int reg)
> > +{
> > +	/* force memory barrier */
> > +	mb();
> > +	return ioread32(hw->hw_addr + reg) & GENMASK(15, 0);
> 
> I'm no expert on memory barriers, but what are you trying to achieve
> here? Probably the most used pattern of an mb() is to flush out writes
> to hardware before doing a special write which triggers the hardware
> to do something. That is not what is happening here.
> 

Got it, I will check and fix it.

> > +static void mucse_mbx_inc_pf_req(struct mucse_hw *hw,
> > +				 enum MBX_ID mbx_id)
> > +{
> > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > +	u32 reg, v;
> > +	u16 req;
> > +
> > +	reg = (mbx_id == MBX_FW) ? PF2FW_COUNTER(mbx) :
> > +				   PF2VF_COUNTER(mbx, mbx_id);
> > +	v = mbx_rd32(hw, reg);
> > +	req = (v & GENMASK(15, 0));
> > +	req++;
> > +	v &= GENMASK(31, 16);
> > +	v |= req;
> > +	/* force before write to hw */
> > +	mb();
> > +	mbx_wr32(hw, reg, v);
> > +	/* update stats */
> > +	hw->mbx.stats.msgs_tx++;
> 
> What are you forcing? As i said, i'm no expert on memory barriers, but
> to me, it looks like whoever wrote this code also does not understand
> memory barriers.
> 

Got it, I will check and fix it.

> > +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
> > +{
> > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > +	int try_cnt = 5000, ret;
> > +	u32 reg;
> > +
> > +	reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> > +				   PF2VF_MBOX_CTRL(mbx, mbx_id);
> > +	while (try_cnt-- > 0) {
> > +		/* Take ownership of the buffer */
> > +		mbx_wr32(hw, reg, MBOX_PF_HOLD);
> > +		/* force write back before check */
> > +		wmb();
> > +		if (mbx_rd32(hw, reg) & MBOX_PF_HOLD)
> > +			return 0;
> > +		udelay(100);
> > +	}
> > +	return ret;
> 
> I've not compiled this, but isn't ret uninitialized here? I would also
> expect it to return -ETIMEDOUT?
> 
> 	Andrew
> 

Yes, ret is uninitialized. I will fix this.
Thanks for your feedback.