[PATCH v3 0/7] USB: don't recover device if suspend fails in system sleep

Hi,

This patch adds comments on interface driver suspend callback
to emphasize that the failure return value is ignored by
USB core in system sleep context, so do not try to recover
device for this case, otherwise the recovery things may confuse
resume().

Also fixes USB HID and several usbnet drivers which may recover
device in suspend failure path of system sleep.

v3:
	- only 2/7 changed, don't change code but add comments to
	restrict subdriver->suspend(), suggested by Johan Hovold.

v2:
        - improve comments on suspend callback as suggested by Alan
        - update kerneldoc for usb_suspend_both as suggested by Alan
        - remove previous check of PMSG_IS_AUTO(message) in cdc_mbim/
        qmi_wwan and add comments on suspend failure case, since Bjørn
        doesn't like the check.
        - add comments on smsc95xx/smsc75xx
v1:
        - fix compile failure
        - add comments about handling suspend failure in resume()

 drivers/hid/usbhid/hid-core.c   |   14 +++++---------
 drivers/net/usb/cdc_mbim.c      |    5 +++++
 drivers/net/usb/qmi_wwan.c      |    5 +++++
 drivers/net/usb/smsc75xx.c      |    6 +++++-
 drivers/net/usb/smsc95xx.c      |    6 +++++-
 drivers/usb/core/driver.c       |   11 ++++++++---
 drivers/usb/serial/usb-serial.c |    5 +++++
 include/linux/usb.h             |    7 ++++++-
 8 files changed, 44 insertions(+), 15 deletions(-)


Thanks
--
Ming Lei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

If suspend callback fails in system sleep context, usb core will
ignore the failure and let the system sleep go ahead further, so this
patch doesn't recover device under this situation, otherwise
may cause resume() confused.

Acked-by: Jiri Kosina <redacted>
Signed-off-by: Ming Lei <redacted>
---
 drivers/hid/usbhid/hid-core.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index effcd3d..9941828 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c

@@ -1520,7 +1520,7 @@ static int hid_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct hid_device *hid = usb_get_intfdata(intf);
 	struct usbhid_device *usbhid = hid->driver_data;
-	int status;
+	int status = 0;
 	bool driver_suspended = false;
 
 	if (PMSG_IS_AUTO(message)) {

@@ -1547,19 +1547,15 @@ static int hid_suspend(struct usb_interface *intf, pm_message_t message)
 		}
 
 	} else {
-		if (hid->driver && hid->driver->suspend) {
+		/* TODO: resume() might need to handle suspend failure */
+		if (hid->driver && hid->driver->suspend)
 			status = hid->driver->suspend(hid, message);
-			if (status < 0)
-				return status;
-		}
 		driver_suspended = true;
 		spin_lock_irq(&usbhid->lock);
 		set_bit(HID_SUSPENDED, &usbhid->iofl);
 		spin_unlock_irq(&usbhid->lock);
-		if (usbhid_wait_io(hid) < 0) {
+		if (usbhid_wait_io(hid) < 0)
 			status = -EIO;
-			goto failed;
-		}
 	}
 
 	hid_cancel_delayed_stuff(usbhid);

@@ -1571,7 +1567,7 @@ static int hid_suspend(struct usb_interface *intf, pm_message_t message)
 		goto failed;
 	}
 	dev_dbg(&intf->dev, "suspend\n");
-	return 0;
+	return status;
 
  failed:
 	hid_resume_common(hid, driver_suspended);

-- 
1.7.9.5

If suspend callback fails in system sleep context, usb core will
ignore the failure and let system sleep go ahead further, so
this patch comments on the case and requires that both
usbnet_suspend() and subdriver->suspend() MUST return 0 in
system sleep context.

Acked-by: David S. Miller <davem@davemloft.net>
Acked-by: Bjørn Mork <bjorn@mork.no>
Signed-off-by: Ming Lei <redacted>
---
 drivers/net/usb/cdc_mbim.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index 248d2dc..406a34d 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c

@@ -332,6 +332,11 @@ static int cdc_mbim_suspend(struct usb_interface *intf, pm_message_t message)
 		goto error;
 	}
 
+	/*
+	 * Both usbnet_suspend() and subdriver->suspend() MUST return 0
+	 * in system sleep context, otherwise, the resume callback has
+	 * to recover device from previous suspend failure.
+	 */
 	ret = usbnet_suspend(intf, message);
 	if (ret < 0)
 		goto error;

-- 
1.7.9.5

If suspend callback fails in system sleep context, usb core will
ignore the failure and let system sleep go ahead further, so
this patch doesn't recover device under this situation.

Also add comments on the case.

Acked-by: David S. Miller <davem@davemloft.net>
Cc: Steve Glendinning <steve.glendinning@shawell.net>
Signed-off-by: Ming Lei <redacted>
---
 drivers/net/usb/smsc95xx.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index e6d2dea..3f38ba8 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c

@@ -1660,7 +1660,11 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 	ret = smsc95xx_enter_suspend0(dev);
 
 done:
-	if (ret)
+	/*
+	 * TODO: resume() might need to handle the suspend failure
+	 * in system sleep
+	 */
+	if (ret && PMSG_IS_AUTO(message))
 		usbnet_resume(intf);
 	return ret;
 }

-- 
1.7.9.5

If suspend callback fails in system sleep context, usb core will
ignore the failure and let system sleep go ahead further, so
this patch doesn't recover device under this situation.

Also add comments on this case.

Acked-by: David S. Miller <davem@davemloft.net>
Cc: Steve Glendinning <steve.glendinning@shawell.net>
Signed-off-by: Ming Lei <redacted>
---
 drivers/net/usb/smsc75xx.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 9abe517..21b607a 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c

@@ -2011,7 +2011,11 @@ static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
 	ret = smsc75xx_enter_suspend0(dev);
 
 done:
-	if (ret)
+	/*
+	 * TODO: resume() might need to handle the suspend failure
+	 * in system sleep
+	 */
+	if (ret && PMSG_IS_AUTO(message))
 		usbnet_resume(intf);
 	return ret;
 }

-- 
1.7.9.5