[PATCH net-next v1 4/8] net: dsa: microchip: ksz8: Refactor ksz8_r_dyn_mac_table() for readability

Oleksij Rempel posted 8 patches 1 year, 10 months ago
There is a newer version of this series
[PATCH net-next v1 4/8] net: dsa: microchip: ksz8: Refactor ksz8_r_dyn_mac_table() for readability
Posted by Oleksij Rempel 1 year, 10 months ago
Move the code out of a long if statement scope in ksz8_r_dyn_mac_table()
to improve code readability.

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

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index b70aa2c0a85ec..1962195e16a6f 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -416,7 +416,9 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 	const u32 *masks;
 	const u16 *regs;
 	u16 ctrl_addr;
+	u64 buf = 0;
 	u8 data;
+	int cnt;
 	int rc;
 
 	shifts = dev->info->shifts;
@@ -432,38 +434,38 @@ static int ksz8_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,
 	if (rc == -EAGAIN) {
 		if (addr == 0)
 			*entries = 0;
+		goto unlock_alu;
 	} else if (rc == -ENXIO) {
 		*entries = 0;
-	/* At least one valid entry in the table. */
-	} else {
-		u64 buf = 0;
-		int cnt;
-
-		ksz_read64(dev, regs[REG_IND_DATA_HI], &buf);
-		data_hi = (u32)(buf >> 32);
-		data_lo = (u32)buf;
-
-		/* Check out how many valid entry in the table. */
-		cnt = data & masks[DYNAMIC_MAC_TABLE_ENTRIES_H];
-		cnt <<= shifts[DYNAMIC_MAC_ENTRIES_H];
-		cnt |= (data_hi & masks[DYNAMIC_MAC_TABLE_ENTRIES]) >>
-			shifts[DYNAMIC_MAC_ENTRIES];
-		*entries = cnt + 1;
-
-		*fid = (data_hi & masks[DYNAMIC_MAC_TABLE_FID]) >>
-			shifts[DYNAMIC_MAC_FID];
-		*src_port = (data_hi & masks[DYNAMIC_MAC_TABLE_SRC_PORT]) >>
-			shifts[DYNAMIC_MAC_SRC_PORT];
-
-		mac_addr[5] = (u8)data_lo;
-		mac_addr[4] = (u8)(data_lo >> 8);
-		mac_addr[3] = (u8)(data_lo >> 16);
-		mac_addr[2] = (u8)(data_lo >> 24);
-
-		mac_addr[1] = (u8)data_hi;
-		mac_addr[0] = (u8)(data_hi >> 8);
-		rc = 0;
+		goto unlock_alu;
 	}
+
+	ksz_read64(dev, regs[REG_IND_DATA_HI], &buf);
+	data_hi = (u32)(buf >> 32);
+	data_lo = (u32)buf;
+
+	/* Check out how many valid entry in the table. */
+	cnt = data & masks[DYNAMIC_MAC_TABLE_ENTRIES_H];
+	cnt <<= shifts[DYNAMIC_MAC_ENTRIES_H];
+	cnt |= (data_hi & masks[DYNAMIC_MAC_TABLE_ENTRIES]) >>
+		shifts[DYNAMIC_MAC_ENTRIES];
+	*entries = cnt + 1;
+
+	*fid = (data_hi & masks[DYNAMIC_MAC_TABLE_FID]) >>
+		shifts[DYNAMIC_MAC_FID];
+	*src_port = (data_hi & masks[DYNAMIC_MAC_TABLE_SRC_PORT]) >>
+		shifts[DYNAMIC_MAC_SRC_PORT];
+
+	mac_addr[5] = (u8)data_lo;
+	mac_addr[4] = (u8)(data_lo >> 8);
+	mac_addr[3] = (u8)(data_lo >> 16);
+	mac_addr[2] = (u8)(data_lo >> 24);
+
+	mac_addr[1] = (u8)data_hi;
+	mac_addr[0] = (u8)(data_hi >> 8);
+	rc = 0;
+
+unlock_alu:
 	mutex_unlock(&dev->alu_mutex);
 
 	return rc;
-- 
2.39.2
Re: [PATCH net-next v1 4/8] net: dsa: microchip: ksz8: Refactor ksz8_r_dyn_mac_table() for readability
Posted by Arun.Ramadoss@microchip.com 1 year, 10 months ago
Hi Oleksij,

On Tue, 2024-04-02 at 15:13 +0200, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Move the code out of a long if statement scope in
> ksz8_r_dyn_mac_table()
> to improve code readability.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz8795.c | 60 +++++++++++++++----------
> ----
>  1 file changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index b70aa2c0a85ec..1962195e16a6f 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -416,7 +416,9 @@ static int ksz8_r_dyn_mac_table(struct ksz_device
> *dev, u16 addr, u8 *mac_addr,
>         const u32 *masks;
>         const u16 *regs;
>         u16 ctrl_addr;
> +       u64 buf = 0;
>         u8 data;
> +       int cnt;
>         int rc;

If rc is initialized with 0, we don't need to assign rc = 0 in the
success path.

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

> 
>         shifts = dev->info->shifts;
> @@ -432,38 +434,38 @@ static int ksz8_r_dyn_mac_table(struct
> ksz_device *dev, u16 addr, u8 *mac_addr,
>         if (rc == -EAGAIN) {
>                 if (addr == 0)
>                         *entries = 0;
> +               goto unlock_alu;
>         } else if (rc == -ENXIO) {
>                 *entries = 0;
> -       /* At least one valid entry in the table. */
> -       } else {
> -               u64 buf = 0;
> -               int cnt;
> -
> -               ksz_read64(dev, regs[REG_IND_DATA_HI], &buf);
> -               data_hi = (u32)(buf >> 32);
> -               data_lo = (u32)buf;
> -
> -               /* Check out how many valid entry in the table. */
> -               cnt = data & masks[DYNAMIC_MAC_TABLE_ENTRIES_H];
> -               cnt <<= shifts[DYNAMIC_MAC_ENTRIES_H];
> -               cnt |= (data_hi & masks[DYNAMIC_MAC_TABLE_ENTRIES])
> >>
> -                       shifts[DYNAMIC_MAC_ENTRIES];
> -               *entries = cnt + 1;
> -
> -               *fid = (data_hi & masks[DYNAMIC_MAC_TABLE_FID]) >>
> -                       shifts[DYNAMIC_MAC_FID];
> -               *src_port = (data_hi &
> masks[DYNAMIC_MAC_TABLE_SRC_PORT]) >>
> -                       shifts[DYNAMIC_MAC_SRC_PORT];
> -
> -               mac_addr[5] = (u8)data_lo;
> -               mac_addr[4] = (u8)(data_lo >> 8);
> -               mac_addr[3] = (u8)(data_lo >> 16);
> -               mac_addr[2] = (u8)(data_lo >> 24);
> -
> -               mac_addr[1] = (u8)data_hi;
> -               mac_addr[0] = (u8)(data_hi >> 8);
> -               rc = 0;
> +               goto unlock_alu;
>         }
> +
> +       ksz_read64(dev, regs[REG_IND_DATA_HI], &buf);
> +       data_hi = (u32)(buf >> 32);
> +       data_lo = (u32)buf;
> +
> +       /* Check out how many valid entry in the table. */
> +       cnt = data & masks[DYNAMIC_MAC_TABLE_ENTRIES_H];
> +       cnt <<= shifts[DYNAMIC_MAC_ENTRIES_H];
> +       cnt |= (data_hi & masks[DYNAMIC_MAC_TABLE_ENTRIES]) >>
> +               shifts[DYNAMIC_MAC_ENTRIES];
> +       *entries = cnt + 1;
> +
> +       *fid = (data_hi & masks[DYNAMIC_MAC_TABLE_FID]) >>
> +               shifts[DYNAMIC_MAC_FID];
> +       *src_port = (data_hi & masks[DYNAMIC_MAC_TABLE_SRC_PORT]) >>
> +               shifts[DYNAMIC_MAC_SRC_PORT];
> +
> +       mac_addr[5] = (u8)data_lo;
> +       mac_addr[4] = (u8)(data_lo >> 8);
> +       mac_addr[3] = (u8)(data_lo >> 16);
> +       mac_addr[2] = (u8)(data_lo >> 24);
> +
> +       mac_addr[1] = (u8)data_hi;
> +       mac_addr[0] = (u8)(data_hi >> 8);
> +       rc = 0;
> +
> +unlock_alu:
>         mutex_unlock(&dev->alu_mutex);
> 
>         return rc;
> --
> 2.39.2
> 
Re: [PATCH net-next v1 4/8] net: dsa: microchip: ksz8: Refactor ksz8_r_dyn_mac_table() for readability
Posted by Jakub Kicinski 1 year, 10 months ago
On Tue, 2 Apr 2024 15:44:30 +0000 Arun.Ramadoss@microchip.com wrote:
> >         int rc;  
> 
> If rc is initialized with 0, we don't need to assign rc = 0 in the
> success path.

Not so sure -- it's easier for the compiler to catch uninitialized
variables than "accidentally stale" values.