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

[Sean Anderson]

On 11/23/21 1:15 PM, Vladimir Oltean wrote:
> On Tue, Nov 23, 2021 at 12:30:33PM -0500, Sean Anderson wrote:
>> 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."
>
> Yes, but that is not a reason why you'd want to _remove_ one. Just don't
> call phylink_set_pcs() in the first place, there you go, no PCS.

Yeah.

>> 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.
>
> Yes, but on the same port on the same board? The MAC-side PCS is an
> integral part of serial Ethernet links, be it modeled as a discrete part
> by hardware manufacturers or not. We are effectively talking about a
> situation where a serial link would become parallel, or the other way
> around. Have you seen such a thing?

I have not. It's certainly possible to create (since the serial link
often uses different physical pins from the parallel link). I think
we can cross that bridge if/when we ever come to it.

>> 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/
>
> Marek's patches are for reconfiguring the SERDES protocol on the same
> lanes. But the lanes are still physically there, and you'd need a PCS to
> talk to them no matter what you do, they won't magically turn into RGMII.
> If you need to switch the MAC PCS you're configuring with another MAC
> PCS (within the same hardware block more or less) due to the fact that
> the SERDES protocol is changing, that doesn't count as removing the PCS,
> does it? Or what are you thinking of when you say PCS? Phylink doesn't
> support any other kind of PCS than a MAC-side PCS.

I mean that with that patch applied, phylink will no longer try and
validate modes which aren't supported on a particular board (see
phylink_validate_any). Although, it looks like set_pcs never was called
in the validate path in the first place (looks like I misremembered).

--Sean