[PATCH] net: phy: Trigger link_change_notify on PHY_HALTED

[PATCH] net: phy: Trigger link_change_notify on PHY_HALTED

[Marek Vasut]

In case the PHY transitions to PHY_HALTED state in phy_stop(), the
link_change_notify callback is not triggered. That's because the
phydev->state = PHY_HALTED in phy_stop() is assigned first, and
phy_state_machine() is called afterward. For phy_state_machine(),
no state transition happens, because old_state = PHY_HALTED and
phy_dev->state = PHY_HALTED.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: David S. Miller <davem@davemloft.net>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 45f75533c47c..fca8c3eebc5d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1004,6 +1004,7 @@ EXPORT_SYMBOL(phy_free_interrupt);
 void phy_stop(struct phy_device *phydev)
 {
 	struct net_device *dev = phydev->attached_dev;
+	enum phy_state old_state;
 
 	if (!phy_is_started(phydev) && phydev->state != PHY_DOWN) {
 		WARN(1, "called from state %s\n",
@@ -1021,8 +1022,17 @@ void phy_stop(struct phy_device *phydev)
 	if (phydev->sfp_bus)
 		sfp_upstream_stop(phydev->sfp_bus);
 
+	old_state = phydev->state;
 	phydev->state = PHY_HALTED;
 
+	if (old_state != phydev->state) {
+		phydev_err(phydev, "PHY state change %s -> %s\n",
+			   phy_state_to_str(old_state),
+			   phy_state_to_str(phydev->state));
+		if (phydev->drv && phydev->drv->link_change_notify)
+			phydev->drv->link_change_notify(phydev);
+	}
+
 	mutex_unlock(&phydev->lock);
 
 	phy_state_machine(&phydev->state_queue.work);
-- 
2.29.2

Re: [PATCH] net: phy: Trigger link_change_notify on PHY_HALTED

[Heiner Kallweit]

On 05.01.2021 17:11, Marek Vasut wrote:
> In case the PHY transitions to PHY_HALTED state in phy_stop(), the
> link_change_notify callback is not triggered. That's because the
> phydev->state = PHY_HALTED in phy_stop() is assigned first, and
> phy_state_machine() is called afterward. For phy_state_machine(),
> no state transition happens, because old_state = PHY_HALTED and
> phy_dev->state = PHY_HALTED.
> 

There are a few formal issues with this patch:
- It misses a net/net-next annotation. If it's meant to be a fix,
  then the Fixes tag is missing. I just checked the existing
  link_change_notify handlers and nobody is interested in state
  transitions to PHY_HALTED. Therefore I think it's more of an
  improvement. However AFAICS net-next is still closed.

- The maintainers should be in To: and the list(s) on cc.
- Seems that Russell and Jakub are missing as maintainers.


> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 45f75533c47c..fca8c3eebc5d 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1004,6 +1004,7 @@ EXPORT_SYMBOL(phy_free_interrupt);
>  void phy_stop(struct phy_device *phydev)
>  {
>  	struct net_device *dev = phydev->attached_dev;
> +	enum phy_state old_state;
>  
>  	if (!phy_is_started(phydev) && phydev->state != PHY_DOWN) {
>  		WARN(1, "called from state %s\n",
> @@ -1021,8 +1022,17 @@ void phy_stop(struct phy_device *phydev)
>  	if (phydev->sfp_bus)
>  		sfp_upstream_stop(phydev->sfp_bus);
>  
> +	old_state = phydev->state;
>  	phydev->state = PHY_HALTED;
>  
> +	if (old_state != phydev->state) {

This check shouldn't be needed because it shouldn't happen that
phy_stop() is called from status PHY_HALTED. In this case the
WARN() a few lines above would have fired already.

> +		phydev_err(phydev, "PHY state change %s -> %s\n",
> +			   phy_state_to_str(old_state),
> +			   phy_state_to_str(phydev->state));
> +		if (phydev->drv && phydev->drv->link_change_notify)
> +			phydev->drv->link_change_notify(phydev);

Instead of duplicating this code it could be factored out into
a small helper that is used by phy_stop() and phy_state_machine().

> +	}
> +
>  	mutex_unlock(&phydev->lock);
>  
>  	phy_state_machine(&phydev->state_queue.work);
> 

Re: [PATCH] net: phy: Trigger link_change_notify on PHY_HALTED

[Russell King - ARM Linux admin]

On Tue, Jan 05, 2021 at 05:58:21PM +0100, Heiner Kallweit wrote:
> On 05.01.2021 17:11, Marek Vasut wrote:
> > @@ -1021,8 +1022,17 @@ void phy_stop(struct phy_device *phydev)
> >  	if (phydev->sfp_bus)
> >  		sfp_upstream_stop(phydev->sfp_bus);
> >  
> > +	old_state = phydev->state;
> >  	phydev->state = PHY_HALTED;
> >  
> > +	if (old_state != phydev->state) {
> 
> This check shouldn't be needed because it shouldn't happen that
> phy_stop() is called from status PHY_HALTED. In this case the
> WARN() a few lines above would have fired already.

That is incorrect. If an error happens with the phy, phy_error() will
be called, which sets phydev->state = PHY_HALTED. If you then
subsequently take the interface down, phy_stop() will be called, but
phydev->state will be set to PHY_HALTED.

This is a long standing bug since you changed the code, and I think is
something I've reported previously, since I've definitely encountered
it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: Trigger link_change_notify on PHY_HALTED

[Heiner Kallweit]

On 05.01.2021 18:05, Russell King - ARM Linux admin wrote:
> On Tue, Jan 05, 2021 at 05:58:21PM +0100, Heiner Kallweit wrote:
>> On 05.01.2021 17:11, Marek Vasut wrote:
>>> @@ -1021,8 +1022,17 @@ void phy_stop(struct phy_device *phydev)
>>>  	if (phydev->sfp_bus)
>>>  		sfp_upstream_stop(phydev->sfp_bus);
>>>  
>>> +	old_state = phydev->state;
>>>  	phydev->state = PHY_HALTED;
>>>  
>>> +	if (old_state != phydev->state) {
>>
>> This check shouldn't be needed because it shouldn't happen that
>> phy_stop() is called from status PHY_HALTED. In this case the
>> WARN() a few lines above would have fired already.
> 
> That is incorrect. If an error happens with the phy, phy_error() will
> be called, which sets phydev->state = PHY_HALTED. If you then
> subsequently take the interface down, phy_stop() will be called, but
> phydev->state will be set to PHY_HALTED.
> 
OK, so we have to fix the way phy_error() works. Still the check isn't
needed here. So far nobody is interested in transitions to PHY_HALTED,
so nothing is broken.

> This is a long standing bug since you changed the code, and I think is
> something I've reported previously, since I've definitely encountered
> it.
> 
IIRC there was a start of a discussion whether phy_error() is useful
at all regarding how it works as of today. A single failed MDIO access
(e.g. timeout) stops the state machine, when e.g. one missed PHY status
update in polling mode doesn't really cause any harm.
If we want to keep the functionality of phy_error(), then I'd say
a separate error phy state (e.g. PHY_ERROR) would make sense, as it
would make clear that the network was stopped due to an error.

Re: [PATCH] net: phy: Trigger link_change_notify on PHY_HALTED

[Heiner Kallweit]

On 05.01.2021 17:11, Marek Vasut wrote:
> In case the PHY transitions to PHY_HALTED state in phy_stop(), the
> link_change_notify callback is not triggered. That's because the
> phydev->state = PHY_HALTED in phy_stop() is assigned first, and
> phy_state_machine() is called afterward. For phy_state_machine(),
> no state transition happens, because old_state = PHY_HALTED and
> phy_dev->state = PHY_HALTED.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 45f75533c47c..fca8c3eebc5d 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1004,6 +1004,7 @@ EXPORT_SYMBOL(phy_free_interrupt);
>  void phy_stop(struct phy_device *phydev)
>  {
>  	struct net_device *dev = phydev->attached_dev;
> +	enum phy_state old_state;
>  
>  	if (!phy_is_started(phydev) && phydev->state != PHY_DOWN) {
>  		WARN(1, "called from state %s\n",
> @@ -1021,8 +1022,17 @@ void phy_stop(struct phy_device *phydev)
>  	if (phydev->sfp_bus)
>  		sfp_upstream_stop(phydev->sfp_bus);
>  
> +	old_state = phydev->state;
>  	phydev->state = PHY_HALTED;
>  
> +	if (old_state != phydev->state) {
> +		phydev_err(phydev, "PHY state change %s -> %s\n",
> +			   phy_state_to_str(old_state),
> +			   phy_state_to_str(phydev->state));
> +		if (phydev->drv && phydev->drv->link_change_notify)
> +			phydev->drv->link_change_notify(phydev);
> +	}
> +
>  	mutex_unlock(&phydev->lock);
>  
>  	phy_state_machine(&phydev->state_queue.work);
> 

Following is RFC as an additional idea. When requesting a new state
from outside the state machine, we could simply provide the old
state to the state machine in a new phy_device member.
Then we shouldn't have to touch phy_stop(), and maybe phy_error().
And it looks cleaner to me than duplicating code from the state
machine to functions like phy_stop().

---
 drivers/net/phy/phy.c        | 10 +++++++++-
 drivers/net/phy/phy_device.c |  1 +
 include/linux/phy.h          |  3 +++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 45f75533c..347e42d90 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -45,6 +45,7 @@ static const char *phy_state_to_str(enum phy_state st)
 {
 	switch (st) {
 	PHY_STATE_STR(DOWN)
+	PHY_STATE_STR(INVALID)
 	PHY_STATE_STR(READY)
 	PHY_STATE_STR(UP)
 	PHY_STATE_STR(RUNNING)
@@ -1021,6 +1022,7 @@ void phy_stop(struct phy_device *phydev)
 	if (phydev->sfp_bus)
 		sfp_upstream_stop(phydev->sfp_bus);
 
+	phydev->old_state = phydev->state;
 	phydev->state = PHY_HALTED;
 
 	mutex_unlock(&phydev->lock);
@@ -1086,7 +1088,13 @@ void phy_state_machine(struct work_struct *work)
 
 	mutex_lock(&phydev->lock);
 
-	old_state = phydev->state;
+	/* set if a new state is requested from outside the state machine */
+	if (phydev->old_state != PHY_INVALID) {
+		old_state = phydev->old_state;
+		phydev->old_state = PHY_INVALID;
+	} else {
+		old_state = phydev->state;
+	}
 
 	switch (phydev->state) {
 	case PHY_DOWN:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 80c2e646c..9f8d9f68b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -620,6 +620,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 	device_initialize(&mdiodev->dev);
 
 	dev->state = PHY_DOWN;
+	dev->old_state = PHY_INVALID;
 
 	mutex_init(&dev->lock);
 	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 9effb511a..df48ea88c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -442,6 +442,7 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
  */
 enum phy_state {
 	PHY_DOWN = 0,
+	PHY_INVALID,
 	PHY_READY,
 	PHY_HALTED,
 	PHY_UP,
@@ -566,6 +567,8 @@ struct phy_device {
 	unsigned interrupts:1;
 
 	enum phy_state state;
+	/* if a new state is requested from outside the state machine */
+	enum phy_state old_state;
 
 	u32 dev_flags;
 
-- 
2.30.0