Re: [RFC] comparing the propesed implementation for standalone PCS drivers

[Simon Horman <horms@kernel.org>]

On Fri, Jun 13, 2025 at 12:06:23PM -0400, Sean Anderson wrote:
> On 6/13/25 08:55, Daniel Golle wrote:
> > Hi netdev folks,
> > 
> > there are currently 2 competing implementations for the groundworks to
> > support standalone PCS drivers.
> > 
> > https://patchwork.kernel.org/project/netdevbpf/list/?series=970582&state=%2A&archive=both
> > 
> > https://patchwork.kernel.org/project/netdevbpf/list/?series=961784&state=%2A&archive=both
> > 
> > They both kinda stalled due to a lack of feedback in the past 2 months
> > since they have been published.
> > 
> > Merging the 2 implementation is not a viable option due to rather large
> > architecture differences:
> > 
> > 				| Sean			| Ansuel
> > --------------------------------+-----------------------+-----------------------
> > Architecture			| Standalone subsystem	| Built into phylink
> > Need OPs wrapped		| Yes			| No
> > resource lifecycle		| New subsystem		| phylink
> > Supports hot remove		| Yes			| Yes
> > Supports hot add		| Yes (*)		| Yes
> > provides generic select_pcs	| No			| Yes
> > support for #pcs-cell-cells	| No			| Yes
> > allows migrating legacy drivers	| Yes			| Yes
> > comes with tested migrations	| Yes			| No
> > 
> > (*) requires MAC driver to also unload and subsequent re-probe for link
> > to work again
> > 
> > Obviously both architectures have pros and cons, here an incomplete and
> > certainly biased list (please help completing it and discussing all
> > details):
> > 
> > Standalone Subsystem (Sean)
> > 
> > pros
> > ====
> >  * phylink code (mostly) untouched
> >  * doesn't burden systems which don't use dedicated PCS drivers
> >  * series provides tested migrations for all Ethernet drivers currently
> >    using dedicated PCS drivers
> > 
> > cons
> > ====
> >  * needs wrapper for each PCS OP
> >  * more complex resource management (malloc/free) 
> >  * hot add and PCS showing up late (eg. due to deferred probe) are
> >    problematic
> >  * phylink is anyway the only user of that new subsystem
> 
> I mean, if you want I can move the whole thing to live in phylink.c, but
> that just enlarges the kernel if PCSs are not being used. The reverse
> criticism can be made for Ansuel's series: most phylink users do not
> have "dynamic" PCSs but the code is imtimately integrated with phylink
> anyway.

At the risk of stating the obvious it seems to me that a key decision
that needs to be made is weather a new subsystem is the correct direction.

If I understand things correctly it seems that not creating a new subsystem
is likely to lead to a simpler implementation, at least in the near term.
While doing so lends itself towards greater flexibility in terms of users,
I'd suggest a cleaner abstraction layer, and possibly a smaller footprint
(I assume space consumed by unused code) for cases where PCS is not used.

On the last point, I do wonder if there are other approaches to managing
the footprint. And if so, that may tip the balance towards a new subsystem.


Another way of framing this is: Say, hypothetically, Sean was to move his
implementation into phylink.c. Then we might be able to have a clearer
discussion of the merits of each implementation. Possibly driving towards
common ground. But it seems hard to do so if we're unsure if there should
be a new subsystem or not.

> > phylink-managed standalone PCS drivers (Ansuel)
> > 
> > pros
> > ====
> >  * trivial resource management
> 
> Actually, I would say the resource management is much more complex and
> difficult to follow due to being spread out over many different
> functions.
> 
> >  * no wrappers needed
> >  * full support for hot-add and deferred probe
> >  * avoids code duplication by providing generic select_pcs
> >    implementation
> >  * supports devices which provide more than one PCS port per device
> >    ('#pcs-cell-cells')
> > 
> > cons
> > ====
> >  * inclusion in phylink means more (dead) code on platforms not using
> >    dedicated PCS
> >  * series does not provide migrations for existing drivers
> >    (but that can be done after)
> >  * probably a bit harder to review as one needs to know phylink very well
> > 
> > 
> > It would be great if more people can take a look and help deciding the
> > general direction to go.
> 
> I also encourage netdev maintainers to have a look; Russell does not
> seem to have the time to review either system.
> 
> > There are many drivers awaiting merge which require such
> > infrastructure (most are fine with either of the two), some for more
> > than a year by now.
> 
> This is the major thing. PCS drivers should have been supported from the
> start of phylink, and the longer there is no solution the more legacy
> code there is to migrate.

This seems to be something we can all agree on :)