| 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
| ||
|
Date: Fri, 4 Mar 2011 10:01:38 -0800
From: Paul Stewart <pstew@...omium.org>
To: Indrek Peri <Indrek.Peri@...csson.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
David Brownell <david-b@...bell.net>
Subject: Re: [RFC] Keep track of interrupt URB status and resubmit in bh if necessary
Thanks for helping clear this up a bit. I'm still a little foggy on
the details, especially when it comes to selective suspend (although I
currently don't work with a system that is actively using it, so I
might not see these issues).
On Fri, Mar 4, 2011 at 3:01 AM, Indrek Peri <Indrek.Peri@...csson.com> wrote:
>
> To my understanding usbnet has design questions, like EVENT_DEV_ASLEEP,
> device states and events. In struct "flags" member holds an event to
> execute deferred work. Actually, in usbnet_bh, EVENT_DEV_ASLEEP is
> really needed in if-statement. Without that, in selective suspend
> case driver crashes.
>
> Another problem was that "suspend" removed interrupt URB. I tought in
> the same way as Paul, I added insertion of interrupt URB. Paul added
> it in usbnet_bh and I in "resume". I do not see a problem to add
> interrupt URB in "resume". My patch had a typo, in "resume" we
> should use GFP_ATOMIC in usb_submit_urb. Now is the question is
> this good design? My interpretation of David is that driver needs
> "resume driver" transition where RX and INT URBs are activated.
My interpretation of his statement "shouldn't this be just another
part of the "resume driver" action, like refilling the RX urb queue?"
was implicitly asking "shouldn't you move this to usbnet_bh, which is
where the RX urb queue is refilled?" This is why I moved things
there. Hopefully David can shed some light on it (now that I think
I've typed in his email address correctly. :-)
Now, I am also seeing what appear to be RX stalls with some
probability after suspend/resume, so it seems even with the current bh
there is still work to be done...
> I guess that usbnet needs a redesign and rewriting.
> usbnet is used by many USB-Ethernet devices that actually do not
> use selective suspend.
>
> BR, Indrek
>
>
>
> On 03/03/2011 06:45 AM, Paul Stewart wrote:
>> It appears that a patch from Indrek Peri similar to the one below
>> without resolution. I'm new to this problem (suspend-resume causing
>> interrupt URBs to stop delivering) and am curious about what the correct
>> solution would should like. Before becoming aware of this thread, I
>> just added a "usb_submit_urb" of "dev->interrupt" into "usbnet_resume()"
>> and that worked to solve the issue I was having. Apparently this isn't
>> the correct solution though, from David's response to Indrek. So, I'm
>> curious about what the right code should be.
>>
>> I'll note is that submitting the interrupt URB seems fairly benign. If
>> we are in a situation where we should not have sent an URB (e.g, the
>> netif wasn't running) intr_complete correctly handles this case and does
>> not re-submit the URB, so at most we get one "rogue" interrupt URB after
>> resume-from-suspend. The only nasty thing is that this URB should
>> probably not be submitted from interrupt, which the resume function
>> almost certainly is. I'm guessing this is part of why David NAKed
>> Indrek's patch. Am I correct?
>>
>> Does something like the patch below seem like a resonable solution?
>>
>> Cc: David Brownell <dbrownell@...rs.sourceforge.org>
>> Cc: Indrek Peri <Indrek.Peri@...csson.com>
>> Signed-off-by: Paul Stewart <pstew@...gle.com>
>> ---
>> drivers/net/usb/usbnet.c | 21 +++++++++++++++++++--
>> include/linux/usb/usbnet.h | 1 +
>> 2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index 02d25c7..bc6a8e0 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -471,6 +471,8 @@ static void intr_complete (struct urb *urb)
>> struct usbnet *dev = urb->context;
>> int status = urb->status;
>>
>> + dev->interrupt_urb_running = 0;
>> +
>> switch (status) {
>> /* success */
>> case 0:
>> @@ -497,7 +499,9 @@ static void intr_complete (struct urb *urb)
>>
>> memset(urb->transfer_buffer, 0, urb->transfer_buffer_length);
>> status = usb_submit_urb (urb, GFP_ATOMIC);
>> - if (status != 0 && netif_msg_timer (dev))
>> + if (status == 0)
>> + dev->interrupt_urb_running = 1;
>> + else if (netif_msg_timer (dev))
>> deverr(dev, "intr resubmit --> %d", status);
>> }
>>
>> @@ -580,6 +584,7 @@ static int usbnet_stop (struct net_device *net)
>> remove_wait_queue (&unlink_wakeup, &wait);
>>
>> usb_kill_urb(dev->interrupt);
>> + dev->interrupt_urb_running = 0;
>>
>> /* deferred work (task, timer, softirq) must also stop.
>> * can't flush_scheduled_work() until we drop rtnl (later),
>> @@ -640,7 +645,8 @@ static int usbnet_open (struct net_device *net)
>> if (netif_msg_ifup (dev))
>> deverr (dev, "intr submit %d", retval);
>> goto done;
>> - }
>> + } else
>> + dev->interrupt_urb_running = 1;
>> }
>>
>> netif_start_queue (net);
>> @@ -1065,6 +1071,17 @@ static void usbnet_bh (unsigned long param)
>> if (dev->txq.qlen < TX_QLEN (dev))
>> netif_wake_queue (dev->net);
>> }
>> +
>> + // Do we need to re-enable interrupt URBs?
>> + if (netif_running (dev->net) &&
>> + netif_device_present (dev->net) &&
>> + dev->interrupt_urb_running == 0) {
>> + usb_submit_urb (dev->interrupt, GFP_KERNEL);
>> +
>> + /* Unconditionally mark as running so we don't retry */
>> + dev->interrupt_urb_running = 1;
>> + }
>> +
>> }
>>
>>
>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
>> index ba09fe8..1b8ed8a 100644
>> --- a/include/linux/usb/usbnet.h
>> +++ b/include/linux/usb/usbnet.h
>> @@ -49,6 +49,7 @@ struct usbnet {
>> u32 hard_mtu; /* count any extra framing */
>> size_t rx_urb_size; /* size for rx urbs */
>> struct mii_if_info mii;
>> + int interrupt_urb_running;
>>
>> /* various kinds of pending driver work */
>> struct sk_buff_head rxq;
>
>
--
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
Powered by blists - more mailing lists