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.
