[PATCH net-next 1/1] e1000e: Enable Link Partner Advertised Support

[PATCH net-next 1/1] e1000e: Enable Link Partner Advertised Support

[Tony Nguyen]

From: Jamie Gloudon <jamie.gloudon@gmx.fr>

This enables link partner advertised support to show link modes and
pause frame use.

Signed-off-by: Jamie Gloudon <jamie.gloudon@gmx.fr>
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/e1000e/ethtool.c | 10 +++++++++-
 drivers/net/ethernet/intel/e1000e/phy.c     |  9 +++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index 59e82d131d88..721f86fd5802 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -110,9 +110,9 @@ static const char e1000_gstrings_test[][ETH_GSTRING_LEN] = {
 static int e1000_get_link_ksettings(struct net_device *netdev,
 				    struct ethtool_link_ksettings *cmd)
 {
+	u32 speed, supported, advertising, lp_advertising, lpa_t;
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 	struct e1000_hw *hw = &adapter->hw;
-	u32 speed, supported, advertising;
 
 	if (hw->phy.media_type == e1000_media_type_copper) {
 		supported = (SUPPORTED_10baseT_Half |
@@ -120,7 +120,9 @@ static int e1000_get_link_ksettings(struct net_device *netdev,
 			     SUPPORTED_100baseT_Half |
 			     SUPPORTED_100baseT_Full |
 			     SUPPORTED_1000baseT_Full |
+			     SUPPORTED_Asym_Pause |
 			     SUPPORTED_Autoneg |
+			     SUPPORTED_Pause |
 			     SUPPORTED_TP);
 		if (hw->phy.type == e1000_phy_ife)
 			supported &= ~SUPPORTED_1000baseT_Full;
@@ -192,10 +194,16 @@ static int e1000_get_link_ksettings(struct net_device *netdev,
 	if (hw->phy.media_type != e1000_media_type_copper)
 		cmd->base.eth_tp_mdix_ctrl = ETH_TP_MDI_INVALID;
 
+	lpa_t = mii_stat1000_to_ethtool_lpa_t(adapter->phy_regs.stat1000);
+	lp_advertising = lpa_t |
+	mii_lpa_to_ethtool_lpa_t(adapter->phy_regs.lpa);
+
 	ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported,
 						supported);
 	ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.advertising,
 						advertising);
+	ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.lp_advertising,
+						lp_advertising);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/intel/e1000e/phy.c b/drivers/net/ethernet/intel/e1000e/phy.c
index 060b263348ce..08c3d477dd6f 100644
--- a/drivers/net/ethernet/intel/e1000e/phy.c
+++ b/drivers/net/ethernet/intel/e1000e/phy.c
@@ -2,6 +2,7 @@
 /* Copyright(c) 1999 - 2018 Intel Corporation. */
 
 #include "e1000.h"
+#include <linux/ethtool.h>
 
 static s32 e1000_wait_autoneg(struct e1000_hw *hw);
 static s32 e1000_access_phy_wakeup_reg_bm(struct e1000_hw *hw, u32 offset,
@@ -1011,6 +1012,8 @@ static s32 e1000_phy_setup_autoneg(struct e1000_hw *hw)
 		 */
 		mii_autoneg_adv_reg &=
 		    ~(ADVERTISE_PAUSE_ASYM | ADVERTISE_PAUSE_CAP);
+		phy->autoneg_advertised &=
+		    ~(ADVERTISED_Pause | ADVERTISED_Asym_Pause);
 		break;
 	case e1000_fc_rx_pause:
 		/* Rx Flow control is enabled, and Tx Flow control is
@@ -1024,6 +1027,8 @@ static s32 e1000_phy_setup_autoneg(struct e1000_hw *hw)
 		 */
 		mii_autoneg_adv_reg |=
 		    (ADVERTISE_PAUSE_ASYM | ADVERTISE_PAUSE_CAP);
+		phy->autoneg_advertised |=
+		    (ADVERTISED_Pause | ADVERTISED_Asym_Pause);
 		break;
 	case e1000_fc_tx_pause:
 		/* Tx Flow control is enabled, and Rx Flow control is
@@ -1031,6 +1036,8 @@ static s32 e1000_phy_setup_autoneg(struct e1000_hw *hw)
 		 */
 		mii_autoneg_adv_reg |= ADVERTISE_PAUSE_ASYM;
 		mii_autoneg_adv_reg &= ~ADVERTISE_PAUSE_CAP;
+		phy->autoneg_advertised |= ADVERTISED_Asym_Pause;
+		phy->autoneg_advertised &= ~ADVERTISED_Pause;
 		break;
 	case e1000_fc_full:
 		/* Flow control (both Rx and Tx) is enabled by a software
@@ -1038,6 +1045,8 @@ static s32 e1000_phy_setup_autoneg(struct e1000_hw *hw)
 		 */
 		mii_autoneg_adv_reg |=
 		    (ADVERTISE_PAUSE_ASYM | ADVERTISE_PAUSE_CAP);
+		phy->autoneg_advertised |=
+		    (ADVERTISED_Pause | ADVERTISED_Asym_Pause);
 		break;
 	default:
 		e_dbg("Flow control param set incorrectly\n");
-- 
2.38.1

Re: [PATCH net-next 1/1] e1000e: Enable Link Partner Advertised Support

[Andrew Lunn]

> @@ -192,10 +194,16 @@ static int e1000_get_link_ksettings(struct net_device *netdev,
>  	if (hw->phy.media_type != e1000_media_type_copper)
>  		cmd->base.eth_tp_mdix_ctrl = ETH_TP_MDI_INVALID;
>  
> +	lpa_t = mii_stat1000_to_ethtool_lpa_t(adapter->phy_regs.stat1000);
> +	lp_advertising = lpa_t |
> +	mii_lpa_to_ethtool_lpa_t(adapter->phy_regs.lpa);

The indentation here is a bit odd.

I would also suggest you split this patch into two, since you are
making two changes, adding pause, and adding LPA.

       Andrew

Re: [PATCH net-next 1/1] e1000e: Enable Link Partner Advertised Support

[Andrew Lunn]

> --- a/drivers/net/ethernet/intel/e1000e/phy.c
> +++ b/drivers/net/ethernet/intel/e1000e/phy.c
> @@ -2,6 +2,7 @@
>  /* Copyright(c) 1999 - 2018 Intel Corporation. */
>  
>  #include "e1000.h"
> +#include <linux/ethtool.h>
>  
>  static s32 e1000_wait_autoneg(struct e1000_hw *hw);
>  static s32 e1000_access_phy_wakeup_reg_bm(struct e1000_hw *hw, u32 offset,
> @@ -1011,6 +1012,8 @@ static s32 e1000_phy_setup_autoneg(struct e1000_hw *hw)
>  		 */
>  		mii_autoneg_adv_reg &=
>  		    ~(ADVERTISE_PAUSE_ASYM | ADVERTISE_PAUSE_CAP);
> +		phy->autoneg_advertised &=
> +		    ~(ADVERTISED_Pause | ADVERTISED_Asym_Pause);
>  		break;
>  	case e1000_fc_rx_pause:
>  		/* Rx Flow control is enabled, and Tx Flow control is
> @@ -1024,6 +1027,8 @@ static s32 e1000_phy_setup_autoneg(struct e1000_hw *hw)
>  		 */
>  		mii_autoneg_adv_reg |=
>  		    (ADVERTISE_PAUSE_ASYM | ADVERTISE_PAUSE_CAP);
> +		phy->autoneg_advertised |=
> +		    (ADVERTISED_Pause | ADVERTISED_Asym_Pause);
>  		break;
>  	case e1000_fc_tx_pause:
>  		/* Tx Flow control is enabled, and Rx Flow control is
> @@ -1031,6 +1036,8 @@ static s32 e1000_phy_setup_autoneg(struct e1000_hw *hw)
>  		 */
>  		mii_autoneg_adv_reg |= ADVERTISE_PAUSE_ASYM;
>  		mii_autoneg_adv_reg &= ~ADVERTISE_PAUSE_CAP;
> +		phy->autoneg_advertised |= ADVERTISED_Asym_Pause;
> +		phy->autoneg_advertised &= ~ADVERTISED_Pause;
>  		break;
>  	case e1000_fc_full:
>  		/* Flow control (both Rx and Tx) is enabled by a software
> @@ -1038,6 +1045,8 @@ static s32 e1000_phy_setup_autoneg(struct e1000_hw *hw)
>  		 */
>  		mii_autoneg_adv_reg |=
>  		    (ADVERTISE_PAUSE_ASYM | ADVERTISE_PAUSE_CAP);
> +		phy->autoneg_advertised |=
> +		    (ADVERTISED_Pause | ADVERTISED_Asym_Pause);
>  		break;
>  	default:
>  		e_dbg("Flow control param set incorrectly\n");
> -- 

I don't know this driver at all. What i don't see anywhere here is
using the results of the pause auto-neg. Is there some code somewhere
that looks at the local and link peer advertising values and runs a
resolve algorithm to determine what pause should be used, and program
it into the MAC?

    Andrew

Re: [PATCH net-next 1/1] e1000e: Enable Link Partner Advertised Support

[Jamie Gloudon]

On Wed, Jan 04, 2023 at 01:47:01AM +0100, Andrew Lunn wrote:
> > --- a/drivers/net/ethernet/intel/e1000e/phy.c
> > +++ b/drivers/net/ethernet/intel/e1000e/phy.c
> > @@ -2,6 +2,7 @@
> >  /* Copyright(c) 1999 - 2018 Intel Corporation. */
> >  
> >  #include "e1000.h"
> > +#include <linux/ethtool.h>
> >  
> >  static s32 e1000_wait_autoneg(struct e1000_hw *hw);
> >  static s32 e1000_access_phy_wakeup_reg_bm(struct e1000_hw *hw, u32 offset,
> > @@ -1011,6 +1012,8 @@ static s32 e1000_phy_setup_autoneg(struct e1000_hw *hw)
> >  		 */
> >  		mii_autoneg_adv_reg &=
> >  		    ~(ADVERTISE_PAUSE_ASYM | ADVERTISE_PAUSE_CAP);
> > +		phy->autoneg_advertised &=
> > +		    ~(ADVERTISED_Pause | ADVERTISED_Asym_Pause);
> >  		break;
> >  	case e1000_fc_rx_pause:
> >  		/* Rx Flow control is enabled, and Tx Flow control is
> > @@ -1024,6 +1027,8 @@ static s32 e1000_phy_setup_autoneg(struct e1000_hw *hw)
> >  		 */
> >  		mii_autoneg_adv_reg |=
> >  		    (ADVERTISE_PAUSE_ASYM | ADVERTISE_PAUSE_CAP);
> > +		phy->autoneg_advertised |=
> > +		    (ADVERTISED_Pause | ADVERTISED_Asym_Pause);
> >  		break;
> >  	case e1000_fc_tx_pause:
> >  		/* Tx Flow control is enabled, and Rx Flow control is
> > @@ -1031,6 +1036,8 @@ static s32 e1000_phy_setup_autoneg(struct e1000_hw *hw)
> >  		 */
> >  		mii_autoneg_adv_reg |= ADVERTISE_PAUSE_ASYM;
> >  		mii_autoneg_adv_reg &= ~ADVERTISE_PAUSE_CAP;
> > +		phy->autoneg_advertised |= ADVERTISED_Asym_Pause;
> > +		phy->autoneg_advertised &= ~ADVERTISED_Pause;
> >  		break;
> >  	case e1000_fc_full:
> >  		/* Flow control (both Rx and Tx) is enabled by a software
> > @@ -1038,6 +1045,8 @@ static s32 e1000_phy_setup_autoneg(struct e1000_hw *hw)
> >  		 */
> >  		mii_autoneg_adv_reg |=
> >  		    (ADVERTISE_PAUSE_ASYM | ADVERTISE_PAUSE_CAP);
> > +		phy->autoneg_advertised |=
> > +		    (ADVERTISED_Pause | ADVERTISED_Asym_Pause);
> >  		break;
> >  	default:
> >  		e_dbg("Flow control param set incorrectly\n");
> > -- 
> 
> I don't know this driver at all. What i don't see anywhere here is
> using the results of the pause auto-neg. Is there some code somewhere
> that looks at the local and link peer advertising values and runs a
> resolve algorithm to determine what pause should be used, and program
> it into the MAC?
> 
>     Andrew
This is a old patch i had laying around, If i remember correctly, phy->autoneg_advertised plugs in "Link partner
advertised pause frame use link" line in ethtool everytime the nic renegotiate.

Re: [PATCH net-next 1/1] e1000e: Enable Link Partner Advertised Support

[Andrew Lunn]

> > I don't know this driver at all. What i don't see anywhere here is
> > using the results of the pause auto-neg. Is there some code somewhere
> > that looks at the local and link peer advertising values and runs a
> > resolve algorithm to determine what pause should be used, and program
> > it into the MAC?
> > 
> >     Andrew
> This is a old patch i had laying around, If i remember correctly, phy->autoneg_advertised plugs in "Link partner
> advertised pause frame use link" line in ethtool everytime the nic renegotiate.

Hi Jamie

Could you point me at the code which interprets the results of the
auto neg and configures the MAC for the correct pause.

Thanks
	Andrew

Re: [PATCH net-next 1/1] e1000e: Enable Link Partner Advertised Support

[Jamie Gloudon]

On Wed, Jan 04, 2023 at 06:44:09PM +0100, Andrew Lunn wrote:
> > > I don't know this driver at all. What i don't see anywhere here is
> > > using the results of the pause auto-neg. Is there some code somewhere
> > > that looks at the local and link peer advertising values and runs a
> > > resolve algorithm to determine what pause should be used, and program
> > > it into the MAC?
> > > 
> > >     Andrew
> > This is a old patch i had laying around, If i remember correctly, phy->autoneg_advertised plugs in "Link partner
> > advertised pause frame use link" line in ethtool everytime the nic renegotiate.
> 
> Hi Jamie
> 
> Could you point me at the code which interprets the results of the
> auto neg and configures the MAC for the correct pause.
> 
> Thanks
> 	Andrew
Oh I now understand what you're saying. No clue. The guys at intel should have a better idea. Btw my old nic is not working anymore, Gosh!

Re: [PATCH net-next 1/1] e1000e: Enable Link Partner Advertised Support

[Neftin, Sasha]

On 1/4/2023 19:44, Andrew Lunn wrote:
>>> I don't know this driver at all. What i don't see anywhere here is
>>> using the results of the pause auto-neg. Is there some code somewhere
>>> that looks at the local and link peer advertising values and runs a
>>> resolve algorithm to determine what pause should be used, and program
>>> it into the MAC?
>>>
>>>      Andrew
>> This is a old patch i had laying around, If i remember correctly, phy->autoneg_advertised plugs in "Link partner
>> advertised pause frame use link" line in ethtool everytime the nic renegotiate.
> 
> Hi Jamie
> 
> Could you point me at the code which interprets the results of the
> auto neg and configures the MAC for the correct pause.
probably you look at  e1000e_config_fc_after_link_up method
(this is very legacy code 
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/e1000e/mac.c#L1001)
I've no objection to this patch - should show link partner advertisements.
> 
> Thanks
> 	Andrew

Re: [PATCH net-next 1/1] e1000e: Enable Link Partner Advertised Support

[Andrew Lunn]

On Mon, Jan 09, 2023 at 10:40:47AM +0200, Neftin, Sasha wrote:
> On 1/4/2023 19:44, Andrew Lunn wrote:
> > > > I don't know this driver at all. What i don't see anywhere here is
> > > > using the results of the pause auto-neg. Is there some code somewhere
> > > > that looks at the local and link peer advertising values and runs a
> > > > resolve algorithm to determine what pause should be used, and program
> > > > it into the MAC?
> > > > 
> > > >      Andrew
> > > This is a old patch i had laying around, If i remember correctly, phy->autoneg_advertised plugs in "Link partner
> > > advertised pause frame use link" line in ethtool everytime the nic renegotiate.
> > 
> > Hi Jamie
> > 
> > Could you point me at the code which interprets the results of the
> > auto neg and configures the MAC for the correct pause.
> probably you look at  e1000e_config_fc_after_link_up method
> (this is very legacy code https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/e1000e/mac.c#L1001)

Thanks. That is exactly what i wanted to see.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 1/1] e1000e: Enable Link Partner Advertised Support

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  3 Jan 2023 15:06:53 -0800 you wrote:
> From: Jamie Gloudon <jamie.gloudon@gmx.fr>
> 
> This enables link partner advertised support to show link modes and
> pause frame use.
> 
> Signed-off-by: Jamie Gloudon <jamie.gloudon@gmx.fr>
> Tested-by: Naama Meir <naamax.meir@linux.intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> 
> [...]

Here is the summary with links:
  - [net-next,1/1] e1000e: Enable Link Partner Advertised Support
    https://git.kernel.org/netdev/net-next/c/cbdbb58b6c79

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html