[PATCH] net: gianfar: add ethtool eee support

[PATCH] net: gianfar: add ethtool eee support

[Yangbo Lu]

From: Shaohui Xie <Shaohui.Xie@nxp.com>

Gianfar does not support EEE, but it can connect to a PHY which supports
EEE and the PHY advertises EEE by default, and its link partner also
advertises EEE, so the PHY enters low power mode when traffic rate is low,
which causes packet loss if an application's traffic rate is low. This
patch provides .get_eee and .set_eee so that to disable the EEE
advertisement via ethtool if needed, other EEE features are not supported.

Signed-off-by: Shaohui Xie <Shaohui.Xie@nxp.com>
Signed-off-by: Yangbo Lu <Yangbo.lu@nxp.com>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 28 ++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 56588f2e1d91..8953650b36f3 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -184,6 +184,32 @@ static void gfar_gdrvinfo(struct net_device *dev,
 	strlcpy(drvinfo->bus_info, "N/A", sizeof(drvinfo->bus_info));
 }
 
+static int gfar_get_eee(struct net_device *dev, struct ethtool_eee *et_eee)
+{
+	struct phy_device *phydev = dev->phydev;
+
+	if (!phydev)
+		return -ENODEV;
+
+	return phy_ethtool_get_eee(phydev, et_eee);
+}
+
+static int gfar_set_eee(struct net_device *dev, struct ethtool_eee *et_eee)
+{
+	struct phy_device *phydev = dev->phydev;
+
+	if (!phydev)
+		return -ENODEV;
+
+	if (et_eee->eee_enabled ||
+	    et_eee->tx_lpi_enabled ||
+	    et_eee->tx_lpi_timer) {
+		return -EOPNOTSUPP;
+	}
+
+	return phy_ethtool_set_eee(phydev, et_eee);
+}
+
 /* Return the length of the register structure */
 static int gfar_reglen(struct net_device *dev)
 {
@@ -1535,6 +1561,8 @@ static int gfar_get_ts_info(struct net_device *dev,
 }
 
 const struct ethtool_ops gfar_ethtool_ops = {
+	.get_eee = gfar_get_eee,
+	.set_eee = gfar_set_eee,
 	.get_drvinfo = gfar_gdrvinfo,
 	.get_regs_len = gfar_reglen,
 	.get_regs = gfar_get_regs,
-- 
2.14.1

Re: [PATCH] net: gianfar: add ethtool eee support

[Andrew Lunn]

On Tue, Nov 28, 2017 at 02:29:01PM +0800, Yangbo Lu wrote:
> From: Shaohui Xie <Shaohui.Xie@nxp.com>
> 
> Gianfar does not support EEE, but it can connect to a PHY which supports
> EEE and the PHY advertises EEE by default, and its link partner also
> advertises EEE, so the PHY enters low power mode when traffic rate is low,
> which causes packet loss if an application's traffic rate is low. This
> patch provides .get_eee and .set_eee so that to disable the EEE
> advertisement via ethtool if needed, other EEE features are not supported.

Hi Yangbo

This patch is good, as far as it goes. However, as you said, some PHYs
advertise EEE by default. So i think it would be good to add a call to
phy_ethtool_set_eee() to disable advertisement, e.g. in init_phy().

Thanks
	      Andrew

[PATCH net] gianfar: Disable EEE autoneg by default

[Claudiu Manoil]

This controller does not support EEE, but it may connect to a PHY
which supports EEE and advertises EEE by default, while its link
partner also advertises EEE. If this happens, the PHY enters low
power mode when the traffic rate is low and causes packet loss.
This patch disables EEE advertisement by default for any PHY that
gianfar connects to, to prevent the above unwanted outcome.

Signed-off-by: Shaohui Xie <Shaohui.Xie@nxp.com>
Tested-by: Yangbo Lu <Yangbo.lu@nxp.com>
Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 81a73af..7f83700 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -1792,6 +1792,7 @@ static int init_phy(struct net_device *dev)
 		GFAR_SUPPORTED_GBIT : 0;
 	phy_interface_t interface;
 	struct phy_device *phydev;
+	struct ethtool_eee edata;
 
 	priv->oldlink = 0;
 	priv->oldspeed = 0;
@@ -1816,6 +1817,10 @@ static int init_phy(struct net_device *dev)
 	/* Add support for flow control, but don't advertise it by default */
 	phydev->supported |= (SUPPORTED_Pause | SUPPORTED_Asym_Pause);
 
+	/* disable EEE autoneg, EEE not supported by eTSEC */
+	memset(&edata, 0, sizeof(struct ethtool_eee));
+	phy_ethtool_set_eee(phydev, &edata);
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH net] gianfar: Disable EEE autoneg by default

[Andrew Lunn]

On Thu, Dec 07, 2017 at 06:44:23PM +0200, Claudiu Manoil wrote:
> This controller does not support EEE, but it may connect to a PHY
> which supports EEE and advertises EEE by default, while its link
> partner also advertises EEE. If this happens, the PHY enters low
> power mode when the traffic rate is low and causes packet loss.
> This patch disables EEE advertisement by default for any PHY that
> gianfar connects to, to prevent the above unwanted outcome.
> 
> Signed-off-by: Shaohui Xie <Shaohui.Xie@nxp.com>
> Tested-by: Yangbo Lu <Yangbo.lu@nxp.com>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>

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

    Andrew

Re: [PATCH net] gianfar: Disable EEE autoneg by default

[David Miller]

From: Claudiu Manoil <claudiu.manoil@nxp.com>
Date: Thu, 7 Dec 2017 18:44:23 +0200

> This controller does not support EEE, but it may connect to a PHY
> which supports EEE and advertises EEE by default, while its link
> partner also advertises EEE. If this happens, the PHY enters low
> power mode when the traffic rate is low and causes packet loss.
> This patch disables EEE advertisement by default for any PHY that
> gianfar connects to, to prevent the above unwanted outcome.
> 
> Signed-off-by: Shaohui Xie <Shaohui.Xie@nxp.com>
> Tested-by: Yangbo Lu <Yangbo.lu@nxp.com>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>

Applied, thank you.

[PATCH] net: gianfar: add ethtool eee support

[Shaohui Xie]

Gianfar does not support EEE, but it can connect to a PHY which supports
EEE and the PHY advertises EEE by default, and its link partner also
advertises EEE, so the PHY enters low power mode when traffic rate is low,
which causes packet loss if an application's traffic rate is low. This
patch provides .get_eee and .set_eee so that to disable the EEE
advertisement via ethtool if needed, other EEE features are not supported.

Signed-off-by: Shaohui Xie <Shaohui.Xie@nxp.com>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 28 ++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 56588f2..9375078 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -184,6 +184,32 @@ static void gfar_gdrvinfo(struct net_device *dev,
 	strlcpy(drvinfo->bus_info, "N/A", sizeof(drvinfo->bus_info));
 }
 
+static int gfar_get_eee(struct net_device *dev, struct ethtool_eee *et_eee)
+{
+	struct phy_device *phydev = dev->phydev;
+
+	if (!phydev)
+		return -ENODEV;
+
+	return phy_ethtool_get_eee(phydev, et_eee);
+}
+
+static int gfar_set_eee(struct net_device *dev, struct ethtool_eee *et_eee)
+{
+	struct phy_device *phydev = dev->phydev;
+
+	if (!phydev)
+		return -ENODEV;
+
+	if (et_eee->eee_enabled ||
+	    et_eee->tx_lpi_enabled ||
+	    et_eee->tx_lpi_timer) {
+		return -EOPNOTSUPP;
+	}
+
+	return phy_ethtool_set_eee(phydev, et_eee);
+}
+
 /* Return the length of the register structure */
 static int gfar_reglen(struct net_device *dev)
 {
@@ -1548,6 +1574,8 @@ const struct ethtool_ops gfar_ethtool_ops = {
 	.get_strings = gfar_gstrings,
 	.get_sset_count = gfar_sset_count,
 	.get_ethtool_stats = gfar_fill_stats,
+	.get_eee = gfar_get_eee,
+	.set_eee = gfar_set_eee,
 	.get_msglevel = gfar_get_msglevel,
 	.set_msglevel = gfar_set_msglevel,
 #ifdef CONFIG_PM
-- 
2.1.0.27.g96db324

Re: [PATCH] net: gianfar: add ethtool eee support

[David Miller]

From: Shaohui Xie <Shaohui.Xie@nxp.com>
Date: Thu, 8 Dec 2016 19:27:06 +0800

> Gianfar does not support EEE, but it can connect to a PHY which supports
> EEE and the PHY advertises EEE by default, and its link partner also
> advertises EEE, so the PHY enters low power mode when traffic rate is low,
> which causes packet loss if an application's traffic rate is low. This
> patch provides .get_eee and .set_eee so that to disable the EEE
> advertisement via ethtool if needed, other EEE features are not supported.
> 
> Signed-off-by: Shaohui Xie <Shaohui.Xie@nxp.com>

This is not the way to fix this.

If the Gianfar MAC does not support EEE properly, then the gianfar
driver should not create a situation where the PHY advertises EEE
in the first place.

Re: [PATCH] net: gianfar: add ethtool eee support

[Florian Fainelli]

On 12/08/2016 03:19 PM, David Miller wrote:
> From: Shaohui Xie <Shaohui.Xie@nxp.com>
> Date: Thu, 8 Dec 2016 19:27:06 +0800
> 
>> Gianfar does not support EEE, but it can connect to a PHY which supports
>> EEE and the PHY advertises EEE by default, and its link partner also
>> advertises EEE, so the PHY enters low power mode when traffic rate is low,
>> which causes packet loss if an application's traffic rate is low. This
>> patch provides .get_eee and .set_eee so that to disable the EEE
>> advertisement via ethtool if needed, other EEE features are not supported.
>>
>> Signed-off-by: Shaohui Xie <Shaohui.Xie@nxp.com>
> 
> This is not the way to fix this.
> 
> If the Gianfar MAC does not support EEE properly, then the gianfar
> driver should not create a situation where the PHY advertises EEE
> in the first place.

Agreed, you should have gianfar mask out the EEE advertisement from the
PHY it connects to, in order to make sure that EEE is properly disabled.
This should be no different from e.g: connecting a 10/100/1000 PHY to a
10/100 only controller for instance.
-- 
Florian

RE: [PATCH] net: gianfar: add ethtool eee support

[S.H. Xie]

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Friday, December 09, 2016 7:22 AM
> To: David Miller <davem@davemloft.net>; S.H. Xie <shaohui.xie@nxp.com>
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH] net: gianfar: add ethtool eee support
> 
> On 12/08/2016 03:19 PM, David Miller wrote:
> > From: Shaohui Xie <Shaohui.Xie@nxp.com>
> > Date: Thu, 8 Dec 2016 19:27:06 +0800
> >
> >> Gianfar does not support EEE, but it can connect to a PHY which
> >> supports EEE and the PHY advertises EEE by default, and its link
> >> partner also advertises EEE, so the PHY enters low power mode when
> >> traffic rate is low, which causes packet loss if an application's
> >> traffic rate is low. This patch provides .get_eee and .set_eee so
> >> that to disable the EEE advertisement via ethtool if needed, other EEE features
> are not supported.
> >>
> >> Signed-off-by: Shaohui Xie <Shaohui.Xie@nxp.com>
> >
> > This is not the way to fix this.
> >
> > If the Gianfar MAC does not support EEE properly, then the gianfar
> > driver should not create a situation where the PHY advertises EEE in
> > the first place.
> 
> Agreed, you should have gianfar mask out the EEE advertisement from the PHY it
> connects to, in order to make sure that EEE is properly disabled.
> This should be no different from e.g: connecting a 10/100/1000 PHY to a
> 10/100 only controller for instance.
[S.H] The EEE advertisement does not belong to PHY device, gianfar cannot mask out the EEE
Advertisement like advertising, should use phy_write_mmd_indirect() to disable it?

And by disabling EEE advertisement always, the PHY cannot enter low power mode even if there 
is no traffic. Using ethtool could provide user options to handle this.

Thanks.
Shaohui