[PATCH] net phy: Check for aneg completion before setting state to PHY_RUNNING

[PATCH] net phy: Check for aneg completion before setting state to PHY_RUNNING

[Balakumaran Kannan]

phy_state_machine should check whether auto-negotiatin is completed
before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If
auto-negotiation is not completed phydev->state should be set to
PHY_AN.

Signed-off-by: Balakumaran Kannan <kumaran.4353@gmail.com>
---
 drivers/net/phy/phy.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1b6d09a..a972056 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -765,6 +765,17 @@ void phy_state_machine(struct work_struct *work)
 			break;
 
 		if (phydev->link) {
+			if (AUTONEG_ENABLE == phydev->autoneg) {
+				err = phy_aneg_done(phydev);
+				if (err < 0)
+					break;
+
+				if (!err) {
+					phydev->state = PHY_AN;
+					phydev->link_timeout = PHY_AN_TIMEOUT;
+					break;
+				}
+			}
 			phydev->state = PHY_RUNNING;
 			netif_carrier_on(phydev->attached_dev);
 			phydev->adjust_link(phydev->attached_dev);
-- 
1.8.2.1

Re: [PATCH] net phy: Check for aneg completion before setting state to PHY_RUNNING

[Florian Fainelli]

2014-04-23 19:52 GMT-07:00 Balakumaran Kannan <kumaran.4353@gmail.com>:
> phy_state_machine should check whether auto-negotiatin is completed
> before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If
> auto-negotiation is not completed phydev->state should be set to
> PHY_AN.

I need some time to review this. Just out of curiosity, is this fixing
a bug you have encountered or was this found by state machine
inspection?

>
> Signed-off-by: Balakumaran Kannan <kumaran.4353@gmail.com>
> ---
>  drivers/net/phy/phy.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 1b6d09a..a972056 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -765,6 +765,17 @@ void phy_state_machine(struct work_struct *work)
>                         break;
>
>                 if (phydev->link) {
> +                       if (AUTONEG_ENABLE == phydev->autoneg) {
> +                               err = phy_aneg_done(phydev);
> +                               if (err < 0)
> +                                       break;
> +
> +                               if (!err) {
> +                                       phydev->state = PHY_AN;
> +                                       phydev->link_timeout = PHY_AN_TIMEOUT;
> +                                       break;
> +                               }
> +                       }
>                         phydev->state = PHY_RUNNING;
>                         netif_carrier_on(phydev->attached_dev);
>                         phydev->adjust_link(phydev->attached_dev);
> --
> 1.8.2.1



-- 
Florian

Re: [PATCH] net phy: Check for aneg completion before setting state to PHY_RUNNING

[Balakumaran Kannan]

On Fri, Apr 25, 2014 at 12:23 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 2014-04-23 19:52 GMT-07:00 Balakumaran Kannan <kumaran.4353@gmail.com>:
>> phy_state_machine should check whether auto-negotiatin is completed
>> before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If
>> auto-negotiation is not completed phydev->state should be set to
>> PHY_AN.
>
> I need some time to review this. Just out of curiosity, is this fixing
> a bug you have encountered or was this found by state machine
> inspection?

Actually I encountered a problem that Linux-3.4 sends Neighbor Solicitation (NS)
before hardware completes auto-negotiation. The driver being used for
my hardware
is smsc911x.

While investigation on this, I came to know that state of phydev
follows this sequence
PHY_READY --> PHY_UP (in phy_start)
PHY_UP --> PHY_AN (in phy_start_aneg called by phy_state_machine)
PHY_AN --> PHY_NOLINK (in phy_state_machine. Link is down)
PHY_NOLINK --> PHY_RUNNING (in phy_state_machine. Link becomes ready)

So I felt that the sequence of PHY_AN --> PHY_RUNNING while link is up and
PHY_NOLINK --> PHY_RUNNING are different. In the earlier case aneg completion
is taken into account but not in later.

But this patch doesn't fix my problem. I made the smsc driver's open function to
poll on phy_aneg_done until auto-negotiation completes.

Here I have a doubt.
 * Linux network initialization and phy_state_machine runs in separate threads.
 * And driver doesn't bother about state of phydev.
Is it correct?
Doesn't driver need to confirm phydev is ready(PHY_RUNNING) before
returning to kernel?

Regards,
K.Balakumaran

Re: [PATCH] net phy: Check for aneg completion before setting state to PHY_RUNNING

[David Miller]

From: Balakumaran Kannan <kumaran.4353@gmail.com>
Date: Fri, 25 Apr 2014 11:23:03 +0530

> Here I have a doubt.
>  * Linux network initialization and phy_state_machine runs in separate threads.
>  * And driver doesn't bother about state of phydev.
> Is it correct?
> Doesn't driver need to confirm phydev is ready(PHY_RUNNING) before
> returning to kernel?

The one comment I will make about things here is that the phylib layer
really should not call netif_carrier_on() until the PHY really is
confirmed to be in a state where it will successfully pass traffic.

If it is doing so before auto-negotiation completes and succeeds,
that would be a bug.

Re: [PATCH] net phy: Check for aneg completion before setting state to PHY_RUNNING

[David Miller]

From: Balakumaran Kannan <kumaran.4353@gmail.com>
Date: Thu, 24 Apr 2014 08:22:47 +0530

> phy_state_machine should check whether auto-negotiatin is completed
> before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If
> auto-negotiation is not completed phydev->state should be set to
> PHY_AN.
> 
> Signed-off-by: Balakumaran Kannan <kumaran.4353@gmail.com>

Florian can you please review this at your earliest possible
convenience?

Thanks!

Re: [PATCH] net phy: Check for aneg completion before setting state to PHY_RUNNING

[Florian Fainelli]

2014-04-24 22:53 GMT-07:00 Balakumaran Kannan <kumaran.4353@gmail.com>:
> On Fri, Apr 25, 2014 at 12:23 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> 2014-04-23 19:52 GMT-07:00 Balakumaran Kannan <kumaran.4353@gmail.com>:
>>> phy_state_machine should check whether auto-negotiatin is completed
>>> before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If
>>> auto-negotiation is not completed phydev->state should be set to
>>> PHY_AN.
>>
>> I need some time to review this. Just out of curiosity, is this fixing
>> a bug you have encountered or was this found by state machine
>> inspection?
>
> Actually I encountered a problem that Linux-3.4 sends Neighbor Solicitation (NS)
> before hardware completes auto-negotiation. The driver being used for
> my hardware
> is smsc911x.
>
> While investigation on this, I came to know that state of phydev
> follows this sequence
> PHY_READY --> PHY_UP (in phy_start)
> PHY_UP --> PHY_AN (in phy_start_aneg called by phy_state_machine)
> PHY_AN --> PHY_NOLINK (in phy_state_machine. Link is down)
> PHY_NOLINK --> PHY_RUNNING (in phy_state_machine. Link becomes ready)
>
> So I felt that the sequence of PHY_AN --> PHY_RUNNING while link is up and
> PHY_NOLINK --> PHY_RUNNING are different. In the earlier case aneg completion
> is taken into account but not in later.

I see, we should indeed go back to PHY_AN

>
> But this patch doesn't fix my problem. I made the smsc driver's open function to
> poll on phy_aneg_done until auto-negotiation completes.
>
> Here I have a doubt.
>  * Linux network initialization and phy_state_machine runs in separate threads.
>  * And driver doesn't bother about state of phydev.
> Is it correct?

That is correct, once your driver calls phy_start(), the PHY state
machine executes in the background within the context of a separate
workqueue.

> Doesn't driver need to confirm phydev is ready(PHY_RUNNING) before
> returning to kernel?

phy_start() really starts the PHY state machine, but does not block
until the PHY is in the PHY_RUNNING state, which is something that
might never happen if e.g: the cable is disconnected.

What really tells the network stack whether the link is ready, is when
the PHY state machine calls netif_carrier_on().
-- 
Florian

Re: [PATCH] net phy: Check for aneg completion before setting state to PHY_RUNNING

[Florian Fainelli]

2014-04-23 19:52 GMT-07:00 Balakumaran Kannan <kumaran.4353@gmail.com>:
> phy_state_machine should check whether auto-negotiatin is completed
> before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If
> auto-negotiation is not completed phydev->state should be set to
> PHY_AN.
>
> Signed-off-by: Balakumaran Kannan <kumaran.4353@gmail.com>

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

> ---
>  drivers/net/phy/phy.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 1b6d09a..a972056 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -765,6 +765,17 @@ void phy_state_machine(struct work_struct *work)
>                         break;
>
>                 if (phydev->link) {
> +                       if (AUTONEG_ENABLE == phydev->autoneg) {
> +                               err = phy_aneg_done(phydev);
> +                               if (err < 0)
> +                                       break;
> +
> +                               if (!err) {
> +                                       phydev->state = PHY_AN;
> +                                       phydev->link_timeout = PHY_AN_TIMEOUT;
> +                                       break;
> +                               }
> +                       }
>                         phydev->state = PHY_RUNNING;
>                         netif_carrier_on(phydev->attached_dev);
>                         phydev->adjust_link(phydev->attached_dev);
> --
> 1.8.2.1



-- 
Florian

Re: [PATCH] net phy: Check for aneg completion before setting state to PHY_RUNNING

[David Miller]

From: Balakumaran Kannan <kumaran.4353@gmail.com>
Date: Thu, 24 Apr 2014 08:22:47 +0530

> phy_state_machine should check whether auto-negotiatin is completed
> before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If
> auto-negotiation is not completed phydev->state should be set to
> PHY_AN.
> 
> Signed-off-by: Balakumaran Kannan <kumaran.4353@gmail.com>

Applied, thanks.