| 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
| ||
|
Date: Tue, 7 Jun 2022 14:29:39 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
CC: Alvin Šipraga <alvin@...s.dk>,
Linus Walleij <linus.walleij@...aro.org>,
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>,
Russell King <linux@...linux.org.uk>,
"open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 5/5] net: dsa: realtek: rtl8365mb: handle PHY
interface modes correctly
On Tue, Jun 07, 2022 at 11:23:40AM -0300, Luiz Angelo Daros de Luca wrote:
> > From: Alvin Šipraga <alsi@...g-olufsen.dk>
> >
> > Realtek switches in the rtl8365mb family always have at least one port
> > with a so-called external interface, supporting PHY interface modes such
> > as RGMII or SGMII. The purpose of this patch is to improve the driver's
> > handling of these ports.
> >
> > A new struct rtl8365mb_chip_info is introduced. A static instance of
> > this struct is added for each supported switch, which is distinguished
> > by its chip ID and version. Embedded in each chip_info struct is an
> > array of struct rtl8365mb_extint, describing the external interfaces
> > available. This is more specific than the old rtl8365mb_extint_port_map,
> > which was only valid for switches with up to 6 ports.
> >
> > The struct rtl8365mb_extint also contains a bitmask of supported PHY
> > interface modes, which allows the driver to distinguish which ports
> > support RGMII. This corrects a previous mistake in the driver whereby it
> > was assumed that any port with an external interface supports RGMII.
> > This is not actually the case: for example, the RTL8367S has two
> > external interfaces, only the second of which supports RGMII. The first
> > supports only SGMII and HSGMII. This new design will make it easier to
> > add support for other interface modes.
> >
> > 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.
>
> That's a nice patch. But while dealing with ext interfaces, wouldn't
> it be nice to also add
> a mask for user ports? We could easily validate if a declared dsa port
> really exists.
At best this will be useful to emit a warning for the device tree author.
Can you see any other benefit?
>
> ...
>
> > @@ -1997,33 +2122,27 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> > case RTL8365MB_CHIP_ID_8365MB_VC:
> > switch (chip_ver) {
> > case RTL8365MB_CHIP_VER_8365MB_VC:
> > - dev_info(priv->dev,
> > - "found an RTL8365MB-VC switch (ver=0x%04x)\n",
> > - chip_ver);
> > + mb->chip_info = &rtl8365mb_chip_info_8365mb_vc;
> > break;
> > case RTL8365MB_CHIP_VER_8367RB_VB:
> > - dev_info(priv->dev,
> > - "found an RTL8367RB-VB switch (ver=0x%04x)\n",
> > - chip_ver);
> > + mb->chip_info = &rtl8365mb_chip_info_8367rb_vb;
> > break;
> > case RTL8365MB_CHIP_VER_8367S:
> > - dev_info(priv->dev,
> > - "found an RTL8367S switch (ver=0x%04x)\n",
> > - chip_ver);
> > + mb->chip_info = &rtl8365mb_chip_info_8367s;
> > break;
> > default:
> > - dev_err(priv->dev, "unrecognized switch version (ver=0x%04x)",
> > - chip_ver);
> > + dev_err(priv->dev,
> > + "unrecognized switch (id=0x%04x, ver=0x%04x)",
> > + chip_id, chip_ver);
> > return -ENODEV;
> > }
>
> With this patch, we now have a nice chip_info for each device. If we
> group all of them in an array, we could iterate over them instead of
> switching over chip_id/ver. In this case, adding a new variant is just
> a matter of creating a new chip_info and adding to the array. When the
> chip id/ver is only used inside a single chip_info, I don't know if we
> should keep a macro declaring each value. For example,
>
> #define RTL8365MB_CHIP_ID_8367S 0x6367
> #define RTL8365MB_CHIP_VER_8367S 0x00A0
> ...
> +static const struct rtl8365mb_chip_info rtl8365mb_chip_info_8367s = {
> + .name = "RTL8367S",
> + .chip_id = RTL8365MB_CHIP_ID_8367S,
> + .chip_ver = RTL8365MB_CHIP_VER_8367S,
> + .extints = {
> + { 6, 1, PHY_INTF(SGMII) | PHY_INTF(HSGMII) },
> + { 7, 2, PHY_INTF(MII) | PHY_INTF(TMII) | PHY_INTF(RMII) |
> + PHY_INTF(RGMII) },
> + { /* sentinel */ }
> + },
> + .jam_table = rtl8365mb_init_jam_8365mb_vc,
> + .jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc),
> +};
>
> seems to be as clear as:
>
> +static const struct rtl8365mb_chip_info rtl8365mb_chip_info_8367s = {
> + .name = "RTL8367S",
> + .chip_id = 0x6367,
> + .chip_ver = 0x00A0,
> + .extints = {
> + { 6, 1, PHY_INTF(SGMII) | PHY_INTF(HSGMII) },
> + { 7, 2, PHY_INTF(MII) | PHY_INTF(TMII) | PHY_INTF(RMII) |
> + PHY_INTF(RGMII) },
> + { /* sentinel */ }
> + },
> + .jam_table = rtl8365mb_init_jam_8365mb_vc,
> + .jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc),
> +};
>
> but smaller and not spread over the source.
Good idea! Albeit at the cost of one more level of indentation ;-)
I'll give it a go for v2.
Kind regards,
Alvin
Powered by blists - more mailing lists