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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 23 Nov 2021 14:04:03 -0500
From:   Sean Anderson <sean.anderson@...o.com>
To:     Vladimir Oltean <olteanv@...il.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 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.

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