netdev - Re: [PATCHv4] usbnet: Resubmit interrupt URB once if halted

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]

Message-ID: <BANLkTik7Da6K4Fn7=dpo8RwwctCp8kV_iw@mail.gmail.com>
Date:	Thu, 21 Apr 2011 07:58:06 -0700
From:	Paul Stewart <pstew@...omium.org>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	davem@...emloft.net, greg@...ah.com
Subject: Re: [PATCHv4] usbnet: Resubmit interrupt URB once if halted

On Thu, Apr 21, 2011 at 7:03 AM, Alan Stern <stern@...land.harvard.edu> wrote:
> On Tue, 19 Apr 2011, Paul Stewart wrote:
>
>> Set a flag if the interrupt URB completes with ENOENT as this
>> occurs legitimately during system suspend.  When the
>> usbnet_resume is called, test this flag and try once to resubmit
>> the interrupt URB.
>
> I still don't think this is the best way to go.
>
>> This version of the patch moves the urb submit directly into
>> usbnet_resume.  Is it okay to submit a GFP_KERNEL urb from
>> usbnet_resume()?
>
> Yes, it is.
>
>> Signed-off-by: Paul Stewart <pstew@...omium.org>
>> ---
>>  drivers/net/usb/usbnet.c   |   13 ++++++++++++-
>>  include/linux/usb/usbnet.h |    1 +
>>  2 files changed, 13 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index 02d25c7..3651a48 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -482,6 +482,7 @@ static void intr_complete (struct urb *urb)
>>       case -ESHUTDOWN:        /* hardware gone */
>>               if (netif_msg_ifdown (dev))
>>                       devdbg (dev, "intr shutdown, code %d", status);
>> +             set_bit(EVENT_INTR_HALT, &dev->flags);
>
> Is this new flag really needed?
>
>>               return;
>>
>>       /* NOTE:  not throttling like RX/TX, since this endpoint
>> @@ -1294,9 +1295,19 @@ int usbnet_resume (struct usb_interface *intf)
>>  {
>>       struct usbnet           *dev = usb_get_intfdata(intf);
>>
>> -     if (!--dev->suspend_count)
>> +     if (!--dev->suspend_count) {
>>               tasklet_schedule (&dev->bh);
>>
>> +             /* resubmit interrupt URB if it was halted by suspend */
>> +             if (dev->interrupt && netif_running(dev->net) &&
>> +                 netif_device_present(dev->net) &&
>> +                 test_bit(EVENT_INTR_HALT, &dev->flags)) {
>
> Why do you need the test_bit()?  If the other conditions are all true,
> don't you want to resubmit the interrupt URB regardless?

I'm trying to handle two separate issues, one of which I can't say I
fully understand.  The first, which is the most straightforward, is
for systems in which USB devices remained powered across
suspend-resume.  For this case for sure, we don't need a flag.  The
interrupt URBs are halted (either done so by the HCD as I've observed,
or the drive can choose to kill them in usbnet_suspend()).  On system
resume, we're guaranteed URBs have stopped, and we can just submit
one.

In a second scenario, for other systems USB devices go unpowered
during suspend.  At resume time, there's a quick succession where the
device appears to detach and reattach and enumerate.  This is where
things get strange.  It appears that since the enumeration happens in
the course of system resume, when usbnet_open() gets called, and
usb_autopm_get_interface(), there's a call path that leads to
usbnet_resume(). If there's no flag, then we submit the interrupt urb
from usbnet_resume(), so the submit_urb() in usbnet_open() fails in an
error.  This makes actions like "ifconfig eth0 up" fail on the
interface after resume from suspend.

>
>> +                     clear_bit(EVENT_INTR_HALT, &dev->flags);
>> +                     usb_submit_urb(dev->interrupt, GFP_KERNEL);
>> +             }
>> +     }
>> +}
>> +
>>       return 0;
>>  }
>
> Alan Stern
>
>
--
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