| Message ID | 20160919235611.31975-1-vivien.didelot@savoirfairelinux.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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.
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);
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(-)