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