| Message ID | 1362456103-24956-5-git-send-email-ming.lei@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 either. Whether the caller will ignore the error or not, is irrelevant. 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
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
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 -- 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
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
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
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 -- 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
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 -- 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
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? > 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-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- 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
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
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 -- 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
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. 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
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
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 -- 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
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:
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(-)