Date:   Mon, 3 Dec 2018 12:43:24 +0200
From:   Anssi Hannula <anssi.hannula@...wise.fi>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH 2/2] net: phy: ensure autoneg is configured when resuming
 a phydev

On 1.12.2018 0:16, Heiner Kallweit wrote:
> On 30.11.2018 19:45, Anssi Hannula wrote:
>> When a PHY_HALTED phydev is resumed by phy_start(), it is set to
>> PHY_RESUMING to wait for the link to come up.
>>
>> However, if the phydev was put to PHY_HALTED (by e.g. phy_stop()) before
>> autonegotiation was ever started by phy_state_machine(), autonegotiation
>> remains unconfigured, i.e. phy_config_aneg() is never called.
>>
> phy_stop() should only be called once the PHY was started. But it's
> right that we don't have full control over it because misbehaving
> drivers may call phy_stop() even if the PHY hasn't been started yet.

Having run phy_start() does not guarantee that the phy_state_machine()
has been run at least once, though.

I was able to reproduce the issue by calling phy_start(); phy_stop().
Then on the next phy_start() autoneg is not configured.

> I think phy_error() and phy_stop() should be extended to set the state
> to PHY_HALTED only if the PHY is in a started state, means:
> don't touch the state if it's DOWN, READY, or HALTED.
> In case of phy_error() it's more a precaution, because it's used within
> phylib only and AFAICS it can be triggered from a started state only.

This wouldn't fix the issue that occurs when phy_stop() is called right
after phy_start(), though, as PHY is in UP state then.

>
> So yes, there is a theoretical issue. But as you wrote already, I think
> there's a better way to deal with it.
>
> For checking whether PHY is in a started state you could use the helper
> which is being discussed here:
> https://marc.info/?l=linux-netdev&m=154360368104946&w=2
>
>
>> Fix that by going to PHY_UP instead of PHY_RESUMING if autonegotiation
>> has never been configured.
>>
>> Signed-off-by: Anssi Hannula <anssi.hannula@...wise.fi>
>> ---
>>
>> This doesn't feel as clean is I'd like to, though. Maybe there is a
>> better way?
>>
>>
>>  drivers/net/phy/phy.c | 11 ++++++++++-
>>  include/linux/phy.h   |  2 ++
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index d46858694db9..7a650cce7599 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -553,6 +553,8 @@ int phy_start_aneg(struct phy_device *phydev)
>>  	if (err < 0)
>>  		goto out_unlock;
>>  
>> +	phydev->autoneg_configured = 1;
>> +
>>  	if (phydev->state != PHY_HALTED) {
>>  		if (AUTONEG_ENABLE == phydev->autoneg) {
>>  			err = phy_check_link_status(phydev);
>> @@ -893,7 +895,14 @@ void phy_start(struct phy_device *phydev)
>>  				break;
>>  		}
>>  
>> -		phydev->state = PHY_RESUMING;
>> +		/* if autoneg/forcing was never configured, go back to PHY_UP
>> +		 * to make that happen
>> +		 */
>> +		if (!phydev->autoneg_configured)
>> +			phydev->state = PHY_UP;
>> +		else
>> +			phydev->state = PHY_RESUMING;
>> +
>>  		break;
>>  	default:
>>  		break;
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 8f927246acdb..b306d5ed9d09 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -389,6 +389,8 @@ struct phy_device {
>>  	unsigned loopback_enabled:1;
>>  
>>  	unsigned autoneg:1;
>> +	/* autoneg has been configured at least once */
>> +	unsigned autoneg_configured:1;
>>  	/* The most recently read link state */
>>  	unsigned link:1;
>>  
>>

-- 
Anssi Hannula / Bitwise Oy

Powered by blists - more mailing lists