| 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
| ||
|
Date: Mon, 6 Jun 2022 13:47:08 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
CC: Alvin Šipraga <alvin@...s.dk>,
"luizluca@...il.com" <luizluca@...il.com>,
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 Mon, Jun 06, 2022 at 02:31:16PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 06, 2022 at 03:01:30PM +0200, Alvin Šipraga wrote:
> > From: Alvin Šipraga <alsi@...g-olufsen.dk>
> >
> > phylib defaults to GMII when no phy-mode or phy-connection-type property
> > is specified in a DSA port node.
> >
> > Commit a5dba0f207e5 ("net: dsa: rtl8365mb: add GMII as user port mode")
> > introduced implicit support for GMII mode on ports with internal PHY to
> > allow a PHY connection for device trees where the phy-mode is not
> > explicitly set to "internal".
> >
> > Commit 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")
> > then broke this behaviour by discarding the usage of
> > rtl8365mb_phy_mode_supported() - where this GMII support was indicated -
> > while switching to the new .phylink_get_caps API.
> >
> > With the new API, rtl8365mb_phy_mode_supported() is no longer needed.
> > Remove it altogether and add back the GMII capability - this time to
> > rtl8365mb_phylink_get_caps() - so that the above default behaviour works
> > for ports with internal PHY again.
>
> Oops - I guess this has been caused by the delay between my patch being
> initially prepared, it sitting around in my tree for many months while
> other patches get merged, and it eventually seeing the light of day.
>
> Sorry about that.
No problem, it's actually my bad, but I am still getting to grips with the
responsibility of maintaining a Linux driver :)
>
> > Fixes: 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")
> > Signed-off-by: Alvin Šipraga <alsi@...g-olufsen.dk>
> > ---
> >
> > 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.
For what it's worth, I believe the patch is correct and applicable in its
current form - it just lacks an explanation as to why this ever worked in the
first place.
>
> >
> > ---
> > drivers/net/dsa/realtek/rtl8365mb.c | 38 +++++++----------------------
> > 1 file changed, 9 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> > index 3bb42a9f236d..769f672e9128 100644
> > --- a/drivers/net/dsa/realtek/rtl8365mb.c
> > +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> > @@ -955,35 +955,21 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
> > return 0;
> > }
> >
> > -static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
> > - phy_interface_t interface)
> > -{
> > - int ext_int;
> > -
> > - ext_int = rtl8365mb_extint_port_map[port];
> > -
> > - if (ext_int < 0 &&
> > - (interface == PHY_INTERFACE_MODE_NA ||
> > - interface == PHY_INTERFACE_MODE_INTERNAL ||
> > - interface == PHY_INTERFACE_MODE_GMII))
> > - /* Internal PHY */
> > - return true;
> > - else if ((ext_int >= 1) &&
> > - phy_interface_mode_is_rgmii(interface))
> > - /* Extension MAC */
> > - return true;
> > -
> > - return false;
> > -}
> > -
> > static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
> > struct phylink_config *config)
> > {
> > - if (dsa_is_user_port(ds, port))
> > + if (dsa_is_user_port(ds, port)) {
>
> Given the updates to rtl8365mb_phy_mode_supported(), this misses out on
> the check of rtl8365mb_extint_port_map[port] introduced in commit
> 6147631c079f ("net: dsa: realtek: rtl8365mb: allow non-cpu extint
> ports").
You are right, but since this is targetting net and not net-next, I figured I
would not introduce any additional changes. I have a separate series lined up
for net-next (just submitted) which fixes this in a much cleaner way.
My rationale here is: yes, this test is incorrect and is a shortcoming of the
driver, but it does not represent a real bug worthy of inclusion in the stable
trees.
>
> > __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > config->supported_interfaces);
> > - else if (dsa_is_cpu_port(ds, port))
> > +
> > + /* GMII is the default interface mode for phylib, so
> > + * we have to support it for ports with integrated PHY.
> > + */
> > + __set_bit(PHY_INTERFACE_MODE_GMII,
> > + config->supported_interfaces);
> > + } else if (dsa_is_cpu_port(ds, port)) {
>
> This test also needs to be updated.
>
> Not sure what rtl8365mb_extint_port_map[port] == 0 is supposed to
> signify - maybe port unusable? Looks that way to me.
Check the other series I just sent for a cleaner implementation of this.
Powered by blists - more mailing lists