Re: [PATCH net-next v2 1/6] net: dsa: add support for phylink mac_select_pcs()

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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.

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