Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[netdev]

On 2022-11-15 23:23, Vladimir Oltean wrote:
> > diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c 
> > b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > index 8a874b6fc8e1..0a57f4e7dd46 100644
> > --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > @@ -12,6 +12,7 @@
> >
> >  #include "chip.h"
> >  #include "global1.h"
> > +#include "switchdev.h"
> >
> >  /* Offset 0x01: ATU FID Register */
> >
> > @@ -426,6 +427,8 @@ static irqreturn_t 
> > mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
> >  	if (err)
> >  		goto out;
> >
> > +	mv88e6xxx_reg_unlock(chip);
> > +
>
> I concur with Ido's suggestion to split up changes which are only
> tangentially related as preparatory patches, with the motivation which
> you explained over email as the commit message. Also, the current "out"
> label needs to become something like "out_unlock", and a new "out"
> created, for the error path jumps below, that don't have the register
> lock held.
>
> >  	spid = entry.state;
> >
> >  	if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) {
> > @@ -446,6 +449,12 @@ static irqreturn_t 
> > mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
> >  				    "ATU miss violation for %pM portvec %x spid %d\n",
> >  				    entry.mac, entry.portvec, spid);
> >  		chip->ports[spid].atu_miss_violation++;
> > +
> > +		if (fid && chip->ports[spid].mab)
> > +			err = mv88e6xxx_handle_violation(chip, spid, &entry,
> > +							 fid, MV88E6XXX_G1_ATU_OP_MISS_VIOLATION);
>
> The check for non-zero FID looks strange until one considers that FID 0
> is MV88E6XXX_FID_STANDALONE. But then again, since only standalone 
> ports
> use FID 0 and standalone ports cannot have the MAB/locked feature 
> enabled,
> I consider the check to be redundant. We should know for sure that the
> FID is non-zero.

I have something like this, using 'mvls vtu' from 
https://github.com/wkz/mdio-tools:
 VID   FID  SID  P  Q  F  0  1  2  3  4  5  6  7  8  9  a
   0     0    0  y  -  -  =  =  =  =  =  =  =  =  =  =  =
   1     2    0  -  -  -  u  u  u  u  u  u  u  u  u  u  =
4095     1    0  -  -  -  =  =  =  =  =  =  =  =  =  =  =

as a vtu table. I don't remember exactly the consequences, but I am 
quite sure that fid=0 gave
incorrect handling, but there might be something that I have missed as 
to other setups.