[PATCH] usbnet: Resubmit interrupt URB more often

[PATCH] usbnet: Resubmit interrupt URB more often

[Paul Stewart]

I previously sent a patch to resubmit the interrupt URB when
coming out of suspend.  I haven't seen much activity on the
list about it, and thought I'd send a slight variant of this
change.  This one unconditionally resubmits the interrupt urb
in usbnet_bh.  The consequences for resubmitting the URB often
are not large.  In most HCI cases this just means usb_submit_urb
returns immediately and leaves the previous request outstanding.

Doing things this way allows us to avoid keeping track of the
URB transmit status, which may change silently over suspend-
resume transitions and is not tracked in any way currently by
usbnet.

I've designed this change on two types of systems: the first
class of system leaves USB devices powered during suspend.
This might silently cause the interrupt URB to disappear (or at
least not be resubmitted in intr_complete).  Resubmission after
system resume will prevent this from causing problems.

The second class of device are those which shut down the device
during suspend.  During a suspend-resume cycle, the device is
re-enumerated at system resume, and for whatever reason
usbnet_resume may be called on the device during the call-tree
from usbnet_open-> usb_autopm_get_interface, which may cause a
race where the first change above may cause the bh to submit the
interrupt urb before usbnet_open() does.  As a result, I've added
an EALREADY check and a fix to urb.c to send one.

Signed-off-by: Paul Stewart <pstew@chromium.org>
Cc: David S. Miller <davem@davemloft.net>

---
 drivers/net/usb/usbnet.c |   12 +++++++++++-
 drivers/usb/core/urb.c   |    5 +++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 02d25c7..3b3c169 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -636,7 +636,10 @@ static int usbnet_open (struct net_device *net)
 	/* start any status interrupt transfer */
 	if (dev->interrupt) {
 		retval = usb_submit_urb (dev->interrupt, GFP_KERNEL);
-		if (retval < 0) {
+		if (retval == -EALREADY) {
+			// It is not an error if interrupt urb is alredy active
+			retval = 0;
+		} else if (retval < 0) {
 			if (netif_msg_ifup (dev))
 				deverr (dev, "intr submit %d", retval);
 			goto done;
@@ -1065,6 +1068,10 @@ static void usbnet_bh (unsigned long param)
 		if (dev->txq.qlen < TX_QLEN (dev))
 			netif_wake_queue (dev->net);
 	}
+
+	// Re-submit interrupt urb (doesn't hurt to retry)
+	if (netif_running (dev->net))
+		usb_submit_urb (dev->interrupt, GFP_KERNEL);
 }
 
 
@@ -1285,6 +1292,9 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
 		 * wake the device
 		 */
 		netif_device_attach (dev->net);
+		// Stop interrupt urbs while in suspend
+		if (dev->interrupt)
+			usb_kill_urb(dev->interrupt);
 	}
 	return 0;
 }
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 4342bd9..e4dbb29 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -295,7 +295,9 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 	struct usb_host_endpoint	*ep;
 	int				is_out;
 
-	if (!urb || urb->hcpriv || !urb->complete)
+	if (urb->hcpriv)
+		return -EALREADY;
+	if (!urb || !urb->complete)
 		return -EINVAL;
 	dev = urb->dev;
 	if ((!dev) || (dev->state < USB_STATE_DEFAULT))
@@ -807,4 +809,3 @@ int usb_anchor_empty(struct usb_anchor *anchor)
 }
 
 EXPORT_SYMBOL_GPL(usb_anchor_empty);
-
-- 
1.7.3.1

Re: [PATCH] usbnet: Resubmit interrupt URB more often

[Ben Hutchings]

On Tue, 2011-04-19 at 09:35 -0700, Paul Stewart wrote:
[...]
> index 4342bd9..e4dbb29 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -295,7 +295,9 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>  	struct usb_host_endpoint	*ep;
>  	int				is_out;
>  
> -	if (!urb || urb->hcpriv || !urb->complete)
> +	if (urb->hcpriv)
> +		return -EALREADY;
> +	if (!urb || !urb->complete)
>  		return -EINVAL;
[...]

The test for !urb must come before the test on urb->hcpriv.

Also, the kernel coding style does not allow '//' comments.  Please use
scripts/checkpatch.pl to check for style and other common errors.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCHv2] usbnet: Resubmit interrupt URB more often

[Paul Stewart]

I previously sent a patch to resubmit the interrupt URB when
coming out of suspend.  I haven't seen much activity on the
list about it, and thought I'd send a slight variant of this
change.  This one unconditionally resubmits the interrupt urb
in usbnet_bh.  The consequences for resubmitting the URB often
are not large.  In most HCI cases this just means usb_submit_urb
returns immediately and leaves the previous request outstanding.

Doing things this way allows us to avoid keeping track of the
URB transmit status, which may change silently over suspend-
resume transitions and is not tracked in any way currently by
usbnet.

I've designed this change on two types of systems: the first
class of system leaves USB devices powered during suspend.
This might silently cause the interrupt URB to disappear (or at
least not be resubmitted in intr_complete).  Resubmission after
system resume will prevent this from causing problems.

The second class of device are those which shut down the device
during suspend.  During a suspend-resume cycle, the device is
re-enumerated at system resume, and for whatever reason
usbnet_resume may be called on the device during the call-tree
from usbnet_open-> usb_autopm_get_interface, which may cause a
race where the first change above may cause the bh to submit the
interrupt urb before usbnet_open() does.  As a result, I've added
an EALREADY check and a fix to urb.c to send one.

Signed-off-by: Paul Stewart <pstew@chromium.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/usb/usbnet.c |   14 +++++++++++++-
 drivers/usb/core/urb.c   |    5 +++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 02d25c7..1718898 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -636,7 +636,12 @@ static int usbnet_open (struct net_device *net)
 	/* start any status interrupt transfer */
 	if (dev->interrupt) {
 		retval = usb_submit_urb (dev->interrupt, GFP_KERNEL);
-		if (retval < 0) {
+		if (retval == -EALREADY) {
+			/*
+			 * It is not an error if interrupt urb is alredy active
+			 */
+			retval = 0;
+		} else if (retval < 0) {
 			if (netif_msg_ifup (dev))
 				deverr (dev, "intr submit %d", retval);
 			goto done;
@@ -1065,6 +1070,10 @@ static void usbnet_bh (unsigned long param)
 		if (dev->txq.qlen < TX_QLEN (dev))
 			netif_wake_queue (dev->net);
 	}
+
+	/* Re-submit interrupt urb (doesn't hurt to retry) */
+	if (netif_running(dev->net))
+		usb_submit_urb(dev->interrupt, GFP_KERNEL);
 }
 
 
@@ -1285,6 +1294,9 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
 		 * wake the device
 		 */
 		netif_device_attach (dev->net);
+		/* Stop interrupt urbs while in suspend */
+		if (dev->interrupt)
+			usb_kill_urb(dev->interrupt);
 	}
 	return 0;
 }
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 4342bd9..ff85f50 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -295,8 +295,10 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 	struct usb_host_endpoint	*ep;
 	int				is_out;
 
-	if (!urb || urb->hcpriv || !urb->complete)
+	if (!urb)
 		return -EINVAL;
+	if (!urb->complete || urb->hcpriv)
+		return -EALREADY;
 	dev = urb->dev;
 	if ((!dev) || (dev->state < USB_STATE_DEFAULT))
 		return -ENODEV;
@@ -807,4 +809,3 @@ int usb_anchor_empty(struct usb_anchor *anchor)
 }
 
 EXPORT_SYMBOL_GPL(usb_anchor_empty);
-
-- 
1.7.3.1

[PATCHv3] usbnet: Resubmit interrupt URB once if halted

[Paul Stewart]

Set a flag if the interrupt URB completes with ENOENT as this
occurs legitimately during system suspend.  When the usbnet_bh
is called after resume, test this flag and try once to resubmit
the interrupt URB.

Signed-off-by: Paul Stewart <pstew-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/net/usb/usbnet.c   |    8 ++++++++
 include/linux/usb/usbnet.h |    1 +
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 02d25c7..9ac4bae 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);
 		return;
 
 	/* NOTE:  not throttling like RX/TX, since this endpoint
@@ -1065,6 +1066,13 @@ static void usbnet_bh (unsigned long param)
 		if (dev->txq.qlen < TX_QLEN (dev))
 			netif_wake_queue (dev->net);
 	}
+
+	// try once to resume interrupt URBs if they were halted before
+	if (dev->interrupt && netif_running(dev->net) &&
+	    test_bit(EVENT_INTR_HALT, &dev->flags)) {
+		clear_bit(EVENT_INTR_HALT, &dev->flags);
+		usb_submit_urb(dev->interrupt, GFP_KERNEL);
+	}
 }
 
 
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index ba09fe8..6c4b5f8 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -64,6 +64,7 @@ struct usbnet {
 #		define EVENT_RX_MEMORY	2
 #		define EVENT_STS_SPLIT	3
 #		define EVENT_LINK_RESET	4
+#		define EVENT_INTR_HALT	5
 };
 
 static inline struct usb_driver *driver_of(struct usb_interface *intf)
-- 
1.7.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4] usbnet: Resubmit interrupt URB once if halted

[Paul Stewart]

Set a flag if the interrupt URB completes with ENOENT as this
occurs legitimately during system suspend.  When the usbnet_bh
is called after resume, test this flag and try once to resubmit
the interrupt URB.

Signed-off-by: Paul Stewart <pstew@chromium.org>
---
 drivers/net/usb/usbnet.c   |    8 ++++++++
 include/linux/usb/usbnet.h |    1 +
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 02d25c7..9ac4bae 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);
 		return;
 
 	/* NOTE:  not throttling like RX/TX, since this endpoint
@@ -1065,6 +1066,13 @@ static void usbnet_bh (unsigned long param)
 		if (dev->txq.qlen < TX_QLEN (dev))
 			netif_wake_queue (dev->net);
 	}
+
+	/* try once to resume interrupt URBs if they were halted before */
+	if (dev->interrupt && netif_running(dev->net) &&
+	    test_bit(EVENT_INTR_HALT, &dev->flags)) {
+		clear_bit(EVENT_INTR_HALT, &dev->flags);
+		usb_submit_urb(dev->interrupt, GFP_KERNEL);
+	}
 }
 
 
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index ba09fe8..6c4b5f8 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -64,6 +64,7 @@ struct usbnet {
 #		define EVENT_RX_MEMORY	2
 #		define EVENT_STS_SPLIT	3
 #		define EVENT_LINK_RESET	4
+#		define EVENT_INTR_HALT	5
 };
 
 static inline struct usb_driver *driver_of(struct usb_interface *intf)
-- 
1.7.3.1

[PATCHv4] usbnet: Resubmit interrupt URB once if halted

[Paul Stewart]

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.

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()?

Signed-off-by: Paul Stewart <pstew-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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);
 		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)) {
+			clear_bit(EVENT_INTR_HALT, &dev->flags);
+			usb_submit_urb(dev->interrupt, GFP_KERNEL);
+		}
+	}
+}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(usbnet_resume);
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index ba09fe8..6c4b5f8 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -64,6 +64,7 @@ struct usbnet {
 #		define EVENT_RX_MEMORY	2
 #		define EVENT_STS_SPLIT	3
 #		define EVENT_LINK_RESET	4
+#		define EVENT_INTR_HALT	5
 };
 
 static inline struct usb_driver *driver_of(struct usb_interface *intf)
-- 
1.7.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbnet: Resubmit interrupt URB more often

[Paul Stewart]

Thanks, Ben.  I've made some changes.  usbnet.c is a little bit a of
mess style-wise, and I let myself get sucked in. :-)

--
Paul

On Tue, Apr 19, 2011 at 10:11 AM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
>
> On Tue, 2011-04-19 at 09:35 -0700, Paul Stewart wrote:
> [...]
> > index 4342bd9..e4dbb29 100644
> > --- a/drivers/usb/core/urb.c
> > +++ b/drivers/usb/core/urb.c
> > @@ -295,7 +295,9 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
> >       struct usb_host_endpoint        *ep;
> >       int                             is_out;
> >
> > -     if (!urb || urb->hcpriv || !urb->complete)
> > +     if (urb->hcpriv)
> > +             return -EALREADY;
> > +     if (!urb || !urb->complete)
> >               return -EINVAL;
> [...]
>
> The test for !urb must come before the test on urb->hcpriv.
>
> Also, the kernel coding style does not allow '//' comments.  Please use
> scripts/checkpatch.pl to check for style and other common errors.
>
> Ben.
>
> --
> Ben Hutchings, Senior Software Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>

Re: [PATCHv2] usbnet: Resubmit interrupt URB more often

[David Miller]

From: Paul Stewart <pstew@chromium.org>
Date: Tue, 19 Apr 2011 10:44:12 -0700

> The second class of device are those which shut down the device
> during suspend.  During a suspend-resume cycle, the device is
> re-enumerated at system resume, and for whatever reason
> usbnet_resume may be called on the device during the call-tree
> from usbnet_open-> usb_autopm_get_interface, which may cause a
> race where the first change above may cause the bh to submit the
> interrupt urb before usbnet_open() does.  As a result, I've added
> an EALREADY check and a fix to urb.c to send one.

You'll need to submit the generic urb submission change via the USB
maintainers, not networking.

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

[Alan Stern]

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_bh
> is called after resume, test this flag and try once to resubmit
> the interrupt URB.

No doubt there's a good reason for doing things this way, but it isn't
clear.  Why wait until usbnet_bh() is called after resume?  Why not
resubmit the interrupt URB _during_ usbnet_resume()?  This would seem
to be the logical approach, seeing as how usbnet_suspend() kills the
interrupt URB.

For that matter, where does the interrupt URB get deallocated?  It 
obviously gets allocated in init_status(), but it doesn't seem to be 
freed anywhere.

Alan Stern

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

[Paul Stewart]

On Wed, Apr 20, 2011 at 2:08 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> 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_bh
>> is called after resume, test this flag and try once to resubmit
>> the interrupt URB.
>
> No doubt there's a good reason for doing things this way, but it isn't
> clear.  Why wait until usbnet_bh() is called after resume?  Why not
> resubmit the interrupt URB _during_ usbnet_resume()?

Actually, I was doing this in the bh because of feedback I had gained
early in this process about not doing submit_urb in the resume().  If
that issue doesn't exist, that makes my work a lot easier.  In testing
I found that just setting this to happen in the bh might be problematic
due to firing too early, so this is good news.

> This would seem
> to be the logical approach, seeing as how usbnet_suspend() kills the
> interrupt URB.

Aha!  But you'll see from the current version of my patch that we don't
actually ever kill the interrupt URB.  It gets killed all on its own (by the
hcd?) and handed back to us in intr_complete().  This last bit about the
complete function being called was lost on me for a while which is why
in a previous iteration of the patch I was trying to kill the urb in suspend().

> For that matter, where does the interrupt URB get deallocated?  It
> obviously gets allocated in init_status(), but it doesn't seem to be
> freed anywhere.

That's a very interesting question and one that I noticed as well.
I was going to tackle that as a separate issue.

>
> Alan Stern
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 2065 bytes --]

On Wed, 20 Apr 2011, Paul Stewart wrote:

> On Wed, Apr 20, 2011 at 2:08 PM, Alan Stern <stern@rowland.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_bh
> >> is called after resume, test this flag and try once to resubmit
> >> the interrupt URB.
> >
> > No doubt there's a good reason for doing things this way, but it isn't
> > clear.  Why wait until usbnet_bh() is called after resume?  Why not
> > resubmit the interrupt URB _during_ usbnet_resume()?
> 
> Actually, I was doing this in the bh because of feedback I had gained
> early in this process about not doing submit_urb in the resume().

Do you have a URL for that feedback?  In general, there's no reason not 
to resubmit URBs during a resume callback; lots of drivers do it.  But 
usbnet may have some special requirements of its own that I'm not aware 
of.

>  If
> that issue doesn't exist, that makes my work a lot easier.  In testing
> I found that just setting this to happen in the bh might be problematic
> due to firing too early, so this is good news.
> 
> > This would seem
> > to be the logical approach, seeing as how usbnet_suspend() kills the
> > interrupt URB.
> 
> Aha!  But you'll see from the current version of my patch that we don't
> actually ever kill the interrupt URB.  It gets killed all on its own (by the
> hcd?) and handed back to us in intr_complete().  This last bit about the
> complete function being called was lost on me for a while which is why
> in a previous iteration of the patch I was trying to kill the urb in suspend().

Why not kill the interrupt URB while suspending?  It's the proper thing
to do.  Otherwise you run the risk that an event might happen at just
the wrong time, causing the interrupt URB to complete normally, but
_after_ the driver has finished suspending.  There's a good chance the 
driver would not process the event correctly.

Alan Stern

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

[Alan Stern]

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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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?

> +			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 linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Paul Stewart]

On Thu, Apr 21, 2011 at 6:43 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Wed, 20 Apr 2011, Paul Stewart wrote:
>
>> On Wed, Apr 20, 2011 at 2:08 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLrNAH6kLmebB@public.gmane.orgdu> 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_bh
>> >> is called after resume, test this flag and try once to resubmit
>> >> the interrupt URB.
>> >
>> > No doubt there's a good reason for doing things this way, but it isn't
>> > clear. �Why wait until usbnet_bh() is called after resume? �Why not
>> > resubmit the interrupt URB _during_ usbnet_resume()?
>>
>> Actually, I was doing this in the bh because of feedback I had gained
>> early in this process about not doing submit_urb in the resume().
>
> Do you have a URL for that feedback?  In general, there's no reason not
> to resubmit URBs during a resume callback; lots of drivers do it.  But
> usbnet may have some special requirements of its own that I'm not aware
> of.
>
>>  If
>> that issue doesn't exist, that makes my work a lot easier.  In testing
>> I found that just setting this to happen in the bh might be problematic
>> due to firing too early, so this is good news.
>>
>> >�This would seem
>> > to be the logical approach, seeing as how usbnet_suspend() kills the
>> > interrupt URB.
>>
>> Aha!  But you'll see from the current version of my patch that we don't
>> actually ever kill the interrupt URB.  It gets killed all on its own (by the
>> hcd?) and handed back to us in intr_complete().  This last bit about the
>> complete function being called was lost on me for a while which is why
>> in a previous iteration of the patch I was trying to kill the urb in suspend().
>
> Why not kill the interrupt URB while suspending?  It's the proper thing
> to do.  Otherwise you run the risk that an event might happen at just
> the wrong time, causing the interrupt URB to complete normally, but
> _after_ the driver has finished suspending.  There's a good chance the
> driver would not process the event correctly.

I don't mind killing the URB. I'd want to set the halt flag as well
(more on why have a flag in response to your other email).  You're
right that there may be a race between an interrupt URB arriving and
the onset of suspend, but I really can't imagine why I can't solve
that by setting the flag if a submit_urb() fails in intr_complete().

>
> Alan Stern
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Paul Stewart]

On Thu, Apr 21, 2011 at 7:03 AM, Alan Stern <stern@rowland.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@chromium.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
>
>

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

[Alan Stern]

On Thu, 21 Apr 2011, Paul Stewart wrote:

> 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.

Okay, good.

> In a second scenario, for other systems USB devices go unpowered
> during suspend.

As happens during hibernation, for example.

>  At resume time, there's a quick succession where the
> device appears to detach and reattach and enumerate.

Right.  It's called reset-resume, and drivers have a special method for 
it, distinct from regular resume.  In theory it shouldn't make any 
difference.

>  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().

Only if the interface was suspended when usbnet_open() was called.  It
might have gotten suspended automatically following the system resume,
if it wasn't in use.  But this should work out the same whether or not
there was a reset-rseume.

>  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.

The driver needs better coordination between open/stop and
resume/suspend.  The interrupt and receive URBs are supposed to be
active whenever the interface is up and not suspended, right?  Which
means that usbnet_resume() shouldn't submit anything if the interface 
isn't up.

Alan Stern

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

[Oliver Neukum]

Am Donnerstag, 21. April 2011, 16:03:34 schrieb Alan Stern:
> On Tue, 19 Apr 2011, Paul Stewart wrote:

> > 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()?

Suppose a device of two interfaces one of them storage is autosuspended.
GFP_KERNEL in the first device to be resumed triggers a pageout to the
suspended storage device.

	Regards
		Oliver

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

[Alan Stern]

On Thu, 21 Apr 2011, Oliver Neukum wrote:

> Am Donnerstag, 21. April 2011, 16:03:34 schrieb Alan Stern:
> > On Tue, 19 Apr 2011, Paul Stewart wrote:
> 
> > > 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()?
> 
> Suppose a device of two interfaces one of them storage is autosuspended.
> GFP_KERNEL in the first device to be resumed triggers a pageout to the
> suspended storage device.

True enough, I had forgotten about that.  A resume routine should 
always use GFP_NOIO, not GFP_KERNEL.

In fact this restriction is true in general, not just for USB devices 
containing a mass-storage interface.  The backing device for an evicted 
page might not be resumed until later on in the resume sequence.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html