linux-kernel - Re: [PATCH net-next v2 5/5] net: dsa: realtek: rtl8365mb: handle PHY interface modes correctly

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]

Date:   Fri, 10 Jun 2022 17:36:39 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Alvin Šipraga <alvin@...s.dk>
Cc:     hauke@...ke-m.de, Linus Walleij <linus.walleij@...aro.org>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 5/5] net: dsa: realtek: rtl8365mb: handle PHY
 interface modes correctly

On Fri, Jun 10, 2022 at 05:38:29PM +0200, Alvin Šipraga wrote:
> Finally, rtl8365mb_phylink_get_caps() is fixed up to return supported
> capabilities based on the external interface properties described above.
> This allows for ports with an external interface to be treated as DSA
> user ports, and for ports with an internal PHY to be treated as DSA CPU
> ports.

I've needed to read that a few times... and I'm still not sure. You seem
to be saying that:
- ports with an internal PHY (which presumably provide baseT connections?)
  are used as DSA CPU ports.
- ports with an external interface supporting a range of RGMII, SGMII and
  HSGMII interface modes are DSA user ports.

With Marvell switches, it's the other way around - the ports with an
internal PHY are normally DSA user ports. Other ports can be a user,
inter-switch or CPU port.

So, I'm slightly confused by your description.

In any ase, looking at the get_caps() (and only that):

> @@ -953,7 +1026,13 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  				       struct phylink_config *config)
>  {
> -	if (dsa_is_user_port(ds, port)) {
> +	const struct rtl8365mb_extint *extint =
> +		rtl8365mb_get_port_extint(ds->priv, port);
> +
> +	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> +				   MAC_10 | MAC_100 | MAC_1000FD;

MAC capabilities are constant across all interfaces - okay.

> +
> +	if (!extint) {
>  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>  			  config->supported_interfaces);
>  
> @@ -962,12 +1041,16 @@ static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  		 */
>  		__set_bit(PHY_INTERFACE_MODE_GMII,
>  			  config->supported_interfaces);
> -	} else if (dsa_is_cpu_port(ds, port)) {
> -		phy_interface_set_rgmii(config->supported_interfaces);
> +		return;

Internal ports need to support phylib, so both internal and gmii
interface modes - okay.

>  	}
>  
> -	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> -				   MAC_10 | MAC_100 | MAC_1000FD;
> +	/* Populate according to the modes supported by _this driver_,
> +	 * not necessarily the modes supported by the hardware, some of
> +	 * which remain unimplemented.
> +	 */
> +
> +	if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_RGMII)
> +		phy_interface_set_rgmii(config->supported_interfaces);

External ports that support RGMII get all RGMII modes - also okay.

So, for the get_cops() function, I'm fine with this new code, and for
this alone:

Acked-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>

I haven't looked at the remainder of the changes.

Thanks.

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