* [PATCH v2 net-next 0/2] net: dsa: realtek: fix PHY register read corruption @ 2022-02-21 18:46 Alvin Šipraga 2022-02-21 18:46 ` [PATCH v2 net-next 1/2] net: dsa: realtek: allow subdrivers to externally lock regmap Alvin Šipraga ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Alvin Šipraga @ 2022-02-21 18:46 UTC (permalink / raw) To: Linus Walleij, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, Jakub Kicinski, Michael Rasmussen, Alvin Šipraga Cc: Luiz Angelo Daros de Luca, Arınç ÜNAL, netdev, linux-kernel From: Alvin Šipraga <alsi@bang-olufsen.dk> These two patches fix the issue reported by Arınç where PHY register reads sometimes return garbage data. v1 -> v2: - no code changes - just update the commit message of patch 2 to reflect the conclusion of further investigation requested by Vladimir Alvin Šipraga (2): net: dsa: realtek: allow subdrivers to externally lock regmap net: dsa: realtek: rtl8365mb: serialize indirect PHY register access drivers/net/dsa/realtek/realtek-mdio.c | 46 +++++++++++++++++++++- drivers/net/dsa/realtek/realtek-smi.c | 48 +++++++++++++++++++++-- drivers/net/dsa/realtek/realtek.h | 2 + drivers/net/dsa/realtek/rtl8365mb.c | 54 ++++++++++++++++---------- 4 files changed, 124 insertions(+), 26 deletions(-) -- 2.35.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 net-next 1/2] net: dsa: realtek: allow subdrivers to externally lock regmap 2022-02-21 18:46 [PATCH v2 net-next 0/2] net: dsa: realtek: fix PHY register read corruption Alvin Šipraga @ 2022-02-21 18:46 ` Alvin Šipraga 2022-02-21 19:06 ` Vladimir Oltean 2022-02-21 18:46 ` [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access Alvin Šipraga 2022-02-23 12:30 ` [PATCH v2 net-next 0/2] net: dsa: realtek: fix PHY register read corruption patchwork-bot+netdevbpf 2 siblings, 1 reply; 10+ messages in thread From: Alvin Šipraga @ 2022-02-21 18:46 UTC (permalink / raw) To: Linus Walleij, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, Jakub Kicinski, Michael Rasmussen, Alvin Šipraga Cc: Luiz Angelo Daros de Luca, Arınç ÜNAL, netdev, linux-kernel From: Alvin Šipraga <alsi@bang-olufsen.dk> 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 1/2] net: dsa: realtek: allow subdrivers to externally lock regmap 2022-02-21 18:46 ` [PATCH v2 net-next 1/2] net: dsa: realtek: allow subdrivers to externally lock regmap Alvin Šipraga @ 2022-02-21 19:06 ` Vladimir Oltean 0 siblings, 0 replies; 10+ messages in thread From: Vladimir Oltean @ 2022-02-21 19:06 UTC (permalink / raw) To: Alvin Šipraga Cc: Linus Walleij, Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller, Jakub Kicinski, Michael Rasmussen, Alvin Šipraga, Luiz Angelo Daros de Luca, Arınç ÜNAL, netdev, linux-kernel On Mon, Feb 21, 2022 at 07:46:30PM +0100, Alvin Šipraga wrote: > From: Alvin Šipraga <alsi@bang-olufsen.dk> > > 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> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > 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) I looked even at the build results for v1 and I don't know exactly why sparse doesn't complain about the lack of __acquires() and __releases() annotations, to avoid context imbalance warnings. https://patchwork.kernel.org/project/netdevbpf/patch/20220216160500.2341255-2-alvin@pqrs.dk/ Anyway, those aren't of much use IMO (you can't get it to do anything but very trivial checks), and if sparse doesn't complain for whatever reason, I certainly won't request you to add them. I see other implementations don't have them either - like vexpress_config_lock. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access 2022-02-21 18:46 [PATCH v2 net-next 0/2] net: dsa: realtek: fix PHY register read corruption Alvin Šipraga 2022-02-21 18:46 ` [PATCH v2 net-next 1/2] net: dsa: realtek: allow subdrivers to externally lock regmap Alvin Šipraga @ 2022-02-21 18:46 ` Alvin Šipraga 2022-02-21 19:09 ` Vladimir Oltean 2022-02-21 23:21 ` Linus Walleij 2022-02-23 12:30 ` [PATCH v2 net-next 0/2] net: dsa: realtek: fix PHY register read corruption patchwork-bot+netdevbpf 2 siblings, 2 replies; 10+ messages in thread From: Alvin Šipraga @ 2022-02-21 18:46 UTC (permalink / raw) To: Linus Walleij, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, Jakub Kicinski, Michael Rasmussen, Alvin Šipraga Cc: Luiz Angelo Daros de Luca, Arınç ÜNAL, netdev, linux-kernel From: Alvin Šipraga <alsi@bang-olufsen.dk> Realtek switches in the rtl8365mb family can access the PHY registers of the internal PHYs via the switch registers. This method is called indirect access. At a high level, the indirect PHY register access method involves reading and writing some special switch registers in a particular sequence. This works for both SMI and MDIO connected switches. Currently the rtl8365mb driver does not take any care to serialize the aforementioned access to the switch registers. In particular, it is permitted for other driver code to access other switch registers while the indirect PHY register access is ongoing. Locking is only done at the regmap level. This, however, is a bug: concurrent register access, even to unrelated switch registers, risks corrupting the PHY register value read back via the indirect access method described above. Arınç reported that the switch sometimes returns nonsense data when reading the PHY registers. In particular, a value of 0 causes the kernel's PHY subsystem to think that the link is down, but since most reads return correct data, the link then flip-flops between up and down over a period of time. The aforementioned bug can be readily observed by: 1. Enabling ftrace events for regmap and mdio 2. Polling BSMR PHY register for a connected port; it should always read the same (e.g. 0x79ed) 3. Wait for step 2 to give a different value Example command for step 2: while true; do phytool read swp2/2/0x01; done On my i.MX8MM, the above steps will yield a bogus value for the BSMR PHY register within a matter of seconds. The interleaved register access it then evident in the trace log: kworker/3:4-70 [003] ....... 1927.139849: regmap_reg_write: ethernet-switch reg=1004 val=bd phytool-16816 [002] ....... 1927.139979: regmap_reg_read: ethernet-switch reg=1f01 val=0 kworker/3:4-70 [003] ....... 1927.140381: regmap_reg_read: ethernet-switch reg=1005 val=0 phytool-16816 [002] ....... 1927.140468: regmap_reg_read: ethernet-switch reg=1d15 val=a69 kworker/3:4-70 [003] ....... 1927.140864: regmap_reg_read: ethernet-switch reg=1003 val=0 phytool-16816 [002] ....... 1927.140955: regmap_reg_write: ethernet-switch reg=1f02 val=2041 kworker/3:4-70 [003] ....... 1927.141390: regmap_reg_read: ethernet-switch reg=1002 val=0 phytool-16816 [002] ....... 1927.141479: regmap_reg_write: ethernet-switch reg=1f00 val=1 kworker/3:4-70 [003] ....... 1927.142311: regmap_reg_write: ethernet-switch reg=1004 val=be phytool-16816 [002] ....... 1927.142410: regmap_reg_read: ethernet-switch reg=1f01 val=0 kworker/3:4-70 [003] ....... 1927.142534: regmap_reg_read: ethernet-switch reg=1005 val=0 phytool-16816 [002] ....... 1927.142618: regmap_reg_read: ethernet-switch reg=1f04 val=0 phytool-16816 [002] ....... 1927.142641: mdio_access: SMI-0 read phy:0x02 reg:0x01 val:0x0000 <- ?! kworker/3:4-70 [003] ....... 1927.143037: regmap_reg_read: ethernet-switch reg=1001 val=0 kworker/3:4-70 [003] ....... 1927.143133: regmap_reg_read: ethernet-switch reg=1000 val=2d89 kworker/3:4-70 [003] ....... 1927.143213: regmap_reg_write: ethernet-switch reg=1004 val=be kworker/3:4-70 [003] ....... 1927.143291: regmap_reg_read: ethernet-switch reg=1005 val=0 kworker/3:4-70 [003] ....... 1927.143368: regmap_reg_read: ethernet-switch reg=1003 val=0 kworker/3:4-70 [003] ....... 1927.143443: regmap_reg_read: ethernet-switch reg=1002 val=6 The kworker here is polling MIB counters for stats, as evidenced by the register 0x1004 that we are writing to (RTL8365MB_MIB_ADDRESS_REG). This polling is performed every 3 seconds, but is just one example of such unsynchronized access. In Arınç's case, the driver was not using the switch IRQ, so the PHY subsystem was itself doing polling analogous to phytool in the above example. A test module was created [see second Link] to simulate such spurious switch register accesses while performing indirect PHY register reads and writes. Realtek was also consulted to confirm whether this is a known issue or not. The conclusion of these lines of inquiry is as follows: 1. Reading of PHY registers via indirect access will be aborted if, after executing the read operation (via a write to the INDIRECT_ACCESS_CTRL_REG), any register is accessed, other than INDIRECT_ACCESS_STATUS_REG. 2. The PHY register indirect read is only complete when INDIRECT_ACCESS_STATUS_REG reads zero. 3. The INDIRECT_ACCESS_DATA_REG, which is read to get the result of the PHY read, will contain the result of the last successful read operation. If there was spurious register access and the indirect read was aborted, then this register is not guaranteed to hold anything meaningful and the PHY read will silently fail. 4. PHY writes do not appear to be affected by this mechanism. 5. Other similar access routines, such as for MIB counters, although similar to the PHY indirect access method, are actually table access. Table access is not affected by spurious reads or writes of other registers. However, concurrent table access is not allowed. Currently this is protected via mib_lock, so there is nothing to fix. The above statements are corroborated both via the test module and through consultation with Realtek. In particular, Realtek states that this is simply a property of the hardware design and is not a hardware bug. To fix this problem, one must guard against regmap access while the PHY indirect register read is executing. Fix this by using the newly introduced "nolock" regmap in all PHY-related functions, and by aquiring the regmap mutex at the top level of the PHY register access callbacks. Although no issue has been observed with PHY register _writes_, this change also serializes the indirect access method there. This is done purely as a matter of convenience and for reasons of symmetry. Fixes: 4af2950c50c8 ("net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC") Link: https://lore.kernel.org/netdev/CAJq09z5FCgG-+jVT7uxh1a-0CiiFsoKoHYsAWJtiKwv7LXKofQ@mail.gmail.com/ Link: https://lore.kernel.org/netdev/871qzwjmtv.fsf@bang-olufsen.dk/ Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com> Reported-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk> --- drivers/net/dsa/realtek/rtl8365mb.c | 54 ++++++++++++++++++----------- 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c index 2ed592147c20..c39d6b744597 100644 --- a/drivers/net/dsa/realtek/rtl8365mb.c +++ b/drivers/net/dsa/realtek/rtl8365mb.c @@ -590,7 +590,7 @@ static int rtl8365mb_phy_poll_busy(struct realtek_priv *priv) { u32 val; - return regmap_read_poll_timeout(priv->map, + return regmap_read_poll_timeout(priv->map_nolock, RTL8365MB_INDIRECT_ACCESS_STATUS_REG, val, !val, 10, 100); } @@ -604,7 +604,7 @@ static int rtl8365mb_phy_ocp_prepare(struct realtek_priv *priv, int phy, /* Set OCP prefix */ val = FIELD_GET(RTL8365MB_PHY_OCP_ADDR_PREFIX_MASK, ocp_addr); ret = regmap_update_bits( - priv->map, RTL8365MB_GPHY_OCP_MSB_0_REG, + priv->map_nolock, RTL8365MB_GPHY_OCP_MSB_0_REG, RTL8365MB_GPHY_OCP_MSB_0_CFG_CPU_OCPADR_MASK, FIELD_PREP(RTL8365MB_GPHY_OCP_MSB_0_CFG_CPU_OCPADR_MASK, val)); if (ret) @@ -617,8 +617,8 @@ static int rtl8365mb_phy_ocp_prepare(struct realtek_priv *priv, int phy, ocp_addr >> 1); val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_9_6_MASK, ocp_addr >> 6); - ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG, - val); + ret = regmap_write(priv->map_nolock, + RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG, val); if (ret) return ret; @@ -631,36 +631,42 @@ static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy, u32 val; int ret; + mutex_lock(&priv->map_lock); + ret = rtl8365mb_phy_poll_busy(priv); if (ret) - return ret; + goto out; ret = rtl8365mb_phy_ocp_prepare(priv, phy, ocp_addr); if (ret) - return ret; + goto out; /* Execute read operation */ val = FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_MASK, RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_VALUE) | FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_CTRL_RW_MASK, RTL8365MB_INDIRECT_ACCESS_CTRL_RW_READ); - ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_CTRL_REG, val); + ret = regmap_write(priv->map_nolock, RTL8365MB_INDIRECT_ACCESS_CTRL_REG, + val); if (ret) - return ret; + goto out; ret = rtl8365mb_phy_poll_busy(priv); if (ret) - return ret; + goto out; /* Get PHY register data */ - ret = regmap_read(priv->map, RTL8365MB_INDIRECT_ACCESS_READ_DATA_REG, - &val); + ret = regmap_read(priv->map_nolock, + RTL8365MB_INDIRECT_ACCESS_READ_DATA_REG, &val); if (ret) - return ret; + goto out; *data = val & 0xFFFF; - return 0; +out: + mutex_unlock(&priv->map_lock); + + return ret; } static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy, @@ -669,32 +675,38 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy, u32 val; int ret; + mutex_lock(&priv->map_lock); + ret = rtl8365mb_phy_poll_busy(priv); if (ret) - return ret; + goto out; ret = rtl8365mb_phy_ocp_prepare(priv, phy, ocp_addr); if (ret) - return ret; + goto out; /* Set PHY register data */ - ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_WRITE_DATA_REG, - data); + ret = regmap_write(priv->map_nolock, + RTL8365MB_INDIRECT_ACCESS_WRITE_DATA_REG, data); if (ret) - return ret; + goto out; /* Execute write operation */ val = FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_MASK, RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_VALUE) | FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_CTRL_RW_MASK, RTL8365MB_INDIRECT_ACCESS_CTRL_RW_WRITE); - ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_CTRL_REG, val); + ret = regmap_write(priv->map_nolock, RTL8365MB_INDIRECT_ACCESS_CTRL_REG, + val); if (ret) - return ret; + goto out; ret = rtl8365mb_phy_poll_busy(priv); if (ret) - return ret; + goto out; + +out: + mutex_unlock(&priv->map_lock); return 0; } -- 2.35.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access 2022-02-21 18:46 ` [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access Alvin Šipraga @ 2022-02-21 19:09 ` Vladimir Oltean 2022-02-21 23:21 ` Linus Walleij 1 sibling, 0 replies; 10+ messages in thread From: Vladimir Oltean @ 2022-02-21 19:09 UTC (permalink / raw) To: Alvin Šipraga Cc: Linus Walleij, Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller, Jakub Kicinski, Michael Rasmussen, Alvin Šipraga, Luiz Angelo Daros de Luca, Arınç ÜNAL, netdev, linux-kernel On Mon, Feb 21, 2022 at 07:46:31PM +0100, Alvin Šipraga wrote: > To fix this problem, one must guard against regmap access while the > PHY indirect register read is executing. Fix this by using the newly > introduced "nolock" regmap in all PHY-related functions, and by aquiring > the regmap mutex at the top level of the PHY register access callbacks. > Although no issue has been observed with PHY register _writes_, this > change also serializes the indirect access method there. This is done > purely as a matter of convenience and for reasons of symmetry. Reviewed-by: Vladimir Oltean <olteanv@gmail.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access 2022-02-21 18:46 ` [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access Alvin Šipraga 2022-02-21 19:09 ` Vladimir Oltean @ 2022-02-21 23:21 ` Linus Walleij 2022-02-22 0:19 ` Alvin Šipraga 1 sibling, 1 reply; 10+ messages in thread From: Linus Walleij @ 2022-02-21 23:21 UTC (permalink / raw) To: Alvin Šipraga Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, Jakub Kicinski, Michael Rasmussen, Alvin Šipraga, Luiz Angelo Daros de Luca, Arınç ÜNAL, netdev, linux-kernel On Mon, Feb 21, 2022 at 7:46 PM Alvin Šipraga <alvin@pqrs.dk> wrote: > Realtek switches in the rtl8365mb family can access the PHY registers of > the internal PHYs via the switch registers. This method is called > indirect access. At a high level, the indirect PHY register access > method involves reading and writing some special switch registers in a > particular sequence. This works for both SMI and MDIO connected > switches. What I worry about is whether we need to do a similar patch to rtl8366rb_phy_read() and rtl8366rb_phy_write() in rtl8366rb.c? And what about the upcoming rtl8367 driver? > To fix this problem, one must guard against regmap access while the > PHY indirect register read is executing. Fix this by using the newly > introduced "nolock" regmap in all PHY-related functions, and by aquiring > the regmap mutex at the top level of the PHY register access callbacks. > Although no issue has been observed with PHY register _writes_, this > change also serializes the indirect access method there. This is done > purely as a matter of convenience and for reasons of symmetry. > > Fixes: 4af2950c50c8 ("net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC") > Link: https://lore.kernel.org/netdev/CAJq09z5FCgG-+jVT7uxh1a-0CiiFsoKoHYsAWJtiKwv7LXKofQ@mail.gmail.com/ > Link: https://lore.kernel.org/netdev/871qzwjmtv.fsf@bang-olufsen.dk/ > Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com> > Reported-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk> This is a beautiful patch. Excellent job. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access 2022-02-21 23:21 ` Linus Walleij @ 2022-02-22 0:19 ` Alvin Šipraga 2022-02-22 15:14 ` Linus Walleij 0 siblings, 1 reply; 10+ messages in thread From: Alvin Šipraga @ 2022-02-22 0:19 UTC (permalink / raw) To: Linus Walleij Cc: Alvin Šipraga, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, Jakub Kicinski, Michael Rasmussen, Luiz Angelo Daros de Luca, Arınç ÜNAL, netdev, linux-kernel Hi Linus, Linus Walleij <linus.walleij@linaro.org> writes: > On Mon, Feb 21, 2022 at 7:46 PM Alvin Šipraga <alvin@pqrs.dk> wrote: > >> Realtek switches in the rtl8365mb family can access the PHY registers of >> the internal PHYs via the switch registers. This method is called >> indirect access. At a high level, the indirect PHY register access >> method involves reading and writing some special switch registers in a >> particular sequence. This works for both SMI and MDIO connected >> switches. > > 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. 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/ > > And what about the upcoming rtl8367 driver? Is this a hypothetical driver or have I missed something on the list? If you mean the changes Luiz has sent for net-next, then it is covered by this series. Those switches (RTL8367S, RTL8367RB?) are in the same family as the RTL8365MB-VC and are supported by rtl8365mb.c. > >> To fix this problem, one must guard against regmap access while the >> PHY indirect register read is executing. Fix this by using the newly >> introduced "nolock" regmap in all PHY-related functions, and by aquiring >> the regmap mutex at the top level of the PHY register access callbacks. >> Although no issue has been observed with PHY register _writes_, this >> change also serializes the indirect access method there. This is done >> purely as a matter of convenience and for reasons of symmetry. >> >> Fixes: 4af2950c50c8 ("net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC") >> Link: https://lore.kernel.org/netdev/CAJq09z5FCgG-+jVT7uxh1a-0CiiFsoKoHYsAWJtiKwv7LXKofQ@mail.gmail.com/ >> Link: https://lore.kernel.org/netdev/871qzwjmtv.fsf@bang-olufsen.dk/ >> Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com> >> Reported-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> >> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk> > > This is a beautiful patch. Excellent job. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Thank you Linus for your kind words. Kind regards, Alvin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access 2022-02-22 0:19 ` Alvin Šipraga @ 2022-02-22 15:14 ` Linus Walleij 2022-02-22 16:06 ` Alvin Šipraga 0 siblings, 1 reply; 10+ messages in thread From: Linus Walleij @ 2022-02-22 15:14 UTC (permalink / raw) To: Alvin Šipraga Cc: Alvin Šipraga, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, Jakub Kicinski, Michael Rasmussen, Luiz Angelo Daros de Luca, Arınç ÜNAL, netdev, linux-kernel 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. > 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! Yours, Linus Walleij ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access 2022-02-22 15:14 ` Linus Walleij @ 2022-02-22 16:06 ` Alvin Šipraga 0 siblings, 0 replies; 10+ messages in thread From: Alvin Šipraga @ 2022-02-22 16:06 UTC (permalink / raw) To: Linus Walleij Cc: Alvin Šipraga, Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S. Miller, Jakub Kicinski, Michael Rasmussen, Luiz Angelo Daros de Luca, Arınç ÜNAL, netdev, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2141 bytes --] 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 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-TEST-net-dsa-realtek-rtl8366rb-provoke-stray-reg-acc.patch --] [-- Type: text/x-patch; name="0001-TEST-net-dsa-realtek-rtl8366rb-provoke-stray-reg-acc.patch", Size: 2843 bytes --] 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 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0001-net-dsa-realtek-allow-subdrivers-to-externally-lock-.patch --] [-- Type: text/x-patch; name="0001-net-dsa-realtek-allow-subdrivers-to-externally-lock-.patch", Size: 8156 bytes --] 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 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #4: 0002-TEST-net-dsa-realtek-rtl8366rb-serialize-indirect-PH.patch --] [-- Type: text/x-patch; name="0002-TEST-net-dsa-realtek-rtl8366rb-serialize-indirect-PH.patch", Size: 2866 bytes --] 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net-next 0/2] net: dsa: realtek: fix PHY register read corruption 2022-02-21 18:46 [PATCH v2 net-next 0/2] net: dsa: realtek: fix PHY register read corruption Alvin Šipraga 2022-02-21 18:46 ` [PATCH v2 net-next 1/2] net: dsa: realtek: allow subdrivers to externally lock regmap Alvin Šipraga 2022-02-21 18:46 ` [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access Alvin Šipraga @ 2022-02-23 12:30 ` patchwork-bot+netdevbpf 2 siblings, 0 replies; 10+ messages in thread From: patchwork-bot+netdevbpf @ 2022-02-23 12:30 UTC (permalink / raw) To: =?utf-8?b?QWx2aW4gxaBpcHJhZ2EgPGFsdmluQHBxcnMuZGs+?= Cc: linus.walleij, andrew, vivien.didelot, f.fainelli, olteanv, davem, kuba, mir, alsi, luizluca, arinc.unal, netdev, linux-kernel Hello: This series was applied to netdev/net-next.git (master) by David S. Miller <davem@davemloft.net>: On Mon, 21 Feb 2022 19:46:29 +0100 you wrote: > From: Alvin Šipraga <alsi@bang-olufsen.dk> > > These two patches fix the issue reported by Arınç where PHY register > reads sometimes return garbage data. > > v1 -> v2: > > [...] Here is the summary with links: - [v2,net-next,1/2] net: dsa: realtek: allow subdrivers to externally lock regmap https://git.kernel.org/netdev/net-next/c/907e772f6f6d - [v2,net-next,2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access https://git.kernel.org/netdev/net-next/c/2796728460b8 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-02-23 12:30 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-21 18:46 [PATCH v2 net-next 0/2] net: dsa: realtek: fix PHY register read corruption Alvin Šipraga 2022-02-21 18:46 ` [PATCH v2 net-next 1/2] net: dsa: realtek: allow subdrivers to externally lock regmap Alvin Šipraga 2022-02-21 19:06 ` Vladimir Oltean 2022-02-21 18:46 ` [PATCH v2 net-next 2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access Alvin Šipraga 2022-02-21 19:09 ` Vladimir Oltean 2022-02-21 23:21 ` Linus Walleij 2022-02-22 0:19 ` Alvin Šipraga 2022-02-22 15:14 ` Linus Walleij 2022-02-22 16:06 ` Alvin Šipraga 2022-02-23 12:30 ` [PATCH v2 net-next 0/2] net: dsa: realtek: fix PHY register read corruption patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).