[PATCH] net: phy: Make sure PHY_RESUMING state change is always processed

[PATCH] net: phy: Make sure PHY_RESUMING state change is always processed

[Tim Beale]

If phy_start_aneg() was called while the phydev is in the PHY_RESUMING
state, then its state would immediately transition to PHY_AN (or
PHY_FORCING). This meant the phy_state_machine() never processed the
PHY_RESUMING state change, which meant interrupts weren't enabled for the
PHY. If the PHY used low-power mode (i.e. using BMCR_PDOWN), then the
physical link wouldn't get powered up again.

There seems no point for phy_start_aneg() to make the PHY_RESUMING -->
PHY_AN transition, as the state machine will do this anyway. I'm not sure
about the case where autoneg is disabled, as my patch will change
behaviour so that the PHY goes to PHY_NOLINK instead of PHY_FORCING. An
alternative solution would be to move the phy_config_interrupt() and
phy_resume() work out of the state machine and into phy_start().

The background behind this: we're running linux v3.16.7 and from user-space
we want to enable the eth port (i.e. do a SIOCSIFFLAGS ioctl with the
IFF_UP flag) and immediately afterward set the interface's speed/duplex.
Enabling the interface calls .ndo_open() then phy_start() and the PHY
transitions PHY_HALTED --> PHY_RESUMING. Setting the speed/duplex ends up
calling phy_ethtool_sset(), which calls phy_start_aneg() (meanwhile the
phy_state_machine() hasn't processed the PHY_RESUMING state change yet).

Signed-off-by: Tim Beale <tim.beale@alliedtelesis.co.nz>
---
 drivers/net/phy/phy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 52cd8db..9855b96 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -465,7 +465,7 @@ int phy_start_aneg(struct phy_device *phydev)
 	if (err < 0)
 		goto out_unlock;
 
-	if (phydev->state != PHY_HALTED) {
+	if (phydev->state != PHY_HALTED && phydev->state != PHY_RESUMING) {
 		if (AUTONEG_ENABLE == phydev->autoneg) {
 			phydev->state = PHY_AN;
 			phydev->link_timeout = PHY_AN_TIMEOUT;
-- 
1.9.1

Re: [PATCH] net: phy: Make sure PHY_RESUMING state change is always processed

[Florian Fainelli]

Le 05/12/15 18:55, Tim Beale a écrit :
> If phy_start_aneg() was called while the phydev is in the PHY_RESUMING
> state, then its state would immediately transition to PHY_AN (or
> PHY_FORCING). This meant the phy_state_machine() never processed the
> PHY_RESUMING state change, which meant interrupts weren't enabled for the
> PHY. If the PHY used low-power mode (i.e. using BMCR_PDOWN), then the
> physical link wouldn't get powered up again.
> 
> There seems no point for phy_start_aneg() to make the PHY_RESUMING -->
> PHY_AN transition, as the state machine will do this anyway. I'm not sure
> about the case where autoneg is disabled, as my patch will change
> behaviour so that the PHY goes to PHY_NOLINK instead of PHY_FORCING. An
> alternative solution would be to move the phy_config_interrupt() and
> phy_resume() work out of the state machine and into phy_start().
> 
> The background behind this: we're running linux v3.16.7 and from user-space
> we want to enable the eth port (i.e. do a SIOCSIFFLAGS ioctl with the
> IFF_UP flag) and immediately afterward set the interface's speed/duplex.
> Enabling the interface calls .ndo_open() then phy_start() and the PHY
> transitions PHY_HALTED --> PHY_RESUMING. Setting the speed/duplex ends up
> calling phy_ethtool_sset(), which calls phy_start_aneg() (meanwhile the
> phy_state_machine() hasn't processed the PHY_RESUMING state change yet).

This looks correct at first glance but I would like to give it a try
first before acking this. Thanks!

> 
> Signed-off-by: Tim Beale <tim.beale@alliedtelesis.co.nz>
> ---
>  drivers/net/phy/phy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 52cd8db..9855b96 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -465,7 +465,7 @@ int phy_start_aneg(struct phy_device *phydev)
>  	if (err < 0)
>  		goto out_unlock;
>  
> -	if (phydev->state != PHY_HALTED) {
> +	if (phydev->state != PHY_HALTED && phydev->state != PHY_RESUMING) {
>  		if (AUTONEG_ENABLE == phydev->autoneg) {
>  			phydev->state = PHY_AN;
>  			phydev->link_timeout = PHY_AN_TIMEOUT;
> 


-- 
Florian

Re: [PATCH] net: phy: Make sure PHY_RESUMING state change is always processed

[Florian Fainelli]

On 12/05/15 18:55, Tim Beale wrote:
> If phy_start_aneg() was called while the phydev is in the PHY_RESUMING
> state, then its state would immediately transition to PHY_AN (or
> PHY_FORCING). This meant the phy_state_machine() never processed the
> PHY_RESUMING state change, which meant interrupts weren't enabled for the
> PHY. If the PHY used low-power mode (i.e. using BMCR_PDOWN), then the
> physical link wouldn't get powered up again.
> 
> There seems no point for phy_start_aneg() to make the PHY_RESUMING -->
> PHY_AN transition, as the state machine will do this anyway. I'm not sure
> about the case where autoneg is disabled, as my patch will change
> behaviour so that the PHY goes to PHY_NOLINK instead of PHY_FORCING. An
> alternative solution would be to move the phy_config_interrupt() and
> phy_resume() work out of the state machine and into phy_start().

Could you prepare a patch which does that? I do not have a setup where
the PHY IRQ is a dedicated interrupt line, but I might be able to test
something with a hack.

> 
> The background behind this: we're running linux v3.16.7 and from user-space
> we want to enable the eth port (i.e. do a SIOCSIFFLAGS ioctl with the
> IFF_UP flag) and immediately afterward set the interface's speed/duplex.
> Enabling the interface calls .ndo_open() then phy_start() and the PHY
> transitions PHY_HALTED --> PHY_RESUMING. Setting the speed/duplex ends up
> calling phy_ethtool_sset(), which calls phy_start_aneg() (meanwhile the
> phy_state_machine() hasn't processed the PHY_RESUMING state change yet).
> 
> Signed-off-by: Tim Beale <tim.beale@alliedtelesis.co.nz>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  drivers/net/phy/phy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 52cd8db..9855b96 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -465,7 +465,7 @@ int phy_start_aneg(struct phy_device *phydev)
>  	if (err < 0)
>  		goto out_unlock;
>  
> -	if (phydev->state != PHY_HALTED) {
> +	if (phydev->state != PHY_HALTED && phydev->state != PHY_RESUMING) {
>  		if (AUTONEG_ENABLE == phydev->autoneg) {
>  			phydev->state = PHY_AN;
>  			phydev->link_timeout = PHY_AN_TIMEOUT;
> 


-- 
Florian

Re: [PATCH] net: phy: Make sure PHY_RESUMING state change is always processed

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 15 May 2015 14:25:49 -0700

> On 12/05/15 18:55, Tim Beale wrote:
>> If phy_start_aneg() was called while the phydev is in the PHY_RESUMING
>> state, then its state would immediately transition to PHY_AN (or
>> PHY_FORCING). This meant the phy_state_machine() never processed the
>> PHY_RESUMING state change, which meant interrupts weren't enabled for the
>> PHY. If the PHY used low-power mode (i.e. using BMCR_PDOWN), then the
>> physical link wouldn't get powered up again.
>> 
>> There seems no point for phy_start_aneg() to make the PHY_RESUMING -->
>> PHY_AN transition, as the state machine will do this anyway. I'm not sure
>> about the case where autoneg is disabled, as my patch will change
>> behaviour so that the PHY goes to PHY_NOLINK instead of PHY_FORCING. An
>> alternative solution would be to move the phy_config_interrupt() and
>> phy_resume() work out of the state machine and into phy_start().
> 
> Could you prepare a patch which does that? I do not have a setup where
> the PHY IRQ is a dedicated interrupt line, but I might be able to test
> something with a hack.
> 
>> 
>> The background behind this: we're running linux v3.16.7 and from user-space
>> we want to enable the eth port (i.e. do a SIOCSIFFLAGS ioctl with the
>> IFF_UP flag) and immediately afterward set the interface's speed/duplex.
>> Enabling the interface calls .ndo_open() then phy_start() and the PHY
>> transitions PHY_HALTED --> PHY_RESUMING. Setting the speed/duplex ends up
>> calling phy_ethtool_sset(), which calls phy_start_aneg() (meanwhile the
>> phy_state_machine() hasn't processed the PHY_RESUMING state change yet).
>> 
>> Signed-off-by: Tim Beale <tim.beale@alliedtelesis.co.nz>
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Applied, thanks everyone.

RE: [PATCH] net: phy: Make sure PHY_RESUMING state change is always processed

[Tim Beale]

>> There seems no point for phy_start_aneg() to make the PHY_RESUMING -->
>> PHY_AN transition, as the state machine will do this anyway. I'm not sure
>> about the case where autoneg is disabled, as my patch will change
>> behaviour so that the PHY goes to PHY_NOLINK instead of PHY_FORCING. An
>> alternative solution would be to move the phy_config_interrupt() and
>> phy_resume() work out of the state machine and into phy_start().

> Could you prepare a patch which does that? I do not have a setup where
> the PHY IRQ is a dedicated interrupt line, but I might be able to test
> something with a hack.

Hi Florian,

I'll send another patch to the list that implements the solution in an 
alternative way (moving the phy_config_interrupt()/phy_resume() work into
phy_start()). Have a look and see what you think.

Let me know if you need anything else.

Thanks,
Tim

[PATCH] net: phy: Make sure phy_start() always re-enables the phy interrupts

[Tim Beale]

This is an alternative way of fixing:
 commit db9683fb412d ("net: phy: Make sure PHY_RESUMING state change
                      is always processed")

When the PHY state transitions from PHY_HALTED to PHY_RESUMING, there are
two things we need to do:
1). Re-enable interrupts (and power up the physical link, if powered down)
2). Update the PHY state and net-device based on the link status.

There's no strict reason why #1 has to be done from within the main
phy_state_machine() function. There is a risk that other changes to the
PHY (e.g. setting speed/duplex, which calls phy_start_aneg()) could cause
a subsequent state transition before phy_state_machine() has processed
the PHY_RESUMING state change. This would leave the PHY with interrupts
disabled and/or still in the BMCR_PDOWN/low-power mode.

Moving enabling the interrupts and phy_resume() into phy_start() will
guarantee this work always gets done. As the PHY is already in the HALTED
state and interrupts are disabled, it shouldn't conflict with any work
being done in phy_state_machine(). The downside of this change is that if
the PHY_RESUMING state is ever entered from anywhere else, it'll also have
to repeat this work.

Signed-off-by: Tim Beale <tim.beale@alliedtelesis.co.nz>
---
 drivers/net/phy/phy.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 9855b96..87677e6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -465,7 +465,7 @@ int phy_start_aneg(struct phy_device *phydev)
 	if (err < 0)
 		goto out_unlock;
 
-	if (phydev->state != PHY_HALTED && phydev->state != PHY_RESUMING) {
+	if (phydev->state != PHY_HALTED) {
 		if (AUTONEG_ENABLE == phydev->autoneg) {
 			phydev->state = PHY_AN;
 			phydev->link_timeout = PHY_AN_TIMEOUT;
@@ -742,6 +742,9 @@ EXPORT_SYMBOL(phy_stop);
  */
 void phy_start(struct phy_device *phydev)
 {
+	bool do_resume = false;
+	int err = 0;
+
 	mutex_lock(&phydev->lock);
 
 	switch (phydev->state) {
@@ -752,11 +755,22 @@ void phy_start(struct phy_device *phydev)
 		phydev->state = PHY_UP;
 		break;
 	case PHY_HALTED:
+		/* make sure interrupts are re-enabled for the PHY */
+		err = phy_enable_interrupts(phydev);
+		if (err < 0)
+			break;
+
 		phydev->state = PHY_RESUMING;
+		do_resume = true;
+		break;
 	default:
 		break;
 	}
 	mutex_unlock(&phydev->lock);
+
+	/* if phy was suspended, bring the physical link up again */
+	if (do_resume)
+		phy_resume(phydev);
 }
 EXPORT_SYMBOL(phy_start);
 
@@ -769,7 +783,7 @@ void phy_state_machine(struct work_struct *work)
 	struct delayed_work *dwork = to_delayed_work(work);
 	struct phy_device *phydev =
 			container_of(dwork, struct phy_device, state_queue);
-	bool needs_aneg = false, do_suspend = false, do_resume = false;
+	bool needs_aneg = false, do_suspend = false;
 	int err = 0;
 
 	mutex_lock(&phydev->lock);
@@ -888,14 +902,6 @@ void phy_state_machine(struct work_struct *work)
 		}
 		break;
 	case PHY_RESUMING:
-		err = phy_clear_interrupt(phydev);
-		if (err)
-			break;
-
-		err = phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
-		if (err)
-			break;
-
 		if (AUTONEG_ENABLE == phydev->autoneg) {
 			err = phy_aneg_done(phydev);
 			if (err < 0)
@@ -933,7 +939,6 @@ void phy_state_machine(struct work_struct *work)
 			}
 			phydev->adjust_link(phydev->attached_dev);
 		}
-		do_resume = true;
 		break;
 	}
 
@@ -943,8 +948,6 @@ void phy_state_machine(struct work_struct *work)
 		err = phy_start_aneg(phydev);
 	else if (do_suspend)
 		phy_suspend(phydev);
-	else if (do_resume)
-		phy_resume(phydev);
 
 	if (err < 0)
 		phy_error(phydev);
-- 
1.9.1

Re: [PATCH] net: phy: Make sure phy_start() always re-enables the phy interrupts

[David Miller]

From: Tim Beale <tim.beale@alliedtelesis.co.nz>
Date: Mon, 18 May 2015 15:38:38 +1200

> This is an alternative way of fixing:
>  commit db9683fb412d ("net: phy: Make sure PHY_RESUMING state change
>                       is always processed")
> 
> When the PHY state transitions from PHY_HALTED to PHY_RESUMING, there are
> two things we need to do:
> 1). Re-enable interrupts (and power up the physical link, if powered down)
> 2). Update the PHY state and net-device based on the link status.
> 
> There's no strict reason why #1 has to be done from within the main
> phy_state_machine() function. There is a risk that other changes to the
> PHY (e.g. setting speed/duplex, which calls phy_start_aneg()) could cause
> a subsequent state transition before phy_state_machine() has processed
> the PHY_RESUMING state change. This would leave the PHY with interrupts
> disabled and/or still in the BMCR_PDOWN/low-power mode.
> 
> Moving enabling the interrupts and phy_resume() into phy_start() will
> guarantee this work always gets done. As the PHY is already in the HALTED
> state and interrupts are disabled, it shouldn't conflict with any work
> being done in phy_state_machine(). The downside of this change is that if
> the PHY_RESUMING state is ever entered from anywhere else, it'll also have
> to repeat this work.
> 
> Signed-off-by: Tim Beale <tim.beale@alliedtelesis.co.nz>

Applied.