diff mbox series

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

Message ID 20240402131339.1525330-5-o.rempel@pengutronix.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: microchip: ksz8: refactor FDB dump path | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 943 this patch: 943
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 954 this patch: 954
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 954 this patch: 954
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 76 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-04-03--09-00 (tests: 950)

Commit Message

Oleksij Rempel April 2, 2024, 1:13 p.m. UTC 
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(-)

Comments

Arun Ramadoss April 2, 2024, 3:44 p.m. UTC | #1 
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
>
Jakub Kicinski April 2, 2024, 5:07 p.m. UTC | #2 
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.
diff mbox series

Patch

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;