[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 <sta...@vger.kernel.org> # v4.9+
Signed-off-by: Roger Quadros <rog...@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

[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 <sta...@vger.kernel.org> # v4.9+
> Signed-off-by: Roger Quadros <rog...@ti.com>

[...]

MBR, Sergei

[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 <sta...@vger.kernel.org> # v4.9+
>> Signed-off-by: Roger Quadros <rog...@ti.com>
> [...]
>
> MBR, Sergei
>

cheers,
-roger

[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 <sta...@vger.kernel.org> # v4.9+
Signed-off-by: Roger Quadros <rog...@ti.com>
---

v3: Fix typo in commit message

[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 <sta...@vger.kernel.org> # v4.9+
> Signed-off-by: Roger Quadros <rog...@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

[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 <sta...@vger.kernel.org> # v4.9+
> Signed-off-by: Roger Quadros <rog...@ti.com>
> ---
> v3: Fix typo in commit message
>
> drivers/net/phy/phy.c | 4 ++++
> 1 file changed, 4 insertions(+)

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

[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 <sta...@vger.kernel.org> # v4.9+
> Signed-off-by: Roger Quadros <rog...@ti.com>

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

[Roger Quadros]

Florian,

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

[Roger Quadros]

Hi,

Any comments on my questions?

cheers,
-roger

[Florian Fainelli]

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