Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities for all ports

From: Colin Foster <colin.foster@in-advantage.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: "Russell King (Oracle)" <linux@armlinux.org.uk>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Paolo Abeni <pabeni@redhat.com>, Jakub Kicinski <kuba@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	"UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Lee Jones <lee@kernel.org>
Subject: Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities for all ports
Date: Mon, 12 Sep 2022 08:47:47 -0700	[thread overview]
Message-ID: <Yx9Uo9RiI7bRZvLC@colin-ia-desktop> (raw)
In-Reply-To: <20220912101621.ttnsxmjmaor2cd7d@skbuf>

On Mon, Sep 12, 2022 at 10:16:21AM +0000, Vladimir Oltean wrote:
> 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.

As you mentioned, I do in fact call phylink_generic_validate() in 8/8.

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

As mentioned in the cover letter, I don't expect this to necessarily be
ready by the next merge window. But seemingly I misjudged whether
merging the net-next and Lee's tree would be more tedious for the netdev
maintainers than looking at the RFC for reviewers. I'm trying to create
as little hassle for people as I can. Apologies.