netdev - Re: [PATCH net-next v2] net: dsa: remove obsolete phylink dsa

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: <Zv64OWFyXY5B0B-l@shell.armlinux.org.uk>
Date: Thu, 3 Oct 2024 16:28:57 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Florian Fainelli <f.fainelli@...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
Subject: Re: [PATCH net-next v2] net: dsa: remove obsolete phylink dsa_switch
 operations

On Thu, Oct 03, 2024 at 05:51:03PM +0300, Vladimir Oltean wrote:
> On Thu, Oct 03, 2024 at 12:52:17PM +0100, Russell King (Oracle) wrote:
> > No driver now uses the DSA switch phylink members, so we can now remove
> > the method pointers, but we need to leave empty shim functions to allow
> > those drivers that do not provide phylink MAC operations structure to
> > continue functioning.
> > 
> > Signed-off-by: Russell King (oracle) <rmk+kernel@...linux.org.uk>
> > ---
> > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> > index 668c729946ea..09d2f5d4b3dd 100644
> > --- a/net/dsa/dsa.c
> > +++ b/net/dsa/dsa.c
> > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > index 25258b33e59e..f1e96706a701 100644
> > --- a/net/dsa/port.c
> > +++ b/net/dsa/port.c
> > @@ -1579,40 +1579,19 @@ static struct phylink_pcs *
> >  dsa_port_phylink_mac_select_pcs(struct phylink_config *config,
> >  				phy_interface_t interface)
> >  {
> > -	struct dsa_port *dp = dsa_phylink_to_port(config);
> > -	struct phylink_pcs *pcs = ERR_PTR(-EOPNOTSUPP);
> > -	struct dsa_switch *ds = dp->ds;
> > -
> > -	if (ds->ops->phylink_mac_select_pcs)
> > -		pcs = ds->ops->phylink_mac_select_pcs(ds, dp->index, interface);
> > -
> > -	return pcs;
> > +	return ERR_PTR(-EOPNOTSUPP);
> >  }
> 
> dsa_port_phylink_mac_select_pcs() didn't have to stay, as phylink_mac_select_pcs()
> is entirely optional in phylink. Otherwise:

Yes, that's correct, but let's keep it to this for the moment.

There's more to do with mac_select_pcs(). When it was introdued, we
needed a way to distinguish whether the method was actually implemented
or whether the old phylink_set_pcs() function was being used. Those days
are long gone, so returning ERR_PTR(-EOPNOTSUPP) no longer makes much
sense.

DSA's core code returns this, as does mv88e6xxx when the chip doesn't
have any pcs_ops (I'm not sure now why I did the latter now.)

So, I'd like to (a) make mv88e6xxx_mac_select_pcs() return NULL, then
kill dsa_port_phylink_mac_select_pcs() and then eliminate:

        if (mac_ops->mac_select_pcs &&
            mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
              ERR_PTR(-EOPNOTSUPP))
                using_mac_select_pcs = true;

replacing all other cases of pl->using_mac_select_pcs with a test
for pl->mac_ops->mac_select_pcs being non-NULL.

However, that's for later - I think for this patch, it makes sense
to keep returning the ERR_PTR() value because that's what it was
doing prior to this patch - we're then only removing the members
on the dsa_switch_ops and their callsite in this patch.

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