diff mbox series

[v1,21/26] net: dsa: microchip: ksz8_r_sta_mac_table(): do not use error code for empty entries

Message ID 20221128115958.4049431-22-o.rempel@pengutronix.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: microchip: stats64, fdb, error | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Oleksij Rempel Nov. 28, 2022, 11:59 a.m. UTC 
This is a preparation for the next patch, to make use of read/write errors.

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

Comments

Arun Ramadoss Nov. 29, 2022, 5:25 a.m. UTC | #1 
On Mon, 2022-11-28 at 12:59 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> This is a preparation for the next patch, to make use of read/write
> errors.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz8795.c | 94 +++++++++++++++++--------
> ----
>  1 file changed, 54 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index 1c08103c9f50..b7487be91f67 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -457,7 +457,7 @@ static 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;
> @@ -470,28 +470,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);

u64_to_ether_addr macro can be used to store address into array.

> +       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];

Instead of masks and shifts, you consider using
FIELD_GET(STATIC_MAC_TABLE_FID, data_hi);

> +
> +       *valid = true;
> +
> +       return 0;
>  }
> 
>  void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
> @@ -969,12 +973,13 @@ int ksz8_fdb_dump(struct ksz_device *dev, int
> port,
> 
>         for (i = 0; i  < dev->info->num_statics; i++) {
>                 struct alu_struct alu;
> +               bool valid;
> 
> -               ret = ksz8_r_sta_mac_table(dev, i, &alu);
> -               if (ret == -ENXIO)
> -                       continue;
> +               ret = ksz8_r_sta_mac_table(dev, i, &alu, &valid);
>                 if (ret)
>                         return ret;
> +               if (!valid)
> +                       continue;
> 
>                 if (!(alu.port_forward & BIT(port)))
>                         continue;
> @@ -1010,20 +1015,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 */
> @@ -1053,15 +1063,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;

Variable declaration in separate line.

> 
>         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.30.2
>
Oleksij Rempel Nov. 29, 2022, 5:56 a.m. UTC | #2 
On Tue, Nov 29, 2022 at 05:25:55AM +0000, Arun.Ramadoss@microchip.com wrote:
> On Mon, 2022-11-28 at 12:59 +0100, Oleksij Rempel wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > This is a preparation for the next patch, to make use of read/write
> > errors.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/dsa/microchip/ksz8795.c | 94 +++++++++++++++++--------
> > ----
> >  1 file changed, 54 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz8795.c
> > b/drivers/net/dsa/microchip/ksz8795.c
> > index 1c08103c9f50..b7487be91f67 100644
> > --- a/drivers/net/dsa/microchip/ksz8795.c
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -457,7 +457,7 @@ static 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;
> > @@ -470,28 +470,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);
> 
> u64_to_ether_addr macro can be used to store address into array.

This should not be done in this patch.

> > +       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];
> 
> Instead of masks and shifts, you consider using
> FIELD_GET(STATIC_MAC_TABLE_FID, data_hi);

I would love to do this, but in this case, complete driver should be
splittet in to KSZ88X3 and KSZ879X portions. FIELD_GET can't work with
dynamic masks and shifts.

I would say, it is not for this patch set.

> > +
> > +       *valid = true;
> > +
> > +       return 0;
> >  }
> > 
> >  void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
> > @@ -969,12 +973,13 @@ int ksz8_fdb_dump(struct ksz_device *dev, int
> > port,
> > 
> >         for (i = 0; i  < dev->info->num_statics; i++) {
> >                 struct alu_struct alu;
> > +               bool valid;
> > 
> > -               ret = ksz8_r_sta_mac_table(dev, i, &alu);
> > -               if (ret == -ENXIO)
> > -                       continue;
> > +               ret = ksz8_r_sta_mac_table(dev, i, &alu, &valid);
> >                 if (ret)
> >                         return ret;
> > +               if (!valid)
> > +                       continue;
> > 
> >                 if (!(alu.port_forward & BIT(port)))
> >                         continue;
> > @@ -1010,20 +1015,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 */
> > @@ -1053,15 +1063,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;
> 
> Variable declaration in separate line.

Is it a new coding style requirement? I can't find it here:
https://www.kernel.org/doc/html/v6.0/process/coding-style.html

> > 
> >         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.30.2
> >
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 1c08103c9f50..b7487be91f67 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -457,7 +457,7 @@  static 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;
@@ -470,28 +470,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,
@@ -969,12 +973,13 @@  int ksz8_fdb_dump(struct ksz_device *dev, int port,
 
 	for (i = 0; i  < dev->info->num_statics; i++) {
 		struct alu_struct alu;
+		bool valid;
 
-		ret = ksz8_r_sta_mac_table(dev, i, &alu);
-		if (ret == -ENXIO)
-			continue;
+		ret = ksz8_r_sta_mac_table(dev, i, &alu, &valid);
 		if (ret)
 			return ret;
+		if (!valid)
+			continue;
 
 		if (!(alu.port_forward & BIT(port)))
 			continue;
@@ -1010,20 +1015,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 */
@@ -1053,15 +1063,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 */