| Message ID | 20230531143826.477267-1-alexander.sverdlin@siemens.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5a59a58ec25d44f853c26bdbfda47d73b3067435 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: lan9303: allow vid != 0 in port_fdb_{add|del} methods | expand |
On Wed, May 31, 2023 at 04:38:26PM +0200, A. Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > LAN9303 doesn't associate FDB (ALR) entries with VLANs, it has just one > global Address Logic Resolution table [1]. > > Ignore VID in port_fdb_{add|del} methods, go on with the global table. This > is the same semantics as hellcreek or RZ/N1 implement. > > Visible symptoms: > LAN9303_MDIO 5b050000.ethernet-1:00: port 2 failed to delete 00:xx:xx:xx:xx:cf vid 1 from fdb: -2 > LAN9303_MDIO 5b050000.ethernet-1:00: port 2 failed to add 00:xx:xx:xx:xx:cf vid 1 to fdb: -95 > > [1] https://ww1.microchip.com/downloads/en/DeviceDoc/00002308A.pdf > > Fixes: 0620427ea0d6 ("net: dsa: lan9303: Add fdb/mdb manipulation") > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > --- Thanks for taking a look. Although it would probably be safer to add: Fixes: 2fd186501b1c ("net: dsa: be louder when a non-legacy FDB operation fails") since I'm not sure it has a reason to be backported beyond that. Anyway: Reviewed-by: Vladimir Oltean <olteanv@gmail.com> Yuck.
Hi Vladimir, thank you for quick review! On Wed, 2023-05-31 at 18:16 +0300, Vladimir Oltean wrote: > > LAN9303 doesn't associate FDB (ALR) entries with VLANs, it has just > > one > > global Address Logic Resolution table [1]. > > > > Ignore VID in port_fdb_{add|del} methods, go on with the global > > table. This > > is the same semantics as hellcreek or RZ/N1 implement. > > > > Visible symptoms: > > LAN9303_MDIO 5b050000.ethernet-1:00: port 2 failed to delete > > 00:xx:xx:xx:xx:cf vid 1 from fdb: -2 > > LAN9303_MDIO 5b050000.ethernet-1:00: port 2 failed to add > > 00:xx:xx:xx:xx:cf vid 1 to fdb: -95 > > > > [1] https://ww1.microchip.com/downloads/en/DeviceDoc/00002308A.pdf > > > > Fixes: 0620427ea0d6 ("net: dsa: lan9303: Add fdb/mdb manipulation") > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > --- > > Thanks for taking a look. Although it would probably be safer to add: > > Fixes: 2fd186501b1c ("net: dsa: be louder when a non-legacy FDB > operation fails") > > since I'm not sure it has a reason to be backported beyond that. Well, it's not only about visible errors, but the driver also refused to install the FDB entries, so it's change in behaviour, not only cosmetics. > Anyway: > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
On Wed, May 31, 2023 at 03:20:19PM +0000, Sverdlin, Alexander wrote: > Well, it's not only about visible errors, but the driver also refused > to install the FDB entries, so it's change in behaviour, not only > cosmetics. Ok, makes sense. Let's see what will happen with the backport - to be honest I'm not completely sure. If you want to be completely sure I didn't just throw a wrench into your plans, feel free to resend a v2 with just my review tag (dropping my Fixes tag), and you could also add a comment stating that the ALR is VLAN-unaware while you're at it.
On Wed, 31 May 2023 18:35:19 +0300 Vladimir Oltean wrote: > If you want to be completely sure I didn't just throw a wrench into > your plans, feel free to resend a v2 with just my review tag > (dropping my Fixes tag) FWIW if you worry that the Fixes tag will get added automatically - for whatever reason that still doesn't work. We add them manually when someone provides a tag in response.
On Wed, May 31, 2023 at 09:41:50PM -0700, Jakub Kicinski wrote: > On Wed, 31 May 2023 18:35:19 +0300 Vladimir Oltean wrote: > > If you want to be completely sure I didn't just throw a wrench into > > your plans, feel free to resend a v2 with just my review tag > > (dropping my Fixes tag) > > FWIW if you worry that the Fixes tag will get added automatically - > for whatever reason that still doesn't work. We add them manually > when someone provides a tag in response. Aha, ok. So as long as the maintainer who applies the patch does not append the second Fixes: tag that I had proposed, all is well and this change can be applied as is.
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 31 May 2023 16:38:26 +0200 you wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > LAN9303 doesn't associate FDB (ALR) entries with VLANs, it has just one > global Address Logic Resolution table [1]. > > Ignore VID in port_fdb_{add|del} methods, go on with the global table. This > is the same semantics as hellcreek or RZ/N1 implement. > > [...] Here is the summary with links: - net: dsa: lan9303: allow vid != 0 in port_fdb_{add|del} methods https://git.kernel.org/netdev/net/c/5a59a58ec25d You are awesome, thank you!
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c index cbe831875347..c0215a8770f4 100644 --- a/drivers/net/dsa/lan9303-core.c +++ b/drivers/net/dsa/lan9303-core.c @@ -1188,8 +1188,6 @@ static int lan9303_port_fdb_add(struct dsa_switch *ds, int port, struct lan9303 *chip = ds->priv; dev_dbg(chip->dev, "%s(%d, %pM, %d)\n", __func__, port, addr, vid); - if (vid) - return -EOPNOTSUPP; return lan9303_alr_add_port(chip, addr, port, false); } @@ -1201,8 +1199,6 @@ static int lan9303_port_fdb_del(struct dsa_switch *ds, int port, struct lan9303 *chip = ds->priv; dev_dbg(chip->dev, "%s(%d, %pM, %d)\n", __func__, port, addr, vid); - if (vid) - return -EOPNOTSUPP; lan9303_alr_del_port(chip, addr, port); return 0;