Re: [PATCH net-next 2/3] net: dsa: mv88e6xxx: make switchdev DB ops generic

[Vivien Didelot]

Andrew Lunn  writes:

>> No. mv88e6xxx_port_db_dump already exists, this is the function called
>> mv88e6xxx_port_db_dump_one multiple time. Here, _one refers to an FID
>> (forwarding database). 88E6352 have 4096 FIDs.
>
> So maybe s/_one/_fid/ ?

If I have to respin this series, sure.

Thanks Andrew,

   Vivien

Re: [PATCH net-next 2/3] net: dsa: mv88e6xxx: make switchdev DB ops generic

[Andrew Lunn]

> No. mv88e6xxx_port_db_dump already exists, this is the function called
> mv88e6xxx_port_db_dump_one multiple time. Here, _one refers to an FID
> (forwarding database). 88E6352 have 4096 FIDs.

So maybe s/_one/_fid/ ?

   Andrew

Re: [PATCH net-next 2/3] net: dsa: mv88e6xxx: make switchdev DB ops generic

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  writes:

> Hi Vivien
>
>> -static int _mv88e6xxx_port_fdb_dump_one(struct mv88e6xxx_chip *chip,
>> -u16 fid, u16 vid, int port,
>> -struct switchdev_obj_port_fdb *fdb,
>> -int (*cb)(struct switchdev_obj *obj))
>> +static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
>> +  u16 fid, u16 vid, int port,
>> +  struct switchdev_obj *obj,
>> +  int (*cb)(struct switchdev_obj *obj))
>>  {
>>  struct mv88e6xxx_atu_entry addr = {
>>  .mac = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
>> @@ -,72 +2219,87 @@ static int _mv88e6xxx_port_fdb_dump_one(struct 
>> mv88e6xxx_chip *chip,
>>  do {
>>  err = _mv88e6xxx_atu_getnext(chip, fid, );
>>  if (err)
>> -break;
>> +return err;
>>  
>>  if (addr.state == GLOBAL_ATU_DATA_STATE_UNUSED)
>>  break;
>>  
>> -if (!addr.trunk && addr.portv_trunkid & BIT(port)) {
>> -bool is_static = addr.state ==
>> -(is_multicast_ether_addr(addr.mac) ?
>> - GLOBAL_ATU_DATA_STATE_MC_STATIC :
>> - GLOBAL_ATU_DATA_STATE_UC_STATIC);
>> +if (addr.trunk || (addr.portv_trunkid & BIT(port)) == 0)
>> +continue;
>> +
>> +if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
>> +struct switchdev_obj_port_fdb *fdb;
>>  
>> +if (!is_unicast_ether_addr(addr.mac))
>> +continue;
>> +
>> +fdb = SWITCHDEV_OBJ_PORT_FDB(obj);
>>  fdb->vid = vid;
>>  ether_addr_copy(fdb->addr, addr.mac);
>> -fdb->ndm_state = is_static ? NUD_NOARP : NUD_REACHABLE;
>> -
>> -err = cb(>obj);
>> -if (err)
>> -break;
>> +if (addr.state == GLOBAL_ATU_DATA_STATE_UC_STATIC)
>> +fdb->ndm_state = NUD_NOARP;
>> +else
>> +fdb->ndm_state = NUD_REACHABLE;
>>  }
>> +
>> +err = cb(obj);
>> +if (err)
>> +return err;
>>  } while (!is_broadcast_ether_addr(addr.mac));
>
> Humm, maybe i'm reading this patch wrong
>
> This function is called mv88e6xxx_port_db_dump_one(). But i don't see
> a way out of the while loop, after dumping one. It seems to dump the
> whole table until it reaches the end marker, which is the MAC
> broadcast address.
>
> Should we rename this function, drop the _one?

No. mv88e6xxx_port_db_dump already exists, this is the function called
mv88e6xxx_port_db_dump_one multiple time. Here, _one refers to an FID
(forwarding database). 88E6352 have 4096 FIDs.

The way to dump a single FID is to run the ATU GetNext operation
starting with ff:ff:ff:ff:ff:ff, until that same address is reached
again. This is what mv88e6xxx_port_db_dump_one does.

mv88e6xxx_port_db_dump first dump the port's assigned FID (i.e. VID 0),
then iterate on active VLANs to get and dump their FIDs.

Thanks,

Vivien

Re: [PATCH net-next 2/3] net: dsa: mv88e6xxx: make switchdev DB ops generic

[Andrew Lunn]

Hi Vivien

> -static int _mv88e6xxx_port_fdb_dump_one(struct mv88e6xxx_chip *chip,
> - u16 fid, u16 vid, int port,
> - struct switchdev_obj_port_fdb *fdb,
> - int (*cb)(struct switchdev_obj *obj))
> +static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
> +   u16 fid, u16 vid, int port,
> +   struct switchdev_obj *obj,
> +   int (*cb)(struct switchdev_obj *obj))
>  {
>   struct mv88e6xxx_atu_entry addr = {
>   .mac = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
> @@ -,72 +2219,87 @@ static int _mv88e6xxx_port_fdb_dump_one(struct 
> mv88e6xxx_chip *chip,
>   do {
>   err = _mv88e6xxx_atu_getnext(chip, fid, );
>   if (err)
> - break;
> + return err;
>  
>   if (addr.state == GLOBAL_ATU_DATA_STATE_UNUSED)
>   break;
>  
> - if (!addr.trunk && addr.portv_trunkid & BIT(port)) {
> - bool is_static = addr.state ==
> - (is_multicast_ether_addr(addr.mac) ?
> -  GLOBAL_ATU_DATA_STATE_MC_STATIC :
> -  GLOBAL_ATU_DATA_STATE_UC_STATIC);
> + if (addr.trunk || (addr.portv_trunkid & BIT(port)) == 0)
> + continue;
> +
> + if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
> + struct switchdev_obj_port_fdb *fdb;
>  
> + if (!is_unicast_ether_addr(addr.mac))
> + continue;
> +
> + fdb = SWITCHDEV_OBJ_PORT_FDB(obj);
>   fdb->vid = vid;
>   ether_addr_copy(fdb->addr, addr.mac);
> - fdb->ndm_state = is_static ? NUD_NOARP : NUD_REACHABLE;
> -
> - err = cb(>obj);
> - if (err)
> - break;
> + if (addr.state == GLOBAL_ATU_DATA_STATE_UC_STATIC)
> + fdb->ndm_state = NUD_NOARP;
> + else
> + fdb->ndm_state = NUD_REACHABLE;
>   }
> +
> + err = cb(obj);
> + if (err)
> + return err;
>   } while (!is_broadcast_ether_addr(addr.mac));

Humm, maybe i'm reading this patch wrong

This function is called mv88e6xxx_port_db_dump_one(). But i don't see
a way out of the while loop, after dumping one. It seems to dump the
whole table until it reaches the end marker, which is the MAC
broadcast address.

Should we rename this function, drop the _one?

   Andrew

[PATCH net-next 2/3] net: dsa: mv88e6xxx: make switchdev DB ops generic

[Vivien Didelot]

The MDB support for the mv88e6xxx driver will be very similar to the FDB
support, since it consists of loading/purging/dumping address to/from
the Address Translation Unit (ATU).

Prepare the support for MDB by making the FDB code accessing the ATU
generic. The FDB operations now provide access to the unicast addresses
while the MDB operations will provide access to the multicast addresses.

Signed-off-by: Vivien Didelot 
---
 drivers/net/dsa/mv88e6xxx/chip.c | 98 ++--
 1 file changed, 55 insertions(+), 43 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 750d01d..93abfff 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2093,9 +2093,9 @@ static int _mv88e6xxx_atu_load(struct mv88e6xxx_chip 
*chip,
return _mv88e6xxx_atu_cmd(chip, entry->fid, GLOBAL_ATU_OP_LOAD_DB);
 }
 
-static int _mv88e6xxx_port_fdb_load(struct mv88e6xxx_chip *chip, int port,
-   const unsigned char *addr, u16 vid,
-   u8 state)
+static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
+   const unsigned char *addr, u16 vid,
+   u8 state)
 {
struct mv88e6xxx_atu_entry entry = { 0 };
struct mv88e6xxx_vtu_stu_entry vlan;
@@ -2134,15 +2134,12 @@ static void mv88e6xxx_port_fdb_add(struct dsa_switch 
*ds, int port,
   const struct switchdev_obj_port_fdb *fdb,
   struct switchdev_trans *trans)
 {
-   int state = is_multicast_ether_addr(fdb->addr) ?
-   GLOBAL_ATU_DATA_STATE_MC_STATIC :
-   GLOBAL_ATU_DATA_STATE_UC_STATIC;
struct mv88e6xxx_chip *chip = ds_to_priv(ds);
 
mutex_lock(>reg_lock);
-   if (_mv88e6xxx_port_fdb_load(chip, port, fdb->addr, fdb->vid, state))
-   netdev_err(ds->ports[port].netdev,
-  "failed to load MAC address\n");
+   if (mv88e6xxx_port_db_load_purge(chip, port, fdb->addr, fdb->vid,
+GLOBAL_ATU_DATA_STATE_UC_STATIC))
+   netdev_err(ds->ports[port].netdev, "failed to load unicast MAC 
address\n");
mutex_unlock(>reg_lock);
 }
 
@@ -2150,14 +2147,14 @@ static int mv88e6xxx_port_fdb_del(struct dsa_switch 
*ds, int port,
  const struct switchdev_obj_port_fdb *fdb)
 {
struct mv88e6xxx_chip *chip = ds_to_priv(ds);
-   int ret;
+   int err;
 
mutex_lock(>reg_lock);
-   ret = _mv88e6xxx_port_fdb_load(chip, port, fdb->addr, fdb->vid,
-  GLOBAL_ATU_DATA_STATE_UNUSED);
+   err = mv88e6xxx_port_db_load_purge(chip, port, fdb->addr, fdb->vid,
+  GLOBAL_ATU_DATA_STATE_UNUSED);
mutex_unlock(>reg_lock);
 
-   return ret;
+   return err;
 }
 
 static int _mv88e6xxx_atu_getnext(struct mv88e6xxx_chip *chip, u16 fid,
@@ -2205,10 +2202,10 @@ static int _mv88e6xxx_atu_getnext(struct mv88e6xxx_chip 
*chip, u16 fid,
return 0;
 }
 
-static int _mv88e6xxx_port_fdb_dump_one(struct mv88e6xxx_chip *chip,
-   u16 fid, u16 vid, int port,
-   struct switchdev_obj_port_fdb *fdb,
-   int (*cb)(struct switchdev_obj *obj))
+static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
+ u16 fid, u16 vid, int port,
+ struct switchdev_obj *obj,
+ int (*cb)(struct switchdev_obj *obj))
 {
struct mv88e6xxx_atu_entry addr = {
.mac = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
@@ -,72 +2219,87 @@ static int _mv88e6xxx_port_fdb_dump_one(struct 
mv88e6xxx_chip *chip,
do {
err = _mv88e6xxx_atu_getnext(chip, fid, );
if (err)
-   break;
+   return err;
 
if (addr.state == GLOBAL_ATU_DATA_STATE_UNUSED)
break;
 
-   if (!addr.trunk && addr.portv_trunkid & BIT(port)) {
-   bool is_static = addr.state ==
-   (is_multicast_ether_addr(addr.mac) ?
-GLOBAL_ATU_DATA_STATE_MC_STATIC :
-GLOBAL_ATU_DATA_STATE_UC_STATIC);
+   if (addr.trunk || (addr.portv_trunkid & BIT(port)) == 0)
+   continue;
+
+   if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
+   struct switchdev_obj_port_fdb *fdb;
 
+   if (!is_unicast_ether_addr(addr.mac))
+   continue;
+
+   fdb =