diff mbox series

[net-next,v3,2/4] net: phy: at803x: refactor qca808x cable test get status function

Message ID 20240103124637.3078-3-ansuelsmth@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: phy: at803x: even more generalization | 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: 1113 this patch: 1113
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 1140 this patch: 1140
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: 1140 this patch: 1140
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns
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

Commit Message

Christian Marangi Jan. 3, 2024, 12:46 p.m. UTC 
Refactor qca808x cable test get status function to remove code
duplication and clean things up.

The same logic is applied to each pair hence it can be generalized and
moved to a common function.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 65 +++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 31 deletions(-)

Comments

Simon Horman Jan. 4, 2024, 1:03 p.m. UTC | #1 
On Wed, Jan 03, 2024 at 01:46:33PM +0100, Christian Marangi wrote:
> Refactor qca808x cable test get status function to remove code
> duplication and clean things up.
> 
> The same logic is applied to each pair hence it can be generalized and
> moved to a common function.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/phy/at803x.c | 65 +++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index acf483fa0887..b87293ee736c 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -2037,10 +2037,39 @@ static int qca808x_cable_test_start(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static void qca808x_cable_test_get_pair_status(struct phy_device *phydev, u8 pair,
> +					       u16 status)
> +{
> +	u16 pair_code;
> +	int length;
> +
> +	switch (pair) {
> +	case ETHTOOL_A_CABLE_PAIR_A:
> +		pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_A, status);
> +		break;
> +	case ETHTOOL_A_CABLE_PAIR_B:
> +		pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_B, status);
> +		break;
> +	case ETHTOOL_A_CABLE_PAIR_C:
> +		pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_C, status);
> +		break;
> +	case ETHTOOL_A_CABLE_PAIR_D:
> +		pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_D, status);
> +		break;
> +	}

Hi Christian,

I don't think this can actually happen given current usage,
but if pair doesn't match one of the cases above,
then pair_code is used uninitialised below.

Flagged by Smatch.

> +
> +	ethnl_cable_test_result(phydev, pair,
> +				qca808x_cable_test_result_trans(pair_code));
> +
> +	if (qca808x_cdt_fault_length_valid(pair_code)) {
> +		length = qca808x_cdt_fault_length(phydev, pair);
> +		ethnl_cable_test_fault_length(phydev, pair, length);
> +	}
> +}
> +
>  static int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finished)
>  {
>  	int ret, val;
> -	int pair_a, pair_b, pair_c, pair_d;
>  
>  	*finished = false;
>  

...
Christian Marangi Jan. 4, 2024, 1:15 p.m. UTC | #2 
On Thu, Jan 04, 2024 at 01:03:37PM +0000, Simon Horman wrote:
> On Wed, Jan 03, 2024 at 01:46:33PM +0100, Christian Marangi wrote:
> > Refactor qca808x cable test get status function to remove code
> > duplication and clean things up.
> > 
> > The same logic is applied to each pair hence it can be generalized and
> > moved to a common function.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/phy/at803x.c | 65 +++++++++++++++++++++-------------------
> >  1 file changed, 34 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> > index acf483fa0887..b87293ee736c 100644
> > --- a/drivers/net/phy/at803x.c
> > +++ b/drivers/net/phy/at803x.c
> > @@ -2037,10 +2037,39 @@ static int qca808x_cable_test_start(struct phy_device *phydev)
> >  	return 0;
> >  }
> >  
> > +static void qca808x_cable_test_get_pair_status(struct phy_device *phydev, u8 pair,
> > +					       u16 status)
> > +{
> > +	u16 pair_code;
> > +	int length;
> > +
> > +	switch (pair) {
> > +	case ETHTOOL_A_CABLE_PAIR_A:
> > +		pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_A, status);
> > +		break;
> > +	case ETHTOOL_A_CABLE_PAIR_B:
> > +		pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_B, status);
> > +		break;
> > +	case ETHTOOL_A_CABLE_PAIR_C:
> > +		pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_C, status);
> > +		break;
> > +	case ETHTOOL_A_CABLE_PAIR_D:
> > +		pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_D, status);
> > +		break;
> > +	}
> 
> Hi Christian,
> 
> I don't think this can actually happen given current usage,
> but if pair doesn't match one of the cases above,
> then pair_code is used uninitialised below.
> 
> Flagged by Smatch.
>

In theory it would trigger a warning for the switch case not handled?

But yes I will add a default case.

> > +
> > +	ethnl_cable_test_result(phydev, pair,
> > +				qca808x_cable_test_result_trans(pair_code));
> > +
> > +	if (qca808x_cdt_fault_length_valid(pair_code)) {
> > +		length = qca808x_cdt_fault_length(phydev, pair);
> > +		ethnl_cable_test_fault_length(phydev, pair, length);
> > +	}
> > +}
> > +
> >  static int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finished)
> >  {
> >  	int ret, val;
> > -	int pair_a, pair_b, pair_c, pair_d;
> >  
> >  	*finished = false;
> >  
> 
> ...
diff mbox series

Patch

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index acf483fa0887..b87293ee736c 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -2037,10 +2037,39 @@  static int qca808x_cable_test_start(struct phy_device *phydev)
 	return 0;
 }
 
+static void qca808x_cable_test_get_pair_status(struct phy_device *phydev, u8 pair,
+					       u16 status)
+{
+	u16 pair_code;
+	int length;
+
+	switch (pair) {
+	case ETHTOOL_A_CABLE_PAIR_A:
+		pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_A, status);
+		break;
+	case ETHTOOL_A_CABLE_PAIR_B:
+		pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_B, status);
+		break;
+	case ETHTOOL_A_CABLE_PAIR_C:
+		pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_C, status);
+		break;
+	case ETHTOOL_A_CABLE_PAIR_D:
+		pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_D, status);
+		break;
+	}
+
+	ethnl_cable_test_result(phydev, pair,
+				qca808x_cable_test_result_trans(pair_code));
+
+	if (qca808x_cdt_fault_length_valid(pair_code)) {
+		length = qca808x_cdt_fault_length(phydev, pair);
+		ethnl_cable_test_fault_length(phydev, pair, length);
+	}
+}
+
 static int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finished)
 {
 	int ret, val;
-	int pair_a, pair_b, pair_c, pair_d;
 
 	*finished = false;
 
@@ -2059,36 +2088,10 @@  static int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finish
 	if (val < 0)
 		return val;
 
-	pair_a = FIELD_GET(QCA808X_CDT_CODE_PAIR_A, val);
-	pair_b = FIELD_GET(QCA808X_CDT_CODE_PAIR_B, val);
-	pair_c = FIELD_GET(QCA808X_CDT_CODE_PAIR_C, val);
-	pair_d = FIELD_GET(QCA808X_CDT_CODE_PAIR_D, val);
-
-	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
-				qca808x_cable_test_result_trans(pair_a));
-	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_B,
-				qca808x_cable_test_result_trans(pair_b));
-	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_C,
-				qca808x_cable_test_result_trans(pair_c));
-	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_D,
-				qca808x_cable_test_result_trans(pair_d));
-
-	if (qca808x_cdt_fault_length_valid(pair_a)) {
-		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A);
-		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A, val);
-	}
-	if (qca808x_cdt_fault_length_valid(pair_b)) {
-		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B);
-		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B, val);
-	}
-	if (qca808x_cdt_fault_length_valid(pair_c)) {
-		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C);
-		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C, val);
-	}
-	if (qca808x_cdt_fault_length_valid(pair_d)) {
-		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D);
-		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D, val);
-	}
+	qca808x_cable_test_get_pair_status(phydev, ETHTOOL_A_CABLE_PAIR_A, val);
+	qca808x_cable_test_get_pair_status(phydev, ETHTOOL_A_CABLE_PAIR_B, val);
+	qca808x_cable_test_get_pair_status(phydev, ETHTOOL_A_CABLE_PAIR_C, val);
+	qca808x_cable_test_get_pair_status(phydev, ETHTOOL_A_CABLE_PAIR_D, val);
 
 	*finished = true;