lkml.org 
[lkml]   [2022]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
From
SubjectRe: [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access
Date
Linus Walleij <linus.walleij@linaro.org> writes:

> On Tue, Feb 22, 2022 at 1:19 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
>> > On Mon, Feb 21, 2022 at 7:46 PM Alvin Šipraga <alvin@pqrs.dk> wrote:
>
>> > What I worry about is whether we need to do a similar patch to
>> > rtl8366rb_phy_read() and rtl8366rb_phy_write() in
>> > rtl8366rb.c?
>>
>> Unfortunately I do not have the hardware to test rtl8366rb.c, so I can
>> only speculate. But I gave some hints in the commit message which might
>> help in checking whether or not it is an issue on that hardware as
>> well. The code for rtl8366rb_phy_read() looks similar, but since this is
>> a quirk of the hardware design, it could be that it is not
>> necessary. The only way is to test.
>
> I can test it.

Great! I have attached a test patch to insert these stray register
accesses inside the PHY read/write functions:

0001-TEST-net-dsa-realtek-rtl8366rb-provoke-stray-reg-acc.patch

Set the module parameter stray_access to 1 in order to enable stray
register access. I did this in case you don't build it as a module but
still want to do A/B testing - just remove it if I goofed.

>
>> If you or somebody else can confirm that it is an issue for RTL8366RB as
>> well, I will happily send a patch to the list for testing. You can for
>> example try spamming PHY register reads with phytool while also reading
>> off switch registers via regmap debugfs. See also the discussion in [1].
>>
>> [1] https://lore.kernel.org/netdev/878rukib4f.fsf@bang-olufsen.dk/
>
> If you have time to write a patch I'd love testing it!

Attached also a patch with a proposed fix - if indeed it is an issue -
along the same lines as the second patch in this series. You'll also
need the first patch from this series (attached for convenience):

0001-net-dsa-realtek-allow-subdrivers-to-externally-lock-.patch
0002-TEST-net-dsa-realtek-rtl8366rb-serialize-indirect-PH.patch

All patches are against net-next and build-tested only, so you might
want to double check if the results are unexpected.

Kind regards,
Alvin

>
> Yours,
> Linus Walleij

From 3c0b97bfbe52fc9bc8d73ff730440d63a30c6766 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvin=20=C5=A0ipraga?= <alsi@bang-olufsen.dk>
Date: Tue, 22 Feb 2022 16:55:24 +0100
Subject: [PATCH] TEST: net: dsa: realtek: rtl8366rb: provoke stray reg access

Test for Linus to see if RTL8366RB also suffers from corrupted PHY
register access.

Set module parameter stray_access=1 to enable stray register read/write
in critical sections of the PHY register read/write procedures.
---
drivers/net/dsa/realtek/rtl8366rb.c | 38 +++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)

diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index fb6565e68401..d4bd826e7f39 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1643,6 +1643,38 @@ static int rtl8366rb_enable_vlan4k(struct realtek_priv *priv, bool enable)
enable ? RTL8366RB_SGCR_EN_VLAN_4KTB : 0);
}

+static bool stray_access = false;
+module_param(stray_access, bool, 0644);
+MODULE_PARM_DESC(stray_access, "do stray reg access during PHY read/write");
+
+static void rtl8366rb_stray_reg_access(struct realtek_priv *priv)
+{
+ u32 val;
+ int ret;
+
+ if (!stray_access)
+ return;
+
+ /* Read some crap */
+ ret = regmap_read(priv->map, RTL8366RB_CHIP_ID_REG, &val);
+ if (ret)
+ dev_warn(priv->dev, "XXX stray read CHIP_ID_REG failed: %d\n",
+ ret);
+ else
+ dev_dbg(priv->dev, "XXX stray read CHIP_ID_REG: %04x\n", val);
+
+ /* Write some crap (same value as it should already have) */
+ val = BIT(priv->cpu_port);
+ ret = regmap_write(priv->map, RTL8368RB_CPU_CTRL_REG, val);
+ if (ret)
+ dev_warn(priv->dev, "XXX stray write CPU_CTRL_REG failed: %d\n",
+ ret);
+ else
+ dev_dbg(priv->dev, "XXX stray write CPU_CTRL_REG: %04x\n", val);
+
+ return;
+}
+
static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum)
{
u32 val;
@@ -1657,6 +1689,8 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum)
if (ret)
return ret;

+ rtl8366rb_stray_reg_access(priv);
+
reg = 0x8000 | (1 << (phy + RTL8366RB_PHY_NO_OFFSET)) | regnum;

ret = regmap_write(priv->map, reg, 0);
@@ -1667,6 +1701,8 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum)
return ret;
}

+ rtl8366rb_stray_reg_access(priv);
+
ret = regmap_read(priv->map, RTL8366RB_PHY_ACCESS_DATA_REG, &val);
if (ret)
return ret;
@@ -1691,6 +1727,8 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum,
if (ret)
return ret;

+ rtl8366rb_stray_reg_access(priv);
+
reg = 0x8000 | (1 << (phy + RTL8366RB_PHY_NO_OFFSET)) | regnum;

dev_dbg(priv->dev, "write PHY%d register 0x%04x @ %04x, val -> %04x\n",
--
2.35.1
From cdaed74ba0343485bf9759d36e81f0e181b2fce4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvin=20=C5=A0ipraga?= <alsi@bang-olufsen.dk>
Date: Wed, 16 Feb 2022 14:17:06 +0100
Subject: [PATCH 1/2] net: dsa: realtek: allow subdrivers to externally lock
regmap
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently there is no way for Realtek DSA subdrivers to serialize
consecutive regmap accesses. In preparation for a bugfix relating to
indirect PHY register access - which involves a series of regmap
reads and writes - add a facility for subdrivers to serialize their
regmap access.

Specifically, a mutex is added to the driver private data structure and
the standard regmap is initialized with custom lock/unlock ops which use
this mutex. Then, a "nolock" variant of the regmap is added, which is
functionally equivalent to the existing regmap except that regmap
locking is disabled. Functions that wish to serialize a sequence of
regmap accesses may then lock the newly introduced driver-owned mutex
before using the nolock regmap.

Doing things this way means that subdriver code that doesn't care about
serialized register access - i.e. the vast majority of code - needn't
worry about synchronizing register access with an external lock: it can
just continue to use the original regmap.

Another advantage of this design is that, while regmaps with locking
disabled do not expose a debugfs interface for obvious reasons, there
still exists the original regmap which does expose this interface. This
interface remains safe to use even combined with driver codepaths that
use the nolock regmap, because said codepaths will use the same mutex
to synchronize access.

With respect to disadvantages, it can be argued that having
near-duplicate regmaps is confusing. However, the naming is rather
explicit, and examples will abound.

Finally, while we are at it, rename realtek_smi_mdio_regmap_config to
realtek_smi_regmap_config. This makes it consistent with the naming
realtek_mdio_regmap_config in realtek-mdio.c.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
drivers/net/dsa/realtek/realtek-mdio.c | 46 ++++++++++++++++++++++--
drivers/net/dsa/realtek/realtek-smi.c | 48 ++++++++++++++++++++++++--
drivers/net/dsa/realtek/realtek.h | 2 ++
3 files changed, 91 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 0308be95d00a..31e1f100e48e 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -98,6 +98,20 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val)
return ret;
}

+static void realtek_mdio_lock(void *ctx)
+{
+ struct realtek_priv *priv = ctx;
+
+ mutex_lock(&priv->map_lock);
+}
+
+static void realtek_mdio_unlock(void *ctx)
+{
+ struct realtek_priv *priv = ctx;
+
+ mutex_unlock(&priv->map_lock);
+}
+
static const struct regmap_config realtek_mdio_regmap_config = {
.reg_bits = 10, /* A4..A0 R4..R0 */
.val_bits = 16,
@@ -108,6 +122,21 @@ static const struct regmap_config realtek_mdio_regmap_config = {
.reg_read = realtek_mdio_read,
.reg_write = realtek_mdio_write,
.cache_type = REGCACHE_NONE,
+ .lock = realtek_mdio_lock,
+ .unlock = realtek_mdio_unlock,
+};
+
+static const struct regmap_config realtek_mdio_nolock_regmap_config = {
+ .reg_bits = 10, /* A4..A0 R4..R0 */
+ .val_bits = 16,
+ .reg_stride = 1,
+ /* PHY regs are at 0x8000 */
+ .max_register = 0xffff,
+ .reg_format_endian = REGMAP_ENDIAN_BIG,
+ .reg_read = realtek_mdio_read,
+ .reg_write = realtek_mdio_write,
+ .cache_type = REGCACHE_NONE,
+ .disable_locking = true,
};

static int realtek_mdio_probe(struct mdio_device *mdiodev)
@@ -115,8 +144,9 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
struct realtek_priv *priv;
struct device *dev = &mdiodev->dev;
const struct realtek_variant *var;
- int ret;
+ struct regmap_config rc;
struct device_node *np;
+ int ret;

var = of_device_get_match_data(dev);
if (!var)
@@ -126,13 +156,25 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
if (!priv)
return -ENOMEM;

- priv->map = devm_regmap_init(dev, NULL, priv, &realtek_mdio_regmap_config);
+ mutex_init(&priv->map_lock);
+
+ rc = realtek_mdio_regmap_config;
+ rc.lock_arg = priv;
+ priv->map = devm_regmap_init(dev, NULL, priv, &rc);
if (IS_ERR(priv->map)) {
ret = PTR_ERR(priv->map);
dev_err(dev, "regmap init failed: %d\n", ret);
return ret;
}

+ rc = realtek_mdio_nolock_regmap_config;
+ priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
+ if (IS_ERR(priv->map_nolock)) {
+ ret = PTR_ERR(priv->map_nolock);
+ dev_err(dev, "regmap init failed: %d\n", ret);
+ return ret;
+ }
+
priv->mdio_addr = mdiodev->addr;
priv->bus = mdiodev->bus;
priv->dev = &mdiodev->dev;
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 8806b74bd7a8..2243d3da55b2 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -311,7 +311,21 @@ static int realtek_smi_read(void *ctx, u32 reg, u32 *val)
return realtek_smi_read_reg(priv, reg, val);
}

-static const struct regmap_config realtek_smi_mdio_regmap_config = {
+static void realtek_smi_lock(void *ctx)
+{
+ struct realtek_priv *priv = ctx;
+
+ mutex_lock(&priv->map_lock);
+}
+
+static void realtek_smi_unlock(void *ctx)
+{
+ struct realtek_priv *priv = ctx;
+
+ mutex_unlock(&priv->map_lock);
+}
+
+static const struct regmap_config realtek_smi_regmap_config = {
.reg_bits = 10, /* A4..A0 R4..R0 */
.val_bits = 16,
.reg_stride = 1,
@@ -321,6 +335,21 @@ static const struct regmap_config realtek_smi_mdio_regmap_config = {
.reg_read = realtek_smi_read,
.reg_write = realtek_smi_write,
.cache_type = REGCACHE_NONE,
+ .lock = realtek_smi_lock,
+ .unlock = realtek_smi_unlock,
+};
+
+static const struct regmap_config realtek_smi_nolock_regmap_config = {
+ .reg_bits = 10, /* A4..A0 R4..R0 */
+ .val_bits = 16,
+ .reg_stride = 1,
+ /* PHY regs are at 0x8000 */
+ .max_register = 0xffff,
+ .reg_format_endian = REGMAP_ENDIAN_BIG,
+ .reg_read = realtek_smi_read,
+ .reg_write = realtek_smi_write,
+ .cache_type = REGCACHE_NONE,
+ .disable_locking = true,
};

static int realtek_smi_mdio_read(struct mii_bus *bus, int addr, int regnum)
@@ -385,6 +414,7 @@ static int realtek_smi_probe(struct platform_device *pdev)
const struct realtek_variant *var;
struct device *dev = &pdev->dev;
struct realtek_priv *priv;
+ struct regmap_config rc;
struct device_node *np;
int ret;

@@ -395,14 +425,26 @@ static int realtek_smi_probe(struct platform_device *pdev)
if (!priv)
return -ENOMEM;
priv->chip_data = (void *)priv + sizeof(*priv);
- priv->map = devm_regmap_init(dev, NULL, priv,
- &realtek_smi_mdio_regmap_config);
+
+ mutex_init(&priv->map_lock);
+
+ rc = realtek_smi_regmap_config;
+ rc.lock_arg = priv;
+ priv->map = devm_regmap_init(dev, NULL, priv, &rc);
if (IS_ERR(priv->map)) {
ret = PTR_ERR(priv->map);
dev_err(dev, "regmap init failed: %d\n", ret);
return ret;
}

+ rc = realtek_smi_nolock_regmap_config;
+ priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
+ if (IS_ERR(priv->map_nolock)) {
+ ret = PTR_ERR(priv->map_nolock);
+ dev_err(dev, "regmap init failed: %d\n", ret);
+ return ret;
+ }
+
/* Link forward and backward */
priv->dev = dev;
priv->clk_delay = var->clk_delay;
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h
index e7d3e1bcf8b8..4fa7c6ba874a 100644
--- a/drivers/net/dsa/realtek/realtek.h
+++ b/drivers/net/dsa/realtek/realtek.h
@@ -52,6 +52,8 @@ struct realtek_priv {
struct gpio_desc *mdc;
struct gpio_desc *mdio;
struct regmap *map;
+ struct regmap *map_nolock;
+ struct mutex map_lock;
struct mii_bus *slave_mii_bus;
struct mii_bus *bus;
int mdio_addr;
--
2.35.1
From 8351eca2eaa5bee3ef56f4bd899e592beaaa837c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvin=20=C5=A0ipraga?= <alsi@bang-olufsen.dk>
Date: Tue, 22 Feb 2022 16:29:45 +0100
Subject: [PATCH 2/2] TEST: net: dsa: realtek: rtl8366rb: serialize indirect?
PHY register access

Test patch for Linus - do not merge

Try locking the regmap during the whole PHY register access routines in
rtl8366rb. It is not known whether this is is necessary - Linus first has
to test if stray register access causes any issues.
---
drivers/net/dsa/realtek/rtl8366rb.c | 33 +++++++++++++++++++----------
1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index fb6565e68401..8e905eb23461 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1652,28 +1652,34 @@ static int rtl8366rb_phy_read(struct realtek_priv *priv, int phy, int regnum)
if (phy > RTL8366RB_PHY_NO_MAX)
return -EINVAL;

- ret = regmap_write(priv->map, RTL8366RB_PHY_ACCESS_CTRL_REG,
+ mutex_lock(&priv->map_lock);
+
+ ret = regmap_write(priv->map_nolock, RTL8366RB_PHY_ACCESS_CTRL_REG,
RTL8366RB_PHY_CTRL_READ);
if (ret)
- return ret;
+ goto out;

reg = 0x8000 | (1 << (phy + RTL8366RB_PHY_NO_OFFSET)) | regnum;

- ret = regmap_write(priv->map, reg, 0);
+ ret = regmap_write(priv->map_nolock, reg, 0);
if (ret) {
dev_err(priv->dev,
"failed to write PHY%d reg %04x @ %04x, ret %d\n",
phy, regnum, reg, ret);
- return ret;
+ goto out;
}

- ret = regmap_read(priv->map, RTL8366RB_PHY_ACCESS_DATA_REG, &val);
+ ret = regmap_read(priv->map_nolock, RTL8366RB_PHY_ACCESS_DATA_REG,
+ &val);
if (ret)
- return ret;
+ goto out;

dev_dbg(priv->dev, "read PHY%d register 0x%04x @ %08x, val <- %04x\n",
phy, regnum, reg, val);

+out:
+ mutex_unlock(&priv->map_lock);
+
return val;
}

@@ -1686,21 +1692,26 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum,
if (phy > RTL8366RB_PHY_NO_MAX)
return -EINVAL;

- ret = regmap_write(priv->map, RTL8366RB_PHY_ACCESS_CTRL_REG,
+ mutex_lock(&priv->map_lock);
+
+ ret = regmap_write(priv->map_nolock, RTL8366RB_PHY_ACCESS_CTRL_REG,
RTL8366RB_PHY_CTRL_WRITE);
if (ret)
- return ret;
+ goto out;

reg = 0x8000 | (1 << (phy + RTL8366RB_PHY_NO_OFFSET)) | regnum;

dev_dbg(priv->dev, "write PHY%d register 0x%04x @ %04x, val -> %04x\n",
phy, regnum, reg, val);

- ret = regmap_write(priv->map, reg, val);
+ ret = regmap_write(priv->map_nolock, reg, val);
if (ret)
- return ret;
+ goto out;

- return 0;
+out:
+ mutex_unlock(&priv->map_lock);
+
+ return ret;
}

static int rtl8366rb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum)
--
2.35.1
\
 
 \ /
  Last update: 2022-02-22 17:06    [W:0.068 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site