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: <05f488ea-2fe5-48b6-b4bf-c6e6d5c69461@gmail.com>
Date: Wed, 7 Feb 2024 22:19:03 +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 07.02.2024 20:51, Heiner Kallweit wrote:
> Hi Andrew,
> 
> On 04.02.2024 17:35, Heiner Kallweit wrote:
>> On 04.02.2024 17:26, Heiner Kallweit wrote:
>>> 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.
>>>

When looking a little closer at creating a helper for it, I stumbled across
the following. This register just states that the PHY can operate at a certain
speed, it leaves open which mode(s) are supported at that speed.
I see e.g. 10 different modes for 200Gbps. So it may not be possible to
create a generic helper. A translation to linkmodes is only possible if
the PHY supports just one mode per speed.

>>>>>  
>>>>>  	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;
>>>>>  
>>
>> Similarly linkmode_adv_to_mii_10gbt_adv_t() can be used here.
>>
>>>>>  		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.
>>>
> Is it fine with you to do this in a follow-up patch?
> The series is marked "under review", so Jakub seems to wait for an outcome
> of our discussion.
> 
>>>> 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
>>
> Heiner