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: <4be28ade-366e-4433-afd3-07a1cf3ffec2@lunn.ch>
Date: Wed, 7 Feb 2024 21:28:11 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Heiner Kallweit <hkallweit1@...il.com>
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 Wed, Feb 07, 2024 at 08:51:29PM +0100, 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.

Nothing at the moment. If ixgbe ever gets converted to phylib, or at
least converted to link modes, it could use it. But i think it should
be in mdio.h. That is where people will look for such a helper, and
might overlook it here. We want to encourage such helpers and there
use.

> >>> 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.

Converting to use the standard helps can be a follow-up patch. And
moving the helper into mdio.h as well.

Reviewed-by: Andrew Lunn <andrew@...n.ch>

    Andrew