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:

quoted

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/ (local)
    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.