[PATCH 1/2] gianfar: Deal with link state changes during GFAR_RESETTING dev state

[PATCH 1/2] gianfar: Deal with link state changes during GFAR_RESETTING dev state

[Thomas Graziadei]

From: Thomas Graziadei <thomas.graziadei@omicronenergy.com>

The link state is not correctly set in the case that the network driver
is reconfigured while the link state changes. The phy informs the gianfar
driver, but gfar_update_link_state just exits and therefore looses the
change event. The network driver remains in the old state until a new link
event is sent.

A trace log from a possible scenario at bootup, when the link state in the
network driver stays down even though the phy reports an up link. The test
sends a SIOCSHWTSTAMP ioctl at the right moment (which calls reset_gfar):
         ip-1196 [000]  5.389270: phy_start: state: READY -> UP
kworker/0:2-1195 [000]  5.389784: phy_start_aneg: state: UP -> AN
kworker/0:2-1195 [000]  5.389788: phy_state_machine: state: UP -> AN
kworker/0:2-1195 [000]  6.828064: adjust_link: eth0, link 0 -> 0
kworker/0:2-1195 [000]  6.828599: phy_state_machine: state: AN -> NOLINK
       test-1470 [000]  7.460422: reset_gfar: before locking GFAR_RESETTING
       test-1470 [000]  7.470806: phy_stop: state: NOLINK -> HALTED
       test-1470 [000]  7.478806: phy_start: state: HALTED -> RESUMING
kworker/0:2-1195 [000]  7.479478: adjust_link: eth0, link 0 -> 1
kworker/0:2-1195 [000]  7.479482: phy_state_machine: state: RESUMING -> RUNNING
       test-1470 [000]  7.479826: reset_gfar: after locking GFAR_RESETTING

To resolve the issue adjust_link is called after every GFAR_RESETTING lock
section. Adjust_link itself checks if anything has changed and updates the
link accordingly.

Signed-off-by: Thomas Graziadei <thomas.graziadei@omicronenergy.com>
---
 drivers/net/ethernet/freescale/gianfar.c         |  7 +++++++
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 15 +++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 957bfc2..b3b5c43 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -2617,6 +2617,10 @@ static int gfar_change_mtu(struct net_device *dev, int new_mtu)
 
 	clear_bit_unlock(GFAR_RESETTING, &priv->state);
 
+	// catch maybe missed link state changes
+	if (dev->flags & IFF_UP)
+		adjust_link(dev);
+
 	return 0;
 }
 
@@ -2631,6 +2635,9 @@ void reset_gfar(struct net_device *ndev)
 	startup_gfar(ndev);
 
 	clear_bit_unlock(GFAR_RESETTING, &priv->state);
+
+	// catch maybe missed link state changes
+	adjust_link(ndev);
 }
 
 /* gfar_reset_task gets scheduled when a packet has not been
diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index a93e019..065f05e 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -330,6 +330,7 @@ static int gfar_scoalesce(struct net_device *dev,
 			  struct ethtool_coalesce *cvals)
 {
 	struct gfar_private *priv = netdev_priv(dev);
+	struct phy_device *phydev = dev->phydev;
 	int i, err = 0;
 
 	if (!(priv->device_flags & FSL_GIANFAR_DEV_HAS_COALESCE))
@@ -408,6 +409,10 @@ static int gfar_scoalesce(struct net_device *dev,
 
 	clear_bit_unlock(GFAR_RESETTING, &priv->state);
 
+	// catch maybe missed link state changes
+	if (dev->flags & IFF_UP)
+		phydev->adjust_link(dev);
+
 	return err;
 }
 
@@ -445,6 +450,7 @@ static int gfar_sringparam(struct net_device *dev,
 			   struct ethtool_ringparam *rvals)
 {
 	struct gfar_private *priv = netdev_priv(dev);
+	struct phy_device *phydev = dev->phydev;
 	int err = 0, i;
 
 	if (rvals->rx_pending > GFAR_RX_MAX_RING_SIZE)
@@ -482,6 +488,10 @@ static int gfar_sringparam(struct net_device *dev,
 
 	clear_bit_unlock(GFAR_RESETTING, &priv->state);
 
+	// catch maybe missed link state changes
+	if (dev->flags & IFF_UP)
+		phydev->adjust_link(dev);
+
 	return err;
 }
 
@@ -569,6 +579,7 @@ int gfar_set_features(struct net_device *dev, netdev_features_t features)
 {
 	netdev_features_t changed = dev->features ^ features;
 	struct gfar_private *priv = netdev_priv(dev);
+	struct phy_device *phydev = dev->phydev;
 	int err = 0;
 
 	if (!(changed & (NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX |
@@ -590,6 +601,10 @@ int gfar_set_features(struct net_device *dev, netdev_features_t features)
 
 	clear_bit_unlock(GFAR_RESETTING, &priv->state);
 
+	// catch maybe missed link state changes
+	if (dev->flags & IFF_UP)
+		phydev->adjust_link(dev);
+
 	return err;
 }
 
-- 
2.7.4

[PATCH 2/2] gianfar: Use the same initial values as the phy layer

[Thomas Graziadei]

From: Thomas Graziadei <thomas.graziadei@omicronenergy.com>

The phy layer sets the link status initially to 1 to also support phyless
systems, therefore we set the gianfar drivers initial link status also to
1. This prevents the gfar_update_link_state() method to be called when the
driver and phy have just been initialized but no link state change was
detected yet.

Signed-off-by: Thomas Graziadei <thomas.graziadei@omicronenergy.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index b3b5c43..be0d23e 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -1705,7 +1705,7 @@ static int gfar_restore(struct device *dev)
 
 	gfar_start(priv);
 
-	priv->oldlink = 0;
+	priv->oldlink = 1;
 	priv->oldspeed = 0;
 	priv->oldduplex = -1;
 
@@ -1791,7 +1791,7 @@ static int init_phy(struct net_device *dev)
 	phy_interface_t interface;
 	struct phy_device *phydev;
 
-	priv->oldlink = 0;
+	priv->oldlink = 1;
 	priv->oldspeed = 0;
 	priv->oldduplex = -1;
 
@@ -2212,7 +2212,7 @@ int startup_gfar(struct net_device *ndev)
 	gfar_start(priv);
 
 	/* force link state update after mac reset */
-	priv->oldlink = 0;
+	priv->oldlink = 1;
 	priv->oldspeed = 0;
 	priv->oldduplex = -1;
 
-- 
2.7.4

RE: [PATCH 1/2] gianfar: Deal with link state changes during GFAR_RESETTING dev state

[Claudiu Manoil]

>-----Original Message-----
>From: Thomas Graziadei [mailto:thomas.graziadei@omicronenergy.com]
>Sent: Monday, February 13, 2017 2:22 PM
>To: claudiu.manoil@freescale.com; netdev@vger.kernel.org; linux-
>kernel@vger.kernel.org
>Cc: Thomas Graziadei <thomas.graziadei@omicronenergy.com>
>Subject: [PATCH 1/2] gianfar: Deal with link state changes during GFAR_RESETTING
>dev state
>
>From: Thomas Graziadei <thomas.graziadei@omicronenergy.com>
>
>The link state is not correctly set in the case that the network driver
>is reconfigured while the link state changes. The phy informs the gianfar
>driver, but gfar_update_link_state just exits and therefore looses the
>change event. The network driver remains in the old state until a new link
>event is sent.
>
>A trace log from a possible scenario at bootup, when the link state in the
>network driver stays down even though the phy reports an up link. The test
>sends a SIOCSHWTSTAMP ioctl at the right moment (which calls reset_gfar):
>         ip-1196 [000]  5.389270: phy_start: state: READY -> UP
>kworker/0:2-1195 [000]  5.389784: phy_start_aneg: state: UP -> AN
>kworker/0:2-1195 [000]  5.389788: phy_state_machine: state: UP -> AN
>kworker/0:2-1195 [000]  6.828064: adjust_link: eth0, link 0 -> 0
>kworker/0:2-1195 [000]  6.828599: phy_state_machine: state: AN -> NOLINK
>       test-1470 [000]  7.460422: reset_gfar: before locking GFAR_RESETTING
>       test-1470 [000]  7.470806: phy_stop: state: NOLINK -> HALTED
>       test-1470 [000]  7.478806: phy_start: state: HALTED -> RESUMING
>kworker/0:2-1195 [000]  7.479478: adjust_link: eth0, link 0 -> 1
>kworker/0:2-1195 [000]  7.479482: phy_state_machine: state: RESUMING ->
>RUNNING
>       test-1470 [000]  7.479826: reset_gfar: after locking GFAR_RESETTING
>
>To resolve the issue adjust_link is called after every GFAR_RESETTING lock
>section. Adjust_link itself checks if anything has changed and updates the
>link accordingly.
>

Hi,
Interesting findings. I need more time to check the patches. Btw, we don't 
use "//" for comments on netdev.

Thanks,
Claudiu

RE: [PATCH 1/2] gianfar: Deal with link state changes during GFAR_RESETTING dev state

[Claudiu Manoil]

>-----Original Message-----
>From: Thomas Graziadei [mailto:thomas.graziadei@omicronenergy.com]
>Sent: Monday, February 13, 2017 2:22 PM
>To: claudiu.manoil@freescale.com; netdev@vger.kernel.org; linux-
>kernel@vger.kernel.org
>Cc: Thomas Graziadei <thomas.graziadei@omicronenergy.com>
>Subject: [PATCH 1/2] gianfar: Deal with link state changes during GFAR_RESETTING
>dev state
>
>From: Thomas Graziadei <thomas.graziadei@omicronenergy.com>
>

[...]

>
>To resolve the issue adjust_link is called after every GFAR_RESETTING lock
>section. Adjust_link itself checks if anything has changed and updates the
>link accordingly.
>

Calling adjust_link() from the ethernet driver, even if only following device reset / 
reconfig sections, is racy - the phylib state machine may run adjust_link() at the 
same time or the phydev state may be inconsistent - and may end up in more 
subtle link state issues.  It also defeats the purpose of the adjust link hook, which 
is to be called by the phylib state machine.

I had a look at this, and I don't think there is an easy fix in the driver for your case. 
However, the workaround should be straight forward: wait for the link to stabilize 
to the requested parameters before applying other device configuration changes 
that require device reset (like rx timestamping).  Given that the PHYLIB state machine 
changed its behavior since adjust_link() in gianfar was last modified (i.e. it used to be called
periodically before, from RUNNING<->CHANGELINK states) and given that there is a good part 
of legacy code in the current adjust_link() implementation, I think that driver part needs 
considerable rework to correctly cover this usecase as well.

Thanks.
Claudiu