Re: [PATCH net-next 1/2] net: phy: improve phy_resolve_aneg_linkmode

From: Heiner Kallweit <hkallweit1@gmail.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
	David Miller <davem@davemloft.net>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH net-next 1/2] net: phy: improve phy_resolve_aneg_linkmode
Date: Fri, 15 Feb 2019 19:57:45 +0100	[thread overview]
Message-ID: <205137d8-00a7-e8d5-ba09-6a48f1f870fc@gmail.com> (raw)
In-Reply-To: <20190215135714.GI5699@lunn.ch>

On 15.02.2019 14:57, Andrew Lunn wrote:
> On Thu, Feb 14, 2019 at 10:15:31PM +0100, Heiner Kallweit wrote:
>> We have the settings array of modes which is sorted based on aneg
>> priority. Instead of checking each mode manually let's simply iterate
>> over the sorted settings.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/phy-core.c | 43 +++++++-------------------------------
>>  1 file changed, 7 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
>> index cdea028d1..5d43106fe 100644
>> --- a/drivers/net/phy/phy-core.c
>> +++ b/drivers/net/phy/phy-core.c
>> @@ -349,45 +349,16 @@ size_t phy_speeds(unsigned int *speeds, size_t size,
>>  void phy_resolve_aneg_linkmode(struct phy_device *phydev)
>>  {
>>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
>> +	int i;
>>  
>>  	linkmode_and(common, phydev->lp_advertising, phydev->advertising);
>>  
>> -	if (linkmode_test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, common)) {
>> -		phydev->speed = SPEED_10000;
>> -		phydev->duplex = DUPLEX_FULL;
>> -	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
>> -				     common)) {
>> -		phydev->speed = SPEED_5000;
>> -		phydev->duplex = DUPLEX_FULL;
>> -	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>> -				     common)) {
>> -		phydev->speed = SPEED_2500;
>> -		phydev->duplex = DUPLEX_FULL;
>> -	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
>> -				     common)) {
>> -		phydev->speed = SPEED_1000;
>> -		phydev->duplex = DUPLEX_FULL;
>> -	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
>> -				     common)) {
>> -		phydev->speed = SPEED_1000;
>> -		phydev->duplex = DUPLEX_HALF;
>> -	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
>> -				     common)) {
>> -		phydev->speed = SPEED_100;
>> -		phydev->duplex = DUPLEX_FULL;
>> -	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
>> -				     common)) {
>> -		phydev->speed = SPEED_100;
>> -		phydev->duplex = DUPLEX_HALF;
>> -	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
>> -				     common)) {
>> -		phydev->speed = SPEED_10;
>> -		phydev->duplex = DUPLEX_FULL;
>> -	} else if (linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
>> -				     common)) {
>> -		phydev->speed = SPEED_10;
>> -		phydev->duplex = DUPLEX_HALF;
>> -	}
>> +	for (i = 0; i < ARRAY_SIZE(settings); i++)
>> +		if (test_bit(settings[i].bit, common)) {
>> +			phydev->speed = settings[i].speed;
>> +			phydev->duplex = settings[i].duplex;
>> +			break;
>> +		}
> 
> Hi Heiner
> 
> Nice simplification.
> 
> I just have one thought about this. The original code was limited to
> baseT. The new code could in theory return a non BaseT speed. For that
> to happen, it would require that phydev->lp_advertising and
> phydev->advertising contain a non BaseT link mode? Is that possible?
> I don't think it is.
> 
Currently we set only BaseT modes because that's what the clause 45
standard registers offer. However drivers may come that use vendor
registers for e.g. backplane auto-negotiation (IIRC clause 73).

Now it's even better because this function shouldn't be (and doesn't
have to be) limited to a specific physical link type.

>   Andrew
> 
Heiner