* [PATCH net-next 0/2] net: phy: don't stop state machine in case of MDIO error
@ 2018-12-15 16:17 Heiner Kallweit
2018-12-15 16:18 ` [PATCH net-next 1/2] " Heiner Kallweit
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Heiner Kallweit @ 2018-12-15 16:17 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller, Fugang Duan
Cc: netdev@vger.kernel.org
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next 1/2] net: phy: don't stop state machine in case of MDIO error
2018-12-15 16:17 [PATCH net-next 0/2] net: phy: don't stop state machine in case of MDIO error Heiner Kallweit
@ 2018-12-15 16:18 ` Heiner Kallweit
2018-12-16 8:42 ` Andrew Lunn
2018-12-17 2:14 ` Andy Duan
2018-12-15 16:19 ` [PATCH net-next 2/2] net: fec: remove workaround to restart state machine on " Heiner Kallweit
` (2 subsequent siblings)
3 siblings, 2 replies; 12+ messages in thread
From: Heiner Kallweit @ 2018-12-15 16:18 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller, Fugang Duan
Cc: netdev@vger.kernel.org
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next 2/2] net: fec: remove workaround to restart state machine on MDIO error
2018-12-15 16:17 [PATCH net-next 0/2] net: phy: don't stop state machine in case of MDIO error Heiner Kallweit
2018-12-15 16:18 ` [PATCH net-next 1/2] " Heiner Kallweit
@ 2018-12-15 16:19 ` Heiner Kallweit
2018-12-17 2:05 ` Andy Duan
2018-12-16 8:29 ` [PATCH net-next 0/2] net: phy: don't stop state machine in case of " Heiner Kallweit
2018-12-16 14:00 ` [PATCH net-next] net: fec: remove workaround to restart phylib state machine on MDIO timeout Heiner Kallweit
3 siblings, 1 reply; 12+ messages in thread
From: Heiner Kallweit @ 2018-12-15 16:19 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller, Fugang Duan
Cc: netdev@vger.kernel.org
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/2] net: phy: don't stop state machine in case of MDIO error
2018-12-15 16:17 [PATCH net-next 0/2] net: phy: don't stop state machine in case of MDIO error Heiner Kallweit
2018-12-15 16:18 ` [PATCH net-next 1/2] " Heiner Kallweit
2018-12-15 16:19 ` [PATCH net-next 2/2] net: fec: remove workaround to restart state machine on " Heiner Kallweit
@ 2018-12-16 8:29 ` Heiner Kallweit
2018-12-16 18:43 ` David Miller
2018-12-16 14:00 ` [PATCH net-next] net: fec: remove workaround to restart phylib state machine on MDIO timeout Heiner Kallweit
3 siblings, 1 reply; 12+ messages in thread
From: Heiner Kallweit @ 2018-12-16 8:29 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller, Fugang Duan
Cc: netdev@vger.kernel.org
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: don't stop state machine in case of MDIO error
2018-12-15 16:18 ` [PATCH net-next 1/2] " Heiner Kallweit
@ 2018-12-16 8:42 ` Andrew Lunn
2018-12-16 9:40 ` Heiner Kallweit
2018-12-17 2:14 ` Andy Duan
1 sibling, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2018-12-16 8:42 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Florian Fainelli, David Miller, Fugang Duan,
netdev@vger.kernel.org
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: don't stop state machine in case of MDIO error
2018-12-16 8:42 ` Andrew Lunn
@ 2018-12-16 9:40 ` Heiner Kallweit
2018-12-16 15:30 ` Andrew Lunn
0 siblings, 1 reply; 12+ messages in thread
From: Heiner Kallweit @ 2018-12-16 9:40 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, David Miller, Fugang Duan,
netdev@vger.kernel.org
On 16.12.2018 09:42, Andrew Lunn wrote:
> 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.
>
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next] net: fec: remove workaround to restart phylib state machine on MDIO timeout
2018-12-15 16:17 [PATCH net-next 0/2] net: phy: don't stop state machine in case of MDIO error Heiner Kallweit
` (2 preceding siblings ...)
2018-12-16 8:29 ` [PATCH net-next 0/2] net: phy: don't stop state machine in case of " Heiner Kallweit
@ 2018-12-16 14:00 ` Heiner Kallweit
3 siblings, 0 replies; 12+ messages in thread
From: Heiner Kallweit @ 2018-12-16 14:00 UTC (permalink / raw)
To: David Miller, Fugang Duan; +Cc: netdev@vger.kernel.org
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: don't stop state machine in case of MDIO error
2018-12-16 9:40 ` Heiner Kallweit
@ 2018-12-16 15:30 ` Andrew Lunn
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2018-12-16 15:30 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Florian Fainelli, David Miller, Fugang Duan,
netdev@vger.kernel.org
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/2] net: phy: don't stop state machine in case of MDIO error
2018-12-16 8:29 ` [PATCH net-next 0/2] net: phy: don't stop state machine in case of " Heiner Kallweit
@ 2018-12-16 18:43 ` David Miller
0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2018-12-16 18:43 UTC (permalink / raw)
To: hkallweit1; +Cc: andrew, f.fainelli, fugang.duan, netdev
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 16 Dec 2018 09:29:16 +0100
> 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.
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH net-next 2/2] net: fec: remove workaround to restart state machine on MDIO error
2018-12-15 16:19 ` [PATCH net-next 2/2] net: fec: remove workaround to restart state machine on " Heiner Kallweit
@ 2018-12-17 2:05 ` Andy Duan
2018-12-17 6:42 ` Heiner Kallweit
0 siblings, 1 reply; 12+ messages in thread
From: Andy Duan @ 2018-12-17 2:05 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn, Florian Fainelli, David Miller
Cc: netdev@vger.kernel.org
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 <fugang.duan@nxp.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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH net-next 1/2] net: phy: don't stop state machine in case of MDIO error
2018-12-15 16:18 ` [PATCH net-next 1/2] " Heiner Kallweit
2018-12-16 8:42 ` Andrew Lunn
@ 2018-12-17 2:14 ` Andy Duan
1 sibling, 0 replies; 12+ messages in thread
From: Andy Duan @ 2018-12-17 2:14 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn, Florian Fainelli, David Miller
Cc: netdev@vger.kernel.org
From: Heiner Kallweit <hkallweit1@gmail.com> Sent: 2018年12月16日 0:19
> 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 to stop state machine when PHY is in polling mode, so we have to return here ?
>
> 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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] net: fec: remove workaround to restart state machine on MDIO error
2018-12-17 2:05 ` Andy Duan
@ 2018-12-17 6:42 ` Heiner Kallweit
0 siblings, 0 replies; 12+ messages in thread
From: Heiner Kallweit @ 2018-12-17 6:42 UTC (permalink / raw)
To: Andy Duan, Andrew Lunn, Florian Fainelli, David Miller
Cc: netdev@vger.kernel.org
On 17.12.2018 03:05, Andy Duan wrote:
> 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.
>
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 <fugang.duan@nxp.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
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-12-17 6:42 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-15 16:17 [PATCH net-next 0/2] net: phy: don't stop state machine in case of MDIO error Heiner Kallweit
2018-12-15 16:18 ` [PATCH net-next 1/2] " Heiner Kallweit
2018-12-16 8:42 ` Andrew Lunn
2018-12-16 9:40 ` Heiner Kallweit
2018-12-16 15:30 ` Andrew Lunn
2018-12-17 2:14 ` Andy Duan
2018-12-15 16:19 ` [PATCH net-next 2/2] net: fec: remove workaround to restart state machine on " Heiner Kallweit
2018-12-17 2:05 ` Andy Duan
2018-12-17 6:42 ` Heiner Kallweit
2018-12-16 8:29 ` [PATCH net-next 0/2] net: phy: don't stop state machine in case of " Heiner Kallweit
2018-12-16 18:43 ` David Miller
2018-12-16 14:00 ` [PATCH net-next] net: fec: remove workaround to restart phylib state machine on MDIO timeout Heiner Kallweit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).