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

netdev
2022-11-20T10:21:15Z

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.