| 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: Wed, 9 Feb 2022 21:53:34 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.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>,
"davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next] net: dsa: realtek: rtl8365mb: read phy regs from
memory when MDIO
Hi Luiz,
Luiz Angelo Daros de Luca <luizluca@...il.com> writes:
> In a SMI-connected switch, indirect registers are accessed commanding
> the switch to access an address by reading and writing a sequence of
> chip registers. However, that process might return a null reading
> (0x0000) with no errors if the switch is under stress. When that
> happens, the system will detect a link down, followed by a link up,
> creating temporary link unavailability.
>
> It was observed only with idle SMP devices and interruptions are not in
> use (the system was polling each interface status every second). It
> happened with both SMI and MDIO-connected switches. If the OS is under
> any load, the same problem does not appear, probably because the polling
> process is delayed or serialized.
I am not sure I agree with your description here. It is more likely that
there is just a bug in the driver, rather than some problem with
indirect PHY register access "when the switch is under stress". Please
see my reply to your earlier thread.
Having said that, using the more direct approach makes sense - I think I
said the same in your original MDIO series too. So this change is
justifiable in its own right.
I suggest resending this without talking in too much detail about a bug which
is still unresolved.
Kind regards,
Alvin
>
> Although that method to read indirect registers work independently from
> the management interface, MDIO-connected can skip part of the process.
> Instead of commanding the switch to read a register, the driver can read
> directly from the region after RTL8365MB_PHY_BASE=0x0200. It was enough
> to workaround the null readings.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> ---
> drivers/net/dsa/realtek/rtl8365mb.c | 115 ++++++++++++++++++++++++----
> 1 file changed, 99 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 2ed592147c20..e0c7f64bc56f 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -595,8 +595,7 @@ static int rtl8365mb_phy_poll_busy(struct realtek_priv *priv)
> val, !val, 10, 100);
> }
>
> -static int rtl8365mb_phy_ocp_prepare(struct realtek_priv *priv, int phy,
> - u32 ocp_addr)
> +static int rtl8365mb_phy_set_ocp_prefix(struct realtek_priv *priv, u32 ocp_addr)
> {
> u32 val;
> int ret;
> @@ -610,19 +609,22 @@ static int rtl8365mb_phy_ocp_prepare(struct realtek_priv *priv, int phy,
> if (ret)
> return ret;
>
> - /* Set PHY register address */
> - val = RTL8365MB_PHY_BASE;
> - val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK, phy);
> - val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_5_1_MASK,
> + return 0;
> +}
> +
> +static u32 rtl8365mb_phy_ocp_ind_addr(int phy, u32 ocp_addr)
> +{
> + u32 ind_addr;
> +
> + ind_addr = RTL8365MB_PHY_BASE;
> + ind_addr |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK,
> + phy);
> + ind_addr |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_5_1_MASK,
> ocp_addr >> 1);
> - val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_9_6_MASK,
> + ind_addr |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_9_6_MASK,
> ocp_addr >> 6);
> - ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG,
> - val);
> - if (ret)
> - return ret;
>
> - return 0;
> + return ind_addr;
> }
>
> static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy,
> @@ -635,7 +637,13 @@ static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy,
> if (ret)
> return ret;
>
> - ret = rtl8365mb_phy_ocp_prepare(priv, phy, ocp_addr);
> + ret = rtl8365mb_phy_set_ocp_prefix(priv, ocp_addr);
> + if (ret)
> + return ret;
> +
> + /* Set PHY register address */
> + ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG,
> + rtl8365mb_phy_ocp_ind_addr(phy, ocp_addr));
> if (ret)
> return ret;
>
> @@ -673,7 +681,13 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy,
> if (ret)
> return ret;
>
> - ret = rtl8365mb_phy_ocp_prepare(priv, phy, ocp_addr);
> + ret = rtl8365mb_phy_set_ocp_prefix(priv, ocp_addr);
> + if (ret)
> + return ret;
> +
> + /* Set PHY register address */
> + ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG,
> + rtl8365mb_phy_ocp_ind_addr(phy, ocp_addr));
> if (ret)
> return ret;
>
> @@ -757,13 +771,82 @@ static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum,
>
> static int rtl8365mb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum)
> {
> - return rtl8365mb_phy_read(ds->priv, phy, regnum);
> + struct realtek_priv *priv = ds->priv;
> + u32 ocp_addr;
> + u32 ind_addr;
> + uint val;
> + int ret;
> +
> + if (phy > RTL8365MB_PHYADDRMAX)
> + return -EINVAL;
> +
> + if (regnum > RTL8365MB_PHYREGMAX)
> + return -EINVAL;
> +
> + ocp_addr = RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE + regnum * 2;
> +
> + ret = rtl8365mb_phy_set_ocp_prefix(priv, ocp_addr);
> + if (ret)
> + return ret;
> +
> + ind_addr = rtl8365mb_phy_ocp_ind_addr(phy, ocp_addr);
> +
> + /* MDIO-connected switches can read directly from ind_addr (0x2000..)
> + * although using RTL8365MB_INDIRECT_ACCESS_* does work.
> + */
> + ret = regmap_read(priv->map, ind_addr, &val);
> + if (ret) {
> + dev_err(priv->dev,
> + "failed to read PHY%d reg %02x @ %04x, ret %d\n", phy,
> + regnum, ocp_addr, ret);
> + return ret;
> + }
> +
> + val = val & 0xFFFF;
> +
> + dev_dbg(priv->dev, "read PHY%d register 0x%02x @ %04x, val <- %04x\n",
> + phy, regnum, ocp_addr, val);
> +
> + return val;
> }
>
> static int rtl8365mb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum,
> u16 val)
> {
> - return rtl8365mb_phy_write(ds->priv, phy, regnum, val);
> + struct realtek_priv *priv = ds->priv;
> + u32 ocp_addr;
> + u32 ind_addr;
> + int ret;
> +
> + if (phy > RTL8365MB_PHYADDRMAX)
> + return -EINVAL;
> +
> + if (regnum > RTL8365MB_PHYREGMAX)
> + return -EINVAL;
> +
> + ocp_addr = RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE + regnum * 2;
> +
> + ret = rtl8365mb_phy_set_ocp_prefix(priv, ocp_addr);
> + if (ret)
> + return ret;
> +
> + ind_addr = rtl8365mb_phy_ocp_ind_addr(phy, ocp_addr);
> +
> + /* MDIO-connected switches can write directly from ind_addr (0x2000..)
> + * although using RTL8365MB_INDIRECT_ACCESS_* does work.
> + */
> + ret = regmap_write(priv->map, ind_addr, val);
> + if (ret) {
> + dev_err(priv->dev,
> + "failed to write PHY%d reg %02x @ %04x, ret %d\n", phy,
> + regnum, ocp_addr, ret);
> + return ret;
> + }
> +
> + dev_dbg(priv->dev, "write PHY%d register 0x%02x @ %04x, val -> %04x\n",
> + phy, regnum, ocp_addr, val);
> +
> + return 0;
> }
>
> static enum dsa_tag_protocol
Powered by blists - more mailing lists