Message-ID: <20220221143254.3g3iqysqkqrfu5rm@skbuf>
Date:   Mon, 21 Feb 2022 16:32:54 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Ansuel Smith <ansuelsmth@...il.com>, Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Vivien Didelot <vivien.didelot@...il.com>
Subject: Re: [PATCH net-next v2 1/6] net: dsa: add support for phylink
 mac_select_pcs()

Hello,

On Mon, Feb 21, 2022 at 01:30:09PM +0000, Russell King (Oracle) wrote:
> On Sat, Feb 19, 2022 at 11:22:24PM +0200, Vladimir Oltean wrote:
> > On Sat, Feb 19, 2022 at 11:12:41PM +0200, Vladimir Oltean wrote:
> > > >  static const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
> > > >  	.validate = dsa_port_phylink_validate,
> > > > +	.mac_select_pcs = dsa_port_phylink_mac_select_pcs,
> > > 
> > > This patch breaks probing on DSA switch drivers that weren't converted
> > > to supported_interfaces, due to this check in phylink_create():
> > 
> > And this is only the most superficial layer of breakage. Everywhere in
> > phylink.c where pl->mac_ops->mac_select_pcs() is used, its presence is
> > checked and non-zero return codes from it are treated as hard errors,
> > even -EOPNOTSUPP, even if this particular error code is probably
> > intended to behave identically as the absence of the function pointer,
> > for compatibility.
> 
> I don't understand what problem you're getting at here - and I don't
> think there is a problem.
> 
> While I know it's conventional in DSA to use EOPNOTSUPP to indicate
> that a called method is not implemented, this is not something that
> is common across the board - and is not necessary here.
> 
> The implementation of dsa_port_phylink_mac_select_pcs() returns a
> NULL PCS when the DSA operation for it is not implemented. This
> means that:
> 
> 1) phylink_validate_mac_and_pcs() won't fail due to mac_select_pcs()
>    being present but DSA drivers not implementing it.
> 
> 2) phylink_major_config() will not attempt to call phylink_set_pcs()
>    to change the PCS.
> 
> So, that much is perfectly safe.
> 
> As for your previous email reporting the problem with phylink_create(),
> thanks for the report and sorry for the breakage - the breakage was
> obviously not intended, and came about because of all the patch
> shuffling I've done over the last six months trying to get these
> changes in, and having forgotten about this dependency.
> 
> I imagine the reason you've raised EOPNOTSUPP is because you wanted to
> change dsa_port_phylink_mac_select_pcs() to return an error-pointer
> encoded with that error code rather than NULL, but you then (no
> surprises to me) caused phylink to fail.
> 
> Considering the idea of using EOPNOTSUPP, at the two places we call
> mac_select_pcs(), we would need to treat this the same way we currently
> treat NULL. We would also need phylink_create() to call
> mac_select_pcs() if the method is non-NULL to discover if the DSA
> sub-driver implements the method - but we would need to choose an
> interface at this point.
> 
> I think at this point, I'd rather:
> 
> 1) add a bool in struct phylink to indicate whether we should be calling
>    mac_select_pcs, and replace the
> 
> 	if (pl->mac_ops->mac_select_pcs)
> 
>    with
> 
>         if (pl->using_mac_select_pcs)
> 
> 2) have phylink_create() do:
> 
> 	bool using_mac_select_pcs = false;
> 
> 	if (mac_ops->mac_select_pcs &&
> 	    mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) != 
> 	      ERR_PTR(-EOPNOTSUPP))
> 		using_mac_select_pcs = true;
> 
> 	if (using_mac_select_pcs &&
> 	    phy_interface_empty(config->supported_interfaces)) {
> 		...
> 
> 	...
> 
> 	pl->using_mac_select_pcs = using_mac_select_pcs;
> 
> which should give what was intended until DSA drivers are all updated
> to fill in config->supported_interfaces.

I didn't study the problem enough to be in the position to suggest the
best solution.

As you've explained*, phylink works properly when mac_select_pcs()
returns NULL but not special error codes, so extra handling would be
required for those - and you've shown an approach that seems reasonable
if we use -EOPNOTSUPP.

Alternatively, phylink_create() gets the initial PHY interface mode
passed to it, I wonder, couldn't we call mac_select_pcs() with that in
order to determine whether the function is a stub or not? I see that
only axienet_mac_select_pcs returns NULL based on the interface mode,
and I assume it will never get passed an invalid-to-it PHY interface
mode. For this reason we can't pass PHY_INTERFACE_MODE_NA as long as we
check for NULL instead of -EOPNOTSUPP - because drivers may not expect
this PHY interface type. So this second approach may also work and may
require less phylink rework, although it may be slightly more prone to
subtle breakage, if for whatever reason I don't see right now, the
phylink instance gets created with an undetermined PHY mode.

Either of these solutions is fine by me, with -EOPNOTSUPP probably being
preferable for the extra safety at the expense of extra rework.

*and as I haven't considered, to be honest. When phylink_major_config()
gets called after a SGMII to 10GBaseR switchover, and mac_select_pcs is
called and returns NULL, the current behavior is to keep working with
the PCS for SGMII. Is that intended?

Powered by blists - more mailing lists