Message-ID: <20220912101621.ttnsxmjmaor2cd7d@skbuf>
Date:   Mon, 12 Sep 2022 10:16:21 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
CC:     Colin Foster <colin.foster@...advantage.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Lee Jones <lee@...nel.org>
Subject: Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities
 for all ports

On Mon, Sep 12, 2022 at 09:48:36AM +0100, Russell King (Oracle) wrote:
> On Sun, Sep 11, 2022 at 01:02:42PM -0700, Colin Foster wrote:
> > phylink_generic_validate() requires that mac_capabilities is correctly
> > populated. While no existing drivers have used phylink_generic_validate(),
> > the ocelot_ext.c driver will. Populate this element so the use of existing
> > functions is possible.
> 
> Ocelot always fills in the .phylink_validate method in struct
> dsa_switch_ops, mac_capabilities won't be used as
> phylink_generic_validate() will not be called by
> dsa_port_phylink_validate().

Correct, but felix_phylink_validate() _can_ still directly call
phylink_validate(), right? Colin does not have the full support for
ocelot_ext in this patch set, but this is what he intends to do.

> Also "no existing drivers have used phylink_generic_validate()" I
> wonder which drivers you are referring to there. If you are referring
> to DSA drivers, then it is extensively used. The following is from
> Linus' tree as of today:

By "existing drivers", it is meant felix_vsc9959.c and seville_vsc9953.c,
two drivers in their own right, which use the common felix.c to talk to
(a) DSA and (b) the ocelot switch lib in drivers/net/ethernet/mscc/.
It is true that these existing drivers do not use phylink_generic_validate().
Furthermore, Colin's new ocelot_ext.c is on the same level as
felix_vsc9959.c and seville_vsc9953.c, will use felix.c in the same way,
and will want to use phylink_generic_validate().

> Secondly, I don't see a purpose for this patch in the following
> patches, as Ocelot continues to always fill in .phylink_validate,
> and as I mentioned above, as long as that member is filled in,
> mac_capabilities won't be used unless you explicitly call
> phylink_generic_validate() in your .phylink_validate() callback.

Yes, explicit calling is what Colin explained that he wants to do.

> Therefore, I think you can drop this patch from your series and
> you won't see any functional change.

This is true. I am also a bit surprised at Colin's choices to
(a) not ask the netdev maintainers to pull into net-next the immutable
    branch that Lee provided here:
    https://lore.kernel.org/lkml/YxrjyHcceLOFlT%2Fc@google.com/
    and instead send some patches for review which are difficult to
    apply directly to any tree
(b) split the work he submitted such that he populates mac_capabilities
    but does not make any use of it (not call phylink_generic_validate
    from anywhere). We try as much as possible to not leave dead code
    behind in the mainline tree, even if future work is intended to
    bring it to life. I do understand that this is an RFC so the patches
    weren't intended to be applied as is, but it is still confusing to
    review a change which, as you've correctly pointed out, has no
    effect to the git tree as it stands.

Powered by blists - more mailing lists