[v2,net-next,2/2] net: dsa: realtek: rtl8365mb: serialize indirect PHY register access

Commit Message

Alvin Šipraga Feb. 21, 2022, 6:46 p.m. UTC

From: Alvin Šipraga <[e-mail]>

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/[e-mail]/
Link: https://lore.kernel.org/netdev/[e-mail]/
Reported-by: Arınç ÜNAL <[e-mail]>
Reported-by: Luiz Angelo Daros de Luca <[e-mail]>
Signed-off-by: Alvin Šipraga <[e-mail]>
---
 drivers/net/dsa/realtek/rtl8365mb.c | 54 ++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 21 deletions(-)

Comments

Vladimir Oltean Feb. 21, 2022, 7:09 p.m. UTC | #1

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 <[e-mail]>

Linus Walleij Feb. 21, 2022, 11:21 p.m. UTC | #2

On Mon, Feb 21, 2022 at 7:46 PM Alvin Šipraga <[e-mail]> 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/[e-mail]/
> Link: https://lore.kernel.org/netdev/[e-mail]/
> Reported-by: Arınç ÜNAL <[e-mail]>
> Reported-by: Luiz Angelo Daros de Luca <[e-mail]>
> Signed-off-by: Alvin Šipraga <[e-mail]>

This is a beautiful patch. Excellent job.
Reviewed-by: Linus Walleij <[e-mail]>

Yours,
Linus Walleij

Alvin Šipraga Feb. 22, 2022, 12:19 a.m. UTC | #3

Hi Linus,

Linus Walleij <[e-mail]> writes:

> On Mon, Feb 21, 2022 at 7:46 PM Alvin Šipraga <[e-mail]> 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/[e-mail]/

>
> 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/[e-mail]/
>> Link: https://lore.kernel.org/netdev/[e-mail]/
>> Reported-by: Arınç ÜNAL <[e-mail]>
>> Reported-by: Luiz Angelo Daros de Luca <[e-mail]>
>> Signed-off-by: Alvin Šipraga <[e-mail]>
>
> This is a beautiful patch. Excellent job.
> Reviewed-by: Linus Walleij <[e-mail]>

Thank you Linus for your kind words.

Kind regards,
Alvin

Linus Walleij Feb. 22, 2022, 3:14 p.m. UTC | #4

On Tue, Feb 22, 2022 at 1:19 AM Alvin Šipraga <[e-mail]> wrote:
> > On Mon, Feb 21, 2022 at 7:46 PM Alvin Šipraga <[e-mail]> 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/[e-mail]/

If you have time to write a patch I'd love testing it!

Yours,
Linus Walleij

Alvin Šipraga Feb. 22, 2022, 4:06 p.m. UTC | #5

Linus Walleij <[e-mail]> writes:

> On Tue, Feb 22, 2022 at 1:19 AM Alvin Šipraga <[e-mail]> wrote:
>> > On Mon, Feb 21, 2022 at 7:46 PM Alvin Šipraga <[e-mail]> 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/[e-mail]/
>
> 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

Patch

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;
 }