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; 10+ 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] 10+ messages in thread
* [Intel-wired-lan] igc: fix link speed advertising
@ 2020-11-17 19:50 ` Corinna Vinschen
  0 siblings, 0 replies; 10+ 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] 10+ 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; 10+ 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] 10+ messages in thread
* [Intel-wired-lan] igc: fix link speed advertising
@ 2021-01-26 10:30   ` Corinna Vinschen
  0 siblings, 0 replies; 10+ 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] 10+ 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; 10+ 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] 10+ messages in thread
* [Intel-wired-lan] igc: fix link speed advertising
@ 2021-01-26 20:00     ` Jakub Kicinski
  0 siblings, 0 replies; 10+ 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] 10+ 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; 10+ 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] 10+ messages in thread
* [Intel-wired-lan] igc: fix link speed advertising
@ 2021-01-26 21:31       ` Nguyen, Anthony L
  0 siblings, 0 replies; 10+ 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] 10+ messages in thread
* [Intel-wired-lan] igc: fix link speed advertising
       [not found] ` <79cb04f0-1a93-f463-3131-ad7772eab202@intel.com>
  2021-02-04 10:05   ` Dvora Fuxbrumer
@ 2021-02-04 10:14   ` Dvora Fuxbrumer
  1 sibling, 0 replies; 10+ messages in thread
From: Dvora Fuxbrumer @ 2021-02-04 10:14 UTC (permalink / raw)
  To: intel-wired-lan

> 
> 
> -------- Forwarded Message --------
> Subject: Re: [Fwd: igc: fix link speed advertising]
> Date: Wed, 27 Jan 2021 10:24:21 +0100
> From: Corinna Vinschen <vinschen@redhat.com>
> To: Yang, Lihong <lihong.yang@intel.com>
> CC: Pathi, Pragyansri <pragyansri.pathi@intel.com>, Don Bayly 
> <dbayly@redhat.com>, Neftin, Sasha <sasha.neftin@intel.com>, Nguyen, 
> Anthony L <anthony.l.nguyen@intel.com>
> 
> Hi Lihong,
> Hi Tony,
> 
> 
> Thanks a lot to both of you!
> 
> 
> Corinna
> 
> 
> On Jan 26 23:58, Yang, Lihong wrote:
>> Hi Corinna,
>>
>> A quick update. Your patch[1] was just sent by Tony to net as part of 
>> the bug-fix series. Hopefully it will be accepted soon.
>> Thanks,
>> Lihong
>>
>> [1] 
>> https://patchwork.kernel.org/project/netdevbpf/patch/20210126221035.658124-8-anthony.l.nguyen at intel.com/ 
>>
>>
>>
>> > -----Original Message-----
>> > From: Yang, Lihong
>> > Sent: Tuesday, January 26, 2021 11:02 AM
>> > To: Corinna Vinschen <vinschen@redhat.com>
>> > Cc: Pathi, Pragyansri <pragyansri.pathi@intel.com>; Don Bayly
>> > <dbayly@redhat.com>; Neftin, Sasha <sasha.neftin@intel.com>; Nguyen,
>> > Anthony L <anthony.l.nguyen@intel.com>
>> > Subject: RE: [Fwd: igc: fix link speed advertising]
>> > > Hi Corinna,
>> > > From the info I gathered, the reason your patch has not been sent 
>> Linux
>> > upstream is because it has not been validated from Intel side. Once 
>> it is
>> > tagged with a Tested-by, it should be ready to go up.
>> > > I am adding Sasha from the igc driver team to be aware of your 
>> inquiry,
>> > and Tony our Networking maintainer as an FYI.
>> > > Thanks,
>> > Lihong
>> > > > -----Original Message-----
>> > > From: Corinna Vinschen <vinschen@redhat.com>
>> > > Sent: Tuesday, January 26, 2021 02:37 AM
>> > > To: Yang, Lihong <lihong.yang@intel.com>
>> > > Subject: [Fwd: igc: fix link speed advertising]
>> > >
>> > > Hi Lihong,
>> > >
>> > > I sent this patch to the Intel-wired-lan and netdev mailing lists
>> > > in November already.? I got no reply but it looked like it had been
>> > > included into the test runs from the "kernel test robot" output.
>> > >
>> > > So I expected this is going upstream for the next kernel version, 
>> but it
>> > > didn't make it into net-next yet.? I was hoping this can go into RHEL
>> > > 8.4, but it's getting really late now.
>> > >
>> > > I pinged the mailing lists again, but, would you mind to check
>> > > internally, if there's some reason not to include it?
>> > >
>> > >
>> > > Thanks a lot!
>> > > Corinna
>> > >
>> > >
>> > > ----- Forwarded message from Corinna Vinschen 
>> <vinschen@redhat.com> ----
>> > -
>> > >
>> > > > Date: Tue, 17 Nov 2020 20:50:40 +0100
>> > > > From: Corinna Vinschen <vinschen@redhat.com>
>> > > > To: intel-wired-lan at lists.osuosl.org, netdev at vger.kernel.org
>> > > > Subject: [Intel-wired-lan] igc: fix link speed advertising
>> > > >
>> > > > 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
>> > >
>> > > ----- End forwarded message -----
>>
> 
Tested-by: Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [Intel-wired-lan] igc: fix link speed advertising
       [not found] ` <79cb04f0-1a93-f463-3131-ad7772eab202@intel.com>
@ 2021-02-04 10:05   ` Dvora Fuxbrumer
  2021-02-04 10:14   ` Dvora Fuxbrumer
  1 sibling, 0 replies; 10+ messages in thread
From: Dvora Fuxbrumer @ 2021-02-04 10:05 UTC (permalink / raw)
  To: intel-wired-lan

> 
> 
> 
> -------- Forwarded Message --------
> Subject: Re: [Fwd: igc: fix link speed advertising]
> Date: Wed, 27 Jan 2021 10:24:21 +0100
> From: Corinna Vinschen <vinschen@redhat.com>
> To: Yang, Lihong <lihong.yang@intel.com>
> CC: Pathi, Pragyansri <pragyansri.pathi@intel.com>, Don Bayly 
> <dbayly@redhat.com>, Neftin, Sasha <sasha.neftin@intel.com>, Nguyen, 
> Anthony L <anthony.l.nguyen@intel.com>
> 
> Hi Lihong,
> Hi Tony,
> 
> 
> Thanks a lot to both of you!
> 
> 
> Corinna
> 
> 
> On Jan 26 23:58, Yang, Lihong wrote:
>> Hi Corinna,
>>
>> A quick update. Your patch[1] was just sent by Tony to net as part of 
>> the bug-fix series. Hopefully it will be accepted soon.
>> Thanks,
>> Lihong
>>
>> [1] 
>> https://patchwork.kernel.org/project/netdevbpf/patch/20210126221035.658124-8-anthony.l.nguyen at intel.com/ 
>>
>>
>>
>> > -----Original Message-----
>> > From: Yang, Lihong
>> > Sent: Tuesday, January 26, 2021 11:02 AM
>> > To: Corinna Vinschen <vinschen@redhat.com>
>> > Cc: Pathi, Pragyansri <pragyansri.pathi@intel.com>; Don Bayly
>> > <dbayly@redhat.com>; Neftin, Sasha <sasha.neftin@intel.com>; Nguyen,
>> > Anthony L <anthony.l.nguyen@intel.com>
>> > Subject: RE: [Fwd: igc: fix link speed advertising]
>> > > Hi Corinna,
>> > > From the info I gathered, the reason your patch has not been sent 
>> Linux
>> > upstream is because it has not been validated from Intel side. Once 
>> it is
>> > tagged with a Tested-by, it should be ready to go up.
>> > > I am adding Sasha from the igc driver team to be aware of your 
>> inquiry,
>> > and Tony our Networking maintainer as an FYI.
>> > > Thanks,
>> > Lihong
>> > > > -----Original Message-----
>> > > From: Corinna Vinschen <vinschen@redhat.com>
>> > > Sent: Tuesday, January 26, 2021 02:37 AM
>> > > To: Yang, Lihong <lihong.yang@intel.com>
>> > > Subject: [Fwd: igc: fix link speed advertising]
>> > >
>> > > Hi Lihong,
>> > >
>> > > I sent this patch to the Intel-wired-lan and netdev mailing lists
>> > > in November already.? I got no reply but it looked like it had been
>> > > included into the test runs from the "kernel test robot" output.
>> > >
>> > > So I expected this is going upstream for the next kernel version, 
>> but it
>> > > didn't make it into net-next yet.? I was hoping this can go into RHEL
>> > > 8.4, but it's getting really late now.
>> > >
>> > > I pinged the mailing lists again, but, would you mind to check
>> > > internally, if there's some reason not to include it?
>> > >
>> > >
>> > > Thanks a lot!
>> > > Corinna
>> > >
>> > >
>> > > ----- Forwarded message from Corinna Vinschen 
>> <vinschen@redhat.com> ----
>> > -
>> > >
>> > > > Date: Tue, 17 Nov 2020 20:50:40 +0100
>> > > > From: Corinna Vinschen <vinschen@redhat.com>
>> > > > To: intel-wired-lan at lists.osuosl.org, netdev at vger.kernel.org
>> > > > Subject: [Intel-wired-lan] igc: fix link speed advertising
>> > > >
>> > > > 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
>> > >
>> > > ----- End forwarded message -----
>>
> 
Tested-by: Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-02-04 10:14 UTC | newest]

Thread overview: 10+ 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
     [not found] <20210127092421.GS4393@calimero.vinschen.de>
     [not found] ` <79cb04f0-1a93-f463-3131-ad7772eab202@intel.com>
2021-02-04 10:05   ` Dvora Fuxbrumer
2021-02-04 10:14   ` Dvora Fuxbrumer

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.