| Message ID | 20241118040828.454861-3-justinlai0215@realtek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Correcting switch hardware versions and reported speeds | expand |
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
> > 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
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
> > 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
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;
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(-)