[PATCH v2 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs

[PATCH v2 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs

[Roger Quadros]

he ethernet link on an interrupt driven PHY was not coming up if the
ethernet cable was plugged before the ethernet interface was brought up.

The PHY state machine seems to be stuck from RUNNING to AN state
with no new interrupts from the PHY. So it doesn't know when the
PHY Auto-negotiation has been completed and doesn't transition to RUNNING
state with ANEG done thus netif_carrier_on() is never called.

NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
advertisement parameters didn't change.

Fix this by scheduling the PHY state machine in phy_start_aneg().

Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.")
Cc: stable <stable@vger.kernel.org> # v4.9+
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/phy/phy.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1be69d8..49dedf8 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -630,6 +630,10 @@ int phy_start_aneg(struct phy_device *phydev)
 
 out_unlock:
 	mutex_unlock(&phydev->lock);
+	if (!err && phy_interrupt_is_valid(phydev))
+		queue_delayed_work(system_power_efficient_wq,
+				   &phydev->state_queue, HZ);
+
 	return err;
 }
 EXPORT_SYMBOL(phy_start_aneg);
-- 
2.7.4

Re: [PATCH v2 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs

[Sergei Shtylyov]

Hello!

On 3/22/2017 2:02 PM, Roger Quadros wrote:

> he ethernet link on an interrupt driven PHY was not coming up if the

    s/he/The/?

> ethernet cable was plugged before the ethernet interface was brought up.

    Also, my spell checker trips on "ethernet", perhaps should be capitalized?

> The PHY state machine seems to be stuck from RUNNING to AN state
> with no new interrupts from the PHY. So it doesn't know when the
> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
> state with ANEG done thus netif_carrier_on() is never called.
>
> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
> advertisement parameters didn't change.
>
> Fix this by scheduling the PHY state machine in phy_start_aneg().
>
> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.")
> Cc: stable <stable@vger.kernel.org> # v4.9+
> Signed-off-by: Roger Quadros <rogerq@ti.com>
[...]

MBR, Sergei

Re: [PATCH v2 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs

[Roger Quadros]

On 23/03/17 11:52, Sergei Shtylyov wrote:
> Hello!
> 
> On 3/22/2017 2:02 PM, Roger Quadros wrote:
> 
>> he ethernet link on an interrupt driven PHY was not coming up if the
> 
>    s/he/The/?
> 
>> ethernet cable was plugged before the ethernet interface was brought up.
> 
>    Also, my spell checker trips on "ethernet", perhaps should be capitalized?

Thanks. I'll fix both issues.

> 
>> The PHY state machine seems to be stuck from RUNNING to AN state
>> with no new interrupts from the PHY. So it doesn't know when the
>> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
>> state with ANEG done thus netif_carrier_on() is never called.
>>
>> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
>> advertisement parameters didn't change.
>>
>> Fix this by scheduling the PHY state machine in phy_start_aneg().
>>
>> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.")
>> Cc: stable <stable@vger.kernel.org> # v4.9+
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> [...]
> 
> MBR, Sergei
> 

cheers,
-roger

[PATCH v3 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs

[Roger Quadros]

The Ethernet link on an interrupt driven PHY was not coming up if the
Ethernet cable was plugged before the Ethernet interface was brought up.

The PHY state machine seems to be stuck from RUNNING to AN state
with no new interrupts from the PHY. So it doesn't know when the
PHY Auto-negotiation has been completed and doesn't transition to RUNNING
state with ANEG done thus netif_carrier_on() is never called.

NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
advertisement parameters didn't change.

Fix this by scheduling the PHY state machine in phy_start_aneg().
There is no way of knowing in phy.c whether auto-negotiation was
restarted or not by the PHY driver so we just wait for the next
poll/interrupt to update the PHY state machine.

Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.")
Cc: stable <stable@vger.kernel.org> # v4.9+
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
v3: Fix typo in commit message

 drivers/net/phy/phy.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1be69d8..49dedf8 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -630,6 +630,10 @@ int phy_start_aneg(struct phy_device *phydev)
 
 out_unlock:
 	mutex_unlock(&phydev->lock);
+	if (!err && phy_interrupt_is_valid(phydev))
+		queue_delayed_work(system_power_efficient_wq,
+				   &phydev->state_queue, HZ);
+
 	return err;
 }
 EXPORT_SYMBOL(phy_start_aneg);
-- 
2.7.4

Re: [PATCH v3 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs

[Roger Quadros]

+Andrew Davis & Sekhar.

Hi,

Andrew Davis posted a few comments offline which I'm clarifying here.

On 27/03/17 14:59, Roger Quadros wrote:
> The Ethernet link on an interrupt driven PHY was not coming up if the
> Ethernet cable was plugged before the Ethernet interface was brought up.
> 
> The PHY state machine seems to be stuck from RUNNING to AN state
> with no new interrupts from the PHY. So it doesn't know when the
> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
> state with ANEG done thus netif_carrier_on() is never called.
> 
> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
> advertisement parameters didn't change.

Is phy->config_aneg expected to *always* restart auto-negotiation even if
advertisement parameters didn't change?
If so then we'll need to fix genphy_config_aneg().

> 
> Fix this by scheduling the PHY state machine in phy_start_aneg().
> There is no way of knowing in phy.c whether auto-negotiation was
> restarted or not by the PHY driver so we just wait for the next
> poll/interrupt to update the PHY state machine.
> 
> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.")
> Cc: stable <stable@vger.kernel.org> # v4.9+
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> v3: Fix typo in commit message
> 
>  drivers/net/phy/phy.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 1be69d8..49dedf8 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -630,6 +630,10 @@ int phy_start_aneg(struct phy_device *phydev)
>  
>  out_unlock:
>  	mutex_unlock(&phydev->lock);
> +	if (!err && phy_interrupt_is_valid(phydev))
> +		queue_delayed_work(system_power_efficient_wq,
> +				   &phydev->state_queue, HZ);
> +
>  	return err;
>  }
>  EXPORT_SYMBOL(phy_start_aneg);
> 

There is still room for optimization for interrupt driven PHYs as I still
see a delay of 1 second between "ifconfig ethx up" and link status coming up
if cable was already plugged in. In fact if Auto-negotiation was already completed
and not required to be restarted, the PHY state machine should have move from
AN to RUNNING instantly without expecting a PHY interrupt.

How can we get rid of the unnecessary delay in the case where auto-negotiation
is not restarted?
Should we check for phy_aneg_done() immediately after issuing a phy_start_aneg()
in phy_state_machine() and switch from PHY_AN to PHY_RUNNING?

This should avoid the need to re-schedule the state machine in phy_start_angeg().

cheers,
-roger

Re: [PATCH v3 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs

[Roger Quadros]

Hi,

On 28/03/17 13:05, Roger Quadros wrote:
> +Andrew Davis & Sekhar.
> 
> Hi,
> 
> Andrew Davis posted a few comments offline which I'm clarifying here.
> 
> On 27/03/17 14:59, Roger Quadros wrote:
>> The Ethernet link on an interrupt driven PHY was not coming up if the
>> Ethernet cable was plugged before the Ethernet interface was brought up.
>>
>> The PHY state machine seems to be stuck from RUNNING to AN state
>> with no new interrupts from the PHY. So it doesn't know when the
>> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
>> state with ANEG done thus netif_carrier_on() is never called.
>>
>> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
>> advertisement parameters didn't change.
> 
> Is phy->config_aneg expected to *always* restart auto-negotiation even if
> advertisement parameters didn't change?
> If so then we'll need to fix genphy_config_aneg().
> 
>>
>> Fix this by scheduling the PHY state machine in phy_start_aneg().
>> There is no way of knowing in phy.c whether auto-negotiation was
>> restarted or not by the PHY driver so we just wait for the next
>> poll/interrupt to update the PHY state machine.
>>
>> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.")
>> Cc: stable <stable@vger.kernel.org> # v4.9+
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> v3: Fix typo in commit message
>>
>>  drivers/net/phy/phy.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 1be69d8..49dedf8 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -630,6 +630,10 @@ int phy_start_aneg(struct phy_device *phydev)
>>  
>>  out_unlock:
>>  	mutex_unlock(&phydev->lock);
>> +	if (!err && phy_interrupt_is_valid(phydev))
>> +		queue_delayed_work(system_power_efficient_wq,
>> +				   &phydev->state_queue, HZ);
>> +
>>  	return err;
>>  }
>>  EXPORT_SYMBOL(phy_start_aneg);
>>
> 
> There is still room for optimization for interrupt driven PHYs as I still
> see a delay of 1 second between "ifconfig ethx up" and link status coming up
> if cable was already plugged in. In fact if Auto-negotiation was already completed
> and not required to be restarted, the PHY state machine should have move from
> AN to RUNNING instantly without expecting a PHY interrupt.
> 
> How can we get rid of the unnecessary delay in the case where auto-negotiation
> is not restarted?
> Should we check for phy_aneg_done() immediately after issuing a phy_start_aneg()
> in phy_state_machine() and switch from PHY_AN to PHY_RUNNING?
> 
> This should avoid the need to re-schedule the state machine in phy_start_angeg().

Any comments on my questions?

cheers,
-roger

Re: [PATCH v3 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs

[Florian Fainelli]

On 04/11/2017 04:17 AM, Roger Quadros wrote:
> Hi,
> 
> On 28/03/17 13:05, Roger Quadros wrote:
>> +Andrew Davis & Sekhar.
>>
>> Hi,
>>
>> Andrew Davis posted a few comments offline which I'm clarifying here.
>>
>> On 27/03/17 14:59, Roger Quadros wrote:
>>> The Ethernet link on an interrupt driven PHY was not coming up if the
>>> Ethernet cable was plugged before the Ethernet interface was brought up.
>>>
>>> The PHY state machine seems to be stuck from RUNNING to AN state
>>> with no new interrupts from the PHY. So it doesn't know when the
>>> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
>>> state with ANEG done thus netif_carrier_on() is never called.
>>>
>>> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
>>> advertisement parameters didn't change.
>>
>> Is phy->config_aneg expected to *always* restart auto-negotiation even if
>> advertisement parameters didn't change?
>> If so then we'll need to fix genphy_config_aneg().
>>
>>>
>>> Fix this by scheduling the PHY state machine in phy_start_aneg().
>>> There is no way of knowing in phy.c whether auto-negotiation was
>>> restarted or not by the PHY driver so we just wait for the next
>>> poll/interrupt to update the PHY state machine.
>>>
>>> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.")
>>> Cc: stable <stable@vger.kernel.org> # v4.9+
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> ---
>>> v3: Fix typo in commit message
>>>
>>>  drivers/net/phy/phy.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>> index 1be69d8..49dedf8 100644
>>> --- a/drivers/net/phy/phy.c
>>> +++ b/drivers/net/phy/phy.c
>>> @@ -630,6 +630,10 @@ int phy_start_aneg(struct phy_device *phydev)
>>>  
>>>  out_unlock:
>>>  	mutex_unlock(&phydev->lock);
>>> +	if (!err && phy_interrupt_is_valid(phydev))
>>> +		queue_delayed_work(system_power_efficient_wq,
>>> +				   &phydev->state_queue, HZ);
>>> +
>>>  	return err;
>>>  }
>>>  EXPORT_SYMBOL(phy_start_aneg);
>>>
>>
>> There is still room for optimization for interrupt driven PHYs as I still
>> see a delay of 1 second between "ifconfig ethx up" and link status coming up
>> if cable was already plugged in. In fact if Auto-negotiation was already completed
>> and not required to be restarted, the PHY state machine should have move from
>> AN to RUNNING instantly without expecting a PHY interrupt.
>>
>> How can we get rid of the unnecessary delay in the case where auto-negotiation
>> is not restarted?
>> Should we check for phy_aneg_done() immediately after issuing a phy_start_aneg()
>> in phy_state_machine() and switch from PHY_AN to PHY_RUNNING?

That sounds like a good idea yes. It seems to me like Alexander's patch
actually takes care of that:

http://patchwork.ozlabs.org/patch/752288/

Let's try to merge threads/recipients so we can shoot for a fix to be
included soon.

Thanks!

>>
>> This should avoid the need to re-schedule the state machine in phy_start_angeg().
> 
> Any comments on my questions?
> 
> cheers,
> -roger
> 


-- 
Florian

RE: [PATCH v3 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs

[Madalin-Cristian Bucur]

On March 27, 2017 2:59 PM, Roger Quadros wrote:
> The Ethernet link on an interrupt driven PHY was not coming up if the
> Ethernet cable was plugged before the Ethernet interface was brought up.
> 
> The PHY state machine seems to be stuck from RUNNING to AN state
> with no new interrupts from the PHY. So it doesn't know when the
> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
> state with ANEG done thus netif_carrier_on() is never called.
> 
> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
> advertisement parameters didn't change.
> 
> Fix this by scheduling the PHY state machine in phy_start_aneg().
> There is no way of knowing in phy.c whether auto-negotiation was
> restarted or not by the PHY driver so we just wait for the next
> poll/interrupt to update the PHY state machine.
> 
> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and
> not polling.")
> Cc: stable <stable@vger.kernel.org> # v4.9+
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> v3: Fix typo in commit message
> 
>  drivers/net/phy/phy.c | 4 ++++
>  1 file changed, 4 insertions(+)

Tested-by: Madalin Bucur <madalin.bucur@nxp.com>

Re: [PATCH v3 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs

[Florian Fainelli]

On 03/27/2017 04:59 AM, Roger Quadros wrote:
> The Ethernet link on an interrupt driven PHY was not coming up if the
> Ethernet cable was plugged before the Ethernet interface was brought up.
> 
> The PHY state machine seems to be stuck from RUNNING to AN state
> with no new interrupts from the PHY. So it doesn't know when the
> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
> state with ANEG done thus netif_carrier_on() is never called.
> 
> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
> advertisement parameters didn't change.
> 
> Fix this by scheduling the PHY state machine in phy_start_aneg().
> There is no way of knowing in phy.c whether auto-negotiation was
> restarted or not by the PHY driver so we just wait for the next
> poll/interrupt to update the PHY state machine.
> 
> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.")
> Cc: stable <stable@vger.kernel.org> # v4.9+
> Signed-off-by: Roger Quadros <rogerq@ti.com>

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

Re: [PATCH v3 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs

[Roger Quadros]

Florian,

On 30/03/17 23:02, Florian Fainelli wrote:
> On 03/27/2017 04:59 AM, Roger Quadros wrote:
>> The Ethernet link on an interrupt driven PHY was not coming up if the
>> Ethernet cable was plugged before the Ethernet interface was brought up.
>>
>> The PHY state machine seems to be stuck from RUNNING to AN state
>> with no new interrupts from the PHY. So it doesn't know when the
>> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
>> state with ANEG done thus netif_carrier_on() is never called.
>>
>> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
>> advertisement parameters didn't change.
>>
>> Fix this by scheduling the PHY state machine in phy_start_aneg().
>> There is no way of knowing in phy.c whether auto-negotiation was
>> restarted or not by the PHY driver so we just wait for the next
>> poll/interrupt to update the PHY state machine.
>>
>> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.")
>> Cc: stable <stable@vger.kernel.org> # v4.9+
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
Thanks for the review, but there are a still few unanswered questions in the parallel thread.
Can you please clarify those first before this patch gets picked? Thanks.

cheers,
-roger

[PATCH v2 2/2] net: phy: Don't miss phy_suspend() on PHY_HALTED for PHYs with interrupts

[Roger Quadros]

phy_suspend() doesn't get called as part of phy_stop() for PHYs
using interrupts because the phy state machine is never triggered
after a phy_stop().

Explicitly trigger the PHY state machine in phy_stop() so that it can
see the new PHY state (HALTED) and suspend the PHY.

As most PHYLIB consumers will call phy_stop() with rtnl_lock() held
from ndo_close() so we use don't wait for workqueue cancellation in
phy_trigger_machine() by passing false for the 'sync' argument.

Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.")
Cc: stable <stable@vger.kernel.org> # v4.9+
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/phy/phy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 49dedf8..ab14e7b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -907,6 +907,7 @@ void phy_stop(struct phy_device *phydev)
 	 * of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
 	 * will not reenable interrupts.
 	 */
+	phy_trigger_machine(phydev, false);
 }
 EXPORT_SYMBOL(phy_stop);
 
-- 
2.7.4

Re: [PATCH v2 2/2] net: phy: Don't miss phy_suspend() on PHY_HALTED for PHYs with interrupts

[Florian Fainelli]

On 03/22/2017 04:02 AM, Roger Quadros wrote:
> phy_suspend() doesn't get called as part of phy_stop() for PHYs
> using interrupts because the phy state machine is never triggered
> after a phy_stop().
> 
> Explicitly trigger the PHY state machine in phy_stop() so that it can
> see the new PHY state (HALTED) and suspend the PHY.
> 
> As most PHYLIB consumers will call phy_stop() with rtnl_lock() held
> from ndo_close() so we use don't wait for workqueue cancellation in
> phy_trigger_machine() by passing false for the 'sync' argument.

Sorry for this long delay in responding. I am not exactly sure if this
thing to do here. phy_stop() does not have a requirement to suspend the
PHY as it is currently defined. You may want to manually suspend the PHY
after a phy_stop() by explicitly calling phy_suspend().

Let me think about it some more.

> 
> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.")
> Cc: stable <stable@vger.kernel.org> # v4.9+
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/net/phy/phy.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 49dedf8..ab14e7b 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -907,6 +907,7 @@ void phy_stop(struct phy_device *phydev)
>  	 * of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
>  	 * will not reenable interrupts.
>  	 */
> +	phy_trigger_machine(phydev, false);
>  }
>  EXPORT_SYMBOL(phy_stop);
>  
> 


-- 
Florian