Re: [net-next PATCH 0/6] net: pcs: Introduce support for PCS OF

[Russell King (Oracle)]

On Wed, Mar 19, 2025 at 12:58:36AM +0100, Christian Marangi wrote:
> A PCS provider have to implement and call of_pcs_add_provider() in
> probe function and define an xlate function to define how the PCS
> should be provided based on the requested interface and phandle spec
> defined in DT (based on the #pcs-cells)
> 
> of_pcs_get() is provided to provide a specific PCS declared in DT
> an index.
> 
> A simple xlate function is provided for simple single PCS
> implementation, of_pcs_simple_get.
> 
> A PCS provider on driver removal should first call
> phylink_pcs_release() to release the PCS from phylink and then
> delete itself as a provider with of_pcs_del_provider() helper.

This is inherently racy.

phylink_pcs_release() may release the PCS from phylink, but there is a
window between calling this and of_pcs_del_provider() where it could
still be "got".

The sequence always has to be:

First, unpublish to prevent new uses.
Then remove from current uses.
Then disable hardware/remove resources.

It makes me exceedingly sad that we make keep implementing the same
mistakes time and time again - it was brought up at one of the OLS
conferences back in the 2000s, probably around the time that the
driver model was just becoming "a thing". At least I can pass on
this knowledge when I spot it and help others to improve!

Note that networking's unregister_netdev() recognises this pattern,
and unregister_netdev() will first unpublish the interface thereby
making it inaccessible to be brought up, then take the interface down
if it were up before returning - thus guaranteeing that when the
function returns, it is safe to dispose of any and all resources that
the driver was using.

Sorry as I seem to be labouring this point.

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