This series introduce a most awaited feature that is correctly
provide PCS with OF without having to use specific export symbol.

The concept is to implement a producer-consumer API similar to other
subsystem like clock or PHY.

That seems to be the best solution to the problem as PCS driver needs
to be detached from phylink and implement a simple way to provide a
PCS while maintaining support for probe defer or driver removal.

To keep the implementation simple, the PCS driver devs needs some
collaboration to correctly implement this. This is O.K. as helper
to correctly implement this are provided hence it's really a matter
of following a pattern to correct follow removal of a PCS driver.

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.

A PCS declared with a PCS provider implementation can be used
by declaring in the MAC OPs the .mac_select_pcs with the helper
of_phylink_mac_select_pcs().

This helper will just try every phandle declared in "pcs-handle"
until one supported for the requested interface is found.

A user for this new implementation is provided as an Airoha PCS
driver. This was also tested downstream with the IPQ95xx QCOM SoC
and with the help of Daniel also on the various Mediatek MT7988
SoC with both SFP cage implementation and DSA attached.

Lots of tests were done with driver unbind/bind and with interface
up/down. It was initially used phylink_stop to handle PCS driver
removal, but it was then decided to use dev_close with
phylink_pcs_release() as it does better handle interface drop
and communicate more info to the user than leaving the interface
in a dangling state.

Christian Marangi (6):
  net: phylink: reset PCS-Phylink double reference on phylink_stop
  net: pcs: Implement OF support for PCS driver
  net: phylink: Correctly handle PCS probe defer from PCS provider
  dt-bindings: net: ethernet-controller: permit to define multiple PCS
  net: pcs: airoha: add PCS driver for Airoha SoC
  dt-bindings: net: pcs: Document support for Airoha Ethernet PCS

 .../bindings/net/ethernet-controller.yaml     |    2 -
 .../bindings/net/pcs/airoha,pcs.yaml          |  112 +
 drivers/net/pcs/Kconfig                       |   13 +
 drivers/net/pcs/Makefile                      |    2 +
 drivers/net/pcs/pcs-airoha.c                  | 2858 +++++++++++++++++
 drivers/net/pcs/pcs.c                         |  185 ++
 drivers/net/phy/phylink.c                     |   46 +-
 include/linux/pcs/pcs-airoha.h                |   11 +
 include/linux/pcs/pcs-provider.h              |   46 +
 include/linux/pcs/pcs.h                       |   62 +
 include/linux/phylink.h                       |    2 +
 11 files changed, 3336 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/airoha,pcs.yaml
 create mode 100644 drivers/net/pcs/pcs-airoha.c
 create mode 100644 drivers/net/pcs/pcs.c
 create mode 100644 include/linux/pcs/pcs-airoha.h
 create mode 100644 include/linux/pcs/pcs-provider.h
 create mode 100644 include/linux/pcs/pcs.h

-- 
2.48.1

Hi Christian,

On 3/18/25 19:58, Christian Marangi wrote:
> This series introduce a most awaited feature that is correctly
> provide PCS with OF without having to use specific export symbol.

I've actually been working on the same problem on and off over the past
several years [1,2]. I saw your patch series and it inspired me to clean
it up a bit [3]. The merge window is closed, so I can't post it (and I
still need to test the lynx conversion a bit more), but please feel free
to have a look.

--Sean

[1] https://lore.kernel.org/netdev/20211004191527.1610759-1-sean.anderson@seco.com/
[2] https://lore.kernel.org/netdev/20221103210650.2325784-1-sean.anderson@seco.com/
[3] https://github.com/sean-anderson-seco/linux/tree/pcs

Il giorno mer 2 apr 2025 alle ore 02:14 Sean Anderson
<sean.anderson@seco.com> ha scritto:
>
> Hi Christian,
>
> On 3/18/25 19:58, Christian Marangi wrote:
> > This series introduce a most awaited feature that is correctly
> > provide PCS with OF without having to use specific export symbol.
>
> I've actually been working on the same problem on and off over the past
> several years [1,2]. I saw your patch series and it inspired me to clean
> it up a bit [3]. The merge window is closed, so I can't post it (and I
> still need to test the lynx conversion a bit more), but please feel free
> to have a look.
>

I'm working hard on v2 of this and will have major change, so I feel it's better
to wait for v2 before adding more ideas on the table.

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!

On Wed, Mar 19, 2025 at 05:29:34PM +0000, Russell King (Oracle) wrote:
> 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".

Ah I hoped the rtnl lock would protect from this. I need to check if
unpublish first cause any harm, in theory no as phylink should still
have his pointer to the pcs struct and select_pcs should not be called
on interface removal.

But this can also be handled by flagging a PCS as "to-be-removed" so
that it gets ignored if in this window it gets parsed by the PCS
provider API.

> 
> 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.
>

No problem, some subsystem are so complex that these info are pure gold
and gets lots after years, especially with global lock in place where
someone can think they are enough to handle everything.

-- 
	Ansuel