Re: [net-next PATCH 3/6] net: phylink: Correctly handle PCS probe defer from PCS provider

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

On Wed, Mar 19, 2025 at 05:18:50PM +0100, Christian Marangi wrote:
> > >  	linkmode_fill(pl->supported);
> > >  	linkmode_copy(pl->link_config.advertising, pl->supported);
> > > -	phylink_validate(pl, pl->supported, &pl->link_config);
> > > +	ret = phylink_validate(pl, pl->supported, &pl->link_config);
> > > +	/* The PCS might not available at the time phylink_create
> > > +	 * is called. Check this and communicate to the MAC driver
> > > +	 * that probe should be retried later.
> > > +	 *
> > > +	 * Notice that this can only happen in probe stage and PCS
> > > +	 * is expected to be avaialble in phylink_major_config.
> > > +	 */
> > > +	if (ret == -EPROBE_DEFER) {
> > > +		kfree(pl);
> > > +		return ERR_PTR(ret);
> > > +	}
> > 
> > This does not solve the problem - what if the interface mode is
> > currently not one that requires a PCS that may not yet be probed?
> 
> Mhhh but what are the actual real world scenario for this? If a MAC
> needs a dedicated PCS to handle multiple mode then it will probably
> follow this new implementation and register as a provider.
> 
> An option to handle your corner case might be an OP that wait for each
> supported interface by the MAC and make sure there is a possible PCS for
> it. And Ideally place it in the codeflow of validate_pcs ?

I think you've fallen in to the trap of the stupid drivers that
implement mac_select_pcs() as:

static struct phylink_pcs *foo_mac_select_pcs(struct phylink_config *config,
					      phy_interface_t interface)
{
	struct foo_private *priv = phylink_to_foo(config);

	return priv->pcs;
}

but what drivers can (and should) be doing is looking at the interface
argument, and working out which interface to return.

Phylink is not designed to be single interface mode, single PCS driver
despite what many MAC drivers do. Checking the phylink_validate()
return code doesn't mean that all PCS exist for the MAC.

> > I don't like the idea that mac_select_pcs() might be doing a complex
> > lookup - that could make scanning the interface modes (as
> > phylink_validate_mask() does) quite slow and unreliable, and phylink
> > currently assumes that a PCS that is validated as present will remain
> > present.
> 
> The assumption "will remain present" is already very fragile with the
> current PCS so I feel this should be changed or improved. Honestly every
> PCS currently implemented can be removed and phylink will stay in an
> undefined state.

The fragility is because of the way networking works - there's nothing
phylink can do about this.

I take issue with "every PCS currently implemented" because it's
actually not a correct statement.

XPCS as used by stmmac does not fall into this.
The PCS used by mvneta and mvpp2 do not fall into this.
The PCS used by the Marvell DSA driver do not fall into this.

It's only relatively recently with pcs-lynx and others that people have
wanted them to be separate driver-model devices that this problem has
occurred, and I've been pushing back on it saying we need to find a
proper solution to it. I really haven't liked that we've merged drivers
that cause this fragility without addressing that fragility.

I've got to the point where I'm now saying no to new drivers that fail
to address this, so we're at a crunch time when it needs to be
addressed.

We need to think about how to get around this fragility. The need to
pre-validate the link modes comes from the netdev ethtool user
interface itself - the need to tell userspace what link modes can be
supported _before_ they get used. This API hasn't been designed with
the idea that parts of a netdev might vanish at any particular time.

> > If it goes away by the time phylink_major_config() is called, then we
> > leave the phylink state no longer reflecting how the hardware is
> > programmed, but we still continue to call mac_link_up() - which should
> > probably be fixed.
> 
> Again, the idea to prevent these kind of chicken-egg problem is to
> enforce correct removal on the PCS driver side.
> 
> > Given that netdev is severely backlogged, I'm not inclined to add to
> > the netdev maintainers workloads by trying to fix this until after
> > the merge window - it looks like they're at least one week behind.
> > Consequently, I'm expecting that most patches that have been
> > submitted during this week will be dropped from patchwork, which
> > means submitting patches this week is likely not useful.
> 
> Ok I will send next revision as RFC to not increase the "load" but IMHO
> it's worth to discuss this... I really feel we need to fix the PCS
> situation ASAP or more driver will come. (there are already 3 in queue
> as stressed in the cover letter)

Yes, we do need to fix it, but we need to recognise _all_ the issues
it creates by doing this, and how we handle it properly.

Right now, it's up to the MAC driver to get all the PCS it needs
during its probe function, and *not* in the mac_select_pcs() method
which has no way to propagate an error to anywhere sensible that
could handle an EPROBE_DEFER response.

My thoughts are that if a PCS goes away after a MAC driver has "got"
it, then:

1. we need to recognise that those PHY interfaces and/or link modes
   are no longer available.
2. if the PCS was in-use, then the link needs to be taken down at
   minimum and the .pcs_disable() method needs to be called to
   release any resources that .pcs_enable() enabled (e.g. irq masks,
   power enables, etc.)
3. the MAC driver needs to be notified that the PCS pointer it
   stashed is no longer valid, so it doesn't return it for
   mac_select_pcs().

There's probably a bunch more that needs to happen, and maybe need
to consider how to deal with "pcs came back".. but I haven't thought
that through yet.

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