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

[PATCH 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 URB traffic scheduled
in recovery of failure path may cross system sleep, and may
cause problems.

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

Thanks,
--
Ming Lei

[PATCH 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.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 include/linux/usb.h |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/usb.h b/include/linux/usb.h
index 4d22d0f..ea9d7cb 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -978,7 +978,10 @@ 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 failed return value will be ignored in system sleep context,
+ *	so do NOT try to recover device for this case.
  * @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

[PATCH 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 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..04ad3d5 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -1144,7 +1144,8 @@ int usb_serial_suspend(struct usb_interface *intf, pm_message_t message)
 		r = serial->type->suspend(serial, message);
 		if (r < 0) {
 			serial->suspending = 0;
-			goto err_out;
+			if (PMSG_IS_AUTO(msg))
+				goto err_out;
 		}
 	}
 
-- 
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

[PATCH 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.

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..a15ec29 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) {
+		/* Ignore failure return value in system sleep */
+		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 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..da83546 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(msg))
 		usbnet_resume(intf);
 
 error:
-- 
1.7.9.5

[PATCH 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..a0c5478 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(msg))
 		usbnet_resume(intf);
 	return ret;
 }
-- 
1.7.9.5

[PATCH 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..f65c506 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(msg))
 		usbnet_resume(intf);
 	return ret;
 }
-- 
1.7.9.5

[PATCH 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@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..fce18be 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(msg))
 		usbnet_resume(intf);
 err:
 	return ret;
-- 
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 0/7] USB: don't recover device if suspend fails in system sleep

[Ming Lei]

On Tue, Mar 5, 2013 at 12:01 PM, Ming Lei <ming.lei@canonical.com> wrote:
> 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 URB traffic scheduled
> in recovery of failure path may cross system sleep, and may
> cause problems.
>
> Also fixes the USB serial, HID and several usbnet drivers
> which may recover device in suspend failure path of system sleep.

Sorry, this patches have compile problem, so please don't apply them,
and I will send v1 later.


Thanks,
--
Ming Lei

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

[Bjørn Mork]

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

> 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 URB traffic scheduled
> in recovery of failure path may cross system sleep, and may
> cause problems.

Well, an unexpected error did happen so problems are to be expected,
yes.

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

I believe all of these are wrong unless you have any real bug which is
fixed by this.

All these drivers suspend in multiple steps, where each step can
fail. If a later step fails then they revert any previously successful
step before returning the failure, thereby ensuring that the
device/driver state when suspend returns is consistently either
suspended or resumed.

The error recovery they do in suspend is not about preventing suspend at
all.  It is about ensuring that that the driver and device is in a
consistent state, which is "resumed" if suspend fails.

Your patch set make the drivers return from suspend in some intermediate
state, where the device and/or driver is neither suspended nor resumed.
This is wrong.  You still did not necessarily kill all URBs, but you
killed some of them.  What is resume() going to do then?

I am going to NAK the cdc_mbim and qmi_wwan pacthes unless you can
convince me that we need to add a "partly-suspended" state for the
system suspend error case.  In which case the patch will need to include
the corresponding resume fix for the "partly-suspended" state.


Bjørn

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

[Bjørn Mork]

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..da83546 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(msg))
>  		usbnet_resume(intf);
>  
>  error:

NAK. We if the device cannot suspend, then it cannot do suspend halfways
either. Whether the caller will ignore the error or not, is irrelevant.


Bjørn
--
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 7/7] usbnet: qmi_wwan: don't recover device if suspend fails in system sleep

[Bjørn Mork]

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/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..fce18be 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(msg))
>  		usbnet_resume(intf);
>  err:
>  	return ret;


NAK. We if the device cannot suspend, then it cannot do suspend halfways
either. Whether the caller will ignore the error or not, is irrelevant.


Bjørn
--
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 0/7] USB: don't recover device if suspend fails in system sleep

[Ming Lei]

On Tue, Mar 5, 2013 at 3:03 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>
>> 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 URB traffic scheduled
>> in recovery of failure path may cross system sleep, and may
>> cause problems.
>
> Well, an unexpected error did happen so problems are to be expected,
> yes.
>
>> Also fixes the USB serial, HID and several usbnet drivers
>> which may recover device in suspend failure path of system sleep.
>
> I believe all of these are wrong unless you have any real bug which is
> fixed by this.

It is really a bug if one driver submits URBs and keeps them scheduled
on bus before system sleep, because the bus transaction can't be kept
across system sleep cycle.

>
> All these drivers suspend in multiple steps, where each step can
> fail. If a later step fails then they revert any previously successful
> step before returning the failure, thereby ensuring that the
> device/driver state when suspend returns is consistently either
> suspended or resumed.

IMO, for autosuspend, that is right, but it is not for system suspend,
and the driver's suspend callback can't return in resumed state
because the USB core will ignore the failure return value and force
to suspend the device.

>
> The error recovery they do in suspend is not about preventing suspend at
> all.  It is about ensuring that that the driver and device is in a
> consistent state, which is "resumed" if suspend fails.

No, only putting the device as "suspended" in system suspend failure
can keep the state as consistent, see the above explanation.

>
> Your patch set make the drivers return from suspend in some intermediate
> state, where the device and/or driver is neither suspended nor resumed.
> This is wrong.  You still did not necessarily kill all URBs, but you
> killed some of them.  What is resume() going to do then?

These patches try to avoid submitting new URBs in the failure path of
system suspend, and we can comments these fixes in each patch one
by one.

>
> I am going to NAK the cdc_mbim and qmi_wwan pacthes unless you can
> convince me that we need to add a "partly-suspended" state for the
> system suspend error case.  In which case the patch will need to include
> the corresponding resume fix for the "partly-suspended" state.
>

Yes, you may argue that the device might be in partly-suspended state,
but it doesn't matter since the device will be put into suspend later and
the resume callback can recover from the partly-suspend state.

These patches doesn't break previous failure path of system suspend
and just avoid to submit new URBs, so how about letting later delta
patches fix for the corresponding resume?


Thanks,
--
Ming Lei
--
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 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep

[Ming Lei]

On Tue, Mar 5, 2013 at 3:09 PM, Bjørn Mork <bjorn@mork.no> wrote:
> 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..da83546 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(msg))
>>               usbnet_resume(intf);
>>
>>  error:
>
> NAK. We if the device cannot suspend, then it cannot do suspend halfways

Sorry, what do you mean that the device cannot suspend?  If you mean
usb_suspend_device(), its failure is still ignored, and generally it is OK
since the suspend action is driven by upstream port.

> either. Whether the caller will ignore the error or not, is irrelevant.

Anyway, usbnet_resume() can't be called to submit new URBs in
the failure path of system suspend, so the patch should be correct.


Thanks,
--
Ming Lei
--
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 7/7] usbnet: qmi_wwan: don't recover device if suspend fails in system sleep

[Ming Lei]

On Tue, Mar 5, 2013 at 3:09 PM, Bjørn Mork <bjorn@mork.no> wrote:
> 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..fce18be 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(msg))
>>               usbnet_resume(intf);
>>  err:
>>       return ret;
>
>
> NAK. We if the device cannot suspend, then it cannot do suspend halfways
> either. Whether the caller will ignore the error or not, is irrelevant.

See my same comments on 4/7.

Thanks,
--
Ming Lei
--
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 0/7] USB: don't recover device if suspend fails in system sleep

[Oliver Neukum]

On Tuesday 05 March 2013 18:55:42 Ming Lei wrote:
> > All these drivers suspend in multiple steps, where each step can
> > fail. If a later step fails then they revert any previously successful
> > step before returning the failure, thereby ensuring that the
> > device/driver state when suspend returns is consistently either
> > suspended or resumed.
> 
> IMO, for autosuspend, that is right, but it is not for system suspend,
> and the driver's suspend callback can't return in resumed state
> because the USB core will ignore the failure return value and force
> to suspend the device.

It seems to me that in this case you just need to make sure that
suspend() not fail for system suspend. Or revisit the decision to
ignore failures.
In other words, if we don't handle errors, there must be no errors,
otherwise it doesn't matter what we do in the error case. We'd leave
the problem to generic layers.

Furthermore there is a small chance that although the device tree
is walked, teh system suspend fails for another later reason that
is not ignored. In that case the drivers need to do error recovery,
albeit in resume().

	Regards
		Oliver

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

[Ming Lei]

On Tue, Mar 5, 2013 at 8:50 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
>>
>> IMO, for autosuspend, that is right, but it is not for system suspend,
>> and the driver's suspend callback can't return in resumed state
>> because the USB core will ignore the failure return value and force
>> to suspend the device.
>
> It seems to me that in this case you just need to make sure that
> suspend() not fail for system suspend. Or revisit the decision to
> ignore failures.

IMO, the current policy is correct.

> In other words, if we don't handle errors, there must be no errors,
> otherwise it doesn't matter what we do in the error case. We'd leave
> the problem to generic layers.

Generic layers can't handle the driver's specific failure.

If driver records its suspend failure state in suspend(), resume()
should and can deal with it without much difficulty.

>
> Furthermore there is a small chance that although the device tree
> is walked, teh system suspend fails for another later reason that
> is not ignored. In that case the drivers need to do error recovery,
> albeit in resume().

Yes,  resume() need to handle the USB system suspend failure
either in normal resume or error recovery, both are basically same.

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 1/7] USB: adds comment on suspend callback

[Ming Lei]

On Tue, Mar 5, 2013 at 12:01 PM, Ming Lei <ming.lei@canonical.com> 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.
>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  include/linux/usb.h |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 4d22d0f..ea9d7cb 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -978,7 +978,10 @@ 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 failed return value will be ignored in system sleep context,
> + *     so do NOT try to recover device for this case.
>   * @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.

On the other hand, resume()/reset_resume() should deal with the previous
suspend failure if the policy to ignore suspend failure is kept.


Thanks,
--
Ming Lei

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

[Bjørn Mork]

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

> On Tue, Mar 5, 2013 at 3:03 PM, Bjørn Mork <bjorn@mork.no> wrote:
>> Ming Lei <ming.lei@canonical.com> writes:
>>
>>> 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 URB traffic scheduled
>>> in recovery of failure path may cross system sleep, and may
>>> cause problems.
>>
>> Well, an unexpected error did happen so problems are to be expected,
>> yes.
>>
>>> Also fixes the USB serial, HID and several usbnet drivers
>>> which may recover device in suspend failure path of system sleep.
>>
>> I believe all of these are wrong unless you have any real bug which is
>> fixed by this.
>
> It is really a bug if one driver submits URBs and keeps them scheduled
> on bus before system sleep, because the bus transaction can't be kept
> across system sleep cycle.

1. You do not fix that.  You just ignore that the driver failed to
   cancel *one* of its URBs and return to the caller in an unknown
   state.  That's not even papering over the bug, it is just adding a
   new one without solving anything

2. I asked for a *real* bug, as opposed to theoretical bug.  Working
  around real issues by papering over them may sometimes be acceptable
  if the real issue is bad enough.  Working around theoretical bugs this
  way never is.

Yes, I can also see the theoretical issue.  Feel free to fix that if you
know how. I don't.

>> All these drivers suspend in multiple steps, where each step can
>> fail. If a later step fails then they revert any previously successful
>> step before returning the failure, thereby ensuring that the
>> device/driver state when suspend returns is consistently either
>> suspended or resumed.
>
> IMO, for autosuspend, that is right, but it is not for system suspend,
> and the driver's suspend callback can't return in resumed state

OK.  You do realize that the code you are changing tries to deal with
the case where suspend already failed, so returning in suspended state
is impossible?  If you cannot return in resumed state, then you have two
options:
 a) define a new state
 b) don't return

You seem to want to do a).  But that is far more work than just ignoring
partial errors.  You need to add the state, make the caller deal with
it, make resume deal with it etc.

Personally I believe that alternative

c) revert to resumed state and return error

like these drivers do is a better option.

> because the USB core will ignore the failure return value and force
> to suspend the device.

Yes, I know that. So fix that then if it is an issue. My claim is that
it is not, and that is why the USB core can ignore the failure.  If it
were an issue then the core could not ignore it.  Simple as that.

You cannot keep pushing this down to "Do not allow operation to ever
fail" for every small task involved in a suspend.  It involves
hardware.  It can and will fail.

>> I am going to NAK the cdc_mbim and qmi_wwan pacthes unless you can
>> convince me that we need to add a "partly-suspended" state for the
>> system suspend error case.  In which case the patch will need to include
>> the corresponding resume fix for the "partly-suspended" state.
>>
>
> Yes, you may argue that the device might be in partly-suspended state,
> but it doesn't matter since the device will be put into suspend later and

Where will it do that?  AFAICS, usb_suspend_both() will resume all
already suspended interfaces if a driver fails suspend.  From the
function docs:

 * This is the central routine for suspending USB devices.  It calls the
 * suspend methods for all the interface drivers in @udev and then calls
 * the suspend method for @udev itself.  If an error occurs at any stage,
 * all the interfaces which were suspended are resumed so that they remain
 * in the same state as the device.


You do want the *failing* interface driver/function to have the same
state as the rest of the device functions, i.e. resumed, on return from
suspend.

> the resume callback can recover from the partly-suspend state.

It can.  But it won't magically just do that by ignoring errors in
suspend.

> These patches doesn't break previous failure path of system suspend
> and just avoid to submit new URBs, so how about letting later delta
> patches fix for the corresponding resume?

From Documentation/usb/power-management.txt :

        The suspend method is called to warn the driver that the
        device is going to be suspended.  If the driver returns a
        negative error code, the suspend will be aborted.  Normally
        the driver will return 0, in which case it must cancel all
        outstanding URBs (usb_kill_urb()) and not submit any more.


The driver need only cancel outstanding URBs if it returns 0.  How the
USB core handles this is up to the USB core.  It is not for the
drivers to deal with.  Unless you modify the API to say that suspend
cannot fail in the !PMSG_IS_AUTO(message) case.

But I really think all that ugly PMSG_IS_AUTO(message) testing in the
USB drivers is contradicting the simple design of the USB core, using a
single common suspend for both runtime and system suspend and leaving it
up to the USB core to handle the differences.

AFAICS, it does that just fine.  If you think it does not, then please
fix that in the USB core or redefine the API.  But don't just randomly
make drivers ignore errors.


Bjørn

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

[Oliver Neukum]

On Tuesday 05 March 2013 21:08:09 Ming Lei wrote:
> On Tue, Mar 5, 2013 at 8:50 PM, Oliver Neukum <oneukum@suse.de> wrote:

> > In other words, if we don't handle errors, there must be no errors,
> > otherwise it doesn't matter what we do in the error case. We'd leave
> > the problem to generic layers.
> 
> Generic layers can't handle the driver's specific failure.

We depend on stopping the HC forcing all devices into suspend.
We know this is problematic. For example some disk enclosures
need to flush cache. Fortunately for us this is done in the SCSI
layer.

> If driver records its suspend failure state in suspend(), resume()
> should and can deal with it without much difficulty.

Yes, but why bother? Either we can safely suspend in any state or
we must not ignore errors.

> > Furthermore there is a small chance that although the device tree
> > is walked, teh system suspend fails for another later reason that
> > is not ignored. In that case the drivers need to do error recovery,
> > albeit in resume().
> 
> Yes,  resume() need to handle the USB system suspend failure
> either in normal resume or error recovery, both are basically same.

In theory yes, in practice usually power is cut.

	Regards
		Oliver

Re: [PATCH 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 Tue, Mar 5, 2013 at 3:09 PM, Bjørn Mork <bjorn@mork.no> wrote:
>> 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..da83546 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(msg))
>>>               usbnet_resume(intf);
>>>
>>>  error:
>>
>> NAK. We if the device cannot suspend, then it cannot do suspend halfways
>
> Sorry, what do you mean that the device cannot suspend?


Now, after inspecting wdm_suspend() which hides behind
info->subdriver->suspend(), I see that this is a completely theoretical
discussion as it always will return 0 if PMSG_IS_AUTO(msg) is true...

Which means that your change really is a noop().  I still don't like it,
because it gives the impression that errors from info->subdriver->suspend() 
isn't dealt with.


>  If you mean
> usb_suspend_device(), its failure is still ignored, and generally it is OK
> since the suspend action is driven by upstream port.


I mean that we do two operations here: first we suspend usbnet, then we
suspend wdm:

	ret = usbnet_suspend(intf, message);
	if (ret < 0)
		goto error;
	if (intf == ctx->control && info->subdriver && info->subdriver->suspend)
		ret = info->subdriver->suspend(intf, message);
	if (ret < 0)
		usbnet_resume(intf);
error:
	return ret;


The case you are trying to modify is when the second fails after the
first succeeded. You know suspend has already failed at this point.  It
is the implication of the error.  Your only task is to decide what to do
about that failure.  You seem to think that you can fix it.  You
cannot.  It already has failed.  If you are going to fix that, then you
have to go back to where it failed.

So your next step if going down this route will naturally be looking
into how you can prevent info->subdriver->suspend() from ever failing.
Which will be easy as it won't.  But assuming for this argument that it
can fail, which I guess can be true for some of the serial driver
callback errors etc you also fixed this way.  I am pretty sure that when
you look into those to make sure they never can fail, you will find
another function you have to ensure never can fail.

Forget it.  suspend can and will fail.   Deal with it in the upper
layers.  In fact, the USB core already does.  If you think it doesn't
then that's where you fix it.

>> either. Whether the caller will ignore the error or not, is irrelevant.
>
> Anyway, usbnet_resume() can't be called to submit new URBs in
> the failure path of system suspend, so the patch should be correct.

I fail to see how this is any different from a composite device with
e.g. a usbnet function and a serial function where suspending the serial
function fails after successfully suspending the usbnet function.
usb_suspend_both() will then resume the usbnet function and return the
error.


Bjørn

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

[Ming Lei]

On Tue, Mar 5, 2013 at 9:28 PM, Oliver Neukum <oneukum@suse.de> wrote:
> On Tuesday 05 March 2013 21:08:09 Ming Lei wrote:
>> On Tue, Mar 5, 2013 at 8:50 PM, Oliver Neukum <oneukum@suse.de> wrote:
>
>> > In other words, if we don't handle errors, there must be no errors,
>> > otherwise it doesn't matter what we do in the error case. We'd leave
>> > the problem to generic layers.
>>
>> Generic layers can't handle the driver's specific failure.
>
> We depend on stopping the HC forcing all devices into suspend.
> We know this is problematic. For example some disk enclosures
> need to flush cache. Fortunately for us this is done in the SCSI
> layer.

I mean only the driver can know its specific suspend failure
in its suspend callback, so it is reasonable to deal with it in its
resume() callback, suppose the policy to ignore suspend failure
isn't changed.

>
>> If driver records its suspend failure state in suspend(), resume()
>> should and can deal with it without much difficulty.
>
> Yes, but why bother? Either we can safely suspend in any state or
> we must not ignore errors.

In fact, for many drivers, its suspend() in system sleep just return 0,
for example, cdc_wcm/cdc_acm/hub, etc, so these drivers simply
ignore any failures from suspend() during system sleep.

For other drivers, if suspend() don't return success in system sleep,
it shouldn't try to recover the device, and its resume has to handle
the failure. Or do you have better approach? Suppose we keep
ignoring the suspend failure.

At least, I think URBs can't be submitted to device in its system
suspend failure path, which is generally one part of suspend
recovery for many usb drivers. Doing such recovery may confuse
resume() very much, and it is really wrong.

That is the inconsistency of handling system suspend between
usb drivers and usb core.

>
>> > Furthermore there is a small chance that although the device tree
>> > is walked, teh system suspend fails for another later reason that
>> > is not ignored. In that case the drivers need to do error recovery,
>> > albeit in resume().
>>
>> Yes,  resume() need to handle the USB system suspend failure
>> either in normal resume or error recovery, both are basically same.
>
> In theory yes, in practice usually power is cut.

reset_resume() or rebind() can handle the power cut case, for example,
the suspend failure state can be ignored in reset_resume(), or rebind()
just forget previous suspend failure.  Correct me if it is wrong.

Thanks,
--
Ming Lei

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

[Ming Lei]

On Tue, Mar 5, 2013 at 9:46 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>
>> On Tue, Mar 5, 2013 at 3:09 PM, Bjørn Mork <bjorn@mork.no> wrote:
>>> 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..da83546 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(msg))
>>>>               usbnet_resume(intf);
>>>>
>>>>  error:
>>>
>>> NAK. We if the device cannot suspend, then it cannot do suspend halfways
>>
>> Sorry, what do you mean that the device cannot suspend?
>
>
> Now, after inspecting wdm_suspend() which hides behind
> info->subdriver->suspend(), I see that this is a completely theoretical
> discussion as it always will return 0 if PMSG_IS_AUTO(msg) is true...
>
> Which means that your change really is a noop().  I still don't like it,
> because it gives the impression that errors from info->subdriver->suspend()
> isn't dealt with.

Yes, it needn't dealt with in system sleep, so it has the document benefit.

>
>>  If you mean
>> usb_suspend_device(), its failure is still ignored, and generally it is OK
>> since the suspend action is driven by upstream port.
>
>
> I mean that we do two operations here: first we suspend usbnet, then we
> suspend wdm:
>
>         ret = usbnet_suspend(intf, message);
>         if (ret < 0)
>                 goto error;
>         if (intf == ctx->control && info->subdriver && info->subdriver->suspend)
>                 ret = info->subdriver->suspend(intf, message);
>         if (ret < 0)
>                 usbnet_resume(intf);
> error:
>         return ret;
>
>
> The case you are trying to modify is when the second fails after the
> first succeeded. You know suspend has already failed at this point.  It
> is the implication of the error.  Your only task is to decide what to do
> about that failure.  You seem to think that you can fix it.  You
> cannot.  It already has failed.  If you are going to fix that, then you
> have to go back to where it failed.
>
> So your next step if going down this route will naturally be looking
> into how you can prevent info->subdriver->suspend() from ever failing.
> Which will be easy as it won't.  But assuming for this argument that it
> can fail, which I guess can be true for some of the serial driver
> callback errors etc you also fixed this way.  I am pretty sure that when
> you look into those to make sure they never can fail, you will find
> another function you have to ensure never can fail.
>
> Forget it.  suspend can and will fail.   Deal with it in the upper
> layers.  In fact, the USB core already does.  If you think it doesn't
> then that's where you fix it.

No, USB core does not handle it, and will always ignore the return
failure from driver's suspend, see usb_suspend_both() for non-autosuspend
case.

>
>>> either. Whether the caller will ignore the error or not, is irrelevant.
>>
>> Anyway, usbnet_resume() can't be called to submit new URBs in
>> the failure path of system suspend, so the patch should be correct.
>
> I fail to see how this is any different from a composite device with
> e.g. a usbnet function and a serial function where suspending the serial
> function fails after successfully suspending the usbnet function.
> usb_suspend_both() will then resume the usbnet function and return the
> error.

No, usb_suspend_both() always ignores the suspend failure from drivers
and does not resume a non-root-hub device during system sleep.

Thanks,
--
Ming Lei
--
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 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 Tue, Mar 5, 2013 at 9:46 PM, Bjørn Mork <bjorn@mork.no> wrote:
>
>> Now, after inspecting wdm_suspend() which hides behind
>> info->subdriver->suspend(), I see that this is a completely theoretical
>> discussion as it always will return 0 if PMSG_IS_AUTO(msg) is true...
>>
>> Which means that your change really is a noop().  I still don't like it,
>> because it gives the impression that errors from info->subdriver->suspend()
>> isn't dealt with.
>
> Yes, it needn't dealt with in system sleep, so it has the document benefit.

The document that the return code is ignored by forcing it to 0 and
always return success.

But am still not convinced this is correct in any way.  AFAICS there is
no documentation stating that the drivers should care about what the USB
core use the return value for. Drivers are allowed to fail and the core
will ignore and clean up if necessary.

I still find any partial failures horrendous.  If we are not going to
fail on errors, then we'll return success and pretend to be suspended.

> No, USB core does not handle it, and will always ignore the return
> failure from driver's suspend, see usb_suspend_both() for non-autosuspend
> case.

Right you are.  Somehow I read the inverse.  Sorry about that.

In any case, it ends up here and everythin will be OK, which I assume is
why it can ignore the errors:

        /* If the suspend succeeded then prevent any more URB submissions
         * and flush any outstanding URBs.
         */
        } else {
                udev->can_submit = 0;
                for (i = 0; i < 16; ++i) {
                        usb_hcd_flush_endpoint(udev, udev->ep_out[i]);
                        usb_hcd_flush_endpoint(udev, udev->ep_in[i]);
                }
        }


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 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep

[Ming Lei]

On Tue, Mar 5, 2013 at 11:03 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>> On Tue, Mar 5, 2013 at 9:46 PM, Bjørn Mork <bjorn@mork.no> wrote:
>>
>>> Now, after inspecting wdm_suspend() which hides behind
>>> info->subdriver->suspend(), I see that this is a completely theoretical
>>> discussion as it always will return 0 if PMSG_IS_AUTO(msg) is true...
>>>
>>> Which means that your change really is a noop().  I still don't like it,
>>> because it gives the impression that errors from info->subdriver->suspend()
>>> isn't dealt with.
>>
>> Yes, it needn't dealt with in system sleep, so it has the document benefit.
>
> The document that the return code is ignored by forcing it to 0 and
> always return success.
>
> But am still not convinced this is correct in any way.  AFAICS there is
> no documentation stating that the drivers should care about what the USB

We have source code, :-)

> core use the return value for. Drivers are allowed to fail and the core
> will ignore and clean up if necessary.
>
> I still find any partial failures horrendous.  If we are not going to
> fail on errors, then we'll return success and pretend to be suspended.
>
>> No, USB core does not handle it, and will always ignore the return
>> failure from driver's suspend, see usb_suspend_both() for non-autosuspend
>> case.
>
> Right you are.  Somehow I read the inverse.  Sorry about that.
>
> In any case, it ends up here and everythin will be OK, which I assume is
> why it can ignore the errors:
>
>         /* If the suspend succeeded then prevent any more URB submissions
>          * and flush any outstanding URBs.
>          */
>         } else {
>                 udev->can_submit = 0;
>                 for (i = 0; i < 16; ++i) {
>                         usb_hcd_flush_endpoint(udev, udev->ep_out[i]);
>                         usb_hcd_flush_endpoint(udev, udev->ep_in[i]);
>                 }
>         }

Yes, USB core will flush any outstanding URBs, but the driver still need
to deal with suspend failure carefully, for example, suppose usb_resume()
is called in suspend failure path, and the submitted URBs are killed
by USB core later. But after the device is wakeup, and the resume() will
do nothing since the suspend count is leaked. So it is what the patches
are fixing, and it is better to not depend on the default flushing URBs of
USB core.

Thanks,
--
Ming Lei

Re: [PATCH 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:

> Yes, USB core will flush any outstanding URBs, but the driver still need
> to deal with suspend failure carefully, for example, suppose usb_resume()
> is called in suspend failure path, and the submitted URBs are killed
> by USB core later. But after the device is wakeup, and the resume() will
> do nothing since the suspend count is leaked. So it is what the patches
> are fixing, and it is better to not depend on the default flushing URBs of
> USB core.

I am starting to wonder why the USB core has combined system suspend and
runtime suspend if we are going to end up with every driver testing
PMSG_IS_AUTO(message) and selecting a completely different code path.

You are right that we will end up with problems if usbnet_resume is
called for a device usbnet hasn't suspended.  But I'd still claim that
is a bug in the USB core, which is the one that decided to ignore the
suspend error and still call resume.

I guess proper error handling here require the USB core to see the
interface driver as dead if it fails to suspend on system suspend, and
do forced rebinding on resume.

I am not going to fight this any longer.  The per-driver
PMSG_IS_AUTO(message) testing is an ugly workround for a core problem,
but they are already all over the place...  Still, please make sure the
drivers all return 0 if they are pretending to suspend. No error code
return if the driver ignores the error.


Bjørn

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

[Alan Stern]

On Tue, 5 Mar 2013, Bjørn Mork wrote:

> Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> writes:
> 
> > Yes, USB core will flush any outstanding URBs, but the driver still need
> > to deal with suspend failure carefully, for example, suppose usb_resume()
> > is called in suspend failure path, and the submitted URBs are killed
> > by USB core later. But after the device is wakeup, and the resume() will
> > do nothing since the suspend count is leaked. So it is what the patches
> > are fixing, and it is better to not depend on the default flushing URBs of
> > USB core.
> 
> I am starting to wonder why the USB core has combined system suspend and
> runtime suspend if we are going to end up with every driver testing
> PMSG_IS_AUTO(message) and selecting a completely different code path.

Mainly for historical reasons.  System suspend existed long before 
runtime suspend did.  When runtime suspend was added, it piggybacked 
off the existing code.  Furthermore, originally there was no 
requirement that system suspend always succeed; that was added later.

Also, the code paths are not completely different.  They differ mainly 
in their error handling.  But when you think about it, how serious an 
error can you encounter when you try to _stop_ using a device?

> You are right that we will end up with problems if usbnet_resume is
> called for a device usbnet hasn't suspended.  But I'd still claim that
> is a bug in the USB core, which is the one that decided to ignore the
> suspend error and still call resume.
> 
> I guess proper error handling here require the USB core to see the
> interface driver as dead if it fails to suspend on system suspend, and
> do forced rebinding on resume.

You are welcome to submit a patch to do this.  It shouldn't be hard; we
already have a flag indicating that an interface needs to be unbound at
reprobed at resume time.  You can update the kerneldoc in addition; as
you noticed, it currently does not describe the actual code completely
accurately.

Alan Stern

--
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 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep

[Bjørn Mork]

Alan Stern <stern@rowland.harvard.edu> writes:
> On Tue, 5 Mar 2013, Bjørn Mork wrote:
>> Ming Lei <ming.lei@canonical.com> writes:
>> 
>> > Yes, USB core will flush any outstanding URBs, but the driver still need
>> > to deal with suspend failure carefully, for example, suppose usb_resume()
>> > is called in suspend failure path, and the submitted URBs are killed
>> > by USB core later. But after the device is wakeup, and the resume() will
>> > do nothing since the suspend count is leaked. So it is what the patches
>> > are fixing, and it is better to not depend on the default flushing URBs of
>> > USB core.
>> 
>> I am starting to wonder why the USB core has combined system suspend and
>> runtime suspend if we are going to end up with every driver testing
>> PMSG_IS_AUTO(message) and selecting a completely different code path.
>
> Mainly for historical reasons.  System suspend existed long before 
> runtime suspend did.  When runtime suspend was added, it piggybacked 
> off the existing code.  Furthermore, originally there was no 
> requirement that system suspend always succeed; that was added later.
>
> Also, the code paths are not completely different.  They differ mainly 
> in their error handling.  But when you think about it, how serious an 
> error can you encounter when you try to _stop_ using a device?

Thanks for explaining.

>> You are right that we will end up with problems if usbnet_resume is
>> called for a device usbnet hasn't suspended.  But I'd still claim that
>> is a bug in the USB core, which is the one that decided to ignore the
>> suspend error and still call resume.
>> 
>> I guess proper error handling here require the USB core to see the
>> interface driver as dead if it fails to suspend on system suspend, and
>> do forced rebinding on resume.
>
> You are welcome to submit a patch to do this.  It shouldn't be hard; we
> already have a flag indicating that an interface needs to be unbound at
> reprobed at resume time.  You can update the kerneldoc in addition; as
> you noticed, it currently does not describe the actual code completely
> accurately.

I guess I saw that coming :)  Will take a shot at it when time permits.


Bjørn

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

[Ming Lei]

On Wed, Mar 6, 2013 at 12:08 AM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>
> I am starting to wonder why the USB core has combined system suspend and
> runtime suspend if we are going to end up with every driver testing
> PMSG_IS_AUTO(message) and selecting a completely different code path.
>
> You are right that we will end up with problems if usbnet_resume is
> called for a device usbnet hasn't suspended.  But I'd still claim that
> is a bug in the USB core, which is the one that decided to ignore the
> suspend error and still call resume.
>
> I guess proper error handling here require the USB core to see the
> interface driver as dead if it fails to suspend on system suspend, and
> do forced rebinding on resume.

The idea should be fine, but may cause regression of user space, suppose
one device with suspend failure can be across suspend-resume cycle and
work well before, but it is no longer with your forced rebinding.

>
> I am not going to fight this any longer.  The per-driver
> PMSG_IS_AUTO(message) testing is an ugly workround for a core problem,
> but they are already all over the place...  Still, please make sure the
> drivers all return 0 if they are pretending to suspend. No error code
> return if the driver ignores the error.

I agree, resume() of driver has to handle the suspend failure if there is
the failure returned from suspend().

Thanks,
--
Ming Lei
--
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 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep

[Ming Lei]

On Wed, Mar 6, 2013 at 10:51 AM, Ming Lei <ming.lei@canonical.com> wrote:
> On Wed, Mar 6, 2013 at 12:08 AM, Bjørn Mork <bjorn@mork.no> wrote:
>> Ming Lei <ming.lei@canonical.com> writes:
>>
>> I am starting to wonder why the USB core has combined system suspend and
>> runtime suspend if we are going to end up with every driver testing
>> PMSG_IS_AUTO(message) and selecting a completely different code path.
>>
>> You are right that we will end up with problems if usbnet_resume is
>> called for a device usbnet hasn't suspended.  But I'd still claim that
>> is a bug in the USB core, which is the one that decided to ignore the
>> suspend error and still call resume.
>>
>> I guess proper error handling here require the USB core to see the
>> interface driver as dead if it fails to suspend on system suspend, and
>> do forced rebinding on resume.
>
> The idea should be fine, but may cause regression of user space, suppose
> one device with suspend failure can be across suspend-resume cycle and
> work well before, but it is no longer with your forced rebinding.

Give the potential cost(user space regression) of doing rebind, I think it
is better to try to recover the device in resume() first, then
consider rebinding
as the last straw.  In fact, I am also wondering if resume() can't recover one
device but probe() can, maybe we can always let resume() recover the
device which experienced suspend failure.

I remember that some guys went against rebinding during system sleep before
in the firmware loading discussion.

Thanks,
--
Ming Lei

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

[Bjørn Mork]

Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> writes:
> On Wed, Mar 6, 2013 at 10:51 AM, Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
>> On Wed, Mar 6, 2013 at 12:08 AM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
>>
>>> I guess proper error handling here require the USB core to see the
>>> interface driver as dead if it fails to suspend on system suspend, and
>>> do forced rebinding on resume.
>>
>> The idea should be fine, but may cause regression of user space, suppose
>> one device with suspend failure can be across suspend-resume cycle and
>> work well before, but it is no longer with your forced rebinding.
>
> Give the potential cost(user space regression) of doing rebind, I think it
> is better to try to recover the device in resume() first, then
> consider rebinding
> as the last straw.  In fact, I am also wondering if resume() can't recover one
> device but probe() can, maybe we can always let resume() recover the
> device which experienced suspend failure.

Yes, sure. Drivers wanting to do this, anticipating typical errors, can
still return 0 from suspend and do whatever they want in resume.

My proposal is to make the USB core properly deal with drivers returning
an error from suspend, and also document the fact that a driver should
always return 0 on system suspend unless it really want forced unbinding
on suspend errors.

> I remember that some guys went against rebinding during system sleep before
> in the firmware loading discussion.

Yes, that is probably relevant. I'll look it up.  Thanks for the
pointer.


Bjørn
--
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 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep

[Ming Lei]

On Wed, Mar 6, 2013 at 3:06 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>> On Wed, Mar 6, 2013 at 10:51 AM, Ming Lei <ming.lei@canonical.com> wrote:
>>> On Wed, Mar 6, 2013 at 12:08 AM, Bjørn Mork <bjorn@mork.no> wrote:
>>>
>>>> I guess proper error handling here require the USB core to see the
>>>> interface driver as dead if it fails to suspend on system suspend, and
>>>> do forced rebinding on resume.
>>>
>>> The idea should be fine, but may cause regression of user space, suppose
>>> one device with suspend failure can be across suspend-resume cycle and
>>> work well before, but it is no longer with your forced rebinding.
>>
>> Give the potential cost(user space regression) of doing rebind, I think it
>> is better to try to recover the device in resume() first, then
>> consider rebinding
>> as the last straw.  In fact, I am also wondering if resume() can't recover one
>> device but probe() can, maybe we can always let resume() recover the
>> device which experienced suspend failure.
>
> Yes, sure. Drivers wanting to do this, anticipating typical errors, can
> still return 0 from suspend and do whatever they want in resume.
>
> My proposal is to make the USB core properly deal with drivers returning
> an error from suspend, and also document the fact that a driver should
> always return 0 on system suspend unless it really want forced unbinding
> on suspend errors.
>
>> I remember that some guys went against rebinding during system sleep before
>> in the firmware loading discussion.
>
> Yes, that is probably relevant. I'll look it up.  Thanks for the
> pointer.

You are welcome!

Here is one link I found:

       http://marc.info/?l=linux-usb&m=132557022902261&w=2

IMO, we unbind interface which hasn't suspend/resume callback
during suspend because there is no better way to handle the case.
But for the suspend failure case, maybe rebind isn't necessary, and
we can document that drivers have to handle their system suspend
failure in resume(), where it is very suitable to do PM recovery.

Also we may store the failure code into usb_interface, and let
USB core check if the suspend failure has been handled/cleared
after resume().

Thanks,
--
Ming Lei
--
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 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:

> Here is one link I found:
>
>        http://marc.info/?l=linux-usb&m=132557022902261&w=2

Thanks.  This section caught my eye:

    "The USB suspend/resume code is full of those crazy "let's use one
     function, and pass it as an *argument* what to do". It's a disease.
     The PM layer used to do the same thing (PMSG_xyz crap), and we've
     largely gotten rid of it, but USB still plays around with those things
     and makes it even *worse* exactly with these kinds of
     "do_unbind_rebind()" routines that then look at the argument instead
     of having a sane routine for unbinding and another sane routine for
     re-binding."


I take that as supporting my view on all the "if (PMSG_IS_AUTO(msg)"
testing...

But I don't have any ideas on how to fix it now that you all have spoon
fed me the background.

> IMO, we unbind interface which hasn't suspend/resume callback
> during suspend because there is no better way to handle the case.
> But for the suspend failure case, maybe rebind isn't necessary, and
> we can document that drivers have to handle their system suspend
> failure in resume(), where it is very suitable to do PM recovery.

Yup, agreed, although I fear that if Alan's commit messages are confused
then I unable explain anything like this ;)

> Also we may store the failure code into usb_interface, and let
> USB core check if the suspend failure has been handled/cleared
> after resume().

That sounds unnecessarily complicated.  Let the driver deal with
it, keeping the API as simple as possible.


Bjørn