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

["Bjørn Mork" <bjorn@mork.no>]

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