| 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, 10 May 2022 16:32:44 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Hauke Mehrtens <hauke@...ke-m.de>
CC: "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>,
"olteanv@...il.com" <olteanv@...il.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/4] net: dsa: realtek: rtl8365mb: Get chip option
On Mon, May 09, 2022 at 12:48:46AM +0200, Hauke Mehrtens wrote:
> Read the option register in addition to the other registers to identify
> the chip. The SGMII initialization is different for the different chip
> options.
>
> Signed-off-by: Hauke Mehrtens <hauke@...ke-m.de>
> ---
> drivers/net/dsa/realtek/rtl8365mb.c | 43 +++++++++++++++++++++--------
> 1 file changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 2cb722a9e096..be64cfdeccc7 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -127,6 +127,9 @@ static const int rtl8365mb_extint_port_map[] = { -1, -1, -1, -1, -1, -1, 1, 2,
>
> #define RTL8365MB_CHIP_VER_REG 0x1301
>
> +#define RTL8365MB_CHIP_OPTION_REG 0x13C1
> +
> +#define RTL8365MB_MAGIC_OPT_REG 0x13C0
Realtek's driver calls this register PROTECT_ID:
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/files/drivers/net/phy/rtk/rtl8367c/include/rtl8367c_reg.h;h=eb4f48b83e02ce0625d70b337c445c36f071686c;hb=HEAD
18050 #define RTL8367C_REG_PROTECT_ID 0x13c0
Consider renaming it, or at least be consistent with _OPT and _OPTION. I see
both opt and option throughout the patch.
> #define RTL8365MB_MAGIC_REG 0x13C2
> #define RTL8365MB_MAGIC_VALUE 0x0249
Can you repeat this for MAGIC_OPT_REG? It's OK that it's the same.
>
> @@ -579,6 +582,7 @@ struct rtl8365mb {
> int irq;
> u32 chip_id;
> u32 chip_ver;
> + u32 chip_option;
Moreover, do you know what option is supposed to mean here? Likewise the
register map from Realtek calls this CHIP_VER_INTL (internal maybe?):
#define RTL8367C_REG_CHIP_VER_INTL 0x13c1
> u32 port_mask;
> u32 learn_limit_max;
> struct rtl8365mb_cpu cpu;
> @@ -1959,7 +1963,7 @@ static void rtl8365mb_teardown(struct dsa_switch *ds)
> rtl8365mb_irq_teardown(priv);
> }
>
> -static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver)
> +static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver, u32 *option)
Likewise for consistency, consider the name of this function as you add a new
argument to it.
> {
> int ret;
>
> @@ -1983,6 +1987,19 @@ static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver)
> if (ret)
> return ret;
>
> + ret = regmap_write(map, RTL8365MB_MAGIC_OPT_REG, RTL8365MB_MAGIC_VALUE);
Might deserve a comment here too, "the ver2/ver_intl/option/etc. register is
also protected with magic".
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(map, RTL8365MB_CHIP_OPTION_REG, option);
> + if (ret)
> + return ret;
> +
> + /* Reset magic register */
> + ret = regmap_write(map, RTL8365MB_MAGIC_OPT_REG, 0);
> + if (ret)
> + return ret;
> +
> return 0;
> }
>
> @@ -1991,9 +2008,10 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> struct rtl8365mb *mb = priv->chip_data;
> u32 chip_id;
> u32 chip_ver;
> + u32 chip_option;
> int ret;
>
> - ret = rtl8365mb_get_chip_id_and_ver(priv->map, &chip_id, &chip_ver);
> + ret = rtl8365mb_get_chip_id_and_ver(priv->map, &chip_id, &chip_ver, &chip_option);
> if (ret) {
> dev_err(priv->dev, "failed to read chip id and version: %d\n",
> ret);
> @@ -2005,22 +2023,22 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> switch (chip_ver) {
> case RTL8365MB_CHIP_VER_8365MB_VC:
> dev_info(priv->dev,
> - "found an RTL8365MB-VC switch (ver=0x%04x)\n",
> - chip_ver);
> + "found an RTL8365MB-VC switch (ver=0x%04x, opt=0x%04x)\n",
> + chip_ver, chip_option);
> break;
> case RTL8365MB_CHIP_VER_8367RB:
> dev_info(priv->dev,
> - "found an RTL8367RB-VB switch (ver=0x%04x)\n",
> - chip_ver);
> + "found an RTL8367RB-VB switch (ver=0x%04x, opt=0x%04x)\n",
> + chip_ver, chip_option);
> break;
> case RTL8365MB_CHIP_VER_8367S:
> dev_info(priv->dev,
> - "found an RTL8367S switch (ver=0x%04x)\n",
> - chip_ver);
> + "found an RTL8367S switch (ver=0x%04x, opt=0x%04x)\n",
> + chip_ver, chip_option);
> break;
> default:
> - dev_err(priv->dev, "unrecognized switch version (ver=0x%04x)",
> - chip_ver);
> + dev_err(priv->dev, "unrecognized switch version (ver=0x%04x, opt=0x%04x)",
> + chip_ver, chip_option);
> return -ENODEV;
> }
>
> @@ -2029,6 +2047,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> mb->priv = priv;
> mb->chip_id = chip_id;
> mb->chip_ver = chip_ver;
> + mb->chip_option = chip_option;
> mb->port_mask = GENMASK(priv->num_ports - 1, 0);
> mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX;
> mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
> @@ -2043,8 +2062,8 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> break;
> default:
> dev_err(priv->dev,
> - "found an unknown Realtek switch (id=0x%04x, ver=0x%04x)\n",
> - chip_id, chip_ver);
> + "found an unknown Realtek switch (id=0x%04x, ver=0x%04x, opt=0x%04x)\n",
> + chip_id, chip_ver, chip_option);
> return -ENODEV;
> }
>
> --
> 2.30.2
>
Powered by blists - more mailing lists