[PATCH net-next] net: dsa: mv88e6xxx: handle multiple ports in ATU

[PATCH net-next] net: dsa: mv88e6xxx: handle multiple ports in ATU

[Vivien Didelot]

An address can be loaded in the ATU with multiple ports, for instance
when adding multiple ports to a Multicast group with "bridge mdb".

The current code doesn't allow that. Add an helper to get a single entry
from the ATU, then set or clear the requested port, before loading the
entry back in the ATU.

Note that the required _mv88e6xxx_atu_getnext function is defined below
mv88e6xxx_port_db_load_purge, so forward-declare it for the moment. The
ATU code will be isolated in future patches.

Fixes: 83dabd1fa84c ("net: dsa: mv88e6xxx: make switchdev DB ops generic")
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 56 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 49 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 70a812d..1d71802 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2091,12 +2091,48 @@ static int _mv88e6xxx_atu_load(struct mv88e6xxx_chip *chip,
 	return _mv88e6xxx_atu_cmd(chip, entry->fid, GLOBAL_ATU_OP_LOAD_DB);
 }
 
+static int _mv88e6xxx_atu_getnext(struct mv88e6xxx_chip *chip, u16 fid,
+				  struct mv88e6xxx_atu_entry *entry);
+
+static int mv88e6xxx_atu_get(struct mv88e6xxx_chip *chip, int fid,
+			     const u8 *addr, struct mv88e6xxx_atu_entry *entry)
+{
+	struct mv88e6xxx_atu_entry next;
+	int err;
+
+	eth_broadcast_addr(next.mac);
+
+	err = _mv88e6xxx_atu_mac_write(chip, next.mac);
+	if (err)
+		return err;
+
+	do {
+		err = _mv88e6xxx_atu_getnext(chip, fid, &next);
+		if (err)
+			return err;
+
+		if (next.state == GLOBAL_ATU_DATA_STATE_UNUSED)
+			break;
+
+		if (ether_addr_equal(next.mac, addr)) {
+			*entry = next;
+			return 0;
+		}
+	} while (!is_broadcast_ether_addr(next.mac));
+
+	memset(entry, 0, sizeof(*entry));
+	entry->fid = fid;
+	ether_addr_copy(entry->mac, addr);
+
+	return 0;
+}
+
 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;
+	struct mv88e6xxx_atu_entry entry;
 	int err;
 
 	/* Null VLAN ID corresponds to the port private database */
@@ -2107,12 +2143,18 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
 	if (err)
 		return err;
 
-	entry.fid = vlan.fid;
-	entry.state = state;
-	ether_addr_copy(entry.mac, addr);
-	if (state != GLOBAL_ATU_DATA_STATE_UNUSED) {
-		entry.trunk = false;
-		entry.portv_trunkid = BIT(port);
+	err = mv88e6xxx_atu_get(chip, vlan.fid, addr, &entry);
+	if (err)
+		return err;
+
+	/* Purge the ATU entry only if no port is using it anymore */
+	if (state == GLOBAL_ATU_DATA_STATE_UNUSED) {
+		entry.portv_trunkid &= ~BIT(port);
+		if (!entry.portv_trunkid)
+			entry.state = GLOBAL_ATU_DATA_STATE_UNUSED;
+	} else {
+		entry.portv_trunkid |= BIT(port);
+		entry.state = state;
 	}
 
 	return _mv88e6xxx_atu_load(chip, &entry);
-- 
2.10.0

Re: [PATCH net-next] net: dsa: mv88e6xxx: handle multiple ports in ATU

[Andrew Lunn]

Hi Vivien

> +	do {
> +		err = _mv88e6xxx_atu_getnext(chip, fid, &next);
> +		if (err)
> +			return err;
> +
> +		if (next.state == GLOBAL_ATU_DATA_STATE_UNUSED)
> +			break;
> +
> +		if (ether_addr_equal(next.mac, addr)) {
> +			*entry = next;
> +			return 0;
> +		}
> +	} while (!is_broadcast_ether_addr(next.mac));

This is correct, but i wonder how well it scales? When we have a lot
of entries in the ATU, this is going to take time. At some point in
the future, we might want to keep a shadow copy of static entries of
the ATU in RAM. We then don't need to search for them.

But that is for later, once we have some complaints this is too slow.

    Andrew

Re: [PATCH net-next] net: dsa: mv88e6xxx: handle multiple ports in ATU

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> Hi Vivien
>
>> +	do {
>> +		err = _mv88e6xxx_atu_getnext(chip, fid, &next);
>> +		if (err)
>> +			return err;
>> +
>> +		if (next.state == GLOBAL_ATU_DATA_STATE_UNUSED)
>> +			break;
>> +
>> +		if (ether_addr_equal(next.mac, addr)) {
>> +			*entry = next;
>> +			return 0;
>> +		}
>> +	} while (!is_broadcast_ether_addr(next.mac));
>
> This is correct, but i wonder how well it scales? When we have a lot
> of entries in the ATU, this is going to take time. At some point in
> the future, we might want to keep a shadow copy of static entries of
> the ATU in RAM. We then don't need to search for them.

There won't be any issue about time here, because we are searching a
precise FID. What takes time is dumping the whole 4095 FIDs at once.

> But that is for later, once we have some complaints this is too slow.

Yes, there are severals things we can cache in this driver, but to be
honest I'd prefer not to cache anything for the moment, until the driver
is robust. Caching data must be the very last point IMO.

Thanks,

        Vivien

Re: [PATCH net-next] net: dsa: mv88e6xxx: handle multiple ports in ATU

[Andrew Lunn]

On Mon, Sep 19, 2016 at 08:29:40PM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > Hi Vivien
> >
> >> +	do {
> >> +		err = _mv88e6xxx_atu_getnext(chip, fid, &next);
> >> +		if (err)
> >> +			return err;
> >> +
> >> +		if (next.state == GLOBAL_ATU_DATA_STATE_UNUSED)
> >> +			break;
> >> +
> >> +		if (ether_addr_equal(next.mac, addr)) {
> >> +			*entry = next;
> >> +			return 0;
> >> +		}
> >> +	} while (!is_broadcast_ether_addr(next.mac));
> >
> > This is correct, but i wonder how well it scales? When we have a lot
> > of entries in the ATU, this is going to take time. At some point in
> > the future, we might want to keep a shadow copy of static entries of
> > the ATU in RAM. We then don't need to search for them.
> 
> There won't be any issue about time here, because we are searching a
> precise FID.

Ah, i didn't realise you can do that. However, it makes sense, the
hardware needs to do it all the time when it receives a frame.

	 Andrew

Re: [PATCH net-next] net: dsa: mv88e6xxx: handle multiple ports in ATU

[Andrew Lunn]

On Mon, Sep 19, 2016 at 07:56:11PM -0400, Vivien Didelot wrote:
> An address can be loaded in the ATU with multiple ports, for instance
> when adding multiple ports to a Multicast group with "bridge mdb".
> 
> The current code doesn't allow that. Add an helper to get a single entry
> from the ATU, then set or clear the requested port, before loading the
> entry back in the ATU.
> 
> Note that the required _mv88e6xxx_atu_getnext function is defined below
> mv88e6xxx_port_db_load_purge, so forward-declare it for the moment. The
> ATU code will be isolated in future patches.
> 
> Fixes: 83dabd1fa84c ("net: dsa: mv88e6xxx: make switchdev DB ops generic")

Is this a real fixes? You don't make it clear what goes wrong. I
assume adding the same MAC address for a second time but for a
different port removes the first entry for the old port?

> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next] net: dsa: mv88e6xxx: handle multiple ports in ATU

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Mon, Sep 19, 2016 at 07:56:11PM -0400, Vivien Didelot wrote:
>> An address can be loaded in the ATU with multiple ports, for instance
>> when adding multiple ports to a Multicast group with "bridge mdb".
>>
>> The current code doesn't allow that. Add an helper to get a single entry
>> from the ATU, then set or clear the requested port, before loading the
>> entry back in the ATU.
>>
>> Note that the required _mv88e6xxx_atu_getnext function is defined below
>> mv88e6xxx_port_db_load_purge, so forward-declare it for the moment. The
>> ATU code will be isolated in future patches.
>>
>> Fixes: 83dabd1fa84c ("net: dsa: mv88e6xxx: make switchdev DB ops generic")
>
> Is this a real fixes? You don't make it clear what goes wrong. I
> assume adding the same MAC address for a second time but for a
> different port removes the first entry for the old port?

Yes, this is what happens, sorry for the bad message. Below is an
example with the relevant hardware bits.

Here's the current behavior, without this patch:

    # bridge mdb add dev br0 port lan0 grp 238.39.20.86

    FID  MAC Addr                  State         Trunk?  DPV/Trunk ID
    0    01:00:5e:27:14:56         MC_STATIC       n     0 - - - - - -

    # bridge mdb add dev br0 port lan2 grp 238.39.20.86

    FID  MAC Addr                  State         Trunk?  DPV/Trunk ID
    0    01:00:5e:27:14:56         MC_STATIC       n     - - 2 - - - - 

Here's the new behavior, with this patch:

    # bridge mdb add dev br0 port lan0 grp 238.39.20.86

    FID  MAC Addr                  State         Trunk?  DPV/Trunk ID
    0    01:00:5e:27:14:56         MC_STATIC       n     0 - - - - - -

    # bridge mdb add dev br0 port lan2 grp 238.39.20.86

    FID  MAC Addr                  State         Trunk?  DPV/Trunk ID
    0    01:00:5e:27:14:56         MC_STATIC       n     0 - 2 - - - -

Thanks!

        Vivien

Re: [PATCH net-next] net: dsa: mv88e6xxx: handle multiple ports in ATU

[Andrew Lunn]

On Mon, Sep 19, 2016 at 09:07:16PM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > On Mon, Sep 19, 2016 at 07:56:11PM -0400, Vivien Didelot wrote:
> >> An address can be loaded in the ATU with multiple ports, for instance
> >> when adding multiple ports to a Multicast group with "bridge mdb".
> >>
> >> The current code doesn't allow that. Add an helper to get a single entry
> >> from the ATU, then set or clear the requested port, before loading the
> >> entry back in the ATU.
> >>
> >> Note that the required _mv88e6xxx_atu_getnext function is defined below
> >> mv88e6xxx_port_db_load_purge, so forward-declare it for the moment. The
> >> ATU code will be isolated in future patches.
> >>
> >> Fixes: 83dabd1fa84c ("net: dsa: mv88e6xxx: make switchdev DB ops generic")
> >
> > Is this a real fixes? You don't make it clear what goes wrong. I
> > assume adding the same MAC address for a second time but for a
> > different port removes the first entry for the old port?
> 
> Yes, this is what happens, sorry for the bad message. Below is an
> example with the relevant hardware bits.
> 
> Here's the current behavior, without this patch:
> 
>     # bridge mdb add dev br0 port lan0 grp 238.39.20.86
> 
>     FID  MAC Addr                  State         Trunk?  DPV/Trunk ID
>     0    01:00:5e:27:14:56         MC_STATIC       n     0 - - - - - -
> 
>     # bridge mdb add dev br0 port lan2 grp 238.39.20.86
> 
>     FID  MAC Addr                  State         Trunk?  DPV/Trunk ID
>     0    01:00:5e:27:14:56         MC_STATIC       n     - - 2 - - - - 
> 
> Here's the new behavior, with this patch:
> 
>     # bridge mdb add dev br0 port lan0 grp 238.39.20.86
> 
>     FID  MAC Addr                  State         Trunk?  DPV/Trunk ID
>     0    01:00:5e:27:14:56         MC_STATIC       n     0 - - - - - -
> 
>     # bridge mdb add dev br0 port lan2 grp 238.39.20.86
> 
>     FID  MAC Addr                  State         Trunk?  DPV/Trunk ID
>     0    01:00:5e:27:14:56         MC_STATIC       n     0 - 2 - - - -

Hi Vivien

it would be nice to update the commit message with this text. 

Otherwise

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next] net: dsa: mv88e6xxx: handle multiple ports in ATU

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Mon, 19 Sep 2016 19:56:11 -0400

> An address can be loaded in the ATU with multiple ports, for instance
> when adding multiple ports to a Multicast group with "bridge mdb".
> 
> The current code doesn't allow that. Add an helper to get a single entry
> from the ATU, then set or clear the requested port, before loading the
> entry back in the ATU.
> 
> Note that the required _mv88e6xxx_atu_getnext function is defined below
> mv88e6xxx_port_db_load_purge, so forward-declare it for the moment. The
> ATU code will be isolated in future patches.
> 
> Fixes: 83dabd1fa84c ("net: dsa: mv88e6xxx: make switchdev DB ops generic")
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Applied.