diff mbox series

[net-next,v1,2/7] net: dsa: microchip: ksz8: Implement add/del_fdb and use static MAC table operations

Message ID 20230404101842.1382986-3-o.rempel@pengutronix.de
State New
Headers
Series net: dsa: microchip: ksz8: Enhance static MAC table operations and error handling |

Commit Message

Oleksij Rempel April 4, 2023, 10:18 a.m. UTC 
  Add support for add/del_fdb operations and utilize the refactored static
MAC table code. This resolves kernel warnings caused by the lack of fdb
add function support in the current driver.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8.h       |  4 ++++
 drivers/net/dsa/microchip/ksz8795.c    | 12 ++++++++++++
 drivers/net/dsa/microchip/ksz_common.c |  2 ++
 3 files changed, 18 insertions(+)

Comments

Vladimir Oltean April 4, 2023, 11:31 a.m. UTC | #1 
On Tue, Apr 04, 2023 at 12:18:37PM +0200, Oleksij Rempel wrote:
> Add support for add/del_fdb operations and utilize the refactored static
> MAC table code. This resolves kernel warnings caused by the lack of fdb
> add function support in the current driver.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

Side note, I wonder if it's so simple, why this was not done in
e66f840c08a2 ("net: dsa: ksz: Add Microchip KSZ8795 DSA driver")?
Oleksij Rempel April 4, 2023, 12:19 p.m. UTC | #2 
On Tue, Apr 04, 2023 at 02:31:24PM +0300, Vladimir Oltean wrote:
> On Tue, Apr 04, 2023 at 12:18:37PM +0200, Oleksij Rempel wrote:
> > Add support for add/del_fdb operations and utilize the refactored static
> > MAC table code. This resolves kernel warnings caused by the lack of fdb
> > add function support in the current driver.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> 
> Side note, I wonder if it's so simple, why this was not done in
> e66f840c08a2 ("net: dsa: ksz: Add Microchip KSZ8795 DSA driver")?

If I compare KSZ879CLX and KSZ8873MLL datasheets, i do not see direct
answer. The only reason I can imagine is the size of static MAC table.
All KSZ88xx and KSZ87xx variants have only 8 entries. One is already
used for STP (even if STP is not enabled, can be optimized). If
BRIDGE_VLAN compiled, each local address will be configured 2 times.
So, depending on system configuration the static MAC table will full
very soon.

I tested this patch on KSZ8873. Without this patch, if we do not
send any thing from CPU port, local MAC addresses will be forgotten by
the dynamic MAC table. Sending packets to a local MAC address from swp0
will flood packets to CPU and swp1. With this patch, packets fill be
forwarded only to CPU - as expected.

Regards,
Oleksij
Vladimir Oltean April 4, 2023, 12:50 p.m. UTC | #3 
On Tue, Apr 04, 2023 at 02:19:11PM +0200, Oleksij Rempel wrote:
> If I compare KSZ879CLX and KSZ8873MLL datasheets, i do not see direct
> answer. The only reason I can imagine is the size of static MAC table.
> All KSZ88xx and KSZ87xx variants have only 8 entries. One is already
> used for STP (even if STP is not enabled, can be optimized). If
> BRIDGE_VLAN compiled, each local address will be configured 2 times.
> So, depending on system configuration the static MAC table will full
> very soon.

Yikes. KSZ8765 has num_statics = 8 and port_cnt = 5 (so 4 user ports I
assume). So if all 4 user ports had their own MAC address, it would
simply not be possible to put them under a VLAN-aware bridge, since that
would consume 2 BR_FDB_LOCAL entries for each port, so the static MAC
table would be full even without taking the bridge's MAC address into
consideration.

Even with CONFIG_BRIDGE_VLAN_FILTERING turned off or with the bridge
option vlan_default_pvid = 0, this would still consume 4 BR_FDB_LOCAL
entries + one for the bridge's MAC address + 1 for STP, leaving only 2
entries usable for *both* bridge fdb, *and* bridge mdb.

I haven't opened the datasheets of these chips. Is it possible to use
the dynamic MAC table to store static(-ish) entries?
Oleksij Rempel April 4, 2023, 1:06 p.m. UTC | #4 
On Tue, Apr 04, 2023 at 03:50:02PM +0300, Vladimir Oltean wrote:
> On Tue, Apr 04, 2023 at 02:19:11PM +0200, Oleksij Rempel wrote:
> > If I compare KSZ879CLX and KSZ8873MLL datasheets, i do not see direct
> > answer. The only reason I can imagine is the size of static MAC table.
> > All KSZ88xx and KSZ87xx variants have only 8 entries. One is already
> > used for STP (even if STP is not enabled, can be optimized). If
> > BRIDGE_VLAN compiled, each local address will be configured 2 times.
> > So, depending on system configuration the static MAC table will full
> > very soon.
> 
> Yikes. KSZ8765 has num_statics = 8 and port_cnt = 5 (so 4 user ports I
> assume). So if all 4 user ports had their own MAC address, it would
> simply not be possible to put them under a VLAN-aware bridge, since that
> would consume 2 BR_FDB_LOCAL entries for each port, so the static MAC
> table would be full even without taking the bridge's MAC address into
> consideration.
> 
> Even with CONFIG_BRIDGE_VLAN_FILTERING turned off or with the bridge
> option vlan_default_pvid = 0, this would still consume 4 BR_FDB_LOCAL
> entries + one for the bridge's MAC address + 1 for STP, leaving only 2
> entries usable for *both* bridge fdb, *and* bridge mdb.
> 
> I haven't opened the datasheets of these chips. Is it possible to use
> the dynamic MAC table to store static(-ish) entries?

According to KSZ8795CLX datasheet, dynamic MAC table is read-only.
But there is Access Control Lists (ACL) with 16 entries. It is possible
created a forwarding rule with match against DST MAC address.

Beside, I'm working right now on KSZ9477 tc-flower support based on ACL
implementation.

Regards,
Oleksij
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index 9bb19764fa33..ad2c3a72a576 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -32,6 +32,10 @@  void ksz8_freeze_mib(struct ksz_device *dev, int port, bool freeze);
 void ksz8_port_init_cnt(struct ksz_device *dev, int port);
 int ksz8_fdb_dump(struct ksz_device *dev, int port,
 		  dsa_fdb_dump_cb_t *cb, void *data);
+int ksz8_fdb_add(struct ksz_device *dev, int port, const unsigned char *addr,
+		 u16 vid, struct dsa_db db);
+int ksz8_fdb_del(struct ksz_device *dev, int port, const unsigned char *addr,
+		 u16 vid, struct dsa_db db);
 int ksz8_mdb_add(struct ksz_device *dev, int port,
 		 const struct switchdev_obj_port_mdb *mdb, struct dsa_db db);
 int ksz8_mdb_del(struct ksz_device *dev, int port,
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 97a6c5516673..ee68e166fc44 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1073,6 +1073,18 @@  int ksz8_mdb_del(struct ksz_device *dev, int port,
 	return ksz8_del_sta_mac(dev, port, mdb->addr, mdb->vid);
 }
 
+int ksz8_fdb_add(struct ksz_device *dev, int port, const unsigned char *addr,
+		 u16 vid, struct dsa_db db)
+{
+	return ksz8_add_sta_mac(dev, port, addr, vid);
+}
+
+int ksz8_fdb_del(struct ksz_device *dev, int port, const unsigned char *addr,
+		 u16 vid, struct dsa_db db)
+{
+	return ksz8_del_sta_mac(dev, port, addr, vid);
+}
+
 int ksz8_port_vlan_filtering(struct ksz_device *dev, int port, bool flag,
 			     struct netlink_ext_ack *extack)
 {
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 93131347ad98..6e19ad70c671 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -200,6 +200,8 @@  static const struct ksz_dev_ops ksz8_dev_ops = {
 	.freeze_mib = ksz8_freeze_mib,
 	.port_init_cnt = ksz8_port_init_cnt,
 	.fdb_dump = ksz8_fdb_dump,
+	.fdb_add = ksz8_fdb_add,
+	.fdb_del = ksz8_fdb_del,
 	.mdb_add = ksz8_mdb_add,
 	.mdb_del = ksz8_mdb_del,
 	.vlan_filtering = ksz8_port_vlan_filtering,