netdev - RE: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

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: <PH8PR11MB79655D0005E227742CBA1A8A95B22@PH8PR11MB7965.namprd11.prod.outlook.com>
Date: Thu, 1 Aug 2024 23:46:41 +0000
From: <Ronnie.Kunin@...rochip.com>
To: <linux@...linux.org.uk>, <Raju.Lakkaraju@...rochip.com>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>, <kuba@...nel.org>,
	<andrew@...n.ch>, <horms@...nel.org>, <hkallweit1@...il.com>,
	<richardcochran@...il.com>, <rdunlap@...radead.org>,
	<Bryan.Whitehead@...rochip.com>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<linux-kernel@...r.kernel.org>, <UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

Hi Russell,
Raju can comment more on it when he comes online tomorrow (IST time), but I recall this was discussed with you last year and my understanding is that the outcome was that as long as the need to use the dynamic fallback from phy to fixed_phy mode is explained in the commit message - which Raju did in the commit description - , it was acceptable to do this in phylink. Unless the "mechanism where a MAC driver can tell phylink to switch to using a fixed-link with certain parameters" has been implemented since then (apologize if it has, I am not a linux expert by any means, but don't seem to see it), I would guess the reasons for doing this are still valid.

Attached is the email thread with that discussion and the relevant comments are copied below.

> The reason this should work is because the fixed-phy support does emulate a real PHY in software, and phylink will treat that exactly the same way as a real PHY (because when in phylink is in PHY mode, it delegates PHY management to phylib.)
>
>Using fixed-phy with phylink will certainly raise some eyebrows, so the reasons for it will need to be set out in the commit message - and as you've dropped Andrew from this thread, I suspect he will still raise some comments about it.
>
>In the longer term, it would probably make sense for phylink to provide a mechanism where a MAC driver can tell phylink to switch to using a fixed-link with certain parameters.

Best regards,
Ronnie

-----Original Message-----
From: Russell King <linux@...linux.org.uk> 
Sent: Thursday, August 1, 2024 12:27 PM
To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@...rochip.com>
Cc: netdev@...r.kernel.org; davem@...emloft.net; kuba@...nel.org; andrew@...n.ch; horms@...nel.org; hkallweit1@...il.com; richardcochran@...il.com; rdunlap@...radead.org; Bryan Whitehead - C21958 <Bryan.Whitehead@...rochip.com>; edumazet@...gle.com; pabeni@...hat.com; linux-kernel@...r.kernel.org; UNGLinuxDriver <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Thu, Aug 01, 2024 at 04:33:13PM +0530, Raju Lakkaraju wrote:
> > > +     if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
> > > +             phydev = phy_find_first(adapter->mdiobus);
> > > +             if (!phydev) {
> > > +                     if (((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
> > > +                           ID_REV_ID_LAN7431_) || adapter->is_pci11x1x) {
> > > +                             phydev = fixed_phy_register(PHY_POLL,
> > > +                                                         &fphy_status,
> > > +                                                         NULL);
> >
> > I thought something was going to happen with this?
>
> Our SQA confirmed that it's working ping as expected (i.e Speed at 
> 1Gbps with full duplex) with Intel I210 NIC as link partner.
>
> Do you suspect any corner case where it's fail?

Let me restate the review comment from V2:

"Eww. Given that phylink has its own internal fixed-PHY support, can we not find some way to avoid the legacy fixed-PHY usage here?"

Yes, it may work, but fixed-phy is not supposed to be used with phylink.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Content of type "message/rfc822" skipped