[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <D81C71402E58DF29+20250722064530.GC99399@nic-Precision-5820-Tower>
Date: Tue, 22 Jul 2025 14:45:30 +0800
From: Yibo Dong <dong100@...se.com>
To: Andrew Lunn <andrew@...n.ch>
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 v2 03/15] net: rnpgbe: Add basic mbx ops support
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.
Powered by blists - more mailing lists