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

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: 1715 bytes --]

On Thu, 21 Apr 2011, Paul Stewart wrote:

> > 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.
> 
> How do we define "up" here (from a network perspective there are many
> ways to interpret that)?  How does this concept compare to the user's
> "ifconfig up/down" state?

I don't know the details of how network drivers are supposed to work.  
But it doesn't matter -- for your purposes you should define "up" to 
mean "whenever the URBs are supposed to be active (unless the interface 
is suspended)".

>  What call do I use in usbnet_resume() to
> tell that the interface isn't up?  Currently I'm using netif_running()
> which responds true in this condition, which is why I'm resorting to
> the flag.

Again, I don't know.  However, the URBs get submitted from within
usbnet_open() and killed within usbnet_stop(), right?  Therefore you
can use any condition which gets set to True in usbnet_open() and set
to False in usbnet_stop().  (If nothing else is suitable, use a flag of
your own.)  And be careful of the edge case: Since usbnet_open() itself
performs a resume operation, you need to make sure the resume takes
place before the condition becomes True -- otherwise the URBs will get
submitted twice.

One more thing to keep in mind: If the kernel is built without PM
support, the resume and suspend routines will never get called.  
Therefore they must not be the only places where URBs are submitted and
killed.

Alan Stern

[PATCHv5] usbnet: Resubmit interrupt URB once if halted

[Paul Stewart]

Resubmit interrupt URB if device is open.  Use a flag set in
usbnet_open() to determine this state.  Also kill and free
interrupt URB in usbnet_disconnect().

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

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 02d25c7..c7cf4af 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -643,6 +643,7 @@ static int usbnet_open (struct net_device *net)
 		}
 	}
 
+	set_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_start_queue (net);
 	if (netif_msg_ifup (dev)) {
 		char	*framing;
@@ -1105,6 +1106,11 @@ void usbnet_disconnect (struct usb_interface *intf)
 	if (dev->driver_info->unbind)
 		dev->driver_info->unbind (dev, intf);
 
+	if (dev->interrupt) {
+		usb_kill_urb(dev->interrupt);
+		usb_free_urb(dev->interrupt);
+	}
+
 	free_netdev(net);
 	usb_put_dev (xdev);
 }
@@ -1285,6 +1291,10 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
 		 * wake the device
 		 */
 		netif_device_attach (dev->net);
+
+		/* Stop interrupt URBs */
+		if (dev->interrupt)
+			usb_kill_urb(dev->interrupt);
 	}
 	return 0;
 }
@@ -1297,6 +1307,10 @@ int usbnet_resume (struct usb_interface *intf)
 	if (!--dev->suspend_count)
 		tasklet_schedule (&dev->bh);
 
+		/* resume interrupt URBs */
+		if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
+			usb_submit_urb(dev->interrupt, GFP_NOIO);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(usbnet_resume);
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index ba09fe8..d148cca 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_DEV_OPEN	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: [PATCHv5] usbnet: Resubmit interrupt URB once if halted

[Alan Stern]

On Tue, 19 Apr 2011, Paul Stewart wrote:

> Resubmit interrupt URB if device is open.  Use a flag set in
> usbnet_open() to determine this state.  Also kill and free
> interrupt URB in usbnet_disconnect().

Generally good, but a couple of things are questionable...

> Signed-off-by: Paul Stewart <pstew-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/net/usb/usbnet.c   |   14 ++++++++++++++
>  include/linux/usb/usbnet.h |    1 +
>  2 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 02d25c7..c7cf4af 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -643,6 +643,7 @@ static int usbnet_open (struct net_device *net)
>  		}
>  	}
>  
> +	set_bit(EVENT_DEV_OPEN, &dev->flags);
>  	netif_start_queue (net);
>  	if (netif_msg_ifup (dev)) {
>  		char	*framing;

You forgot to clear this flag in usbnet_stop().

By the way, there is FLAG_AVOID_UNLINK_URBS defined in usbnet.h and
used in usbnet_stop().  Is it meant to apply to the interrupt URB as
well as the others?

> @@ -1105,6 +1106,11 @@ void usbnet_disconnect (struct usb_interface *intf)
>  	if (dev->driver_info->unbind)
>  		dev->driver_info->unbind (dev, intf);
>  
> +	if (dev->interrupt) {
> +		usb_kill_urb(dev->interrupt);
> +		usb_free_urb(dev->interrupt);
> +	}
> +

usb_kill_urb and usb_free_urb include their own tests for urb == NULL; 
you don't need to test it here or below.

>  	free_netdev(net);
>  	usb_put_dev (xdev);
>  }
> @@ -1285,6 +1291,10 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
>  		 * wake the device
>  		 */
>  		netif_device_attach (dev->net);
> +
> +		/* Stop interrupt URBs */
> +		if (dev->interrupt)
> +			usb_kill_urb(dev->interrupt);
>  	}
>  	return 0;
>  }

There is a subtle question here: When is the best time to kill the 
interrupt URB?  Without knowing any of the details, I'd guess that the 
interrupt URB reports asynchronous events and the driver could run into 
trouble if one of those events occurred while everything else was 
turned off.  This suggests that the interrupt URB should be killed as 
soon as possible rather than as late as possible.  But maybe it doesn't 
matter; it all depends on the design of the driver.

> @@ -1297,6 +1307,10 @@ int usbnet_resume (struct usb_interface *intf)
>  	if (!--dev->suspend_count)
>  		tasklet_schedule (&dev->bh);
>  
> +		/* resume interrupt URBs */
> +		if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
> +			usb_submit_urb(dev->interrupt, GFP_NOIO);
> +
>  	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

[PATCHv6] usbnet: Resubmit interrupt URB if device is open

[Paul Stewart]

Resubmit interrupt URB if device is open.  Use a flag set in
usbnet_open() to determine this state.  Also kill and free
interrupt URB in usbnet_disconnect().

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

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 02d25c7..b4572c3 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -643,6 +643,7 @@ static int usbnet_open (struct net_device *net)
 		}
 	}
 
+	set_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_start_queue (net);
 	if (netif_msg_ifup (dev)) {
 		char	*framing;
@@ -1105,6 +1106,9 @@ void usbnet_disconnect (struct usb_interface *intf)
 	if (dev->driver_info->unbind)
 		dev->driver_info->unbind (dev, intf);
 
+	usb_kill_urb(dev->interrupt);
+	usb_free_urb(dev->interrupt);
+
 	free_netdev(net);
 	usb_put_dev (xdev);
 }
@@ -1285,6 +1289,9 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
 		 * wake the device
 		 */
 		netif_device_attach (dev->net);
+
+		/* Stop interrupt URBs */
+		usb_kill_urb(dev->interrupt);
 	}
 	return 0;
 }
@@ -1297,6 +1304,10 @@ int usbnet_resume (struct usb_interface *intf)
 	if (!--dev->suspend_count)
 		tasklet_schedule (&dev->bh);
 
+		/* resume interrupt URBs */
+		if (test_bit(EVENT_DEV_OPEN, &dev->flags))
+			usb_submit_urb(dev->interrupt, GFP_NOIO);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(usbnet_resume);
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index ba09fe8..d148cca 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_DEV_OPEN	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: [PATCHv6] usbnet: Resubmit interrupt URB if device is open

[Alan Stern]

On Tue, 19 Apr 2011, Paul Stewart wrote:

> Resubmit interrupt URB if device is open.  Use a flag set in
> usbnet_open() to determine this state.  Also kill and free
> interrupt URB in usbnet_disconnect().
> 
> Signed-off-by: Paul Stewart <pstew-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/net/usb/usbnet.c   |   11 +++++++++++
>  include/linux/usb/usbnet.h |    1 +
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 02d25c7..b4572c3 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -643,6 +643,7 @@ static int usbnet_open (struct net_device *net)
>  		}
>  	}
>  
> +	set_bit(EVENT_DEV_OPEN, &dev->flags);
>  	netif_start_queue (net);
>  	if (netif_msg_ifup (dev)) {
>  		char	*framing;
> @@ -1105,6 +1106,9 @@ void usbnet_disconnect (struct usb_interface *intf)
>  	if (dev->driver_info->unbind)
>  		dev->driver_info->unbind (dev, intf);
>  
> +	usb_kill_urb(dev->interrupt);
> +	usb_free_urb(dev->interrupt);
> +
>  	free_netdev(net);
>  	usb_put_dev (xdev);
>  }
> @@ -1285,6 +1289,9 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
>  		 * wake the device
>  		 */
>  		netif_device_attach (dev->net);
> +
> +		/* Stop interrupt URBs */
> +		usb_kill_urb(dev->interrupt);
>  	}
>  	return 0;
>  }
> @@ -1297,6 +1304,10 @@ int usbnet_resume (struct usb_interface *intf)
>  	if (!--dev->suspend_count)
>  		tasklet_schedule (&dev->bh);
>  
> +		/* resume interrupt URBs */
> +		if (test_bit(EVENT_DEV_OPEN, &dev->flags))
> +			usb_submit_urb(dev->interrupt, GFP_NOIO);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(usbnet_resume);
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index ba09fe8..d148cca 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_DEV_OPEN	5
>  };
>  
>  static inline struct usb_driver *driver_of(struct usb_interface *intf)
  
I have no more objections, but this really should be reviewed by
somebody who has a better understanding of usbnet.  Such as Oliver.
   
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: [PATCHv6] usbnet: Resubmit interrupt URB if device is open

[David Miller]

Please fix the date on your email postings.

Even in the context of the "date" of your commit, you're making changes
and updates even right now, so the date should be updating to be more
in line with "right now"

Yet all of your postings still are dated April 19th.

This screws up my patch queue at:

http://patchwork.ozlabs.org/project/netdev/list/

because instead of your new patches showing up at the top where they
belong, they sneak in near the end and that's a real pain.

Thanks.
--
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: [PATCHv5] usbnet: Resubmit interrupt URB once if halted

[Paul Stewart]

On Fri, Apr 22, 2011 at 8:47 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Tue, 19 Apr 2011, Paul Stewart wrote:
>
>> Resubmit interrupt URB if device is open.  Use a flag set in
>> usbnet_open() to determine this state.  Also kill and free
>> interrupt URB in usbnet_disconnect().
>
> Generally good, but a couple of things are questionable...
>
>> Signed-off-by: Paul Stewart <pstew-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>  drivers/net/usb/usbnet.c   |   14 ++++++++++++++
>>  include/linux/usb/usbnet.h |    1 +
>>  2 files changed, 15 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index 02d25c7..c7cf4af 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -643,6 +643,7 @@ static int usbnet_open (struct net_device *net)
>>               }
>>       }
>>
>> +     set_bit(EVENT_DEV_OPEN, &dev->flags);
>>       netif_start_queue (net);
>>       if (netif_msg_ifup (dev)) {
>>               char    *framing;
>
> You forgot to clear this flag in usbnet_stop().

Actually, I didn't.  Note that flags are set to 0 in usbnet_stop().

>
> By the way, there is FLAG_AVOID_UNLINK_URBS defined in usbnet.h and
> used in usbnet_stop().  Is it meant to apply to the interrupt URB as
> well as the others?

I don't know who to ask about that.  Whether or not the flag is set,
in practice the interrupt URB still seems to be killed by some other
facility, so regardless of that request, they may lose anyway.

>> @@ -1105,6 +1106,11 @@ void usbnet_disconnect (struct usb_interface *intf)
>>       if (dev->driver_info->unbind)
>>               dev->driver_info->unbind (dev, intf);
>>
>> +     if (dev->interrupt) {
>> +             usb_kill_urb(dev->interrupt);
>> +             usb_free_urb(dev->interrupt);
>> +     }
>> +
>
> usb_kill_urb and usb_free_urb include their own tests for urb == NULL;
> you don't need to test it here or below.

Okay.

>
>>       free_netdev(net);
>>       usb_put_dev (xdev);
>>  }
>> @@ -1285,6 +1291,10 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
>>                * wake the device
>>                */
>>               netif_device_attach (dev->net);
>> +
>> +             /* Stop interrupt URBs */
>> +             if (dev->interrupt)
>> +                     usb_kill_urb(dev->interrupt);
>>       }
>>       return 0;
>>  }
>
> There is a subtle question here: When is the best time to kill the
> interrupt URB?  Without knowing any of the details, I'd guess that the
> interrupt URB reports asynchronous events and the driver could run into
> trouble if one of those events occurred while everything else was
> turned off.  This suggests that the interrupt URB should be killed as
> soon as possible rather than as late as possible.  But maybe it doesn't
> matter; it all depends on the design of the driver.

I'm not sure I can answer this question either.  As it stands, nobody
was killing them before.  Just trying to make it better.

>
>> @@ -1297,6 +1307,10 @@ int usbnet_resume (struct usb_interface *intf)
>>       if (!--dev->suspend_count)
>>               tasklet_schedule (&dev->bh);
>>
>> +             /* resume interrupt URBs */
>> +             if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
>> +                     usb_submit_urb(dev->interrupt, GFP_NOIO);
>> +
>>       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: [PATCHv5] usbnet: Resubmit interrupt URB once if halted

[Oliver Neukum]

Am Freitag, 22. April 2011, 17:59:15 schrieb Paul Stewart:
> >
> >>       free_netdev(net);
> >>       usb_put_dev (xdev);
> >>  }
> >> @@ -1285,6 +1291,10 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
> >>                * wake the device
> >>                */
> >>               netif_device_attach (dev->net);
> >> +
> >> +             /* Stop interrupt URBs */
> >> +             if (dev->interrupt)
> >> +                     usb_kill_urb(dev->interrupt);
> >>       }
> >>       return 0;
> >>  }
> >
> > There is a subtle question here: When is the best time to kill the
> > interrupt URB?  Without knowing any of the details, I'd guess that the
> > interrupt URB reports asynchronous events and the driver could run into
> > trouble if one of those events occurred while everything else was
> > turned off.  This suggests that the interrupt URB should be killed as
> > soon as possible rather than as late as possible.  But maybe it doesn't
> > matter; it all depends on the design of the driver.
> 
> I'm not sure I can answer this question either.  As it stands, nobody
> was killing them before.  Just trying to make it better.

Hm. Are we looking at the same code? usbnet_suspend now has:

		/*
		 * accelerate emptying of the rx and queues, to avoid
		 * having everything error out.
		 */
		netif_device_detach (dev->net);
		usbnet_terminate_urbs(dev);
		usb_kill_urb(dev->interrupt);

This suggests that if you want to resubmit the interrupt URB in resume()
you do it first. Which drivers use the interrupt URB?

	Regards
		Oliver

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

[Paul Stewart]

On Sat, Apr 23, 2011 at 11:36 PM, Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org> wrote:
> Am Freitag, 22. April 2011, 17:59:15 schrieb Paul Stewart:
>> >
>> >>       free_netdev(net);
>> >>       usb_put_dev (xdev);
>> >>  }
>> >> @@ -1285,6 +1291,10 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
>> >>                * wake the device
>> >>                */
>> >>               netif_device_attach (dev->net);
>> >> +
>> >> +             /* Stop interrupt URBs */
>> >> +             if (dev->interrupt)
>> >> +                     usb_kill_urb(dev->interrupt);
>> >>       }
>> >>       return 0;
>> >>  }
>> >
>> > There is a subtle question here: When is the best time to kill the
>> > interrupt URB?  Without knowing any of the details, I'd guess that the
>> > interrupt URB reports asynchronous events and the driver could run into
>> > trouble if one of those events occurred while everything else was
>> > turned off.  This suggests that the interrupt URB should be killed as
>> > soon as possible rather than as late as possible.  But maybe it doesn't
>> > matter; it all depends on the design of the driver.
>>
>> I'm not sure I can answer this question either.  As it stands, nobody
>> was killing them before.  Just trying to make it better.
>
> Hm. Are we looking at the same code?

Perhaps not.  I'm working out of the netdev-2.6 git repository.  Is
this the wrong place?

> usbnet_suspend now has:
>
>                /*
>                 * accelerate emptying of the rx and queues, to avoid
>                 * having everything error out.
>                 */
>                netif_device_detach (dev->net);
>                usbnet_terminate_urbs(dev);
>                usb_kill_urb(dev->interrupt);
>
> This suggests that if you want to resubmit the interrupt URB in resume()
> you do it first. Which drivers use the interrupt URB?

The asix driver uses it fo signal link status.

>
>        Regards
>                Oliver
>
--
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

[PATCHv7] usbnet: Resubmit interrupt URB if device is open

[Paul Stewart]

Resubmit interrupt URB if device is open.  Use a flag set in
usbnet_open() to determine this state.  Also kill and free
interrupt URB in usbnet_disconnect().

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

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 02d25c7..623d26f 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -643,6 +643,7 @@ static int usbnet_open (struct net_device *net)
 		}
 	}
 
+	set_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_start_queue (net);
 	if (netif_msg_ifup (dev)) {
 		char	*framing;
@@ -1105,6 +1106,9 @@ void usbnet_disconnect (struct usb_interface *intf)
 	if (dev->driver_info->unbind)
 		dev->driver_info->unbind (dev, intf);
 
+	usb_kill_urb(dev->interrupt);
+	usb_free_urb(dev->interrupt);
+
 	free_netdev(net);
 	usb_put_dev (xdev);
 }
@@ -1285,6 +1289,9 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
 		 * wake the device
 		 */
 		netif_device_attach (dev->net);
+
+		/* Stop interrupt URBs */
+		usb_kill_urb(dev->interrupt);
 	}
 	return 0;
 }
@@ -1294,8 +1301,13 @@ int usbnet_resume (struct usb_interface *intf)
 {
 	struct usbnet		*dev = usb_get_intfdata(intf);
 
-	if (!--dev->suspend_count)
+	if (!--dev->suspend_count) {
+		/* resume interrupt URBs */
+		if (test_bit(EVENT_DEV_OPEN, &dev->flags))
+			usb_submit_urb(dev->interrupt, GFP_NOIO);
+
 		tasklet_schedule (&dev->bh);
+        }
 
 	return 0;
 }
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index ba09fe8..d148cca 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_DEV_OPEN	5
 };
 
 static inline struct usb_driver *driver_of(struct usb_interface *intf)
-- 
1.7.3.1

Re: [PATCHv7] usbnet: Resubmit interrupt URB if device is open

[David Miller]

From: Paul Stewart <pstew-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Date: Tue, 26 Apr 2011 09:58:32 -0700

> Resubmit interrupt URB if device is open.  Use a flag set in
> usbnet_open() to determine this state.  Also kill and free
> interrupt URB in usbnet_disconnect().
> 
> Signed-off-by: Paul Stewart <pstew-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

This doesn't apply at all to Linus's current tree.

Several new EVENT_* flags have been added, and usbnet_resume() does
a lot more things than whatever version your patch is against.

I'm not going to pick a new value EVENT_* mask and force the rest of
this to apply, because all of these other changes that have happened
to the driver meanwhile might interact with your changes in ways I
won't understand.

You'll therefore need to look this over and submit a patch which will
apply cleanly.
--
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

[PATCHv8] usbnet: Resubmit interrupt URB if device is open

[Paul Stewart]

Resubmit interrupt URB if device is open.  Use a flag set in
usbnet_open() to determine this state.  Also kill and free
interrupt URB in usbnet_disconnect().

[Rebased off git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git]

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 069c1cf..009bba3 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -736,6 +736,7 @@ int usbnet_open (struct net_device *net)
 		}
 	}
 
+	set_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_start_queue (net);
 	netif_info(dev, ifup, dev->net,
 		   "open: enable queueing (rx %d, tx %d) mtu %d %s framing\n",
@@ -1259,6 +1260,9 @@ void usbnet_disconnect (struct usb_interface *intf)
 	if (dev->driver_info->unbind)
 		dev->driver_info->unbind (dev, intf);
 
+	usb_kill_urb(dev->interrupt);
+	usb_free_urb(dev->interrupt);
+
 	free_netdev(net);
 	usb_put_dev (xdev);
 }
@@ -1498,6 +1502,10 @@ int usbnet_resume (struct usb_interface *intf)
 	int                     retval;
 
 	if (!--dev->suspend_count) {
+		/* resume interrupt URBs */
+		if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
+			usb_submit_urb(dev->interrupt, GFP_NOIO);
+
 		spin_lock_irq(&dev->txq.lock);
 		while ((res = usb_get_from_anchor(&dev->deferred))) {
 
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 0e18550..605b0aa 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -68,6 +68,7 @@ struct usbnet {
 #		define EVENT_RX_PAUSED	5
 #		define EVENT_DEV_WAKING 6
 #		define EVENT_DEV_ASLEEP 7
+#		define EVENT_DEV_OPEN	8
 };
 
 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: [PATCHv8] usbnet: Resubmit interrupt URB if device is open

[David Miller]

From: Paul Stewart <pstew@chromium.org>
Date: Thu, 28 Apr 2011 08:43:37 -0700

> Resubmit interrupt URB if device is open.  Use a flag set in
> usbnet_open() to determine this state.  Also kill and free
> interrupt URB in usbnet_disconnect().
> 
> [Rebased off git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git]
> 
> Signed-off-by: Paul Stewart <pstew@chromium.org>

Applied, thank you.

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

[Paul Stewart]

On Thu, Apr 21, 2011 at 11:48 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 21 Apr 2011, Paul Stewart wrote:
>
>> > 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.
>>
>> How do we define "up" here (from a network perspective there are many
>> ways to interpret that)?  How does this concept compare to the user's
>> "ifconfig up/down" state?
>
> I don't know the details of how network drivers are supposed to work.
> But it doesn't matter -- for your purposes you should define "up" to
> mean "whenever the URBs are supposed to be active (unless the interface
> is suspended)".
>
>>  What call do I use in usbnet_resume() to
>> tell that the interface isn't up?  Currently I'm using netif_running()
>> which responds true in this condition, which is why I'm resorting to
>> the flag.
>
> Again, I don't know.  However, the URBs get submitted from within
> usbnet_open() and killed within usbnet_stop(), right?  Therefore you
> can use any condition which gets set to True in usbnet_open() and set
> to False in usbnet_stop().  (If nothing else is suitable, use a flag of
> your own.)

This is exactly the situation I'm in.  I couldn't find any other
driver or network state that cleanly represented the stop/start state
of the driver.  I'll post a new patch that uses an OPEN flag instead
of an interrupt halted flag, a GFP_NOIO flag, and kills and frees the
interrupt URB on usb_disconnect().

>  And be careful of the edge case: Since usbnet_open() itself
> performs a resume operation, you need to make sure the resume takes
> place before the condition becomes True -- otherwise the URBs will get
> submitted twice.
>
> One more thing to keep in mind: If the kernel is built without PM
> support, the resume and suspend routines will never get called.
> Therefore they must not be the only places where URBs are submitted and
> killed.
>
> Alan Stern
>
>

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.

[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

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

[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: [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 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

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

[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