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

[Russell King (Oracle)]

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

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:

$ grep -rl 'dsa_switch_ops' drivers/net/dsa | xargs grep -l phylink_mac_ | xargs grep -L phylink_validate
drivers/net/dsa/xrs700x/xrs700x.c
drivers/net/dsa/mt7530.c
drivers/net/dsa/qca/ar9331.c
drivers/net/dsa/qca/qca8k-8xxx.c
drivers/net/dsa/bcm_sf2.c
drivers/net/dsa/rzn1_a5psw.c
drivers/net/dsa/b53/b53_common.c
drivers/net/dsa/mv88e6xxx/chip.c
drivers/net/dsa/microchip/ksz_common.c
drivers/net/dsa/sja1105/sja1105_main.c
drivers/net/dsa/lantiq_gswip.c
drivers/net/dsa/realtek/rtl8366rb.c
drivers/net/dsa/realtek/rtl8365mb.c

So, I don't think the commit description is anywhere near correct.

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.

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

> Signed-off-by: Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>
> ---
> 
> v1 from previous RFC:
>     * New patch
> 
> ---
>  drivers/net/dsa/ocelot/felix.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index 95a5c5d0815c..201bf3bdd67d 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -958,6 +958,9 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port,
>  
>  	__set_bit(ocelot->ports[port]->phy_mode,
>  		  config->supported_interfaces);
> +
> +	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 |
> +				   MAC_100 | MAC_1000FD | MAC_2500FD;
>  }
>  
>  static void felix_phylink_validate(struct dsa_switch *ds, int port,
> -- 
> 2.25.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!