All of lore.kernel.org
 help / color / mirror / Atom feed
* igc: fix link speed advertising
@ 2020-11-17 19:50 ` Corinna Vinschen
  0 siblings, 0 replies; 8+ messages in thread
From: Corinna Vinschen @ 2020-11-17 19:50 UTC (permalink / raw)
  To: intel-wired-lan, netdev; +Cc: darcari

Link speed advertising in igc has two problems:

- When setting the advertisement via ethtool, the link speed is converted
  to the legacy 32 bit representation for the intel PHY code.
  This inadvertently drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT (being
  beyond bit 31).  As a result, any call to `ethtool -s ...' drops the
  2500Mbit/s link speed from the PHY settings.  Only reloading the driver
  alleviates that problem.

  Fix this by converting the ETHTOOL_LINK_MODE_2500baseT_Full_BIT to the
  Intel PHY ADVERTISE_2500_FULL bit explicitely.

- Rather than checking the actual PHY setting, the .get_link_ksettings
  function always fills link_modes.advertising with all link speeds
  the device is capable of.

  Fix this by checking the PHY autoneg_advertised settings and report
  only the actually advertised speeds up to ethtool.

Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 24 +++++++++++++++-----
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 61d331ce38cd..75cb4ca36bac 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1675,12 +1675,18 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
 	cmd->base.phy_address = hw->phy.addr;
 
 	/* advertising link modes */
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Half);
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Full);
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Half);
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Full);
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, 1000baseT_Full);
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full);
+	if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF)
+		ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Half);
+	if (hw->phy.autoneg_advertised & ADVERTISE_10_FULL)
+		ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Full);
+	if (hw->phy.autoneg_advertised & ADVERTISE_100_HALF)
+		ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Half);
+	if (hw->phy.autoneg_advertised & ADVERTISE_100_FULL)
+		ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Full);
+	if (hw->phy.autoneg_advertised & ADVERTISE_1000_FULL)
+		ethtool_link_ksettings_add_link_mode(cmd, advertising, 1000baseT_Full);
+	if (hw->phy.autoneg_advertised & ADVERTISE_2500_FULL)
+		ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full);
 
 	/* set autoneg settings */
 	if (hw->mac.autoneg == 1) {
@@ -1792,6 +1798,12 @@ igc_ethtool_set_link_ksettings(struct net_device *netdev,
 
 	ethtool_convert_link_mode_to_legacy_u32(&advertising,
 						cmd->link_modes.advertising);
+	/* Converting to legacy u32 drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT.
+	 * We have to check this and convert it to ADVERTISE_2500_FULL
+	 * (aka ETHTOOL_LINK_MODE_2500baseX_Full_BIT) explicitely.
+	 */
+	if (ethtool_link_ksettings_test_link_mode(cmd, advertising, 2500baseT_Full))
+		advertising |= ADVERTISE_2500_FULL;
 
 	if (cmd->base.autoneg == AUTONEG_ENABLE) {
 		hw->mac.autoneg = 1;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [Intel-wired-lan] igc: fix link speed advertising
@ 2020-11-17 19:50 ` Corinna Vinschen
  0 siblings, 0 replies; 8+ messages in thread
From: Corinna Vinschen @ 2020-11-17 19:50 UTC (permalink / raw)
  To: intel-wired-lan

Link speed advertising in igc has two problems:

- When setting the advertisement via ethtool, the link speed is converted
  to the legacy 32 bit representation for the intel PHY code.
  This inadvertently drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT (being
  beyond bit 31).  As a result, any call to `ethtool -s ...' drops the
  2500Mbit/s link speed from the PHY settings.  Only reloading the driver
  alleviates that problem.

  Fix this by converting the ETHTOOL_LINK_MODE_2500baseT_Full_BIT to the
  Intel PHY ADVERTISE_2500_FULL bit explicitely.

- Rather than checking the actual PHY setting, the .get_link_ksettings
  function always fills link_modes.advertising with all link speeds
  the device is capable of.

  Fix this by checking the PHY autoneg_advertised settings and report
  only the actually advertised speeds up to ethtool.

Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 24 +++++++++++++++-----
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 61d331ce38cd..75cb4ca36bac 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1675,12 +1675,18 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
 	cmd->base.phy_address = hw->phy.addr;
 
 	/* advertising link modes */
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Half);
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Full);
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Half);
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Full);
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, 1000baseT_Full);
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full);
+	if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF)
+		ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Half);
+	if (hw->phy.autoneg_advertised & ADVERTISE_10_FULL)
+		ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Full);
+	if (hw->phy.autoneg_advertised & ADVERTISE_100_HALF)
+		ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Half);
+	if (hw->phy.autoneg_advertised & ADVERTISE_100_FULL)
+		ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Full);
+	if (hw->phy.autoneg_advertised & ADVERTISE_1000_FULL)
+		ethtool_link_ksettings_add_link_mode(cmd, advertising, 1000baseT_Full);
+	if (hw->phy.autoneg_advertised & ADVERTISE_2500_FULL)
+		ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full);
 
 	/* set autoneg settings */
 	if (hw->mac.autoneg == 1) {
@@ -1792,6 +1798,12 @@ igc_ethtool_set_link_ksettings(struct net_device *netdev,
 
 	ethtool_convert_link_mode_to_legacy_u32(&advertising,
 						cmd->link_modes.advertising);
+	/* Converting to legacy u32 drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT.
+	 * We have to check this and convert it to ADVERTISE_2500_FULL
+	 * (aka ETHTOOL_LINK_MODE_2500baseX_Full_BIT) explicitely.
+	 */
+	if (ethtool_link_ksettings_test_link_mode(cmd, advertising, 2500baseT_Full))
+		advertising |= ADVERTISE_2500_FULL;
 
 	if (cmd->base.autoneg == AUTONEG_ENABLE) {
 		hw->mac.autoneg = 1;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [Intel-wired-lan] igc: fix link speed advertising
  2020-11-17 19:50 ` [Intel-wired-lan] " Corinna Vinschen
@ 2021-01-26 10:30   ` Corinna Vinschen
  -1 siblings, 0 replies; 8+ messages in thread
From: Corinna Vinschen @ 2021-01-26 10:30 UTC (permalink / raw)
  To: intel-wired-lan, netdev; +Cc: sasha.neftin

Ping?

It looks like this patch got lost somehow.  Without this patch,
setting link speed advertising is broken.


Thanks,
Corinna


On Nov 17 20:50, Corinna Vinschen wrote:
> Link speed advertising in igc has two problems:
> 
> - When setting the advertisement via ethtool, the link speed is converted
>   to the legacy 32 bit representation for the intel PHY code.
>   This inadvertently drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT (being
>   beyond bit 31).  As a result, any call to `ethtool -s ...' drops the
>   2500Mbit/s link speed from the PHY settings.  Only reloading the driver
>   alleviates that problem.
> 
>   Fix this by converting the ETHTOOL_LINK_MODE_2500baseT_Full_BIT to the
>   Intel PHY ADVERTISE_2500_FULL bit explicitely.
> 
> - Rather than checking the actual PHY setting, the .get_link_ksettings
>   function always fills link_modes.advertising with all link speeds
>   the device is capable of.
> 
>   Fix this by checking the PHY autoneg_advertised settings and report
>   only the actually advertised speeds up to ethtool.
> 
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 24 +++++++++++++++-----
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 61d331ce38cd..75cb4ca36bac 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -1675,12 +1675,18 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
>  	cmd->base.phy_address = hw->phy.addr;
>  
>  	/* advertising link modes */
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Half);
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Full);
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Half);
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Full);
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, 1000baseT_Full);
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full);
> +	if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF)
> +		ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Half);
> +	if (hw->phy.autoneg_advertised & ADVERTISE_10_FULL)
> +		ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Full);
> +	if (hw->phy.autoneg_advertised & ADVERTISE_100_HALF)
> +		ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Half);
> +	if (hw->phy.autoneg_advertised & ADVERTISE_100_FULL)
> +		ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Full);
> +	if (hw->phy.autoneg_advertised & ADVERTISE_1000_FULL)
> +		ethtool_link_ksettings_add_link_mode(cmd, advertising, 1000baseT_Full);
> +	if (hw->phy.autoneg_advertised & ADVERTISE_2500_FULL)
> +		ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full);
>  
>  	/* set autoneg settings */
>  	if (hw->mac.autoneg == 1) {
> @@ -1792,6 +1798,12 @@ igc_ethtool_set_link_ksettings(struct net_device *netdev,
>  
>  	ethtool_convert_link_mode_to_legacy_u32(&advertising,
>  						cmd->link_modes.advertising);
> +	/* Converting to legacy u32 drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT.
> +	 * We have to check this and convert it to ADVERTISE_2500_FULL
> +	 * (aka ETHTOOL_LINK_MODE_2500baseX_Full_BIT) explicitely.
> +	 */
> +	if (ethtool_link_ksettings_test_link_mode(cmd, advertising, 2500baseT_Full))
> +		advertising |= ADVERTISE_2500_FULL;
>  
>  	if (cmd->base.autoneg == AUTONEG_ENABLE) {
>  		hw->mac.autoneg = 1;
> -- 
> 2.27.0
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan


^ permalink raw reply	[flat|nested] 8+ messages in thread
* [Intel-wired-lan] igc: fix link speed advertising
@ 2021-01-26 10:30   ` Corinna Vinschen
  0 siblings, 0 replies; 8+ messages in thread
From: Corinna Vinschen @ 2021-01-26 10:30 UTC (permalink / raw)
  To: intel-wired-lan

Ping?

It looks like this patch got lost somehow.  Without this patch,
setting link speed advertising is broken.


Thanks,
Corinna


On Nov 17 20:50, Corinna Vinschen wrote:
> Link speed advertising in igc has two problems:
> 
> - When setting the advertisement via ethtool, the link speed is converted
>   to the legacy 32 bit representation for the intel PHY code.
>   This inadvertently drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT (being
>   beyond bit 31).  As a result, any call to `ethtool -s ...' drops the
>   2500Mbit/s link speed from the PHY settings.  Only reloading the driver
>   alleviates that problem.
> 
>   Fix this by converting the ETHTOOL_LINK_MODE_2500baseT_Full_BIT to the
>   Intel PHY ADVERTISE_2500_FULL bit explicitely.
> 
> - Rather than checking the actual PHY setting, the .get_link_ksettings
>   function always fills link_modes.advertising with all link speeds
>   the device is capable of.
> 
>   Fix this by checking the PHY autoneg_advertised settings and report
>   only the actually advertised speeds up to ethtool.
> 
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 24 +++++++++++++++-----
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 61d331ce38cd..75cb4ca36bac 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -1675,12 +1675,18 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
>  	cmd->base.phy_address = hw->phy.addr;
>  
>  	/* advertising link modes */
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Half);
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Full);
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Half);
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Full);
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, 1000baseT_Full);
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full);
> +	if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF)
> +		ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Half);
> +	if (hw->phy.autoneg_advertised & ADVERTISE_10_FULL)
> +		ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Full);
> +	if (hw->phy.autoneg_advertised & ADVERTISE_100_HALF)
> +		ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Half);
> +	if (hw->phy.autoneg_advertised & ADVERTISE_100_FULL)
> +		ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Full);
> +	if (hw->phy.autoneg_advertised & ADVERTISE_1000_FULL)
> +		ethtool_link_ksettings_add_link_mode(cmd, advertising, 1000baseT_Full);
> +	if (hw->phy.autoneg_advertised & ADVERTISE_2500_FULL)
> +		ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full);
>  
>  	/* set autoneg settings */
>  	if (hw->mac.autoneg == 1) {
> @@ -1792,6 +1798,12 @@ igc_ethtool_set_link_ksettings(struct net_device *netdev,
>  
>  	ethtool_convert_link_mode_to_legacy_u32(&advertising,
>  						cmd->link_modes.advertising);
> +	/* Converting to legacy u32 drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT.
> +	 * We have to check this and convert it to ADVERTISE_2500_FULL
> +	 * (aka ETHTOOL_LINK_MODE_2500baseX_Full_BIT) explicitely.
> +	 */
> +	if (ethtool_link_ksettings_test_link_mode(cmd, advertising, 2500baseT_Full))
> +		advertising |= ADVERTISE_2500_FULL;
>  
>  	if (cmd->base.autoneg == AUTONEG_ENABLE) {
>  		hw->mac.autoneg = 1;
> -- 
> 2.27.0
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [Intel-wired-lan] igc: fix link speed advertising
  2021-01-26 10:30   ` Corinna Vinschen
@ 2021-01-26 20:00     ` Jakub Kicinski
  -1 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2021-01-26 20:00 UTC (permalink / raw)
  To: Corinna Vinschen, Tony Nguyen, Jesse Brandeburg
  Cc: intel-wired-lan, netdev, sasha.neftin

On Tue, 26 Jan 2021 11:30:37 +0100 Corinna Vinschen wrote:
> Ping?
> 
> It looks like this patch got lost somehow.  Without this patch,
> setting link speed advertising is broken.

Adding Intel maintainers.

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [Intel-wired-lan] igc: fix link speed advertising
@ 2021-01-26 20:00     ` Jakub Kicinski
  0 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2021-01-26 20:00 UTC (permalink / raw)
  To: intel-wired-lan

On Tue, 26 Jan 2021 11:30:37 +0100 Corinna Vinschen wrote:
> Ping?
> 
> It looks like this patch got lost somehow.  Without this patch,
> setting link speed advertising is broken.

Adding Intel maintainers.

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [Intel-wired-lan] igc: fix link speed advertising
  2021-01-26 20:00     ` Jakub Kicinski
@ 2021-01-26 21:31       ` Nguyen, Anthony L
  -1 siblings, 0 replies; 8+ messages in thread
From: Nguyen, Anthony L @ 2021-01-26 21:31 UTC (permalink / raw)
  To: kuba, Brandeburg, Jesse, vinschen; +Cc: netdev, intel-wired-lan, Neftin, Sasha

On Tue, 2021-01-26 at 12:00 -0800, Jakub Kicinski wrote:
> On Tue, 26 Jan 2021 11:30:37 +0100 Corinna Vinschen wrote:
> > Ping?
> > 
> > It looks like this patch got lost somehow.  Without this patch,
> > setting link speed advertising is broken.
> 
> Adding Intel maintainers.

I talked to the team and they won't be able to get to testing this
patch soon. We have reviewed the code and everything looks correct so
I'll submit it in my pull request today.

Thanks,
Tony

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [Intel-wired-lan] igc: fix link speed advertising
@ 2021-01-26 21:31       ` Nguyen, Anthony L
  0 siblings, 0 replies; 8+ messages in thread
From: Nguyen, Anthony L @ 2021-01-26 21:31 UTC (permalink / raw)
  To: intel-wired-lan

On Tue, 2021-01-26 at 12:00 -0800, Jakub Kicinski wrote:
> On Tue, 26 Jan 2021 11:30:37 +0100 Corinna Vinschen wrote:
> > Ping?
> > 
> > It looks like this patch got lost somehow.  Without this patch,
> > setting link speed advertising is broken.
> 
> Adding Intel maintainers.

I talked to the team and they won't be able to get to testing this
patch soon. We have reviewed the code and everything looks correct so
I'll submit it in my pull request today.

Thanks,
Tony

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-27  8:42 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-17 19:50 igc: fix link speed advertising Corinna Vinschen
2020-11-17 19:50 ` [Intel-wired-lan] " Corinna Vinschen
2021-01-26 10:30 ` Corinna Vinschen
2021-01-26 10:30   ` Corinna Vinschen
2021-01-26 20:00   ` Jakub Kicinski
2021-01-26 20:00     ` Jakub Kicinski
2021-01-26 21:31     ` Nguyen, Anthony L
2021-01-26 21:31       ` Nguyen, Anthony L

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.