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

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

[Ming Lei]

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 the USB serial, HID and several usbnet drivers
which may recover device in suspend failure path of system sleep.

v1:
	- fix compile failure
	- add comments about handling suspend failure in resume()

Thanks,
--
Ming Lei

[PATCH v1 2/7] USB: serial: handle suspend failure path correctly

[Ming Lei]

This patch kills traffic even though type->suspend returns
failure inside usb_serial_suspend from system sleep context
because USB core ignores the failiure and lets system sleep
go ahread, so the serial URB traffic need to be killed
in this case.

Cc: Johan Hovold <jhovold@gmail.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/usb/serial/usb-serial.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index a19ed74..9d0b9c8 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -1142,10 +1142,11 @@ int usb_serial_suspend(struct usb_interface *intf, pm_message_t message)
 
 	if (serial->type->suspend) {
 		r = serial->type->suspend(serial, message);
-		if (r < 0) {
+		if (r < 0 && PMSG_IS_AUTO(message)) {
 			serial->suspending = 0;
 			goto err_out;
 		}
+		/* TODO: resume() might need to handle suspend failure */
 	}
 
 	for (i = 0; i < serial->num_ports; ++i) {
-- 
1.7.9.5

[PATCH v1 3/7] USBHID: don't recover device if suspend fails in system sleep

[Ming Lei]

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.

Cc: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 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 8e0c4bf94..1f9e56b 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1493,7 +1493,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)) {
@@ -1520,19 +1520,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);
@@ -1544,7 +1540,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

[PATCH v1 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep

[Ming Lei]

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.

Cc: Bjørn Mork <bjorn@mork.no>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/cdc_mbim.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index 248d2dc..ec58c2c 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -338,7 +338,7 @@ static int cdc_mbim_suspend(struct usb_interface *intf, pm_message_t message)
 
 	if (intf == ctx->control && info->subdriver && info->subdriver->suspend)
 		ret = info->subdriver->suspend(intf, message);
-	if (ret < 0)
+	if (ret < 0 && PMSG_IS_AUTO(message))
 		usbnet_resume(intf);
 
 error:
-- 
1.7.9.5

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

Re: [PATCH v1 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep

[Bjørn Mork]

Ming Lei <ming.lei@canonical.com> writes:

> 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.
>
> Cc: Bjørn Mork <bjorn@mork.no>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/net/usb/cdc_mbim.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
> index 248d2dc..ec58c2c 100644
> --- a/drivers/net/usb/cdc_mbim.c
> +++ b/drivers/net/usb/cdc_mbim.c
> @@ -338,7 +338,7 @@ static int cdc_mbim_suspend(struct usb_interface *intf, pm_message_t message)
>  
>  	if (intf == ctx->control && info->subdriver && info->subdriver->suspend)
>  		ret = info->subdriver->suspend(intf, message);
> -	if (ret < 0)
> +	if (ret < 0 && PMSG_IS_AUTO(message))
>  		usbnet_resume(intf);
>  
>  error:

This condition will never happen because the subdriver callback always
return 0 if !PMSG_IS_AUTO(message), so adding anything here is purely
for documentation purposes.  That is OK for me.  But I do not see any
point adding incomplete or wrong documentation. 

The above is incomplete because it ignores the failure and still return
an error. And it is wrong because we ignore an error we cannot possibly
handle in resume.  Only the subdriver can do that, which is why we must
delegate this to the subdriver.

I believe the correct here is to document the fact that we require the
subdriver->suspend() callback to always return 0 if !PMSG_IS_AUTO(message).
That is best done with a comment. No need to add any redundant code.


Bjørn

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

Re: [PATCH v1 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep

[Ming Lei]

On Wed, Mar 6, 2013 at 8:04 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
> Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> writes:
>
>> 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.
>>
>> Cc: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
>> Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> ---
>>  drivers/net/usb/cdc_mbim.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
>> index 248d2dc..ec58c2c 100644
>> --- a/drivers/net/usb/cdc_mbim.c
>> +++ b/drivers/net/usb/cdc_mbim.c
>> @@ -338,7 +338,7 @@ static int cdc_mbim_suspend(struct usb_interface *intf, pm_message_t message)
>>
>>       if (intf == ctx->control && info->subdriver && info->subdriver->suspend)
>>               ret = info->subdriver->suspend(intf, message);
>> -     if (ret < 0)
>> +     if (ret < 0 && PMSG_IS_AUTO(message))
>>               usbnet_resume(intf);
>>
>>  error:
>
> This condition will never happen because the subdriver callback always
> return 0 if !PMSG_IS_AUTO(message), so adding anything here is purely
> for documentation purposes.  That is OK for me.  But I do not see any
> point adding incomplete or wrong documentation.
>
> The above is incomplete because it ignores the failure and still return
> an error. And it is wrong because we ignore an error we cannot possibly
> handle in resume.  Only the subdriver can do that, which is why we must
> delegate this to the subdriver.
>
> I believe the correct here is to document the fact that we require the
> subdriver->suspend() callback to always return 0 if !PMSG_IS_AUTO(message).
> That is best done with a comment. No need to add any redundant code.

Considered that the subdriver might change its return value in future, how about
just add the below comment?

        /* TODO: resume() might need to handle suspend failure from subdriver */


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

Re: [PATCH v1 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep

[Bjørn Mork]

Ming Lei <ming.lei@canonical.com> writes:
> On Wed, Mar 6, 2013 at 8:04 PM, Bjørn Mork <bjorn@mork.no> wrote:
>
>> I believe the correct here is to document the fact that we require the
>> subdriver->suspend() callback to always return 0 if !PMSG_IS_AUTO(message).
>> That is best done with a comment. No need to add any redundant code.
>
> Considered that the subdriver might change its return value in future,

Not likely.  It lives under the exact same restrictions as a standalone
USB interface driver.  If it starts returning something else, then this
driver should probably do so as well.

> how about just add the below comment?
>
>         /* TODO: resume() might need to handle suspend failure from subdriver */

OK with me if that includes dropping the redundant code change.

Note that the same issue and comment applies to the usbnet_suspend()
call as well.  We currently rely on both calls to always return 0 on
system suspend.  Which they are guaranteed to do, because they are both
directly used as .suspend callbacks in other USB interface drivers.


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 5/7] usbnet: smsc95xx: don't recover device if suspend fails in system sleep

[Ming Lei]

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.

Cc: Steve Glendinning <steve.glendinning@shawell.net>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/smsc95xx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index e6d2dea..195279c 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1660,7 +1660,7 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 	ret = smsc95xx_enter_suspend0(dev);
 
 done:
-	if (ret)
+	if (ret && PMSG_IS_AUTO(message))
 		usbnet_resume(intf);
 	return ret;
 }
-- 
1.7.9.5

[PATCH v1 6/7] usbnet: smsc75xx: don't recover device if suspend fails in system sleep

[Ming Lei]

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.

Cc: Steve Glendinning <steve.glendinning@shawell.net>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/smsc75xx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 9abe517..997a694 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -2011,7 +2011,7 @@ static int smsc75xx_suspend(struct usb_interface *intf, pm_message_t message)
 	ret = smsc75xx_enter_suspend0(dev);
 
 done:
-	if (ret)
+	if (ret && PMSG_IS_AUTO(message))
 		usbnet_resume(intf);
 	return ret;
 }
-- 
1.7.9.5

[PATCH v1 1/7] USB: adds comment on suspend callback

[Ming Lei]

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 and let resume/reset_resume callback
handle the suspend failure if needed.

Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 include/linux/usb.h |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/usb.h b/include/linux/usb.h
index 4d22d0f..6e66dc4 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -978,7 +978,14 @@ struct usbdrv_wrap {
  *	the "usbfs" filesystem.  This lets devices provide ways to
  *	expose information to user space regardless of where they
  *	do (or don't) show up otherwise in the filesystem.
- * @suspend: Called when the device is going to be suspended by the system.
+ * @suspend: Called when the device is going to be suspended by the
+ *	system either from system sleep or runtime suspend context, and
+ *	its failure return value will be ignored in system sleep context,
+ *	so do NOT try to recover device for this case. It is suggested
+ *	the callback always return 0 if resume/reset_resume callback can
+ *	handle system sususpend failure at default, otherwise driver
+ *	need to record the suspend failure and let resume/reset_resume
+ *	handle previous system suspend failure to recover device.
  * @resume: Called when the device is being resumed by the system.
  * @reset_resume: Called when the suspended device has been reset instead
  *	of being resumed.
-- 
1.7.9.5

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

Re: [PATCH v1 1/7] USB: adds comment on suspend callback

[Alan Stern]

On Wed, 6 Mar 2013, Ming Lei wrote:

> 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 and let resume/reset_resume callback
> handle the suspend failure if needed.

In this same patch you should also update the kerneldoc for 
usb_suspend_both().  It should mention that errors returned by the 
suspend method for @udev will be ignored during system suspend.

> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  include/linux/usb.h |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 4d22d0f..6e66dc4 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -978,7 +978,14 @@ struct usbdrv_wrap {
>   *	the "usbfs" filesystem.  This lets devices provide ways to
>   *	expose information to user space regardless of where they
>   *	do (or don't) show up otherwise in the filesystem.
> - * @suspend: Called when the device is going to be suspended by the system.
> + * @suspend: Called when the device is going to be suspended by the
> + *	system either from system sleep or runtime suspend context, and
> + *	its failure return value will be ignored in system sleep context,

This is an ungrammatical run-on sentence.  s/, and its failure/. The/

> + *	so do NOT try to recover device for this case. It is suggested
> + *	the callback always return 0 if resume/reset_resume callback can
> + *	handle system sususpend failure at default, otherwise driver
> + *	need to record the suspend failure and let resume/reset_resume
> + *	handle previous system suspend failure to recover device.

I don't understand what this is supposed to mean.  How about this
instead?  "so do NOT try to continue using the device if suspend fails
in this case.  Instead, let the resume or reset-resume routine recover
from the failure."

Alan Stern

Re: [PATCH v1 1/7] USB: adds comment on suspend callback

[Ming Lei]

On Wed, Mar 6, 2013 at 11:25 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 6 Mar 2013, Ming Lei wrote:
>
>> 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 and let resume/reset_resume callback
>> handle the suspend failure if needed.
>
> In this same patch you should also update the kerneldoc for
> usb_suspend_both().  It should mention that errors returned by the
> suspend method for @udev will be ignored during system suspend.

OK.

>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  include/linux/usb.h |    9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> index 4d22d0f..6e66dc4 100644
>> --- a/include/linux/usb.h
>> +++ b/include/linux/usb.h
>> @@ -978,7 +978,14 @@ struct usbdrv_wrap {
>>   *   the "usbfs" filesystem.  This lets devices provide ways to
>>   *   expose information to user space regardless of where they
>>   *   do (or don't) show up otherwise in the filesystem.
>> - * @suspend: Called when the device is going to be suspended by the system.
>> + * @suspend: Called when the device is going to be suspended by the
>> + *   system either from system sleep or runtime suspend context, and
>> + *   its failure return value will be ignored in system sleep context,
>
> This is an ungrammatical run-on sentence.  s/, and its failure/. The/

OK.

>
>> + *   so do NOT try to recover device for this case. It is suggested
>> + *   the callback always return 0 if resume/reset_resume callback can
>> + *   handle system sususpend failure at default, otherwise driver
>> + *   need to record the suspend failure and let resume/reset_resume
>> + *   handle previous system suspend failure to recover device.
>
> I don't understand what this is supposed to mean.  How about this
> instead?  "so do NOT try to continue using the device if suspend fails
> in this case.  Instead, let the resume or reset-resume routine recover
> from the failure."

Looks your description is more neat, and will take it in v2.


Thanks,
--
Ming Lei

[PATCH v1 7/7] usbnet: qmi_wwan: don't recover device if suspend fails in system sleep

[Ming Lei]

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.

Cc: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/net/usb/qmi_wwan.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index efb5c7c..1b4367c 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -294,7 +294,7 @@ static int qmi_wwan_suspend(struct usb_interface *intf, pm_message_t message)
 
 	if (intf == info->control && info->subdriver && info->subdriver->suspend)
 		ret = info->subdriver->suspend(intf, message);
-	if (ret < 0)
+	if (ret < 0  && PMSG_IS_AUTO(message))
 		usbnet_resume(intf);
 err:
 	return ret;
-- 
1.7.9.5

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

Re: [PATCH v1 7/7] usbnet: qmi_wwan: don't recover device if suspend fails in system sleep

[Bjørn Mork]

Ming Lei <ming.lei@canonical.com> writes:

> 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.
>
> Cc: Bjørn Mork <bjorn@mork.no>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/net/usb/qmi_wwan.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index efb5c7c..1b4367c 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -294,7 +294,7 @@ static int qmi_wwan_suspend(struct usb_interface *intf, pm_message_t message)
>  
>  	if (intf == info->control && info->subdriver && info->subdriver->suspend)
>  		ret = info->subdriver->suspend(intf, message);
> -	if (ret < 0)
> +	if (ret < 0  && PMSG_IS_AUTO(message))
>  		usbnet_resume(intf);
>  err:
>  	return ret;

Same comment as for cdc_mbim: This is more confusing than helpful
because it makes it look like qmi_wwan_suspend can return an error on
system suspend.  Better adding a comment explaining why the original
code is OK.


Bjørn