[PATCH net-next 0/2] net: phy: don't stop state machine in case of MDIO error

If we detect a MDIO error, it seems to be a little bit too aggressive
to stop the state machine and bring down the PHY completely.
E.g. when polling and we miss one update, then this has no relevant
impact.

Heiner Kallweit (2):
  net: phy: don't stop state machine in case of MDIO error
  net: fec: remove workaround to restart state machine on MDIO error

 drivers/net/ethernet/freescale/fec.h      |  1 -
 drivers/net/ethernet/freescale/fec_main.c | 12 -------
 drivers/net/phy/phy.c                     | 42 +++++------------------
 include/linux/phy.h                       |  2 +-
 4 files changed, 10 insertions(+), 47 deletions(-)

-- 
2.20.0

If we detect a MDIO error, it seems to be a little bit too aggressive
to stop the state machine and bring down the PHY completely.
E.g. when polling and we miss one update, then this has no relevant
impact. And in phy_stop_interrupts() actually interrupts should be
disabled already because phy_stop() is called before. The only caller
of phy_stop_interrupts() isn't interested in the return value,
so let's change return type to void.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 42 +++++++++---------------------------------
 include/linux/phy.h   |  2 +-
 2 files changed, 10 insertions(+), 34 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e24708f1f..f926ec52a 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -714,24 +714,6 @@ void phy_stop_machine(struct phy_device *phydev)
 	mutex_unlock(&phydev->lock);
 }
 
-/**
- * phy_error - enter HALTED state for this PHY device
- * @phydev: target phy_device struct
- *
- * Moves the PHY to the HALTED state in response to a read
- * or write error, and tells the controller the link is down.
- * Must not be called from interrupt context, or while the
- * phydev->lock is held.
- */
-static void phy_error(struct phy_device *phydev)
-{
-	mutex_lock(&phydev->lock);
-	phydev->state = PHY_HALTED;
-	mutex_unlock(&phydev->lock);
-
-	phy_trigger_machine(phydev);
-}
-
 /**
  * phy_disable_interrupts - Disable the PHY interrupts from the PHY side
  * @phydev: target phy_device struct

@@ -769,13 +751,12 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	/* reschedule state queue work to run as soon as possible */
 	phy_trigger_machine(phydev);
 
-	if (phy_clear_interrupt(phydev))
-		goto phy_err;
-	return IRQ_HANDLED;
+	if (phy_clear_interrupt(phydev)) {
+		phydev_err(phydev, "failed to ack interrupt\n");
+		return IRQ_NONE;
+	}
 
-phy_err:
-	phy_error(phydev);
-	return IRQ_NONE;
+	return IRQ_HANDLED;
 }
 
 /**

@@ -821,16 +802,10 @@ EXPORT_SYMBOL(phy_start_interrupts);
  * phy_stop_interrupts - disable interrupts from a PHY device
  * @phydev: target phy_device struct
  */
-int phy_stop_interrupts(struct phy_device *phydev)
+void phy_stop_interrupts(struct phy_device *phydev)
 {
-	int err = phy_disable_interrupts(phydev);
-
-	if (err)
-		phy_error(phydev);
-
+	phy_disable_interrupts(phydev);
 	free_irq(phydev->irq, phydev);
-
-	return err;
 }
 EXPORT_SYMBOL(phy_stop_interrupts);
 

@@ -969,7 +944,8 @@ void phy_state_machine(struct work_struct *work)
 		phy_suspend(phydev);
 
 	if (err < 0)
-		phy_error(phydev);
+		phydev_err(phydev,
+			   "error %d executing PHY state machine\n", err);
 
 	if (old_state != phydev->state)
 		phydev_dbg(phydev, "PHY state change %s -> %s\n",

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 8f927246a..9b2235bd5 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -932,7 +932,7 @@ int phy_aneg_done(struct phy_device *phydev);
 int phy_speed_down(struct phy_device *phydev, bool sync);
 int phy_speed_up(struct phy_device *phydev);
 
-int phy_stop_interrupts(struct phy_device *phydev);
+void phy_stop_interrupts(struct phy_device *phydev);
 int phy_restart_aneg(struct phy_device *phydev);
 int phy_reset_after_clk_enable(struct phy_device *phydev);
 

-- 
2.20.0

Now that the PHY isn't stopped any longer by phylib in case of a
MDIO error, we can remove this workaround.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/freescale/fec.h      |  1 -
 drivers/net/ethernet/freescale/fec_main.c | 12 ------------
 2 files changed, 13 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index bf80855dd..f79e57f73 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h

@@ -531,7 +531,6 @@ struct fec_enet_private {
 
 	/* Phylib and MDIO interface */
 	struct	mii_bus *mii_bus;
-	int	mii_timeout;
 	uint	phy_speed;
 	phy_interface_t	phy_interface;
 	struct device_node *phy_node;

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 6db69ba30..ae0f88bce 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c

@@ -1714,12 +1714,6 @@ static void fec_enet_adjust_link(struct net_device *ndev)
 	struct phy_device *phy_dev = ndev->phydev;
 	int status_change = 0;
 
-	/* Prevent a state halted on mii error */
-	if (fep->mii_timeout && phy_dev->state == PHY_HALTED) {
-		phy_dev->state = PHY_RESUMING;
-		return;
-	}
-
 	/*
 	 * If the netdev is down, or is going down, we're not interested
 	 * in link state events, so just mark our idea of the link as down

@@ -1779,7 +1773,6 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	if (ret < 0)
 		return ret;
 
-	fep->mii_timeout = 0;
 	reinit_completion(&fep->mdio_done);
 
 	/* start a read op */

@@ -1791,7 +1784,6 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	time_left = wait_for_completion_timeout(&fep->mdio_done,
 			usecs_to_jiffies(FEC_MII_TIMEOUT));
 	if (time_left == 0) {
-		fep->mii_timeout = 1;
 		netdev_err(fep->netdev, "MDIO read timeout\n");
 		ret = -ETIMEDOUT;
 		goto out;

@@ -1820,7 +1812,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	else
 		ret = 0;
 
-	fep->mii_timeout = 0;
 	reinit_completion(&fep->mdio_done);
 
 	/* start a write op */

@@ -1833,7 +1824,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	time_left = wait_for_completion_timeout(&fep->mdio_done,
 			usecs_to_jiffies(FEC_MII_TIMEOUT));
 	if (time_left == 0) {
-		fep->mii_timeout = 1;
 		netdev_err(fep->netdev, "MDIO write timeout\n");
 		ret  = -ETIMEDOUT;
 	}

@@ -2001,8 +1991,6 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	fep->mii_timeout = 0;
-
 	/*
 	 * Set MII speed to 2.5 MHz (= clk_get_rate() / 2 * phy_speed)
 	 *

-- 
2.20.0

On 15.12.2018 17:17, Heiner Kallweit wrote:

If we detect a MDIO error, it seems to be a little bit too aggressive
to stop the state machine and bring down the PHY completely.
E.g. when polling and we miss one update, then this has no relevant
impact.

Heiner Kallweit (2):
  net: phy: don't stop state machine in case of MDIO error
  net: fec: remove workaround to restart state machine on MDIO error

 drivers/net/ethernet/freescale/fec.h      |  1 -
 drivers/net/ethernet/freescale/fec_main.c | 12 -------
 drivers/net/phy/phy.c                     | 42 +++++------------------
 include/linux/phy.h                       |  2 +-
 4 files changed, 10 insertions(+), 47 deletions(-)

David, could you please wait with applying this? I should have marked
it RFC (did so in patchwork meanwhile), need to discuss it with
Andrew and Florian.

Heiner

On Sat, Dec 15, 2018 at 05:18:33PM +0100, Heiner Kallweit wrote:

If we detect a MDIO error, it seems to be a little bit too aggressive
to stop the state machine and bring down the PHY completely.

Hi Heiner

My assumption is, if we get one MDIO error, we will gets lots more
MDIO errors. This should be the sort of bus which either works, or it
does not. In that situation, i think it does make sense to stop
everything.

I would like to know more about the FEC. Unfortunately, the commit
adding the timeout handling code does not explain why it is needed.
We might need to dig further backwards in time to figure it out.

   Andrew

On 16.12.2018 09:42, Andrew Lunn wrote:

On Sat, Dec 15, 2018 at 05:18:33PM +0100, Heiner Kallweit wrote:

quoted

If we detect a MDIO error, it seems to be a little bit too aggressive
to stop the state machine and bring down the PHY completely.

Hi Heiner

My assumption is, if we get one MDIO error, we will gets lots more
MDIO errors. This should be the sort of bus which either works, or it
does not. In that situation, i think it does make sense to stop
everything.

I would like to know more about the FEC. Unfortunately, the commit
adding the timeout handling code does not explain why it is needed.
We might need to dig further backwards in time to figure it out.

After checking the fec history it seems that at the time this
workaround was added as part of phylib support (8 years ago),
the MDIO access timeout value was too low and therefore sometimes
MDIO access failed. Later timeout was set to a higher value and
driver switched to an event-driven mechanism to signal end of
MDIO access. So it should be safe to remove the workaround.

One issue with phy_error() is that it silently stops the PHY.
We should at least add a phydev_err() to let the user know.

   Andrew

Heiner

There's a workaround to restart the phylib state machine in case of a
MDIO access timeout. Seems it was introduced to deal with the
consequences of a too small MDIO timeout. See also commit message of
c3b084c24c8a ("net: fec: Adjust ENET MDIO timeouts") which increased
the timeout value later. Due to the later timeout value fix it seems
to be safe to remove the workaround.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/freescale/fec.h      |  1 -
 drivers/net/ethernet/freescale/fec_main.c | 12 ------------
 2 files changed, 13 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index bf80855dd..f79e57f73 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h

@@ -531,7 +531,6 @@ struct fec_enet_private {
 
 	/* Phylib and MDIO interface */
 	struct	mii_bus *mii_bus;
-	int	mii_timeout;
 	uint	phy_speed;
 	phy_interface_t	phy_interface;
 	struct device_node *phy_node;

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 6db69ba30..ae0f88bce 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c

@@ -1714,12 +1714,6 @@ static void fec_enet_adjust_link(struct net_device *ndev)
 	struct phy_device *phy_dev = ndev->phydev;
 	int status_change = 0;
 
-	/* Prevent a state halted on mii error */
-	if (fep->mii_timeout && phy_dev->state == PHY_HALTED) {
-		phy_dev->state = PHY_RESUMING;
-		return;
-	}
-
 	/*
 	 * If the netdev is down, or is going down, we're not interested
 	 * in link state events, so just mark our idea of the link as down

@@ -1779,7 +1773,6 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	if (ret < 0)
 		return ret;
 
-	fep->mii_timeout = 0;
 	reinit_completion(&fep->mdio_done);
 
 	/* start a read op */

@@ -1791,7 +1784,6 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	time_left = wait_for_completion_timeout(&fep->mdio_done,
 			usecs_to_jiffies(FEC_MII_TIMEOUT));
 	if (time_left == 0) {
-		fep->mii_timeout = 1;
 		netdev_err(fep->netdev, "MDIO read timeout\n");
 		ret = -ETIMEDOUT;
 		goto out;

@@ -1820,7 +1812,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	else
 		ret = 0;
 
-	fep->mii_timeout = 0;
 	reinit_completion(&fep->mdio_done);
 
 	/* start a write op */

@@ -1833,7 +1824,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	time_left = wait_for_completion_timeout(&fep->mdio_done,
 			usecs_to_jiffies(FEC_MII_TIMEOUT));
 	if (time_left == 0) {
-		fep->mii_timeout = 1;
 		netdev_err(fep->netdev, "MDIO write timeout\n");
 		ret  = -ETIMEDOUT;
 	}

@@ -2001,8 +1991,6 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	fep->mii_timeout = 0;
-
 	/*
 	 * Set MII speed to 2.5 MHz (= clk_get_rate() / 2 * phy_speed)
 	 *

-- 
2.20.0

After checking the fec history it seems that at the time this
workaround was added as part of phylib support (8 years ago),
the MDIO access timeout value was too low and therefore sometimes
MDIO access failed. Later timeout was set to a higher value and
driver switched to an event-driven mechanism to signal end of
MDIO access. So it should be safe to remove the workaround.

Hi Heiner

Thanks for digging into the details.

One issue with phy_error() is that it silently stops the PHY.
We should at least add a phydev_err() to let the user know.

Yes, that is a good idea.

     Andrew

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 16 Dec 2018 09:29:16 +0100

On 15.12.2018 17:17, Heiner Kallweit wrote:

quoted

If we detect a MDIO error, it seems to be a little bit too aggressive
to stop the state machine and bring down the PHY completely.
E.g. when polling and we miss one update, then this has no relevant
impact.

Heiner Kallweit (2):
  net: phy: don't stop state machine in case of MDIO error
  net: fec: remove workaround to restart state machine on MDIO error

 drivers/net/ethernet/freescale/fec.h      |  1 -
 drivers/net/ethernet/freescale/fec_main.c | 12 -------
 drivers/net/phy/phy.c                     | 42 +++++------------------
 include/linux/phy.h                       |  2 +-
 4 files changed, 10 insertions(+), 47 deletions(-)

David, could you please wait with applying this? I should have marked
it RFC (did so in patchwork meanwhile), need to discuss it with
Andrew and Florian.

Ok.

I'd like to kindly ask that you not modify patchwork state even for your
own changes, and let me take care of it.

When the patch state changes behind my back it's confusing and I start
wondering if I made a mistake changing the state of patches in my queue.

Thanks.

From: Heiner Kallweit <hkallweit1@gmail.com> Sent: 2018年12月16日 0:20

Now that the PHY isn't stopped any longer by phylib in case of a MDIO error,
we can remove this workaround.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

The old workaround can be removed NOW,  thanks.

Acked-by: Fugang Duan <redacted>

From: Heiner Kallweit <hkallweit1@gmail.com> Sent: 2018年12月16日 0:19

If to stop state machine when PHY is in polling mode, so we have to return here ?

On 17.12.2018 03:05, Andy Duan wrote:

From: Heiner Kallweit <hkallweit1@gmail.com> Sent: 2018年12月16日 0:20

quoted

Now that the PHY isn't stopped any longer by phylib in case of a MDIO error,
we can remove this workaround.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

The old workaround can be removed NOW,  thanks.

This patch series has been superseded. After few more discussions we'll
keep the current behavior and just use a WARN() to facilitate debugging
in case of MDIO error.
However for fec it should be safe anyway to remove the old workaround,
see description of my subsequent patch.

Acked-by: Fugang Duan <redacted>

quoted

---
 drivers/net/ethernet/freescale/fec.h      |  1 -
 drivers/net/ethernet/freescale/fec_main.c | 12 ------------
 2 files changed, 13 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h

b/drivers/net/ethernet/freescale/fec.h
index bf80855dd..f79e57f73 100644

--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h

@@ -531,7 +531,6 @@ struct fec_enet_private {

 	/* Phylib and MDIO interface */
 	struct	mii_bus *mii_bus;
-	int	mii_timeout;
 	uint	phy_speed;
 	phy_interface_t	phy_interface;
 	struct device_node *phy_node;

diff --git a/drivers/net/ethernet/freescale/fec_main.c

b/drivers/net/ethernet/freescale/fec_main.c
index 6db69ba30..ae0f88bce 100644

--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c

@@ -1714,12 +1714,6 @@ static void fec_enet_adjust_link(struct net_device

*ndev)
 	struct phy_device *phy_dev = ndev->phydev;
 	int status_change = 0;

-	/* Prevent a state halted on mii error */
-	if (fep->mii_timeout && phy_dev->state == PHY_HALTED) {
-		phy_dev->state = PHY_RESUMING;
-		return;
-	}
-
 	/*
 	 * If the netdev is down, or is going down, we're not interested
 	 * in link state events, so just mark our idea of the link as down @@
-1779,7 +1773,6 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int
mii_id, int regnum)
 	if (ret < 0)
 		return ret;

-	fep->mii_timeout = 0;
 	reinit_completion(&fep->mdio_done);

 	/* start a read op */

@@ -1791,7 +1784,6 @@ static int fec_enet_mdio_read(struct mii_bus *bus,

int mii_id, int regnum)
 	time_left = wait_for_completion_timeout(&fep->mdio_done,
 			usecs_to_jiffies(FEC_MII_TIMEOUT));
 	if (time_left == 0) {
-		fep->mii_timeout = 1;
 		netdev_err(fep->netdev, "MDIO read timeout\n");
 		ret = -ETIMEDOUT;
 		goto out;

@@ -1820,7 +1812,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus,

int mii_id, int regnum,
 	else
 		ret = 0;

-	fep->mii_timeout = 0;
 	reinit_completion(&fep->mdio_done);

 	/* start a write op */

@@ -1833,7 +1824,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus,

int mii_id, int regnum,
 	time_left = wait_for_completion_timeout(&fep->mdio_done,
 			usecs_to_jiffies(FEC_MII_TIMEOUT));
 	if (time_left == 0) {
-		fep->mii_timeout = 1;
 		netdev_err(fep->netdev, "MDIO write timeout\n");
 		ret  = -ETIMEDOUT;
 	}

@@ -2001,8 +1991,6 @@ static int fec_enet_mii_init(struct platform_device

*pdev)
 		return -ENOENT;
 	}

-	fep->mii_timeout = 0;
-
 	/*
 	 * Set MII speed to 2.5 MHz (= clk_get_rate() / 2 * phy_speed)
 	 *
--
2.20.0