[PATCH net v3 0/4] Correcting switch hardware versions and reported speeds

[PATCH net v3 0/4] Correcting switch hardware versions and reported speeds

[Justin Lai]

This patch set mainly involves correcting switch hardware versions and
reported speeds.
Details are as follows:
1. Refactor the rtase_check_mac_version_valid() function.
2. Correct the speed for RTL907XD-V1
3. Corrects error handling of the rtase_check_mac_version_valid()
4. Add defines for hardware version id

v1 -> v2:
- Add Fixes: tag.
- Add defines for hardware version id.
- Modify the error message for an invalid hardware version ID.

v2 -> v3:
- Remove the patch "Add support for RTL907XD-VA PCIe port".

Justin Lai (4):
  rtase: Refactor the rtase_check_mac_version_valid() function
  rtase: Correct the speed for RTL907XD-V1
  rtase: Corrects error handling of the rtase_check_mac_version_valid()
  rtase: Add defines for hardware version id

 drivers/net/ethernet/realtek/rtase/rtase.h    |  7 ++-
 .../net/ethernet/realtek/rtase/rtase_main.c   | 43 +++++++++++++------
 2 files changed, 36 insertions(+), 14 deletions(-)

-- 
2.34.1

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

[Justin Lai]

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

[PATCH net v3 2/4] rtase: Correct the speed for RTL907XD-V1

[Justin Lai]

Correct the speed for RTL907XD-V1.

Fixes: dd7f17c40fd1 ("rtase: Implement ethtool function")
Signed-off-by: Justin Lai <justinlai0215@realtek.com>
---
 drivers/net/ethernet/realtek/rtase/rtase_main.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c
index 0c19c5645d53..5b8012987ea6 100644
--- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
+++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
@@ -1714,10 +1714,21 @@ static int rtase_get_settings(struct net_device *dev,
 			      struct ethtool_link_ksettings *cmd)
 {
 	u32 supported = SUPPORTED_MII | SUPPORTED_Pause | SUPPORTED_Asym_Pause;
+	const struct rtase_private *tp = netdev_priv(dev);
 
 	ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported,
 						supported);
-	cmd->base.speed = SPEED_5000;
+
+	switch (tp->hw_ver) {
+	case 0x00800000:
+	case 0x04000000:
+		cmd->base.speed = SPEED_5000;
+		break;
+	case 0x04800000:
+		cmd->base.speed = SPEED_10000;
+		break;
+	}
+
 	cmd->base.duplex = DUPLEX_FULL;
 	cmd->base.port = PORT_MII;
 	cmd->base.autoneg = AUTONEG_DISABLE;
-- 
2.34.1

[PATCH net v3 3/4] rtase: Corrects error handling of the rtase_check_mac_version_valid()

[Justin Lai]

Corrects error handling of the rtase_check_mac_version_valid().

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_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c
index 5b8012987ea6..26331a2b7b2d 100644
--- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
+++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
@@ -2122,6 +2122,7 @@ static int rtase_init_one(struct pci_dev *pdev,
 		dev_err(&pdev->dev,
 			"unknown chip version: 0x%08x, contact rtase maintainers (see MAINTAINERS file)\n",
 			tp->hw_ver);
+		goto err_out_release_board;
 	}
 
 	rtase_init_software_variable(pdev, tp);
@@ -2196,6 +2197,7 @@ static int rtase_init_one(struct pci_dev *pdev,
 		netif_napi_del(&ivec->napi);
 	}
 
+err_out_release_board:
 	rtase_release_board(pdev, dev, ioaddr);
 
 	return ret;
-- 
2.34.1

[PATCH net v3 4/4] rtase: Add defines for hardware version id

[Justin Lai]

Add defines for hardware version id.

Signed-off-by: Justin Lai <justinlai0215@realtek.com>
---
 drivers/net/ethernet/realtek/rtase/rtase.h      |  5 ++++-
 drivers/net/ethernet/realtek/rtase/rtase_main.c | 12 ++++++------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/realtek/rtase/rtase.h b/drivers/net/ethernet/realtek/rtase/rtase.h
index 547c71937b01..4a4434869b10 100644
--- a/drivers/net/ethernet/realtek/rtase/rtase.h
+++ b/drivers/net/ethernet/realtek/rtase/rtase.h
@@ -9,7 +9,10 @@
 #ifndef RTASE_H
 #define RTASE_H
 
-#define RTASE_HW_VER_MASK 0x7C800000
+#define RTASE_HW_VER_MASK     0x7C800000
+#define RTASE_HW_VER_906X_7XA 0x00800000
+#define RTASE_HW_VER_906X_7XC 0x04000000
+#define RTASE_HW_VER_907XD_V1 0x04800000
 
 #define RTASE_RX_DMA_BURST_256       4
 #define RTASE_TX_DMA_BURST_UNLIMITED 7
diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c
index 26331a2b7b2d..1bfe5ef40c52 100644
--- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
+++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
@@ -1720,11 +1720,11 @@ static int rtase_get_settings(struct net_device *dev,
 						supported);
 
 	switch (tp->hw_ver) {
-	case 0x00800000:
-	case 0x04000000:
+	case RTASE_HW_VER_906X_7XA:
+	case RTASE_HW_VER_906X_7XC:
 		cmd->base.speed = SPEED_5000;
 		break;
-	case 0x04800000:
+	case RTASE_HW_VER_907XD_V1:
 		cmd->base.speed = SPEED_10000;
 		break;
 	}
@@ -1990,9 +1990,9 @@ static int rtase_check_mac_version_valid(struct rtase_private *tp)
 	tp->hw_ver = rtase_r32(tp, RTASE_TX_CONFIG_0) & RTASE_HW_VER_MASK;
 
 	switch (tp->hw_ver) {
-	case 0x00800000:
-	case 0x04000000:
-	case 0x04800000:
+	case RTASE_HW_VER_906X_7XA:
+	case RTASE_HW_VER_906X_7XC:
+	case RTASE_HW_VER_907XD_V1:
 		ret = 0;
 		break;
 	}
-- 
2.34.1

Re: [PATCH net v3 4/4] rtase: Add defines for hardware version id

[Michal Kubiak]

On Mon, Nov 18, 2024 at 12:08:28PM +0800, Justin Lai wrote:
> Add defines for hardware version id.
> 
> Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> ---
>  drivers/net/ethernet/realtek/rtase/rtase.h      |  5 ++++-
>  drivers/net/ethernet/realtek/rtase/rtase_main.c | 12 ++++++------
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 

The patch is addressed to the "net" tree, but "Fixes" tag is missing.
Also, the patch does not look like a bugfix it's rather an improvement
of coding style with no functional changes. That's why I doubt it should go
to the "net" tree.

Thanks,
Michal

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

[Michal Kubiak]

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 2/4] rtase: Correct the speed for RTL907XD-V1

[Michal Kubiak]

On Mon, Nov 18, 2024 at 12:08:26PM +0800, Justin Lai wrote:
> Correct the speed for RTL907XD-V1.
> 

Please add more details about the problem the patch is fixing.

> Fixes: dd7f17c40fd1 ("rtase: Implement ethtool function")
> Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> ---
>  drivers/net/ethernet/realtek/rtase/rtase_main.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> index 0c19c5645d53..5b8012987ea6 100644
> --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> @@ -1714,10 +1714,21 @@ static int rtase_get_settings(struct net_device *dev,
>  			      struct ethtool_link_ksettings *cmd)
>  {
>  	u32 supported = SUPPORTED_MII | SUPPORTED_Pause | SUPPORTED_Asym_Pause;
> +	const struct rtase_private *tp = netdev_priv(dev);
>  
>  	ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported,
>  						supported);
> -	cmd->base.speed = SPEED_5000;
> +
> +	switch (tp->hw_ver) {
> +	case 0x00800000:
> +	case 0x04000000:
> +		cmd->base.speed = SPEED_5000;
> +		break;
> +	case 0x04800000:
> +		cmd->base.speed = SPEED_10000;
> +		break;
> +	}
> +

Above you are adding the code introducing some magic numbers and in your
last patch you are refactoring that newly added code.
Would it be possible to avoid those intermediate results and prepare the
final version of the fix in the series?

>  	cmd->base.duplex = DUPLEX_FULL;
>  	cmd->base.port = PORT_MII;
>  	cmd->base.autoneg = AUTONEG_DISABLE;
> -- 
> 2.34.1
> 
> 

Thanks,
Michal

RE: [PATCH net v3 4/4] rtase: Add defines for hardware version id

[Justin Lai]

> 
> On Mon, Nov 18, 2024 at 12:08:28PM +0800, Justin Lai wrote:
> > Add defines for hardware version id.
> >
> > Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> > ---
> >  drivers/net/ethernet/realtek/rtase/rtase.h      |  5 ++++-
> >  drivers/net/ethernet/realtek/rtase/rtase_main.c | 12 ++++++------
> >  2 files changed, 10 insertions(+), 7 deletions(-)
> >
> 
> The patch is addressed to the "net" tree, but "Fixes" tag is missing.
> Also, the patch does not look like a bugfix it's rather an improvement of coding
> style with no functional changes. That's why I doubt it should go to the "net"
> tree.
> 
> Thanks,
> Michal

Hi Michal,

This patch introduces multiple defines for hardware version IDs, rather
than addressing any issues related to the function logic, which is why it
does not include a "Fixes:" tag. The reason for isolating this change in a
separate patch is to maintain a "single patch, single purpose" approach.
Additionally, these defines will be used in other patches within the same
patch set, which is why they are included in this patch set as a whole.

Justin

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

[Justin Lai]

> 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

RE: [PATCH net v3 2/4] rtase: Correct the speed for RTL907XD-V1

[Justin Lai]

> 
> On Mon, Nov 18, 2024 at 12:08:26PM +0800, Justin Lai wrote:
> > Correct the speed for RTL907XD-V1.
> >
> 
> Please add more details about the problem the patch is fixing.

Ok, I will make the necessary changes to the commit message.
> 
> > Fixes: dd7f17c40fd1 ("rtase: Implement ethtool function")
> > Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> > ---
> >  drivers/net/ethernet/realtek/rtase/rtase_main.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > index 0c19c5645d53..5b8012987ea6 100644
> > --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > @@ -1714,10 +1714,21 @@ static int rtase_get_settings(struct net_device
> *dev,
> >                             struct ethtool_link_ksettings *cmd)  {
> >       u32 supported = SUPPORTED_MII | SUPPORTED_Pause |
> > SUPPORTED_Asym_Pause;
> > +     const struct rtase_private *tp = netdev_priv(dev);
> >
> >
> ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported,
> >                                               supported);
> > -     cmd->base.speed = SPEED_5000;
> > +
> > +     switch (tp->hw_ver) {
> > +     case 0x00800000:
> > +     case 0x04000000:
> > +             cmd->base.speed = SPEED_5000;
> > +             break;
> > +     case 0x04800000:
> > +             cmd->base.speed = SPEED_10000;
> > +             break;
> > +     }
> > +
> 
> Above you are adding the code introducing some magic numbers and in your
> last patch you are refactoring that newly added code.
> Would it be possible to avoid those intermediate results and prepare the final
> version of the fix in the series?

We would still prefer to follow the "single patch, single purpose"
approach for this part. Other reviewers have also provided similar
feedback, so we would like to maintain the current approach.
> 
> >       cmd->base.duplex = DUPLEX_FULL;
> >       cmd->base.port = PORT_MII;
> >       cmd->base.autoneg = AUTONEG_DISABLE;
> > --
> > 2.34.1
> >
> >
> 
> Thanks,
> Michal

Re: [PATCH net v3 4/4] rtase: Add defines for hardware version id

[Michal Kubiak]

On Tue, Nov 19, 2024 at 07:22:46AM +0000, Justin Lai wrote:
> > 
> > On Mon, Nov 18, 2024 at 12:08:28PM +0800, Justin Lai wrote:
> > > Add defines for hardware version id.
> > >
> > > Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> > > ---
> > >  drivers/net/ethernet/realtek/rtase/rtase.h      |  5 ++++-
> > >  drivers/net/ethernet/realtek/rtase/rtase_main.c | 12 ++++++------
> > >  2 files changed, 10 insertions(+), 7 deletions(-)
> > >
> > 
> > The patch is addressed to the "net" tree, but "Fixes" tag is missing.
> > Also, the patch does not look like a bugfix it's rather an improvement of coding
> > style with no functional changes. That's why I doubt it should go to the "net"
> > tree.
> > 
> > Thanks,
> > Michal
> 
> Hi Michal,
> 
> This patch introduces multiple defines for hardware version IDs, rather
> than addressing any issues related to the function logic, which is why it
> does not include a "Fixes:" tag. The reason for isolating this change in a
> separate patch is to maintain a "single patch, single purpose" approach.
> Additionally, these defines will be used in other patches within the same
> patch set, which is why they are included in this patch set as a whole.
> 
> Justin
> 

Hi Justin,

I understand the purpose of the patch, however the patch is addressed
to the "net" tree. Each patch sent to "net" tree should have "Fixes" tag,
because that tree is for fixes only.
You may consider sending the patch to the "net-next", (which is closed
now because of the merge window).

Thanks,
Michal

Re: [PATCH net v3 2/4] rtase: Correct the speed for RTL907XD-V1

[Michal Kubiak]

On Tue, Nov 19, 2024 at 07:23:12AM +0000, Justin Lai wrote:
> > 
> > On Mon, Nov 18, 2024 at 12:08:26PM +0800, Justin Lai wrote:

[...]

> > >
> > >
> > ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported,
> > >                                               supported);
> > > -     cmd->base.speed = SPEED_5000;
> > > +
> > > +     switch (tp->hw_ver) {
> > > +     case 0x00800000:
> > > +     case 0x04000000:
> > > +             cmd->base.speed = SPEED_5000;
> > > +             break;
> > > +     case 0x04800000:
> > > +             cmd->base.speed = SPEED_10000;
> > > +             break;
> > > +     }
> > > +
> > 
> > Above you are adding the code introducing some magic numbers and in your
> > last patch you are refactoring that newly added code.
> > Would it be possible to avoid those intermediate results and prepare the final
> > version of the fix in the series?
> 
> We would still prefer to follow the "single patch, single purpose"
> approach for this part. Other reviewers have also provided similar
> feedback, so we would like to maintain the current approach.
> 

I understand other reviewers' feedback because it's simply hard to
review the series with many intermediate changes in the same code.
Moreover, in this case, those intermediate changes can be easily avoided
by moving the patch #4 to the beginning of the series.
But still - I have doubts if the patch #4 can go into the "net" tree
since it doesn't have any functional fixes.

Thanks,
Michal

RE: [PATCH net v3 4/4] rtase: Add defines for hardware version id

[Justin Lai]

> 
> On Tue, Nov 19, 2024 at 07:22:46AM +0000, Justin Lai wrote:
> > >
> > > On Mon, Nov 18, 2024 at 12:08:28PM +0800, Justin Lai wrote:
> > > > Add defines for hardware version id.
> > > >
> > > > Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> > > > ---
> > > >  drivers/net/ethernet/realtek/rtase/rtase.h      |  5 ++++-
> > > >  drivers/net/ethernet/realtek/rtase/rtase_main.c | 12 ++++++------
> > > >  2 files changed, 10 insertions(+), 7 deletions(-)
> > > >
> > >
> > > The patch is addressed to the "net" tree, but "Fixes" tag is missing.
> > > Also, the patch does not look like a bugfix it's rather an
> > > improvement of coding style with no functional changes. That's why I doubt
> it should go to the "net"
> > > tree.
> > >
> > > Thanks,
> > > Michal
> >
> > Hi Michal,
> >
> > This patch introduces multiple defines for hardware version IDs,
> > rather than addressing any issues related to the function logic, which
> > is why it does not include a "Fixes:" tag. The reason for isolating
> > this change in a separate patch is to maintain a "single patch, single
> purpose" approach.
> > Additionally, these defines will be used in other patches within the
> > same patch set, which is why they are included in this patch set as a whole.
> >
> > Justin
> >
> 
> Hi Justin,
> 
> I understand the purpose of the patch, however the patch is addressed to the
> "net" tree. Each patch sent to "net" tree should have "Fixes" tag, because that
> tree is for fixes only.
> You may consider sending the patch to the "net-next", (which is closed now
> because of the merge window).
> 
> Thanks,
> Michal

Hi Michal,

Thank you for your suggestion. Upon further consideration, I agree
that the addition of the hardware version ID definitions should have been
implemented directly when fixing the related issue. I will make the
necessary changes accordingly.

Justin

RE: [PATCH net v3 2/4] rtase: Correct the speed for RTL907XD-V1

[Justin Lai]

> 
> On Tue, Nov 19, 2024 at 07:23:12AM +0000, Justin Lai wrote:
> > >
> > > On Mon, Nov 18, 2024 at 12:08:26PM +0800, Justin Lai wrote:
> 
> [...]
> 
> > > >
> > > >
> > > ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported,
> > > >                                               supported);
> > > > -     cmd->base.speed = SPEED_5000;
> > > > +
> > > > +     switch (tp->hw_ver) {
> > > > +     case 0x00800000:
> > > > +     case 0x04000000:
> > > > +             cmd->base.speed = SPEED_5000;
> > > > +             break;
> > > > +     case 0x04800000:
> > > > +             cmd->base.speed = SPEED_10000;
> > > > +             break;
> > > > +     }
> > > > +
> > >
> > > Above you are adding the code introducing some magic numbers and in
> > > your last patch you are refactoring that newly added code.
> > > Would it be possible to avoid those intermediate results and prepare
> > > the final version of the fix in the series?
> >
> > We would still prefer to follow the "single patch, single purpose"
> > approach for this part. Other reviewers have also provided similar
> > feedback, so we would like to maintain the current approach.
> >
> 
> I understand other reviewers' feedback because it's simply hard to review the
> series with many intermediate changes in the same code.
> Moreover, in this case, those intermediate changes can be easily avoided by
> moving the patch #4 to the beginning of the series.
> But still - I have doubts if the patch #4 can go into the "net" tree since it doesn't
> have any functional fixes.
> 
> Thanks,
> Michal

Hi Michal,

Thank you for your response. I will integrate the addition of the hardware
version ID definitions into patch #1, as this will address both of your
concerns at once.

Justin