[PATCH 1/2] net/smsc911x: Fix rare soft reset timeout issue due to PHY power-down mode

[PATCH 1/2] net/smsc911x: Fix rare soft reset timeout issue due to PHY power-down mode

[Alexander Kochetkov]

The patch affect SMSC LAN generation 4 chips with integrated PHY (LAN9221).

It is possible that PHY could enter power-down mode (ENERGYON clear),
between ENERGYON bit check in smsc911x_phy_disable_energy_detect and SRST
bit set in smsc911x_soft_reset. This could happen, for example, if someone
disconnect ethernet cable between the checks. The PHY in a power-down mode
would prevent the MAC portion of chip to be software reseted.

Initially found by code review, confirmed later using test case.

This is low probability issue, and in order to reproduce it you have to
run the script:

while true; do
	ifconfig eth0 down
	ifconfig eth0 up || break
done

While the script is running you have to plug/unplug ethernet cable many
times (using gpio controlled ethernet switch, for example) until get:

[ 4516.477783] ADDRCONF(NETDEV_UP): eth0: link is not ready
[ 4516.512207] smsc911x smsc911x.0: eth0: SMSC911x/921x identified at 0xce006000, IRQ: 336
[ 4516.524658] ADDRCONF(NETDEV_UP): eth0: link is not ready
[ 4516.559082] smsc911x smsc911x.0: eth0: SMSC911x/921x identified at 0xce006000, IRQ: 336
[ 4516.571990] ADDRCONF(NETDEV_UP): eth0: link is not ready
ifconfig: SIOCSIFFLAGS: Input/output error

The patch was reviewed by Steve Glendinning and Microchip Team.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Acked-by: Steve Glendinning <steve.glendinning@shawell.net>
---
 drivers/net/ethernet/smsc/smsc911x.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 8564f23..c657184 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1356,12 +1356,8 @@ static int smsc911x_phy_disable_energy_detect(struct smsc911x_data *pdata)
 		return rc;
 	}
 
-	/*
-	 * If energy is detected the PHY is already awake so is not necessary
-	 * to disable the energy detect power-down mode.
-	 */
-	if ((rc & MII_LAN83C185_EDPWRDOWN) &&
-	    !(rc & MII_LAN83C185_ENERGYON)) {
+	/* Only disable if energy detect mode is already enabled */
+	if (rc & MII_LAN83C185_EDPWRDOWN) {
 		/* Disable energy detect mode for this SMSC Transceivers */
 		rc = phy_write(pdata->phy_dev, MII_LAN83C185_CTRL_STATUS,
 			       rc & (~MII_LAN83C185_EDPWRDOWN));
-- 
1.7.9.5

[PATCH 2/2] net/smsc911x: Fix delays in the PHY enable/disable routines

[Alexander Kochetkov]

Increased delay in the smsc911x_phy_disable_energy_detect (from 1ms to 2ms).
Dropped delays in the smsc911x_phy_enable_energy_detect (100ms and 1ms).

The patch affect SMSC LAN generation 4 chips with integrated PHY (LAN9221).

I saw problems with soft reset due to wrong udelay timings.
After I fixed udelay, I measured the time needed to bring integrated PHY
from power-down to operational mode (the time beetween clearing EDPWRDOWN
bit and soft reset complete event). I got 1ms (measured using ktime_get).
The value is equal to the current value (1ms) used in the
smsc911x_phy_disable_energy_detect. It is near the upper bound and in order
to avoid rare soft reset faults it is doubled (2ms).

I don't know official timing for bringing up integrated PHY as specs doesn't
clarify this (or may be I didn't found).

It looks safe to drop delays before and after setting EDPWRDOWN bit
(enable PHY power-down mode). I didn't saw any regressions with the patch.

The patch was reviewed by Steve Glendinning and Microchip Team.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Acked-by: Steve Glendinning <steve.glendinning@shawell.net>
---
 drivers/net/ethernet/smsc/smsc911x.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index c657184..b2b3170 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1366,8 +1366,8 @@ static int smsc911x_phy_disable_energy_detect(struct smsc911x_data *pdata)
 			SMSC_WARN(pdata, drv, "Failed writing PHY control reg");
 			return rc;
 		}
-
-		mdelay(1);
+		/* Allow PHY to wakeup */
+		mdelay(2);
 	}
 
 	return 0;
@@ -1389,7 +1389,6 @@ static int smsc911x_phy_enable_energy_detect(struct smsc911x_data *pdata)
 
 	/* Only enable if energy detect mode is already disabled */
 	if (!(rc & MII_LAN83C185_EDPWRDOWN)) {
-		mdelay(100);
 		/* Enable energy detect mode for this SMSC Transceivers */
 		rc = phy_write(pdata->phy_dev, MII_LAN83C185_CTRL_STATUS,
 			       rc | MII_LAN83C185_EDPWRDOWN);
@@ -1398,8 +1397,6 @@ static int smsc911x_phy_enable_energy_detect(struct smsc911x_data *pdata)
 			SMSC_WARN(pdata, drv, "Failed writing PHY control reg");
 			return rc;
 		}
-
-		mdelay(1);
 	}
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH 2/2] net/smsc911x: Fix delays in the PHY enable/disable routines

[David Miller]

From: Alexander Kochetkov <al.kochet@gmail.com>
Date: Thu, 13 Nov 2014 05:26:20 +0400

> Increased delay in the smsc911x_phy_disable_energy_detect (from 1ms to 2ms).
> Dropped delays in the smsc911x_phy_enable_energy_detect (100ms and 1ms).
> 
> The patch affect SMSC LAN generation 4 chips with integrated PHY (LAN9221).
> 
> I saw problems with soft reset due to wrong udelay timings.
> After I fixed udelay, I measured the time needed to bring integrated PHY
> from power-down to operational mode (the time beetween clearing EDPWRDOWN
> bit and soft reset complete event). I got 1ms (measured using ktime_get).
> The value is equal to the current value (1ms) used in the
> smsc911x_phy_disable_energy_detect. It is near the upper bound and in order
> to avoid rare soft reset faults it is doubled (2ms).
> 
> I don't know official timing for bringing up integrated PHY as specs doesn't
> clarify this (or may be I didn't found).
> 
> It looks safe to drop delays before and after setting EDPWRDOWN bit
> (enable PHY power-down mode). I didn't saw any regressions with the patch.
> 
> The patch was reviewed by Steve Glendinning and Microchip Team.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Acked-by: Steve Glendinning <steve.glendinning@shawell.net>

Applied.

Re: [PATCH 1/2] net/smsc911x: Fix rare soft reset timeout issue due to PHY power-down mode

[David Miller]

From: Alexander Kochetkov <al.kochet@gmail.com>
Date: Thu, 13 Nov 2014 05:26:19 +0400

> The patch affect SMSC LAN generation 4 chips with integrated PHY (LAN9221).
> 
> It is possible that PHY could enter power-down mode (ENERGYON clear),
> between ENERGYON bit check in smsc911x_phy_disable_energy_detect and SRST
> bit set in smsc911x_soft_reset. This could happen, for example, if someone
> disconnect ethernet cable between the checks. The PHY in a power-down mode
> would prevent the MAC portion of chip to be software reseted.
> 
> Initially found by code review, confirmed later using test case.
> 
> This is low probability issue, and in order to reproduce it you have to
> run the script:
> 
> while true; do
> 	ifconfig eth0 down
> 	ifconfig eth0 up || break
> done
> 
> While the script is running you have to plug/unplug ethernet cable many
> times (using gpio controlled ethernet switch, for example) until get:
> 
> [ 4516.477783] ADDRCONF(NETDEV_UP): eth0: link is not ready
> [ 4516.512207] smsc911x smsc911x.0: eth0: SMSC911x/921x identified at 0xce006000, IRQ: 336
> [ 4516.524658] ADDRCONF(NETDEV_UP): eth0: link is not ready
> [ 4516.559082] smsc911x smsc911x.0: eth0: SMSC911x/921x identified at 0xce006000, IRQ: 336
> [ 4516.571990] ADDRCONF(NETDEV_UP): eth0: link is not ready
> ifconfig: SIOCSIFFLAGS: Input/output error
> 
> The patch was reviewed by Steve Glendinning and Microchip Team.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Acked-by: Steve Glendinning <steve.glendinning@shawell.net>

Applied.