netdev - Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a4bea8c5-b7d7-41ed-9c10-47d087e7dff8@gmail.com>
Date: Sun, 4 Feb 2024 17:26:08 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Marek Behún <kabel@...nel.org>,
 Russell King - ARM Linux <linux@...linux.org.uk>,
 Jakub Kicinski <kuba@...nel.org>, David Miller <davem@...emloft.net>,
 Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO
 constants

On 04.02.2024 17:00, Andrew Lunn wrote:
> On Sun, Feb 04, 2024 at 03:17:53PM +0100, Heiner Kallweit wrote:
>> From: Marek Behún <kabel@...nel.org>
>>
>> Drop the ad-hoc MDIO constants used in the driver and use generic
>> constants instead.
>>
>> Signed-off-by: Marek Behún <kabel@...nel.org>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>>  drivers/net/phy/realtek.c | 30 +++++++++++++-----------------
>>  1 file changed, 13 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
>> index 894172a3e..ffc13c495 100644
>> --- a/drivers/net/phy/realtek.c
>> +++ b/drivers/net/phy/realtek.c
>> @@ -57,14 +57,6 @@
>>  #define RTL8366RB_POWER_SAVE			0x15
>>  #define RTL8366RB_POWER_SAVE_ON			BIT(12)
>>  
>> -#define RTL_SUPPORTS_5000FULL			BIT(14)
>> -#define RTL_SUPPORTS_2500FULL			BIT(13)
>> -#define RTL_SUPPORTS_10000FULL			BIT(0)
>> -#define RTL_ADV_2500FULL			BIT(7)
>> -#define RTL_LPADV_10000FULL			BIT(11)
>> -#define RTL_LPADV_5000FULL			BIT(6)
>> -#define RTL_LPADV_2500FULL			BIT(5)
>> -
>>  #define RTL9000A_GINMR				0x14
>>  #define RTL9000A_GINMR_LINK_STATUS		BIT(4)
>>  
>> @@ -674,11 +666,11 @@ static int rtl822x_get_features(struct phy_device *phydev)
>>  		return val;
>>  
>>  	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>> -			 phydev->supported, val & RTL_SUPPORTS_2500FULL);
>> +			 phydev->supported, val & MDIO_PMA_SPEED_2_5G);
>>  	linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
>> -			 phydev->supported, val & RTL_SUPPORTS_5000FULL);
>> +			 phydev->supported, val & MDIO_PMA_SPEED_5G);
>>  	linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
>> -			 phydev->supported, val & RTL_SUPPORTS_10000FULL);
>> +			 phydev->supported, val & MDIO_SPEED_10G);
> 
> Now that this only using generic constants, should it move into mdio.h
> as a shared helper? Is this a standard register defined in 802.3, just
> at a different address?
> 
This is register 1.4 (PMA/PMD speed ability), mapped to a vendor-specific
register. There's very few users of this register, and nothing where such
a helper could be reused.

>>  
>>  	return genphy_read_abilities(phydev);
>>  }
>> @@ -692,10 +684,11 @@ static int rtl822x_config_aneg(struct phy_device *phydev)
>>  
>>  		if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>>  				      phydev->advertising))
>> -			adv2500 = RTL_ADV_2500FULL;
>> +			adv2500 = MDIO_AN_10GBT_CTRL_ADV2_5G;
>>  
>>  		ret = phy_modify_paged_changed(phydev, 0xa5d, 0x12,
>> -					       RTL_ADV_2500FULL, adv2500);
>> +					       MDIO_AN_10GBT_CTRL_ADV2_5G,
>> +					       adv2500);
>>  		if (ret < 0)
>>  			return ret;
>>  	}
>> @@ -714,11 +707,14 @@ static int rtl822x_read_status(struct phy_device *phydev)
>>  			return lpadv;
>>  
>>  		linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
>> -			phydev->lp_advertising, lpadv & RTL_LPADV_10000FULL);
>> +				 phydev->lp_advertising,
>> +				 lpadv & MDIO_AN_10GBT_STAT_LP10G);
>>  		linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
>> -			phydev->lp_advertising, lpadv & RTL_LPADV_5000FULL);
>> +				 phydev->lp_advertising,
>> +				 lpadv & MDIO_AN_10GBT_STAT_LP5G);
>>  		linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>> -			phydev->lp_advertising, lpadv & RTL_LPADV_2500FULL);
>> +				 phydev->lp_advertising,
>> +				 lpadv & MDIO_AN_10GBT_STAT_LP2_5G);
> 
> Is this mii_10gbt_stat_mod_linkmode_lpa_t() ?
> 
Indeed, it is. Thanks for the hint. I'd prefer to submit the patch making use
of this helper as a follow-up patch. Then it's obvious that the helper is
the same as the replaced code.

> Something i've done in the past is to do this sort of conversion to
> standard macros, and the followed up with a patch which says that
> function X is now clearly the same as helper Y, so delete the function
> and use the helper...
> 
>     Andrew
Heiner