linux-kernel - Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac

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]

Message-ID: <Yx7yZESuK6Jh0Q8X@shell.armlinux.org.uk>
Date:   Mon, 12 Sep 2022 09:48:36 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Colin Foster <colin.foster@...advantage.com>
Cc:     netdev@...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,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Lee Jones <lee@...nel.org>
Subject: Re: [RFC v1 net-next 6/8] net: dsa: felix: populate mac_capabilities
 for all ports

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@...advantage.com>
> ---
> 
> 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!