netdev - Re: [PATCH 4/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

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:   Tue, 10 May 2022 19:23:01 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Vladimir Oltean <olteanv@...il.com>
CC:     Hauke Mehrtens <hauke@...ke-m.de>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 4/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII
 support

On Tue, May 10, 2022 at 08:29:10PM +0300, Vladimir Oltean wrote:
> On Mon, May 09, 2022 at 12:48:48AM +0200, Hauke Mehrtens wrote:
> > @@ -983,14 +1295,25 @@ static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, 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))
> > +	int ext_int = rtl8365mb_extint_port_map[port];
> > +
> > +	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> > +				   MAC_10 | MAC_100 | MAC_1000FD;
> > +
> > +	if (dsa_is_user_port(ds, port)) {
> >  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> >  			  config->supported_interfaces);
> > -	else if (dsa_is_cpu_port(ds, port))
> > +	} else if (dsa_is_cpu_port(ds, port)) {
> 
> What does the quality of being a user port or a CPU port have to do with
> which interfaces are supported?

Right, I think this function was actually broken already in a few ways. The
switch will have ports with integrated PHYs, and ports with extension interfaces
like RGMII or SGMII etc. But which of those ports one uses as a CPU port, user
port, or (one day) DSA port, is of no concern to the switch. The supported
interface of a given port is a static property and simply a function of the port
number and switch model. All switch models in the family have between 1 and 2
ports with an extension interface.

Luiz introduced this map:

/* valid for all 6-port or less variants */
static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2, -1, -1};

... which I think is actually what we ought to test on. It can be improved, but
currently it is correct for all supported models.

So something like this would be correct:

static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
				       struct phylink_config *config)
{
	int ext_int = rtl8365mb_extint_port_map[port];
	if (ext_int == -1) {
		/* integrated PHY, set PHY_INTERFACE_MODE_INTERNAL etc. */
	} else {
		/* extension interface available, but here one should really
		 * check the model based on the chip ID/version, because it
		 * varies a lot
		 */
		if (model == RTL8365MB && ext_int == 1)
			/* RGMII */
		else if (model == RTL8367S && ext_int == 1)
			/* SGMII / HSGMII */
		else if (model == RTL8367S && ext_int == 2)
			/* RGMII */
		/* etc */
	}

	/* ... */
}

There are of course various ways to do this.

Hauke, do you follow what I mean?  Would you like me to prepare a patch for the
current supported models/interfaces and then you can rebase your changes on top
of that to indicate support for (H)SGMII? Or do you want to give it a try
yourself?

Kind regards,
Alvin
    

> 
> > +		if (ext_int == 1) {
> > +			__set_bit(PHY_INTERFACE_MODE_SGMII,
> > +				  config->supported_interfaces);
> > +			__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > +				  config->supported_interfaces);
> > +			config->mac_capabilities |= MAC_2500FD;
> > +		}
> >  		phy_interface_set_rgmii(config->supported_interfaces);
> > +	}
> >  
> > -	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> > -				   MAC_10 | MAC_100 | MAC_1000FD;
> >  }
> >  
> >  static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,