[PATCH RFC net-next 8/8] net: phylink: allow PCS to be removed

[Sean Anderson]

On 11/23/21 11:08 AM, Russell King (Oracle) wrote:
> On Tue, Nov 23, 2021 at 02:08:25PM +0200, Vladimir Oltean wrote:
>> On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:
>> > Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
>> > the PCS from phylink. This is only supported on non-legacy drivers
>> > where doing so will have no effect on the mac_config() calling
>> > behaviour.
>> >
>> > Signed-off-by: Russell King (Oracle) <rmk+kernel at armlinux.org.uk>
>> > ---
>> >  drivers/net/phy/phylink.c | 20 +++++++++++++++-----
>> >  1 file changed, 15 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> > index a935655c39c0..9f0f0e0aad55 100644
>> > --- a/drivers/net/phy/phylink.c
>> > +++ b/drivers/net/phy/phylink.c
>> > @@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
>> >   * in mac_prepare() or mac_config() methods if it is desired to dynamically
>> >   * change the PCS.
>> >   *
>> > - * Please note that there are behavioural changes with the mac_config()
>> > - * callback if a PCS is present (denoting a newer setup) so removing a PCS
>> > - * is not supported, and if a PCS is going to be used, it must be registered
>> > - * by calling phylink_set_pcs() at the latest in the first mac_config() call.
>> > + * Please note that for legacy phylink users, there are behavioural changes
>> > + * with the mac_config() callback if a PCS is present (denoting a newer setup)
>> > + * so removing a PCS is not supported. If a PCS is going to be used, it must
>> > + * be registered by calling phylink_set_pcs() at the latest in the first
>> > + * mac_config() call.
>> > + *
>> > + * For modern drivers, this may be called with a NULL pcs argument to
>> > + * disconnect the PCS from phylink.
>> >   */
>> >  void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
>> >  {
>> > +	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
>> > +		phylink_warn(pl,
>> > +			     "Removing PCS is not supported in a legacy driver");
>> > +		return;
>> > +	}
>> > +
>> >  	pl->pcs = pcs;
>> > -	pl->pcs_ops = pcs->ops;
>> > +	pl->pcs_ops = pcs ? pcs->ops : NULL;
>> >  }
>> >  EXPORT_SYMBOL_GPL(phylink_set_pcs);
>> >
>> > --
>> > 2.30.2
>> >
>>
>> I've read the discussion at
>> https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
>> and I still am not sure that I understand what is the use case behind
>> removing a PCS?
>
> Passing that to Sean to answer in detail...

My original feedback was regarding selecting the correct PCS to use. In
response to the question "What PCS do you want to use for this phy
interface mode?" a valid response is "I don't need a PCS," even if for a
different mode a valid response might be "Please use X PCS." Because
this function is used in validate(), it is necessary to evaluate
"what-if" scenarios, even if a scenario requiring a PCS and one
requiring no PCS would never actually be configured.

Typically the PCS is physically attached to the next layer in the link,
even if the hardware could be configured not to use the PCS. So it does
not usually make sense to configure a link to use modes both requiring a
PCS and requiring no PCS. However, it is possible that such a system
could exist. Most systems should use `phy-mode` to restrict the phy
interfaces modes to whatever makes sense for the board. I think Marek's
series (and specifically [1]) is an good step in this regard.

--Sean

[1] https://lore.kernel.org/netdev/20211123164027.15618-5-kabel@kernel.org/