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