Message-ID: <20211123193017.rtvxyvb3oheqoxlz@skbuf>
Date:   Tue, 23 Nov 2021 21:30:17 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Sean Anderson <sean.anderson@...o.com>
Cc:     "Russell King (Oracle)" <linux@...linux.org.uk>,
        Chris Snook <chris.snook@...il.com>,
        Felix Fietkau <nbd@....name>,
        Florian Fainelli <f.fainelli@...il.com>,
        John Crispin <john@...ozen.org>,
        Mark Lee <Mark-MC.Lee@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Michal Simek <michal.simek@...inx.com>,
        Radhey Shyam Pandey <radhey.shyam.pandey@...inx.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org
Subject: Re: [PATCH RFC net-next 8/8] net: phylink: allow PCS to be removed

On Tue, Nov 23, 2021 at 02:04:03PM -0500, Sean Anderson wrote:
> 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@...linux.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.

So what do you mean, something like a MAC which can be routed dynamically
via pinctrl either to RGMII or to a SERDES lane? I would expect that
would require rather intricate interaction with the net device driver.
Can that be done today from user space, even in principle?

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

Sorry if you feel like I am asking too many questions. I just want to
understand what I'm being asked to review here :)

So going back to the initial question. What use case do these patches
help to make some progress with?

As far as I understand, being able to call phylink_set_pcs(NULL) is
basically the end goal of it. Sorry if I'm misinterpreting, Russell says
in the cover letter that "we now need to allow the PCS to be optional
for modern drivers". I really don't know how to interpret what
"optional" means. Just judging from that phrase, I don't interpret
"optional" as "having the ability to remove it dynamically", but rather
"the same driver can either interact with phylink using a pcs [on some
ports] or without a pcs [on other ports]".  But that deeply confuses me
because that's already supported, and many drivers make use of that
ability already, this is why the pl->pcs_ops checks are there.

Powered by blists - more mailing lists