[RFC] Keep track of interrupt URB status and resubmit in bh if necessary

[RFC] Keep track of interrupt URB status and resubmit in bh if necessary

[Paul Stewart]

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@users.sourceforge.org>
Cc: Indrek Peri <Indrek.Peri@Ericsson.com>
Signed-off-by: Paul Stewart <pstew@google.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;
-- 
1.7.3.1

Re: [RFC] Keep track of interrupt URB status and resubmit in bh if necessary

[Indrek Peri]

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.

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@users.sourceforge.org>
> Cc: Indrek Peri <Indrek.Peri@Ericsson.com>
> Signed-off-by: Paul Stewart <pstew@google.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;

Re: [RFC] Keep track of interrupt URB status and resubmit in bh if necessary

[Paul Stewart]

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@ericsson.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@users.sourceforge.org>
>> Cc: Indrek Peri <Indrek.Peri@Ericsson.com>
>> Signed-off-by: Paul Stewart <pstew@google.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;
>
>