linux-kernel - Re: [PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY

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: <Yp9kjD0D9XYIvran@shell.armlinux.org.uk>
Date:   Tue, 7 Jun 2022 15:45:32 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc:     Luiz Angelo Daros de Luca <luizluca@...il.com>,
        Alvin Šipraga <alvin@...s.dk>,
        Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for
 ports with internal PHY

On Tue, Jun 07, 2022 at 02:17:44PM +0000, Alvin Šipraga wrote:
> On Tue, Jun 07, 2022 at 03:05:40PM +0100, Russell King (Oracle) wrote:
> > On Tue, Jun 07, 2022 at 10:52:48AM -0300, Luiz Angelo Daros de Luca wrote:
> > > > > > Luiz, Russel:
> > > > > >
> > > > > > Commit a5dba0f207e5 ought to have had a Fixes: tag I think, because it
> > > > > > claims to have been fixing a regression in the net-next tree - is that
> > > > > > right? I seem to have missed both referenced commits when they were
> > > > > > posted and never hit this issue personally. I only found things now
> > > > > > during some other refactoring and the test for GMII looked weird to me
> > > > > > so I went and investigated.
> > > > > >
> > > > > > Could you please help me identify that Fixes: tag? Just for my own
> > > > > > understanding of what caused this added requirement for GMII on ports
> > > > > > with internal PHY.
> > > > >
> > > > > I have absolutely no idea. I don't think any "requirement" has ever been
> > > > > added - phylib has always defaulted to GMII, so as the driver stood when
> > > > > it was first submitted on Oct 18 2021, I don't see how it could have
> > > > > worked, unless the DT it was being tested with specified a phy-mode of
> > > > > "internal". As you were the one who submitted it, you would have a
> > > > > better idea.
> > > > >
> > > > > The only suggestion I have is to bisect to find out exactly what caused
> > > > > the GMII vs INTERNAL issue to crop up.
> > > >
> > > > Alright, thanks for the quick response. Maybe Luiz has a better idea, otherwise
> > > > I will try bisecting if I find the time.
> > > 
> > > I don't know. I just got hit by the issue after a rebase (sorry, I
> > > don't know exactly from which commit I was rebasing).
> > > But I did test the net (!-next) and left a working commit note. You
> > > can diff 3dd7d40b43..a5dba0f20.
> > > If I'm to guess, I would blame:
> > > 
> > > 21bd64bd717de: net: dsa: consolidate phylink creation
> > 
> > Why do you suspect that commit? I fail to see any functional change in
> > that commit that would cause the problem.
> 
> Agree, seems like the referenced commit makes no functional change.
> 
> But thanks for the range of commits Luiz, I found one that looks like the
> culprit. It's small so I will reproduce the whole thing below. Will test later.

This one I agree could well be the culpret, but it means that the
original premise that PHY_INTERFACE_MODE_INTERNAL was being used is
incorrect - it's actually been relying on using PHY_INTERFACE_MODE_NA.

It instead means that PHY_INTERFACE_MODE_NA was being used, which
really isn't good, because PHY_INTERFACE_MODE_NA internally inside
phylink has always had a special meaning - that being with the
validate step which has been used to get _all_ possible modes from
the MAC. This was never intended to be used for anything except
phylink's internal use to retrieve that information from the MAC
driver to make decisions about what mode(s) a SFP should use.

So yes, this is most likely the culpret, and if proven, please use
it for the Fixes: tag for any fixes to drivers that incorrectly
relied upon that behaviour.

Thanks.

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