netdev - RE: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs

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:   Mon, 22 Jun 2020 13:51:55 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        "michael@...le.cc" <michael@...le.cc>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "olteanv@...il.com" <olteanv@...il.com>
Subject: RE: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops


> Subject: Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops
> 
> On Mon, Jun 22, 2020 at 12:35:06PM +0000, Ioana Ciornei wrote:
> >
> > > Subject: Re: [PATCH net-next v3 5/9] net: dsa: add support for
> > > phylink_pcs_ops
> > >
> > > On Mon, Jun 22, 2020 at 12:10:57PM +0100, Russell King - ARM Linux
> > > admin
> > > wrote:
> > > > On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux
> > > > admin
> > > wrote:
> > > > > On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote:
> > > > > > In order to support split PCS using PHYLINK properly, we need
> > > > > > to add a phylink_pcs_ops structure.
> > > > > >
> > > > > > Note that a DSA driver that wants to use these needs to
> > > > > > implement all 4 of them: the DSA core checks the presence of
> > > > > > these 4 function pointers in dsa_switch_ops and only then does
> > > > > > it add a PCS to PHYLINK. This is done in order to preserve
> > > > > > compatibility with drivers that have not yet been converted,
> > > > > > or don't need, a split PCS
> > > setup.
> > > > > >
> > > > > > Also, when pcs_get_state() and pcs_an_restart() are present,
> > > > > > their mac counterparts (mac_pcs_get_state(), mac_an_restart())
> > > > > > will no longer get called, as can be seen in phylink.c.
> > > > >
> > > > > I don't like this at all, it means we've got all this useless
> > > > > layering, and that layering will force similar layering veneers
> > > > > into other parts of the kernel (such as the DPAA2 MAC driver,
> > > > > when we eventually come to re-use pcs-lynx there.)
> > > > >
> >
> > The veneers that you are talking about are one phylink_pcs_ops
> > structure and 4 functions that call lynx_pcs_* subsequently. We have
> > the same thing for the MAC operations.
> >
> > Also, the "veneers" in DSA are just how it works, and I don't want to
> > change its structure without a really good reason and without a green
> > light from DSA maintainers.
> 
> Right, but we're talking about hardware that is common not only in DSA but
> elsewhere - and we already deal with that outside of DSA with PHYs.

I said before why the PHY use case is different from a PCS tightly
coupled inside the SoC.

> So, what I'm proposing is really nothing new for DSA.
> 
> > > > > I don't think we need that - I think we can get to a position
> > > > > where pcs-lynx is called requesting that it bind to phylink as
> > > > > the PCS, and it calls phylink_add_pcs() directly, which means we
> > > > > do not end up with veneers in DSA nor in the DPAA2 MAC driver -
> > > > > they just need to call the pcs-lynx initialisation function with
> > > > > the phylink instance for it to attach to.
> >
> > What I am most concerned about is that by passing the PCS ops directly
> > to the PCS module we would lose any ability to apply SoC specific
> > quirks at runtime such as errata workarounds.
> 
> Do you know what those errata would be?  I'm only aware of A-011118 in the
> LX2160A which I don't believe will impact this code.  I don't have visibility of
> Ocelot/Felix.

I was mainly looking at this from a software architecture perspective, not having
any explicit erratum in mind.

> 
> > On the other hand, I am not sure what is the concrete benefit of doing
> > it your way. I understand that for a PHY device the MAC is not
> > involved in the call path but in the case of the PCS the expectation
> > is that it's tightly coupled in the silicon and not plug-and-play.
> 
> The advantage is less lines of code to maintain, and a more efficient and
> understandable code structure.  I would much rather start off simple and then
> augment rather than start off with unnecessary complexity and then get stuck
> with it while not really needing it.
> 

I am not going to argue ad infinitum on this. If you think keeping the
phylink_pcs_ops structure outside is the better approach then let's take that route. 

I will go through your feedback on the actual Lynx module and respond/make the
necessary adjustments.

Beside this, what should be our next move? Will you submit the new method of
working with the phylink_pcs_ops structure?

Ioana