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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 5 Mar 2013 22:50:43 +0800
From:	Ming Lei <ming.lei@...onical.com>
To:	Bjørn Mork <bjorn@...k.no>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Kosina <jkosina@...e.cz>,
	Alan Stern <stern@...land.harvard.edu>,
	Oliver Neukum <oneukum@...e.de>, netdev@...r.kernel.org,
	linux-usb@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [PATCH 4/7] usbnet: cdc_mbim: don't recover device if suspend
 fails in system sleep

On Tue, Mar 5, 2013 at 9:46 PM, Bjørn Mork <bjorn@...k.no> wrote:
> Ming Lei <ming.lei@...onical.com> writes:
>
>> On Tue, Mar 5, 2013 at 3:09 PM, Bjørn Mork <bjorn@...k.no> wrote:
>>> Ming Lei <ming.lei@...onical.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@...k.no>
>>>> Signed-off-by: Ming Lei <ming.lei@...onical.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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html