1. Patchew
  2. linux
  3. [PATCH net v3 0/4] Correcting switch hardware versions and reported speeds
  4. View patch

[PATCH net v3 1/4] rtase: Refactor the rtase_check_mac_version_valid() function

Justin Lai posted 4 patches 3 days, 13 hours ago
There is a newer version of this series
[PATCH net v3 1/4] rtase: Refactor the rtase_check_mac_version_valid() function
Posted by Justin Lai 3 days, 13 hours ago 
1. Sets tp->hw_ver.
2. Changes the return type from bool to int.
3. Modify the error message for an invalid hardware version id.

Fixes: a36e9f5cfe9e ("rtase: Add support for a pci table in this module")
Signed-off-by: Justin Lai <justinlai0215@realtek.com>
---
 drivers/net/ethernet/realtek/rtase/rtase.h    |  2 ++
 .../net/ethernet/realtek/rtase/rtase_main.c   | 22 +++++++++++--------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/realtek/rtase/rtase.h b/drivers/net/ethernet/realtek/rtase/rtase.h
index 583c33930f88..547c71937b01 100644
--- a/drivers/net/ethernet/realtek/rtase/rtase.h
+++ b/drivers/net/ethernet/realtek/rtase/rtase.h
@@ -327,6 +327,8 @@ struct rtase_private {
 	u16 int_nums;
 	u16 tx_int_mit;
 	u16 rx_int_mit;
+
+	u32 hw_ver;
 };
 
 #define RTASE_LSO_64K 64000
diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c
index f8777b7663d3..0c19c5645d53 100644
--- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
+++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
@@ -1972,20 +1972,21 @@ static void rtase_init_software_variable(struct pci_dev *pdev,
 	tp->dev->max_mtu = RTASE_MAX_JUMBO_SIZE;
 }
 
-static bool rtase_check_mac_version_valid(struct rtase_private *tp)
+static int rtase_check_mac_version_valid(struct rtase_private *tp)
 {
-	u32 hw_ver = rtase_r32(tp, RTASE_TX_CONFIG_0) & RTASE_HW_VER_MASK;
-	bool known_ver = false;
+	int ret = -ENODEV;
 
-	switch (hw_ver) {
+	tp->hw_ver = rtase_r32(tp, RTASE_TX_CONFIG_0) & RTASE_HW_VER_MASK;
+
+	switch (tp->hw_ver) {
 	case 0x00800000:
 	case 0x04000000:
 	case 0x04800000:
-		known_ver = true;
+		ret = 0;
 		break;
 	}
 
-	return known_ver;
+	return ret;
 }
 
 static int rtase_init_board(struct pci_dev *pdev, struct net_device **dev_out,
@@ -2105,9 +2106,12 @@ static int rtase_init_one(struct pci_dev *pdev,
 	tp->pdev = pdev;
 
 	/* identify chip attached to board */
-	if (!rtase_check_mac_version_valid(tp))
-		return dev_err_probe(&pdev->dev, -ENODEV,
-				     "unknown chip version, contact rtase maintainers (see MAINTAINERS file)\n");
+	ret = rtase_check_mac_version_valid(tp);
+	if (ret != 0) {
+		dev_err(&pdev->dev,
+			"unknown chip version: 0x%08x, contact rtase maintainers (see MAINTAINERS file)\n",
+			tp->hw_ver);
+	}
 
 	rtase_init_software_variable(pdev, tp);
 	rtase_init_hardware(tp);
-- 
2.34.1
Re: [PATCH net v3 1/4] rtase: Refactor the rtase_check_mac_version_valid() function
Posted by Michal Kubiak 3 days, 5 hours ago 
On Mon, Nov 18, 2024 at 12:08:25PM +0800, Justin Lai wrote:
> 1. Sets tp->hw_ver.
> 2. Changes the return type from bool to int.
> 3. Modify the error message for an invalid hardware version id.

The commit message contains too many implementation details (that are
quite obvious after studying the code), but there is no information
about the actual problem the patch is fixing.

> 
> Fixes: a36e9f5cfe9e ("rtase: Add support for a pci table in this module")
> Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> ---
>  drivers/net/ethernet/realtek/rtase/rtase.h    |  2 ++
>  .../net/ethernet/realtek/rtase/rtase_main.c   | 22 +++++++++++--------
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/rtase/rtase.h b/drivers/net/ethernet/realtek/rtase/rtase.h
> index 583c33930f88..547c71937b01 100644
> --- a/drivers/net/ethernet/realtek/rtase/rtase.h
> +++ b/drivers/net/ethernet/realtek/rtase/rtase.h
> @@ -327,6 +327,8 @@ struct rtase_private {
>  	u16 int_nums;
>  	u16 tx_int_mit;
>  	u16 rx_int_mit;
> +
> +	u32 hw_ver;
>  };
>  
>  #define RTASE_LSO_64K 64000
> diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> index f8777b7663d3..0c19c5645d53 100644
> --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> @@ -1972,20 +1972,21 @@ static void rtase_init_software_variable(struct pci_dev *pdev,
>  	tp->dev->max_mtu = RTASE_MAX_JUMBO_SIZE;
>  }
>  
> -static bool rtase_check_mac_version_valid(struct rtase_private *tp)
> +static int rtase_check_mac_version_valid(struct rtase_private *tp)
>  {
> -	u32 hw_ver = rtase_r32(tp, RTASE_TX_CONFIG_0) & RTASE_HW_VER_MASK;
> -	bool known_ver = false;
> +	int ret = -ENODEV;
>  
> -	switch (hw_ver) {
> +	tp->hw_ver = rtase_r32(tp, RTASE_TX_CONFIG_0) & RTASE_HW_VER_MASK;
> +
> +	switch (tp->hw_ver) {
>  	case 0x00800000:
>  	case 0x04000000:
>  	case 0x04800000:
> -		known_ver = true;
> +		ret = 0;
>  		break;
>  	}
>  
> -	return known_ver;
> +	return ret;
>  }
>  
>  static int rtase_init_board(struct pci_dev *pdev, struct net_device **dev_out,
> @@ -2105,9 +2106,12 @@ static int rtase_init_one(struct pci_dev *pdev,
>  	tp->pdev = pdev;
>  
>  	/* identify chip attached to board */
> -	if (!rtase_check_mac_version_valid(tp))
> -		return dev_err_probe(&pdev->dev, -ENODEV,
> -				     "unknown chip version, contact rtase maintainers (see MAINTAINERS file)\n");
> +	ret = rtase_check_mac_version_valid(tp);
> +	if (ret != 0) {

Maybe "if (!ret)" would be more readable?

> +		dev_err(&pdev->dev,
> +			"unknown chip version: 0x%08x, contact rtase maintainers (see MAINTAINERS file)\n",
> +			tp->hw_ver);
> +	}

Also, is it OK to perform further initialization steps although we're
getting an error here? Could you please provide more details in the
commit message?
>  
>  	rtase_init_software_variable(pdev, tp);
>  	rtase_init_hardware(tp);
> -- 
> 2.34.1
> 
> 

Thanks,
Michal
RE: [PATCH net v3 1/4] rtase: Refactor the rtase_check_mac_version_valid() function
Posted by Justin Lai 2 days, 10 hours ago 
> On Mon, Nov 18, 2024 at 12:08:25PM +0800, Justin Lai wrote:
> > 1. Sets tp->hw_ver.
> > 2. Changes the return type from bool to int.
> > 3. Modify the error message for an invalid hardware version id.
> 
> The commit message contains too many implementation details (that are quite
> obvious after studying the code), but there is no information about the actual
> problem the patch is fixing.

Thank you for your feedback. I will make the necessary adjustments to the
commit message.
> 
> >
> > Fixes: a36e9f5cfe9e ("rtase: Add support for a pci table in this
> > module")
> > Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> > ---
> >  drivers/net/ethernet/realtek/rtase/rtase.h    |  2 ++
> >  .../net/ethernet/realtek/rtase/rtase_main.c   | 22 +++++++++++--------
> >  2 files changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/rtase/rtase.h
> > b/drivers/net/ethernet/realtek/rtase/rtase.h
> > index 583c33930f88..547c71937b01 100644
> > --- a/drivers/net/ethernet/realtek/rtase/rtase.h
> > +++ b/drivers/net/ethernet/realtek/rtase/rtase.h
> > @@ -327,6 +327,8 @@ struct rtase_private {
> >       u16 int_nums;
> >       u16 tx_int_mit;
> >       u16 rx_int_mit;
> > +
> > +     u32 hw_ver;
> >  };
> >
> >  #define RTASE_LSO_64K 64000
> > diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > index f8777b7663d3..0c19c5645d53 100644
> > --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > @@ -1972,20 +1972,21 @@ static void rtase_init_software_variable(struct
> pci_dev *pdev,
> >       tp->dev->max_mtu = RTASE_MAX_JUMBO_SIZE;  }
> >
> > -static bool rtase_check_mac_version_valid(struct rtase_private *tp)
> > +static int rtase_check_mac_version_valid(struct rtase_private *tp)
> >  {
> > -     u32 hw_ver = rtase_r32(tp, RTASE_TX_CONFIG_0) &
> RTASE_HW_VER_MASK;
> > -     bool known_ver = false;
> > +     int ret = -ENODEV;
> >
> > -     switch (hw_ver) {
> > +     tp->hw_ver = rtase_r32(tp, RTASE_TX_CONFIG_0) &
> > + RTASE_HW_VER_MASK;
> > +
> > +     switch (tp->hw_ver) {
> >       case 0x00800000:
> >       case 0x04000000:
> >       case 0x04800000:
> > -             known_ver = true;
> > +             ret = 0;
> >               break;
> >       }
> >
> > -     return known_ver;
> > +     return ret;
> >  }
> >
> >  static int rtase_init_board(struct pci_dev *pdev, struct net_device
> > **dev_out, @@ -2105,9 +2106,12 @@ static int rtase_init_one(struct
> pci_dev *pdev,
> >       tp->pdev = pdev;
> >
> >       /* identify chip attached to board */
> > -     if (!rtase_check_mac_version_valid(tp))
> > -             return dev_err_probe(&pdev->dev, -ENODEV,
> > -                                  "unknown chip version, contact
> rtase maintainers (see MAINTAINERS file)\n");
> > +     ret = rtase_check_mac_version_valid(tp);
> > +     if (ret != 0) {
> 
> Maybe "if (!ret)" would be more readable?

Since this function has several instances of this issue that need to be
addressed, I will create a separate patch to make the necessary
corrections.
> 
> > +             dev_err(&pdev->dev,
> > +                     "unknown chip version: 0x%08x, contact rtase
> maintainers (see MAINTAINERS file)\n",
> > +                     tp->hw_ver);
> > +     }
> 
> Also, is it OK to perform further initialization steps although we're getting an
> error here? Could you please provide more details in the commit message?

In [PATCH net v3 3/4] rtase: Corrects error handling of the
rtase_check_mac_version_valid(), it can be observed how errors are
handled in case of an issue.
> >
> >       rtase_init_software_variable(pdev, tp);
> >       rtase_init_hardware(tp);
> > --
> > 2.34.1
> >
> >
> 
> Thanks,
> Michal

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.