[PATCH 0/3] r8152: Avoid writing garbage to the adapter's registers

This 3-patch series is the result of a cooperative debug effort
between Realtek and the ChromeOS team. On ChromeOS, we've noticed that
Realtek Ethernet adapters can sometimes get so wedged that even a
reboot of the host can't get them to enumerate again, assuming that
the adapter was on a powered hub and din't lose power when the host
rebooted. This is sometimes seen in the ChromeOS automated testing
lab. The only way to recover adapters in this state is to manually
power cycle them.

I managed to reproduce one instance of this wedging (unknown if this
is truly related to what the test lab sees) by doing this:
1. Start a flood ping from a host to the device.
2. Drop the device into kdb.
3. Wait 90 seconds.
4. Resume from kdb (the "g" command).
5. Wait another 45 seconds.

Upon analysis, Realtek realized this was happening:

1. The Linux driver was getting a "Tx timeout" after resuming from kdb
   and then trying to reset itself.
2. As part of the reset, the Linux driver was attempting to do a
   read-modify-write of the adapter's registers.
3. The read would fail (due to a timeout) and the driver pretended
   that the register contained all 0xFFs. See commit f53a7ad18959
   ("r8152: Set memory to all 0xFFs on failed reg reads")
4. The driver would take this value of all 0xFFs, modify it, and
   attempt to write it back to the adapter.
5. By this time the USB channel seemed to recover and thus we'd
   successfully write a value that was mostly 0xFFs to the adpater.
6. The adapter didn't like this and would wedge itself.

Another Engineer also managed to reproduce wedging of the Realtek
Ethernet adpater during a reboot test on an AMD Chromebook. In that
case he was sometimes seeing -EPIPE returned from the control
transfers.

This patch series fixes both issues.


Douglas Anderson (3):
  r8152: Increase USB control msg timeout to 5000ms as per spec
  r8152: Retry register reads/writes
  r8152: Block future register access if register access fails

 drivers/net/usb/r8152.c | 124 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 115 insertions(+), 9 deletions(-)

-- 
2.42.0.515.g380fc7ccd1-goog

According to the comment next to USB_CTRL_GET_TIMEOUT and
USB_CTRL_SET_TIMEOUT, although sending/receiving control messages is
usually quite fast, the spec allows them to take up to 5 seconds.
Let's increase the timeout in the Realtek driver from 500ms to 5000ms
(using the #defines) to account for this.

This is not just a theoretical change. The need for the longer timeout
was seen in testing. Specifically, if you drop a sc7180-trogdor based
Chromebook into the kdb debugger and then "go" again after sitting in
the debugger for a while, the next USB control message takes a long
time. Out of ~40 tests the slowest USB control message was 4.5
seconds.

While dropping into kdb is not exactly an end-user scenario, the above
is similar to what could happen due to an temporary interrupt storm,
what could happen if there was a host controller (HW or SW) issue, or
what could happen if the Realtek device got into a confused state and
needed time to recover.

This change is fairly critical since the r8152 driver in Linux doesn't
expect register reads/writes (which are backed by USB control
messages) to fail.

Fixes: ac718b69301c ("net/usb: new driver for RTL8152")
Suggested-by: Hayes Wang <redacted>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/net/usb/r8152.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0c13d9950cd8..482957beae66 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -1212,7 +1212,7 @@ int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 
 	ret = usb_control_msg(tp->udev, tp->pipe_ctrl_in,
 			      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
-			      value, index, tmp, size, 500);
+			      value, index, tmp, size, USB_CTRL_GET_TIMEOUT);
 	if (ret < 0)
 		memset(data, 0xff, size);
 	else

@@ -1235,7 +1235,7 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 
 	ret = usb_control_msg(tp->udev, tp->pipe_ctrl_out,
 			      RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
-			      value, index, tmp, size, 500);
+			      value, index, tmp, size, USB_CTRL_SET_TIMEOUT);
 
 	kfree(tmp);
 

@@ -9494,7 +9494,8 @@ static u8 __rtl_get_hw_ver(struct usb_device *udev)
 
 	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 			      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
-			      PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500);
+			      PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp),
+			      USB_CTRL_GET_TIMEOUT);
 	if (ret > 0)
 		ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK;
 

-- 
2.42.0.515.g380fc7ccd1-goog

As talked about in the patch ("r8152: Retry register reads/writes"),
much of the r8152 driver doesn't check for error codes when modifying
the adapter's registers. Specifically, the worst part of this appears
to be when the driver does a read-modify-write of a register. If the
read is garbage (because the read failed) and the write succeeds, we
end up writing modified garbage to the adapter and this appears to be
able to hard wedge it.

After we added the retries, it should be nearly impossible to see this
problem. After all, the problem shows up if the register read failed
and _then_ the register write worked. If the read fails for many times
in a row, chances are things are really dead and no future writes will
succeed.

However, nearly impossible isn't actually impossible. To be safe,
let's fully lock out all register access if all of our retries
fail. When we do this, we'll try to queue up a USB reset and try to
unlock register access after the reset. To avoid looping forever,
we'll only attempt this a handful of times.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This patch was tested with REGISTER_ACCESS_TRIES set to 1, effectively
disabling the patch ("r8152: Retry register reads/writes"). I also
decreased the timeout back to 500ms so I could use kgdb to reproduce
failures. To confirm that the reset queuing worked, I allowed queueing
a reset even if pre-reset was running. That worked fine, but did cause
a double-reset.

 drivers/net/usb/r8152.c | 80 +++++++++++++++++++++++++++++++++++------
 1 file changed, 69 insertions(+), 11 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 976d6caf2f04..a7a55849fa1a 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -953,6 +953,10 @@ struct r8152 {
 	u8 version;
 	u8 duplex;
 	u8 autoneg;
+
+	bool reg_access_fail;
+	bool in_pre_reset;
+	unsigned int reg_access_reset_count;
 };
 
 /**

@@ -1210,14 +1214,22 @@ static unsigned int agg_buf_sz = 16384;
  */
 #define REGISTER_ACCESS_TRIES	3
 
+/* If register access fails too many times, stop trying to access the registers
+ * and issue a reset to fix the issue.
+ */
+#define REGISTER_ACCESS_MAX_RESETS	3
+
 static
 int r8152_control_msg(struct usb_device *udev, unsigned int pipe, __u8 request,
 		      __u8 requesttype, __u16 value, __u16 index, void *data,
-		      __u16 size, const char *msg_tag)
+		      __u16 size, const char *msg_tag, struct r8152 *tp)
 {
 	int i;
 	int ret;
 
+	if (tp && tp->reg_access_fail)
+		return -EIO;
+
 	for (i = 0; i < REGISTER_ACCESS_TRIES; i++) {
 		ret = usb_control_msg(udev, pipe, request, requesttype,
 				      value, index, data, size,

@@ -1233,14 +1245,54 @@ int r8152_control_msg(struct usb_device *udev, unsigned int pipe, __u8 request,
 			break;
 	}
 
-	if (ret < 0) {
+	if (ret >= 0) {
+		if (tp)
+			tp->reg_access_reset_count = 0;
+
+		if (i != 0)
+			dev_warn(&udev->dev,
+				 "Needed %d tries to %s %d bytes at %#06x/%#06x\n",
+				 i + 1, msg_tag, size, value, index);
+
+		return ret;
+	}
+
+	dev_err(&udev->dev,
+		"Failed to %s %d bytes at %#06x/%#06x (%d)\n",
+		msg_tag, size, value, index, ret);
+
+	/* If tp is NULL then we're called from early probe or from the
+	 * r8153_ecm module. In either case, the retries above are pretty much
+	 * all we can do. Bail.
+	 */
+	if (!tp)
+		return ret;
+
+	/* Block all future register access until we reset. This prevents the
+	 * parts of the driver that do read-modify-write without checking for
+	 * errors from writing back modified garbage to the adapter.
+	 */
+	tp->reg_access_fail = true;
+
+	/* Failing to access registers in pre-reset is not surprising since we
+	 * wouldn't be resetting if things were behaving normally. The register
+	 * access we do in pre-reset isn't truly mandatory--we're just reusing
+	 * the disable() function and trying to be nice by powering the
+	 * adapter down before resetting it.
+	 *
+	 * If we're in pre-reset, we'll return right away and not try to queue
+	 * up yet another reset. We know the post-reset is already coming.
+	 */
+	if (tp->in_pre_reset)
+		return ret;
+
+	if (tp->reg_access_reset_count < REGISTER_ACCESS_MAX_RESETS) {
+		usb_queue_reset_device(tp->intf);
+		tp->reg_access_reset_count++;
+	} else if (tp->reg_access_reset_count == REGISTER_ACCESS_MAX_RESETS) {
 		dev_err(&udev->dev,
-			"Failed to %s %d bytes at %#06x/%#06x (%d)\n",
-			msg_tag, size, value, index, ret);
-	} else if (i != 0) {
-		dev_warn(&udev->dev,
-			 "Needed %d tries to %s %d bytes at %#06x/%#06x\n",
-			 i + 1, msg_tag, size, value, index);
+			"Tried to reset %d times; giving up.\n",
+			REGISTER_ACCESS_MAX_RESETS);
 	}
 
 	return ret;

@@ -1258,7 +1310,7 @@ int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 
 	ret = r8152_control_msg(tp->udev, tp->pipe_ctrl_in,
 				RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
-				value, index, tmp, size, "read");
+				value, index, tmp, size, "read", tp);
 
 	if (ret < 0)
 		memset(data, 0xff, size);

@@ -1282,7 +1334,7 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 
 	ret = r8152_control_msg(tp->udev, tp->pipe_ctrl_out,
 				RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
-				value, index, tmp, size, "write");
+				value, index, tmp, size, "write", tp);
 
 	kfree(tmp);
 

@@ -8317,7 +8369,9 @@ static int rtl8152_pre_reset(struct usb_interface *intf)
 	napi_disable(&tp->napi);
 	if (netif_carrier_ok(netdev)) {
 		mutex_lock(&tp->control);
+		tp->in_pre_reset = true;
 		tp->rtl_ops.disable(tp);
+		tp->in_pre_reset = false;
 		mutex_unlock(&tp->control);
 	}
 

@@ -8333,6 +8387,10 @@ static int rtl8152_post_reset(struct usb_interface *intf)
 	if (!tp)
 		return 0;
 
+	mutex_lock(&tp->control);
+	tp->reg_access_fail = false;
+	mutex_unlock(&tp->control);
+
 	/* reset the MAC address in case of policy change */
 	if (determine_ethernet_addr(tp, &sa) >= 0) {
 		rtnl_lock();

@@ -9542,7 +9600,7 @@ static u8 __rtl_get_hw_ver(struct usb_device *udev)
 	ret = r8152_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 				RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
 				PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp),
-				"read");
+				"read", NULL);
 	if (ret > 0)
 		ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK;
 

-- 
2.42.0.515.g380fc7ccd1-goog

Even though the functions to read/write registers can fail, most of
the places in the r8152 driver that read/write register values don't
check error codes. The lack of error code checking is problematic in
at least two ways.

The first problem is that the r8152 driver often uses code patterns
similar to this:
  x = read_register()
  x = x | SOME_BIT;
  write_register(x);

...with the above pattern, if the read_register() fails and returns
garbage then we'll end up trying to write modified garbage back to the
Realtek adapter. If the write_register() succeeds that's bad. Note
that as of commit f53a7ad18959 ("r8152: Set memory to all 0xFFs on
failed reg reads") the "garbage" returned by read_register() will at
least be consistent garbage, but it is still garbage.

It turns out that this problem is very serious. Writing garbage to
some of the hardware registers on the Ethernet adapter can put the
adapter in such a bad state that it needs to be power cycled (fully
unplugged and plugged in again) before it can enumerate again.

The second problem is that the r8152 driver generally has functions
that are long sequences of register writes. Assuming everything will
be OK if a random register write fails in the middle isn't a great
assumption.

One might wonder if the above two problems are real. You could ask if
we would really have a successful write after a failed read. It turns
out that the answer appears to be "yes, this can happen". In fact,
we've seen at least two distinct failure modes where this happens.

On a sc7180-trogdor Chromebook if you drop into kdb for a while and
then resume, you can see:
1. We get a "Tx timeout"
2. The "Tx timeout" queues up a USB reset.
3. In rtl8152_pre_reset() we try to reinit the hardware.
4. The first several (2-9) register accesses fail with a timeout, then
   things recover.

The above test case was actually fixed by the patch ("r8152: Increase
USB control msg timeout to 5000ms as per spec") but at least shows
that we really can see successful calls after failed ones.

On a different (AMD) based Chromebook, we found that during reboot
tests we'd also sometimes get a transitory failure. In this case we
saw -EPIPE being returned sometimes. Retrying one time worked fine.

To keep things robust, let's try register access up to 3 times. If we
get 3 5-second timeouts in a row this could block things for 15
seconds but that hasn't been seen in practice. If we see that
happening and there is a better way to solve it then we can add a
special case for that later.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Originally when looking at this problem I thought that the obvious
solution was to "just" add better error handling to the driver. This
_sounds_ appealing, but it's a massive change and touches a
significant portion of the lines in this driver. It's also not always
obvious what the driver should be doing to handle errors.

 drivers/net/usb/r8152.c | 67 +++++++++++++++++++++++++++++++++++------
 1 file changed, 57 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 482957beae66..976d6caf2f04 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -1200,6 +1200,52 @@ static unsigned int agg_buf_sz = 16384;
 
 #define RTL_LIMITED_TSO_SIZE	(size_to_mtu(agg_buf_sz) - sizeof(struct tx_desc))
 
+/* If we get a failure and the USB device is still attached when trying to read
+ * or write registers then we'll retry a few times. Failures accessing registers
+ * shouldn't be common and this adds robustness. Much code in the driver doesn't
+ * check for errors. Notably, many parts of the driver do a read/modify/write
+ * of a register value without confirming that the read succeeded. Writing back
+ * modified garbage like this can fully wedge the adapter, requiring a power
+ * cycle.
+ */
+#define REGISTER_ACCESS_TRIES	3
+
+static
+int r8152_control_msg(struct usb_device *udev, unsigned int pipe, __u8 request,
+		      __u8 requesttype, __u16 value, __u16 index, void *data,
+		      __u16 size, const char *msg_tag)
+{
+	int i;
+	int ret;
+
+	for (i = 0; i < REGISTER_ACCESS_TRIES; i++) {
+		ret = usb_control_msg(udev, pipe, request, requesttype,
+				      value, index, data, size,
+				      USB_CTRL_GET_TIMEOUT);
+
+		/* No need to retry or spam errors if the USB device got
+		 * unplugged; just return immediately.
+		 */
+		if (udev->state == USB_STATE_NOTATTACHED)
+			return ret;
+
+		if (ret >= 0)
+			break;
+	}
+
+	if (ret < 0) {
+		dev_err(&udev->dev,
+			"Failed to %s %d bytes at %#06x/%#06x (%d)\n",
+			msg_tag, size, value, index, ret);
+	} else if (i != 0) {
+		dev_warn(&udev->dev,
+			 "Needed %d tries to %s %d bytes at %#06x/%#06x\n",
+			 i + 1, msg_tag, size, value, index);
+	}
+
+	return ret;
+}
+
 static
 int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 {

@@ -1210,9 +1256,10 @@ int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 	if (!tmp)
 		return -ENOMEM;
 
-	ret = usb_control_msg(tp->udev, tp->pipe_ctrl_in,
-			      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
-			      value, index, tmp, size, USB_CTRL_GET_TIMEOUT);
+	ret = r8152_control_msg(tp->udev, tp->pipe_ctrl_in,
+				RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
+				value, index, tmp, size, "read");
+
 	if (ret < 0)
 		memset(data, 0xff, size);
 	else

@@ -1233,9 +1280,9 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
 	if (!tmp)
 		return -ENOMEM;
 
-	ret = usb_control_msg(tp->udev, tp->pipe_ctrl_out,
-			      RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
-			      value, index, tmp, size, USB_CTRL_SET_TIMEOUT);
+	ret = r8152_control_msg(tp->udev, tp->pipe_ctrl_out,
+				RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
+				value, index, tmp, size, "write");
 
 	kfree(tmp);
 

@@ -9492,10 +9539,10 @@ static u8 __rtl_get_hw_ver(struct usb_device *udev)
 	if (!tmp)
 		return 0;
 
-	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-			      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
-			      PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp),
-			      USB_CTRL_GET_TIMEOUT);
+	ret = r8152_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+				RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
+				PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp),
+				"read");
 	if (ret > 0)
 		ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK;
 

-- 
2.42.0.515.g380fc7ccd1-goog

On Tue, Sep 26, 2023 at 02:27:27PM -0700, Douglas Anderson wrote:

Rather than testing udev->state, it would be better to check whether
ret == -ENODEV.  udev->state is meant primarily for use by the USB core
and it's subject to races.

Alan Stern

Hi,

On Wed, Sep 27, 2023 at 6:43 AM Alan Stern [off-list ref] wrote:

On Tue, Sep 26, 2023 at 02:27:27PM -0700, Douglas Anderson wrote:

quoted

+
+static
+int r8152_control_msg(struct usb_device *udev, unsigned int pipe, __u8 request,
+                   __u8 requesttype, __u16 value, __u16 index, void *data,
+                   __u16 size, const char *msg_tag)
+{
+     int i;
+     int ret;
+
+     for (i = 0; i < REGISTER_ACCESS_TRIES; i++) {
+             ret = usb_control_msg(udev, pipe, request, requesttype,
+                                   value, index, data, size,
+                                   USB_CTRL_GET_TIMEOUT);
+
+             /* No need to retry or spam errors if the USB device got
+              * unplugged; just return immediately.
+              */
+             if (udev->state == USB_STATE_NOTATTACHED)
+                     return ret;

Rather than testing udev->state, it would be better to check whether
ret == -ENODEV.  udev->state is meant primarily for use by the USB core
and it's subject to races.

Thanks for looking my patch over!

Happy to change this to -ENODEV. In my early drafts of this patch I
looked at -ENODEV but I noticed that other places in the driver were
checking `udev->state == USB_STATE_NOTATTACHED` so I changed it. In
reality I think for this code path it doesn't matter a whole lot. The
only thing it's doing is avoiding a few extra retries and avoiding a
log message. :-)

I'll wait a few more days to see if there is any other feedback on
this series and then send a new version with that addressed. If
someone needs me to send a new version sooner then please yell.

-Doug