[PATCH net-next v1 0/7] net: dsa: microchip: ksz8: Enhance static MAC table operations and error handling

[PATCH net-next v1 0/7] net: dsa: microchip: ksz8: Enhance static MAC table operations and error handling

[Oleksij Rempel]

This patch series improves the Microchip ksz8 driver by refactoring
static MAC table operations for code reuse, implementing add/del_fdb
functions, and making better use of error values in
ksz8_r_sta_mac_table() and ksz8_w_sta_mac_table(). The changes aim to
provide a more robust and maintainable driver with improved error
handling.

Oleksij Rempel (7):
  net: dsa: microchip: ksz8: Separate static MAC table operations for
    code reuse
  net: dsa: microchip: ksz8: Implement add/del_fdb and use static MAC
    table operations
  net: dsa: microchip: ksz8: Make ksz8_r_sta_mac_table() static
  net: dsa: microchip: ksz8_r_sta_mac_table(): Avoid using error code
    for empty entries
  net: dsa: microchip: ksz8_r_sta_mac_table(): Utilize error values from
    read/write functions
  net: dsa: microchip: Make ksz8_w_sta_mac_table() static
  net: dsa: microchip: Utilize error values in ksz8_w_sta_mac_table()

 drivers/net/dsa/microchip/ksz8.h       |   8 +-
 drivers/net/dsa/microchip/ksz8795.c    | 179 ++++++++++++++++---------
 drivers/net/dsa/microchip/ksz_common.c |   2 +
 3 files changed, 121 insertions(+), 68 deletions(-)

-- 
2.39.2

[PATCH net-next v1 1/7] net: dsa: microchip: ksz8: Separate static MAC table operations for code reuse

[Oleksij Rempel]

Move static MAC table operations to separate functions in order to reuse
the code for add/del_fdb. This is needed to address 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/ksz8795.c | 34 +++++++++++++++++++----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 8917f22f90d2..97a6c5516673 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -989,8 +989,8 @@ int ksz8_fdb_dump(struct ksz_device *dev, int port,
 	return ret;
 }
 
-int ksz8_mdb_add(struct ksz_device *dev, int port,
-		 const struct switchdev_obj_port_mdb *mdb, struct dsa_db db)
+static int ksz8_add_sta_mac(struct ksz_device *dev, int port,
+			    const unsigned char *addr, u16 vid)
 {
 	struct alu_struct alu;
 	int index;
@@ -1000,8 +1000,8 @@ int ksz8_mdb_add(struct ksz_device *dev, int port,
 	for (index = 0; index < dev->info->num_statics; index++) {
 		if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
 			/* Found one already in static MAC table. */
-			if (!memcmp(alu.mac, mdb->addr, ETH_ALEN) &&
-			    alu.fid == mdb->vid)
+			if (!memcmp(alu.mac, addr, ETH_ALEN) &&
+			    alu.fid == vid)
 				break;
 		/* Remember the first empty entry. */
 		} else if (!empty) {
@@ -1017,23 +1017,23 @@ int ksz8_mdb_add(struct ksz_device *dev, int port,
 	if (index == dev->info->num_statics) {
 		index = empty - 1;
 		memset(&alu, 0, sizeof(alu));
-		memcpy(alu.mac, mdb->addr, ETH_ALEN);
+		memcpy(alu.mac, addr, ETH_ALEN);
 		alu.is_static = true;
 	}
 	alu.port_forward |= BIT(port);
-	if (mdb->vid) {
+	if (vid) {
 		alu.is_use_fid = true;
 
 		/* Need a way to map VID to FID. */
-		alu.fid = mdb->vid;
+		alu.fid = vid;
 	}
 	ksz8_w_sta_mac_table(dev, index, &alu);
 
 	return 0;
 }
 
-int ksz8_mdb_del(struct ksz_device *dev, int port,
-		 const struct switchdev_obj_port_mdb *mdb, struct dsa_db db)
+static int ksz8_del_sta_mac(struct ksz_device *dev, int port,
+			    const unsigned char *addr, u16 vid)
 {
 	struct alu_struct alu;
 	int index;
@@ -1041,8 +1041,8 @@ int ksz8_mdb_del(struct ksz_device *dev, int port,
 	for (index = 0; index < dev->info->num_statics; index++) {
 		if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
 			/* Found one already in static MAC table. */
-			if (!memcmp(alu.mac, mdb->addr, ETH_ALEN) &&
-			    alu.fid == mdb->vid)
+			if (!memcmp(alu.mac, addr, ETH_ALEN) &&
+			    alu.fid == vid)
 				break;
 		}
 	}
@@ -1061,6 +1061,18 @@ int ksz8_mdb_del(struct ksz_device *dev, int port,
 	return 0;
 }
 
+int ksz8_mdb_add(struct ksz_device *dev, int port,
+		 const struct switchdev_obj_port_mdb *mdb, struct dsa_db db)
+{
+	return ksz8_add_sta_mac(dev, port, mdb->addr, mdb->vid);
+}
+
+int ksz8_mdb_del(struct ksz_device *dev, int port,
+		 const struct switchdev_obj_port_mdb *mdb, struct dsa_db db)
+{
+	return ksz8_del_sta_mac(dev, port, mdb->addr, mdb->vid);
+}
+
 int ksz8_port_vlan_filtering(struct ksz_device *dev, int port, bool flag,
 			     struct netlink_ext_ack *extack)
 {
-- 
2.39.2

Re: [PATCH net-next v1 1/7] net: dsa: microchip: ksz8: Separate static MAC table operations for code reuse

[Vladimir Oltean]

On Tue, Apr 04, 2023 at 12:18:36PM +0200, Oleksij Rempel wrote:
> Move static MAC table operations to separate functions in order to reuse
> the code for add/del_fdb. This is needed to address kernel warnings
> caused by the lack of fdb add function support in the current driver.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

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

[Oleksij Rempel]

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(+)

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,
-- 
2.39.2

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

[Vladimir Oltean]

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")?

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

[Oleksij Rempel]

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
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Vladimir Oltean]

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?

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

[Oleksij Rempel]

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
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH net-next v1 3/7] net: dsa: microchip: ksz8: Make ksz8_r_sta_mac_table() static

[Oleksij Rempel]

As ksz8_r_sta_mac_table() is only used within ksz8795.c, there is no need
to export it. Make the function static for better encapsulation.

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

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index ad2c3a72a576..d87f8ebc6323 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -21,8 +21,6 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
 int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
 int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 			 u8 *fid, u8 *src_port, u8 *timestamp, u16 *entries);
-int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
-			 struct alu_struct *alu);
 void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
 			  struct alu_struct *alu);
 void ksz8_r_mib_cnt(struct ksz_device *dev, int port, u16 addr, u64 *cnt);
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index ee68e166fc44..fc079d6eee80 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -457,8 +457,8 @@ int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 	return rc;
 }
 
-int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
-			 struct alu_struct *alu)
+static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
+				struct alu_struct *alu)
 {
 	u32 data_hi, data_lo;
 	const u8 *shifts;
-- 
2.39.2

Re: [PATCH net-next v1 3/7] net: dsa: microchip: ksz8: Make ksz8_r_sta_mac_table() static

[Vladimir Oltean]

On Tue, Apr 04, 2023 at 12:18:38PM +0200, Oleksij Rempel wrote:
> As ksz8_r_sta_mac_table() is only used within ksz8795.c, there is no need
> to export it. Make the function static for better encapsulation.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v1 3/7] net: dsa: microchip: ksz8: Make ksz8_r_sta_mac_table() static

[Arun.Ramadoss]

Hi Oleksij,
On Tue, 2023-04-04 at 12:18 +0200, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> As ksz8_r_sta_mac_table() is only used within ksz8795.c, there is no
> need
> to export it. Make the function static for better encapsulation.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz8.h    | 2 --
>  drivers/net/dsa/microchip/ksz8795.c | 4 ++--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8.h
> b/drivers/net/dsa/microchip/ksz8.h
> index ad2c3a72a576..d87f8ebc6323 100644
> --- a/drivers/net/dsa/microchip/ksz8.h
> +++ b/drivers/net/dsa/microchip/ksz8.h
> @@ -21,8 +21,6 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16
> reg, u16 *val);
>  int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
>  int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8
> *mac_addr,
>                          u8 *fid, u8 *src_port, u8 *timestamp, u16
> *entries);
> -int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
> -                        struct alu_struct *alu);


ksz8_r_dyn_mac_table() also not used outside KSZ8795.h. It can
also be made static

Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>

Re: [PATCH net-next v1 3/7] net: dsa: microchip: ksz8: Make ksz8_r_sta_mac_table() static

[Oleksij Rempel]

On Tue, Apr 04, 2023 at 03:11:02PM +0000, Arun.Ramadoss@microchip.com wrote:
> Hi Oleksij,
> On Tue, 2023-04-04 at 12:18 +0200, Oleksij Rempel wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > As ksz8_r_sta_mac_table() is only used within ksz8795.c, there is no
> > need
> > to export it. Make the function static for better encapsulation.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/dsa/microchip/ksz8.h    | 2 --
> >  drivers/net/dsa/microchip/ksz8795.c | 4 ++--
> >  2 files changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz8.h
> > b/drivers/net/dsa/microchip/ksz8.h
> > index ad2c3a72a576..d87f8ebc6323 100644
> > --- a/drivers/net/dsa/microchip/ksz8.h
> > +++ b/drivers/net/dsa/microchip/ksz8.h
> > @@ -21,8 +21,6 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16
> > reg, u16 *val);
> >  int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
> >  int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8
> > *mac_addr,
> >                          u8 *fid, u8 *src_port, u8 *timestamp, u16
> > *entries);
> > -int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
> > -                        struct alu_struct *alu);
> 
> 
> ksz8_r_dyn_mac_table() also not used outside KSZ8795.h. It can
> also be made static

:) Ack, i have pending series of patch for dyn MAC table.

> Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>



-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH net-next v1 4/7] net: dsa: microchip: ksz8_r_sta_mac_table(): Avoid using error code for empty entries

[Oleksij Rempel]

Prepare for the next patch by ensuring that ksz8_r_sta_mac_table() does
not use error codes for empty entries. This change will enable better
handling of read/write errors in the upcoming patch.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 87 +++++++++++++++++------------
 1 file changed, 50 insertions(+), 37 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index fc079d6eee80..4dcf68a7f6c0 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -458,7 +458,7 @@ int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 }
 
 static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
-				struct alu_struct *alu)
+				struct alu_struct *alu, bool *valid)
 {
 	u32 data_hi, data_lo;
 	const u8 *shifts;
@@ -471,28 +471,32 @@ static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
 	ksz8_r_table(dev, TABLE_STATIC_MAC, addr, &data);
 	data_hi = data >> 32;
 	data_lo = (u32)data;
-	if (data_hi & (masks[STATIC_MAC_TABLE_VALID] |
-			masks[STATIC_MAC_TABLE_OVERRIDE])) {
-		alu->mac[5] = (u8)data_lo;
-		alu->mac[4] = (u8)(data_lo >> 8);
-		alu->mac[3] = (u8)(data_lo >> 16);
-		alu->mac[2] = (u8)(data_lo >> 24);
-		alu->mac[1] = (u8)data_hi;
-		alu->mac[0] = (u8)(data_hi >> 8);
-		alu->port_forward =
-			(data_hi & masks[STATIC_MAC_TABLE_FWD_PORTS]) >>
-				shifts[STATIC_MAC_FWD_PORTS];
-		alu->is_override =
-			(data_hi & masks[STATIC_MAC_TABLE_OVERRIDE]) ? 1 : 0;
-		data_hi >>= 1;
-		alu->is_static = true;
-		alu->is_use_fid =
-			(data_hi & masks[STATIC_MAC_TABLE_USE_FID]) ? 1 : 0;
-		alu->fid = (data_hi & masks[STATIC_MAC_TABLE_FID]) >>
-				shifts[STATIC_MAC_FID];
+
+	if (!(data_hi & (masks[STATIC_MAC_TABLE_VALID] |
+			 masks[STATIC_MAC_TABLE_OVERRIDE]))) {
+		*valid = false;
 		return 0;
 	}
-	return -ENXIO;
+
+	alu->mac[5] = (u8)data_lo;
+	alu->mac[4] = (u8)(data_lo >> 8);
+	alu->mac[3] = (u8)(data_lo >> 16);
+	alu->mac[2] = (u8)(data_lo >> 24);
+	alu->mac[1] = (u8)data_hi;
+	alu->mac[0] = (u8)(data_hi >> 8);
+	alu->port_forward =
+		(data_hi & masks[STATIC_MAC_TABLE_FWD_PORTS]) >>
+			shifts[STATIC_MAC_FWD_PORTS];
+	alu->is_override = (data_hi & masks[STATIC_MAC_TABLE_OVERRIDE]) ? 1 : 0;
+	data_hi >>= 1;
+	alu->is_static = true;
+	alu->is_use_fid = (data_hi & masks[STATIC_MAC_TABLE_USE_FID]) ? 1 : 0;
+	alu->fid = (data_hi & masks[STATIC_MAC_TABLE_FID]) >>
+		shifts[STATIC_MAC_FID];
+
+	*valid = true;
+
+	return 0;
 }
 
 void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
@@ -993,20 +997,25 @@ static int ksz8_add_sta_mac(struct ksz_device *dev, int port,
 			    const unsigned char *addr, u16 vid)
 {
 	struct alu_struct alu;
-	int index;
+	int index, ret;
 	int empty = 0;
 
 	alu.port_forward = 0;
 	for (index = 0; index < dev->info->num_statics; index++) {
-		if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
-			/* Found one already in static MAC table. */
-			if (!memcmp(alu.mac, addr, ETH_ALEN) &&
-			    alu.fid == vid)
-				break;
-		/* Remember the first empty entry. */
-		} else if (!empty) {
-			empty = index + 1;
+		bool valid;
+
+		ret = ksz8_r_sta_mac_table(dev, index, &alu, &valid);
+		if (ret)
+			return ret;
+		if (!valid) {
+			/* Remember the first empty entry. */
+			if (!empty)
+				empty = index + 1;
+			continue;
 		}
+
+		if (!memcmp(alu.mac, addr, ETH_ALEN) && alu.fid == vid)
+			break;
 	}
 
 	/* no available entry */
@@ -1036,15 +1045,19 @@ static int ksz8_del_sta_mac(struct ksz_device *dev, int port,
 			    const unsigned char *addr, u16 vid)
 {
 	struct alu_struct alu;
-	int index;
+	int index, ret;
 
 	for (index = 0; index < dev->info->num_statics; index++) {
-		if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
-			/* Found one already in static MAC table. */
-			if (!memcmp(alu.mac, addr, ETH_ALEN) &&
-			    alu.fid == vid)
-				break;
-		}
+		bool valid;
+
+		ret = ksz8_r_sta_mac_table(dev, index, &alu, &valid);
+		if (ret)
+			return ret;
+		if (!valid)
+			continue;
+
+		if (!memcmp(alu.mac, addr, ETH_ALEN) && alu.fid == vid)
+			break;
 	}
 
 	/* no available entry */
-- 
2.39.2

Re: [PATCH net-next v1 4/7] net: dsa: microchip: ksz8_r_sta_mac_table(): Avoid using error code for empty entries

[Vladimir Oltean]

On Tue, Apr 04, 2023 at 12:18:39PM +0200, Oleksij Rempel wrote:
> Prepare for the next patch by ensuring that ksz8_r_sta_mac_table() does
> not use error codes for empty entries. This change will enable better
> handling of read/write errors in the upcoming patch.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

FWIW, it looks like for port_fdb_add(), you could skip the search in the
static MAC table, as long as you just keep a reference to the last
populated index, because the bridge won't allow, to my knowledge, adding
the same MAC address twice (this has changed when we stopped allowing
bridge bypass operations in commit b117e1e8a86d ("net: dsa: delete
dsa_legacy_fdb_add and dsa_legacy_fdb_del")), and so, having space would
mean that the last populated index is < dev->info->num_statics - 1.

This guarantee and optimization possibility is different from
port_mdb_add(), because there, you might be asked to modify an existing
entry, and so, you still need the search to find it. But still, you
could limit the search for the remaining 3 operations - port_fdb_del(),
port_mdb_add(), port_mdb_del() - just from 0 to that last populated
entry, not to dev->info->num_statics, which should still accelerate the
operations a bit.

[PATCH net-next v1 5/7] net: dsa: microchip: ksz8_r_sta_mac_table(): Utilize error values from read/write functions

[Oleksij Rempel]

Take advantage of the return values provided by read/write functions in
ksz8_r_sta_mac_table() to handle cases where read/write operations may
fail.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 4dcf68a7f6c0..76a8ca329732 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -336,19 +336,26 @@ void ksz8_port_init_cnt(struct ksz_device *dev, int port)
 	}
 }
 
-static void ksz8_r_table(struct ksz_device *dev, int table, u16 addr, u64 *data)
+static int ksz8_r_table(struct ksz_device *dev, int table, u16 addr, u64 *data)
 {
 	const u16 *regs;
 	u16 ctrl_addr;
+	int ret;
 
 	regs = dev->info->regs;
 
 	ctrl_addr = IND_ACC_TABLE(table | TABLE_READ) | addr;
 
 	mutex_lock(&dev->alu_mutex);
-	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
-	ksz_read64(dev, regs[REG_IND_DATA_HI], data);
+	ret = ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	if (ret)
+		goto unlock_alu;
+
+	ret = ksz_read64(dev, regs[REG_IND_DATA_HI], data);
+unlock_alu:
 	mutex_unlock(&dev->alu_mutex);
+
+	return ret;
 }
 
 static void ksz8_w_table(struct ksz_device *dev, int table, u16 addr, u64 data)
@@ -464,11 +471,15 @@ static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
 	const u8 *shifts;
 	const u32 *masks;
 	u64 data;
+	int ret;
 
 	shifts = dev->info->shifts;
 	masks = dev->info->masks;
 
-	ksz8_r_table(dev, TABLE_STATIC_MAC, addr, &data);
+	ret = ksz8_r_table(dev, TABLE_STATIC_MAC, addr, &data);
+	if (ret)
+		return ret;
+
 	data_hi = data >> 32;
 	data_lo = (u32)data;
 
-- 
2.39.2

Re: [PATCH net-next v1 5/7] net: dsa: microchip: ksz8_r_sta_mac_table(): Utilize error values from read/write functions

[Arun.Ramadoss]

Hi Oleksij,

On Tue, 2023-04-04 at 12:18 +0200, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Take advantage of the return values provided by read/write functions
> in
> ksz8_r_sta_mac_table() to handle cases where read/write operations
> may
> fail.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>

[PATCH net-next v1 6/7] net: dsa: microchip: Make ksz8_w_sta_mac_table() static

[Oleksij Rempel]

Since ksz8_w_sta_mac_table() is only used within ksz8795.c, make it static
to limit its scope.

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

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index d87f8ebc6323..ec02baca726f 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -21,8 +21,6 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
 int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
 int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 			 u8 *fid, u8 *src_port, u8 *timestamp, u16 *entries);
-void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
-			  struct alu_struct *alu);
 void ksz8_r_mib_cnt(struct ksz_device *dev, int port, u16 addr, u64 *cnt);
 void ksz8_r_mib_pkt(struct ksz_device *dev, int port, u16 addr,
 		    u64 *dropped, u64 *cnt);
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 76a8ca329732..81ce1e3fdf56 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -510,8 +510,8 @@ static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
 	return 0;
 }
 
-void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
-			  struct alu_struct *alu)
+static void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
+				 struct alu_struct *alu)
 {
 	u32 data_hi, data_lo;
 	const u8 *shifts;
-- 
2.39.2

Re: [PATCH net-next v1 6/7] net: dsa: microchip: Make ksz8_w_sta_mac_table() static

[Arun.Ramadoss]

Hi Oleksij,

On Tue, 2023-04-04 at 12:18 +0200, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Since ksz8_w_sta_mac_table() is only used within ksz8795.c, make it
> static
> to limit its scope.

Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>

[PATCH net-next v1 7/7] net: dsa: microchip: Utilize error values in ksz8_w_sta_mac_table()

[Oleksij Rempel]

To handle potential read/write operation failures, update
ksz8_w_sta_mac_table() to make use of the return values provided by
read/write functions.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 33 ++++++++++++++++-------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 81ce1e3fdf56..fe17ce27e5e2 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -358,19 +358,26 @@ static int ksz8_r_table(struct ksz_device *dev, int table, u16 addr, u64 *data)
 	return ret;
 }
 
-static void ksz8_w_table(struct ksz_device *dev, int table, u16 addr, u64 data)
+static int ksz8_w_table(struct ksz_device *dev, int table, u16 addr, u64 data)
 {
 	const u16 *regs;
 	u16 ctrl_addr;
+	int ret;
 
 	regs = dev->info->regs;
 
 	ctrl_addr = IND_ACC_TABLE(table) | addr;
 
 	mutex_lock(&dev->alu_mutex);
-	ksz_write64(dev, regs[REG_IND_DATA_HI], data);
-	ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+	ret = ksz_write64(dev, regs[REG_IND_DATA_HI], data);
+	if (ret)
+		goto unlock_alu;
+
+	ret = ksz_write16(dev, regs[REG_IND_CTRL_0], ctrl_addr);
+unlock_alu:
 	mutex_unlock(&dev->alu_mutex);
+
+	return ret;
 }
 
 static int ksz8_valid_dyn_entry(struct ksz_device *dev, u8 *data)
@@ -510,8 +517,8 @@ static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
 	return 0;
 }
 
-static void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
-				 struct alu_struct *alu)
+static int ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
+				struct alu_struct *alu)
 {
 	u32 data_hi, data_lo;
 	const u8 *shifts;
@@ -539,7 +546,8 @@ static void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
 		data_hi &= ~masks[STATIC_MAC_TABLE_OVERRIDE];
 
 	data = (u64)data_hi << 32 | data_lo;
-	ksz8_w_table(dev, TABLE_STATIC_MAC, addr, data);
+
+	return ksz8_w_table(dev, TABLE_STATIC_MAC, addr, data);
 }
 
 static void ksz8_from_vlan(struct ksz_device *dev, u32 vlan, u8 *fid,
@@ -1047,9 +1055,8 @@ static int ksz8_add_sta_mac(struct ksz_device *dev, int port,
 		/* Need a way to map VID to FID. */
 		alu.fid = vid;
 	}
-	ksz8_w_sta_mac_table(dev, index, &alu);
 
-	return 0;
+	return ksz8_w_sta_mac_table(dev, index, &alu);
 }
 
 static int ksz8_del_sta_mac(struct ksz_device *dev, int port,
@@ -1073,16 +1080,14 @@ static int ksz8_del_sta_mac(struct ksz_device *dev, int port,
 
 	/* no available entry */
 	if (index == dev->info->num_statics)
-		goto exit;
+		return 0;
 
 	/* clear port */
 	alu.port_forward &= ~BIT(port);
 	if (!alu.port_forward)
 		alu.is_static = false;
-	ksz8_w_sta_mac_table(dev, index, &alu);
 
-exit:
-	return 0;
+	return ksz8_w_sta_mac_table(dev, index, &alu);
 }
 
 int ksz8_mdb_add(struct ksz_device *dev, int port,
@@ -1446,9 +1451,7 @@ int ksz8_enable_stp_addr(struct ksz_device *dev)
 	alu.is_override = true;
 	alu.port_forward = dev->info->cpu_ports;
 
-	ksz8_w_sta_mac_table(dev, 0, &alu);
-
-	return 0;
+	return ksz8_w_sta_mac_table(dev, 0, &alu);
 }
 
 int ksz8_setup(struct dsa_switch *ds)
-- 
2.39.2

Re: [PATCH net-next v1 7/7] net: dsa: microchip: Utilize error values in ksz8_w_sta_mac_table()

[Arun.Ramadoss]

Hi Oleksij,

On Tue, 2023-04-04 at 12:18 +0200, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> To handle potential read/write operation failures, update
> ksz8_w_sta_mac_table() to make use of the return values provided by
> read/write functions.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>

Re: [PATCH net-next v1 0/7] net: dsa: microchip: ksz8: Enhance static MAC table operations and error handling

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Tue,  4 Apr 2023 12:18:35 +0200 you wrote:
> This patch series improves the Microchip ksz8 driver by refactoring
> static MAC table operations for code reuse, implementing add/del_fdb
> functions, and making better use of error values in
> ksz8_r_sta_mac_table() and ksz8_w_sta_mac_table(). The changes aim to
> provide a more robust and maintainable driver with improved error
> handling.
> 
> [...]

Here is the summary with links:
  - [net-next,v1,1/7] net: dsa: microchip: ksz8: Separate static MAC table operations for code reuse
    https://git.kernel.org/netdev/net-next/c/f6636ff69ec4
  - [net-next,v1,2/7] net: dsa: microchip: ksz8: Implement add/del_fdb and use static MAC table operations
    https://git.kernel.org/netdev/net-next/c/57795412a447
  - [net-next,v1,3/7] net: dsa: microchip: ksz8: Make ksz8_r_sta_mac_table() static
    https://git.kernel.org/netdev/net-next/c/b5751cdd7dbe
  - [net-next,v1,4/7] net: dsa: microchip: ksz8_r_sta_mac_table(): Avoid using error code for empty entries
    https://git.kernel.org/netdev/net-next/c/559901b46810
  - [net-next,v1,5/7] net: dsa: microchip: ksz8_r_sta_mac_table(): Utilize error values from read/write functions
    https://git.kernel.org/netdev/net-next/c/ec2312f33735
  - [net-next,v1,6/7] net: dsa: microchip: Make ksz8_w_sta_mac_table() static
    https://git.kernel.org/netdev/net-next/c/c8e04374f9e1
  - [net-next,v1,7/7] net: dsa: microchip: Utilize error values in ksz8_w_sta_mac_table()
    https://git.kernel.org/netdev/net-next/c/3c2e6b54e4e9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html