On Tue, Mar 5, 2013 at 9:46 PM, Bjørn Mork <bjorn@xxxxxxx> wrote: > Ming Lei <ming.lei@xxxxxxxxxxxxx> writes: > >> On Tue, Mar 5, 2013 at 3:09 PM, Bjørn Mork <bjorn@xxxxxxx> wrote: >>> Ming Lei <ming.lei@xxxxxxxxxxxxx> 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@xxxxxxx> >>>> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxxxxx> >>>> --- >>>> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
