Patchwork [1/2,v4] usbnet: allow status interrupt URB to always be active

login
register
mail settings
Submitter Dan Williams
Date April 9, 2013, 11:02 p.m.
Message ID <1365548547.23372.2.camel@dcbw.foobar.com>
Download mbox | patch
Permalink /patch/235258/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Dan Williams - April 9, 2013, 11:02 p.m.
Some drivers (sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware.  Add functions to refcount
the status interrupt URB submit/kill operation so that
sub-drivers and the generic driver don't fight over whether
the status interrupt URB is active or not.

A sub-driver can call usbnet_status_start() at any time, but
the URB is only submitted the first time the function is
called.  Likewise, when the sub-driver is done with the URB,
it calls usbnet_status_stop() but the URB is only killed when
all users have stopped it.  The URB is still killed and
re-submitted for suspend/resume, as before, with the same
refcount it had at suspend.

Signed-off-by: Dan Williams <dcbw@redhat.com>
---
 drivers/net/usb/usbnet.c   | 79 ++++++++++++++++++++++++++++++++++++++++++----
 include/linux/usb/usbnet.h |  5 +++
 2 files changed, 77 insertions(+), 7 deletions(-)
Oliver Neukum - April 10, 2013, 7:23 a.m.
On Tuesday 09 April 2013 18:02:27 Dan Williams wrote:
> Some drivers (sierra_net) need the status interrupt URB
> active even when the device is closed, because they receive
> custom indications from firmware.  Add functions to refcount
> the status interrupt URB submit/kill operation so that
> sub-drivers and the generic driver don't fight over whether
> the status interrupt URB is active or not.
> 
> A sub-driver can call usbnet_status_start() at any time, but
> the URB is only submitted the first time the function is
> called.  Likewise, when the sub-driver is done with the URB,
> it calls usbnet_status_stop() but the URB is only killed when
> all users have stopped it.  The URB is still killed and
> re-submitted for suspend/resume, as before, with the same
> refcount it had at suspend.
> 
> Signed-off-by: Dan Williams <dcbw@redhat.com>
> ---
>  drivers/net/usb/usbnet.c   | 79 ++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/usb/usbnet.h |  5 +++
>  2 files changed, 77 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 51f3192..e8b363a 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -42,7 +42,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/mii.h>
>  #include <linux/usb.h>
> -#include <linux/usb/usbnet.h>
> +#include "usbnet.h"
>  #include <linux/slab.h>
>  #include <linux/kernel.h>
>  #include <linux/pm_runtime.h>
> @@ -252,6 +252,70 @@ static int init_status (struct usbnet *dev, struct usb_interface *intf)
>  	return 0;
>  }
>  
> +/* Submit the interrupt URB if it hasn't been submitted yet */
> +static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
> +					bool force)
> +{
> +	int ret = 0;
> +	bool submit = false;
> +
> +	if (!dev->interrupt)
> +		return 0;
> +
> +	mutex_lock(&dev->interrupt_mutex);
> +
> +	if (force) {

That design means that interrupt_count isn't accurate if force is used.
That is extremely ugly.

> +		/* Only submit now if the URB was previously submitted */
> +		if (dev->interrupt_count)
> +			submit = true;
> +	} else if (++dev->interrupt_count == 1)
> +		submit = true;
> +
> +	if (submit)
> +		ret = usb_submit_urb(dev->interrupt, mem_flags);
> +
> +	dev_dbg(&dev->udev->dev, "incremented interrupt URB count to %d\n",
> +		dev->interrupt_count);
> +	mutex_unlock(&dev->interrupt_mutex);
> +	return ret;
> +}
> +
> +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> +{
> +	/* Only drivers that implement a status hook should call this */
> +	BUG_ON(dev->interrupt == NULL);
> +
> +	if (test_bit(EVENT_DEV_ASLEEP, &dev->flags))
> +		return -EINVAL;

This looks like a race condition.

> +	return __usbnet_status_start(dev, mem_flags, false);
> +}
> +EXPORT_SYMBOL_GPL(usbnet_status_start);
> +
> +/* Kill the interrupt URB if all submitters want it killed */
> +static void __usbnet_status_stop(struct usbnet *dev, bool force)
> +{
> +	if (dev->interrupt) {
> +		mutex_lock(&dev->interrupt_mutex);
> +		if (!force)
> +			BUG_ON(dev->interrupt_count == 0);
> +
> +		if (force || --dev->interrupt_count == 0)
> +			usb_kill_urb(dev->interrupt);

Why so complicated? If it may be on, kill it unconditionally.

> +
> +		dev_dbg(&dev->udev->dev,
> +			"decremented interrupt URB count to %d\n",
> +			dev->interrupt_count);
> +		mutex_unlock(&dev->interrupt_mutex);
> +	}
> +}
> +

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - April 10, 2013, 12:49 p.m.
On Wed, 2013-04-10 at 09:23 +0200, Oliver Neukum wrote:
> On Tuesday 09 April 2013 18:02:27 Dan Williams wrote:
> > Some drivers (sierra_net) need the status interrupt URB
> > active even when the device is closed, because they receive
> > custom indications from firmware.  Add functions to refcount
> > the status interrupt URB submit/kill operation so that
> > sub-drivers and the generic driver don't fight over whether
> > the status interrupt URB is active or not.
> > 
> > A sub-driver can call usbnet_status_start() at any time, but
> > the URB is only submitted the first time the function is
> > called.  Likewise, when the sub-driver is done with the URB,
> > it calls usbnet_status_stop() but the URB is only killed when
> > all users have stopped it.  The URB is still killed and
> > re-submitted for suspend/resume, as before, with the same
> > refcount it had at suspend.
> > 
> > Signed-off-by: Dan Williams <dcbw@redhat.com>
> > ---
> >  drivers/net/usb/usbnet.c   | 79 ++++++++++++++++++++++++++++++++++++++++++----
> >  include/linux/usb/usbnet.h |  5 +++
> >  2 files changed, 77 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> > index 51f3192..e8b363a 100644
> > --- a/drivers/net/usb/usbnet.c
> > +++ b/drivers/net/usb/usbnet.c
> > @@ -42,7 +42,7 @@
> >  #include <linux/workqueue.h>
> >  #include <linux/mii.h>
> >  #include <linux/usb.h>
> > -#include <linux/usb/usbnet.h>
> > +#include "usbnet.h"
> >  #include <linux/slab.h>
> >  #include <linux/kernel.h>
> >  #include <linux/pm_runtime.h>
> > @@ -252,6 +252,70 @@ static int init_status (struct usbnet *dev, struct usb_interface *intf)
> >  	return 0;
> >  }
> >  
> > +/* Submit the interrupt URB if it hasn't been submitted yet */
> > +static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
> > +					bool force)
> > +{
> > +	int ret = 0;
> > +	bool submit = false;
> > +
> > +	if (!dev->interrupt)
> > +		return 0;
> > +
> > +	mutex_lock(&dev->interrupt_mutex);
> > +
> > +	if (force) {
> 
> That design means that interrupt_count isn't accurate if force is used.
> That is extremely ugly.

True; the problem here is that the URB isn't always submitted when
suspend is used.  For example, in a normal driver that doesn't need the
URB submitted all the time, interrupt_count will be 0 while !IFF_UP.
Then if the system suspends, we can't decrement interrupt_count because
it's zero.

Besides, if the system is suspended, no driver can call
usbnet_interrupt_start() or usbnet_interrupt_stop(), correct?  Suspend
is a special condition here and nothing that starts/stops the urbs will
ever run while the system is suspended.

> > +		/* Only submit now if the URB was previously submitted */
> > +		if (dev->interrupt_count)
> > +			submit = true;
> > +	} else if (++dev->interrupt_count == 1)
> > +		submit = true;
> > +
> > +	if (submit)
> > +		ret = usb_submit_urb(dev->interrupt, mem_flags);
> > +
> > +	dev_dbg(&dev->udev->dev, "incremented interrupt URB count to %d\n",
> > +		dev->interrupt_count);
> > +	mutex_unlock(&dev->interrupt_mutex);
> > +	return ret;
> > +}
> > +
> > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> > +{
> > +	/* Only drivers that implement a status hook should call this */
> > +	BUG_ON(dev->interrupt == NULL);
> > +
> > +	if (test_bit(EVENT_DEV_ASLEEP, &dev->flags))
> > +		return -EINVAL;
> 
> This looks like a race condition.

True, I'll have to fix this.  But it looks like EVENT_DEV_ASLEEP is
protected by *either* rxq.lock (rx_submit) or txq.lock
(usbnet_start_xmit, usbnet_suspend, usbnet_resume).  That doesn't seem
right, actually...  shouldn't it be protected all by one lock, not two
different ones?

> > +	return __usbnet_status_start(dev, mem_flags, false);
> > +}
> > +EXPORT_SYMBOL_GPL(usbnet_status_start);
> > +
> > +/* Kill the interrupt URB if all submitters want it killed */
> > +static void __usbnet_status_stop(struct usbnet *dev, bool force)
> > +{
> > +	if (dev->interrupt) {
> > +		mutex_lock(&dev->interrupt_mutex);
> > +		if (!force)
> > +			BUG_ON(dev->interrupt_count == 0);
> > +
> > +		if (force || --dev->interrupt_count == 0)
> > +			usb_kill_urb(dev->interrupt);
> 
> Why so complicated? If it may be on, kill it unconditionally.

This function isn't only called from suspend.  It's also called if the
sub-driver doesn't need the interrupt urb open anymore, because earlier
you indicated that we didn't want to unconditionally keep the URB open
if something didn't need it, because it's wasteful of resources.

So for example, sierra_net will call usbnet_status_start() at driver
init time, and then it could call usbnet_status_stop() when it has
received the RESTART indication about 2 seconds after driver init, all
before the interface is IFF_UP and before usbnet would ever have
submitted the URB.  However, if during that 2 seconds, somethign *does*
set the interface IFF_UP, you don't want sierra_net causing the urb to
be killed right underneath usbnet.  Hence the refcounting scheme here.

force is used only for suspend/resume specifically to ensure that the
URB is unconditionally killed at suspend time.

Dan

> > +
> > +		dev_dbg(&dev->udev->dev,
> > +			"decremented interrupt URB count to %d\n",
> > +			dev->interrupt_count);
> > +		mutex_unlock(&dev->interrupt_mutex);
> > +	}
> > +}
> > +
> 
> 	Regards
> 		Oliver
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum - April 10, 2013, 1:06 p.m.
On Wednesday 10 April 2013 07:49:11 Dan Williams wrote:
> On Wed, 2013-04-10 at 09:23 +0200, Oliver Neukum wrote:
> > On Tuesday 09 April 2013 18:02:27 Dan Williams wrote:

> > > +/* Submit the interrupt URB if it hasn't been submitted yet */
> > > +static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
> > > +					bool force)
> > > +{
> > > +	int ret = 0;
> > > +	bool submit = false;
> > > +
> > > +	if (!dev->interrupt)
> > > +		return 0;
> > > +
> > > +	mutex_lock(&dev->interrupt_mutex);
> > > +
> > > +	if (force) {
> > 
> > That design means that interrupt_count isn't accurate if force is used.
> > That is extremely ugly.
> 
> True; the problem here is that the URB isn't always submitted when
> suspend is used.  For example, in a normal driver that doesn't need the
> URB submitted all the time, interrupt_count will be 0 while !IFF_UP.
> Then if the system suspends, we can't decrement interrupt_count because
> it's zero.

We don't need to. You ought to understand interrupt_count as
valid only while the device is not suspended.

> Besides, if the system is suspended, no driver can call
> usbnet_interrupt_start() or usbnet_interrupt_stop(), correct?  Suspend
> is a special condition here and nothing that starts/stops the urbs will
> ever run while the system is suspended.

Unfortunately there's also runtime power management.

> > > +		/* Only submit now if the URB was previously submitted */
> > > +		if (dev->interrupt_count)
> > > +			submit = true;
> > > +	} else if (++dev->interrupt_count == 1)
> > > +		submit = true;
> > > +
> > > +	if (submit)
> > > +		ret = usb_submit_urb(dev->interrupt, mem_flags);
> > > +
> > > +	dev_dbg(&dev->udev->dev, "incremented interrupt URB count to %d\n",
> > > +		dev->interrupt_count);
> > > +	mutex_unlock(&dev->interrupt_mutex);
> > > +	return ret;
> > > +}
> > > +
> > > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> > > +{
> > > +	/* Only drivers that implement a status hook should call this */
> > > +	BUG_ON(dev->interrupt == NULL);
> > > +
> > > +	if (test_bit(EVENT_DEV_ASLEEP, &dev->flags))
> > > +		return -EINVAL;
> > 
> > This looks like a race condition.
> 
> True, I'll have to fix this.  But it looks like EVENT_DEV_ASLEEP is
> protected by *either* rxq.lock (rx_submit) or txq.lock
> (usbnet_start_xmit, usbnet_suspend, usbnet_resume).  That doesn't seem
> right, actually...  shouldn't it be protected all by one lock, not two
> different ones?

Yes.

> > > +	return __usbnet_status_start(dev, mem_flags, false);
> > > +}
> > > +EXPORT_SYMBOL_GPL(usbnet_status_start);
> > > +
> > > +/* Kill the interrupt URB if all submitters want it killed */
> > > +static void __usbnet_status_stop(struct usbnet *dev, bool force)
> > > +{
> > > +	if (dev->interrupt) {
> > > +		mutex_lock(&dev->interrupt_mutex);
> > > +		if (!force)
> > > +			BUG_ON(dev->interrupt_count == 0);

BTW: please unify this in case somebody compiles out BUG_ON

> > > +
> > > +		if (force || --dev->interrupt_count == 0)
> > > +			usb_kill_urb(dev->interrupt);
> > 
> > Why so complicated? If it may be on, kill it unconditionally.
> 
> This function isn't only called from suspend.  It's also called if the
> sub-driver doesn't need the interrupt urb open anymore, because earlier
> you indicated that we didn't want to unconditionally keep the URB open
> if something didn't need it, because it's wasteful of resources.
> 
> So for example, sierra_net will call usbnet_status_start() at driver
> init time, and then it could call usbnet_status_stop() when it has
> received the RESTART indication about 2 seconds after driver init, all
> before the interface is IFF_UP and before usbnet would ever have
> submitted the URB.  However, if during that 2 seconds, somethign *does*
> set the interface IFF_UP, you don't want sierra_net causing the urb to
> be killed right underneath usbnet.  Hence the refcounting scheme here.
> 
> force is used only for suspend/resume specifically to ensure that the
> URB is unconditionally killed at suspend time.

It is likely to be more elegant to drop force and have an unconditional kill
in suspend.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - April 10, 2013, 1:18 p.m.
On Wed, 2013-04-10 at 15:06 +0200, Oliver Neukum wrote:
> On Wednesday 10 April 2013 07:49:11 Dan Williams wrote:
> > On Wed, 2013-04-10 at 09:23 +0200, Oliver Neukum wrote:
> > > On Tuesday 09 April 2013 18:02:27 Dan Williams wrote:
> 
> > > > +/* Submit the interrupt URB if it hasn't been submitted yet */
> > > > +static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
> > > > +					bool force)
> > > > +{
> > > > +	int ret = 0;
> > > > +	bool submit = false;
> > > > +
> > > > +	if (!dev->interrupt)
> > > > +		return 0;
> > > > +
> > > > +	mutex_lock(&dev->interrupt_mutex);
> > > > +
> > > > +	if (force) {
> > > 
> > > That design means that interrupt_count isn't accurate if force is used.
> > > That is extremely ugly.
> > 
> > True; the problem here is that the URB isn't always submitted when
> > suspend is used.  For example, in a normal driver that doesn't need the
> > URB submitted all the time, interrupt_count will be 0 while !IFF_UP.
> > Then if the system suspends, we can't decrement interrupt_count because
> > it's zero.
> 
> We don't need to. You ought to understand interrupt_count as
> valid only while the device is not suspended.

Ok, so at suspend we just drop the count to zero, force-kill the URB,
and then on resume it's not re-submitted again?  That seems odd, since
the usbnet driver handles submit/resubmit internally if the interface is
IFF_UP, but when the interface is !IFF_UP then sub-drivers would have to
track whether they submitted the urb or not, and then clear that on
suspend?  Having separate behavior for when the sub-driver starts the
URB and when usbnet does seems inconsistent and error-prone.

What approach would you suggest here?

> > Besides, if the system is suspended, no driver can call
> > usbnet_interrupt_start() or usbnet_interrupt_stop(), correct?  Suspend
> > is a special condition here and nothing that starts/stops the urbs will
> > ever run while the system is suspended.
> 
> Unfortunately there's also runtime power management.

Hmm, right.

> > > > +		/* Only submit now if the URB was previously submitted */
> > > > +		if (dev->interrupt_count)
> > > > +			submit = true;
> > > > +	} else if (++dev->interrupt_count == 1)
> > > > +		submit = true;
> > > > +
> > > > +	if (submit)
> > > > +		ret = usb_submit_urb(dev->interrupt, mem_flags);
> > > > +
> > > > +	dev_dbg(&dev->udev->dev, "incremented interrupt URB count to %d\n",
> > > > +		dev->interrupt_count);
> > > > +	mutex_unlock(&dev->interrupt_mutex);
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> > > > +{
> > > > +	/* Only drivers that implement a status hook should call this */
> > > > +	BUG_ON(dev->interrupt == NULL);
> > > > +
> > > > +	if (test_bit(EVENT_DEV_ASLEEP, &dev->flags))
> > > > +		return -EINVAL;
> > > 
> > > This looks like a race condition.
> > 
> > True, I'll have to fix this.  But it looks like EVENT_DEV_ASLEEP is
> > protected by *either* rxq.lock (rx_submit) or txq.lock
> > (usbnet_start_xmit, usbnet_suspend, usbnet_resume).  That doesn't seem
> > right, actually...  shouldn't it be protected all by one lock, not two
> > different ones?
> 
> Yes.
> 
> > > > +	return __usbnet_status_start(dev, mem_flags, false);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(usbnet_status_start);
> > > > +
> > > > +/* Kill the interrupt URB if all submitters want it killed */
> > > > +static void __usbnet_status_stop(struct usbnet *dev, bool force)
> > > > +{
> > > > +	if (dev->interrupt) {
> > > > +		mutex_lock(&dev->interrupt_mutex);
> > > > +		if (!force)
> > > > +			BUG_ON(dev->interrupt_count == 0);
> 
> BTW: please unify this in case somebody compiles out BUG_ON

Can do.

> > > > +
> > > > +		if (force || --dev->interrupt_count == 0)
> > > > +			usb_kill_urb(dev->interrupt);
> > > 
> > > Why so complicated? If it may be on, kill it unconditionally.
> > 
> > This function isn't only called from suspend.  It's also called if the
> > sub-driver doesn't need the interrupt urb open anymore, because earlier
> > you indicated that we didn't want to unconditionally keep the URB open
> > if something didn't need it, because it's wasteful of resources.
> > 
> > So for example, sierra_net will call usbnet_status_start() at driver
> > init time, and then it could call usbnet_status_stop() when it has
> > received the RESTART indication about 2 seconds after driver init, all
> > before the interface is IFF_UP and before usbnet would ever have
> > submitted the URB.  However, if during that 2 seconds, somethign *does*
> > set the interface IFF_UP, you don't want sierra_net causing the urb to
> > be killed right underneath usbnet.  Hence the refcounting scheme here.
> > 
> > force is used only for suspend/resume specifically to ensure that the
> > URB is unconditionally killed at suspend time.
> 
> It is likely to be more elegant to drop force and have an unconditional kill
> in suspend.

See my questions above.  Then we'd have to have the sub-drivers
implement suspend/resume hooks so they'd be able to resubmit the
interrupt URB on resume, and the whole point of this patch was to handle
all that in usbnet.  The sub-drivers don't know what the core driver's
suspend/resume count is, because dev->suspend_count isn't exposed to
subdrivers, and thus they don't know whether the device is actually
suspended or not.

The core problem is this... the sub-driver submits the URB before
IFF_UP, and then at IFF_UP time usbnet wants to submit the driver.
Let's say later the sub-driver doesn't need its private interrupt URB
submission anymore, but it can't kill the URB because usbnet has
submitted it too.  Hence the refcounting.

Dan

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork - April 10, 2013, 1:25 p.m.
Dan Williams <dcbw@redhat.com> writes:

> +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> +{
> +	/* Only drivers that implement a status hook should call this */
> +	BUG_ON(dev->interrupt == NULL);
> 
I still don't think there is any reason to BUG out. See for example
http://article.gmane.org/gmane.linux.kernel/52102

> +static void __usbnet_status_stop(struct usbnet *dev, bool force)
> +{
> +	if (dev->interrupt) {
> +		mutex_lock(&dev->interrupt_mutex);
> +		if (!force)
> +			BUG_ON(dev->interrupt_count == 0);

Same here.  You can deal with this just fine.  Warn once, and go on
ignoring the problem.  Why kill the machine because of some minor driver
issue?



Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum - April 10, 2013, 1:29 p.m.
On Wednesday 10 April 2013 08:18:57 Dan Williams wrote:
> On Wed, 2013-04-10 at 15:06 +0200, Oliver Neukum wrote:
> > On Wednesday 10 April 2013 07:49:11 Dan Williams wrote:
> > > On Wed, 2013-04-10 at 09:23 +0200, Oliver Neukum wrote:
> > > > On Tuesday 09 April 2013 18:02:27 Dan Williams wrote:
> > 
> > > > > +/* Submit the interrupt URB if it hasn't been submitted yet */
> > > > > +static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
> > > > > +					bool force)
> > > > > +{
> > > > > +	int ret = 0;
> > > > > +	bool submit = false;
> > > > > +
> > > > > +	if (!dev->interrupt)
> > > > > +		return 0;
> > > > > +
> > > > > +	mutex_lock(&dev->interrupt_mutex);
> > > > > +
> > > > > +	if (force) {
> > > > 
> > > > That design means that interrupt_count isn't accurate if force is used.
> > > > That is extremely ugly.
> > > 
> > > True; the problem here is that the URB isn't always submitted when
> > > suspend is used.  For example, in a normal driver that doesn't need the
> > > URB submitted all the time, interrupt_count will be 0 while !IFF_UP.
> > > Then if the system suspends, we can't decrement interrupt_count because
> > > it's zero.
> > 
> > We don't need to. You ought to understand interrupt_count as
> > valid only while the device is not suspended.
> 
> Ok, so at suspend we just drop the count to zero, force-kill the URB,

No, at suspend() ignore interrupt_count. Just kill.

> and then on resume it's not re-submitted again?  That seems odd, since

On resume() evaluate interrupt_count.

> the usbnet driver handles submit/resubmit internally if the interface is
> IFF_UP, but when the interface is !IFF_UP then sub-drivers would have to
> track whether they submitted the urb or not, and then clear that on
> suspend?  Having separate behavior for when the sub-driver starts the
> URB and when usbnet does seems inconsistent and error-prone.
> 
> What approach would you suggest here?

Religiously use interrupt_count. With one exception.
The start/stop helpers are good. Just don't use them at suspend().

[..]
> See my questions above.  Then we'd have to have the sub-drivers
> implement suspend/resume hooks so they'd be able to resubmit the
> interrupt URB on resume, and the whole point of this patch was to handle
> all that in usbnet.  The sub-drivers don't know what the core driver's
> suspend/resume count is, because dev->suspend_count isn't exposed to
> subdrivers, and thus they don't know whether the device is actually
> suspended or not.
> 
> The core problem is this... the sub-driver submits the URB before
> IFF_UP, and then at IFF_UP time usbnet wants to submit the driver.
> Let's say later the sub-driver doesn't need its private interrupt URB
> submission anymore, but it can't kill the URB because usbnet has
> submitted it too.  Hence the refcounting.

The refcounting is very good. Just don't mess around with "force"

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum - April 10, 2013, 1:31 p.m.
On Wednesday 10 April 2013 15:25:49 Bjørn Mork wrote:
> Dan Williams <dcbw@redhat.com> writes:
> 
> > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> > +{
> > +	/* Only drivers that implement a status hook should call this */
> > +	BUG_ON(dev->interrupt == NULL);
> > 
> I still don't think there is any reason to BUG out. See for example
> http://article.gmane.org/gmane.linux.kernel/52102

On second consideration, yes, WARN_ON() would do the job.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - April 10, 2013, 1:54 p.m.
On Wed, 2013-04-10 at 15:29 +0200, Oliver Neukum wrote:
> On Wednesday 10 April 2013 08:18:57 Dan Williams wrote:
> > On Wed, 2013-04-10 at 15:06 +0200, Oliver Neukum wrote:
> > > On Wednesday 10 April 2013 07:49:11 Dan Williams wrote:
> > > > On Wed, 2013-04-10 at 09:23 +0200, Oliver Neukum wrote:
> > > > > On Tuesday 09 April 2013 18:02:27 Dan Williams wrote:
> > > 
> > > > > > +/* Submit the interrupt URB if it hasn't been submitted yet */
> > > > > > +static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
> > > > > > +					bool force)
> > > > > > +{
> > > > > > +	int ret = 0;
> > > > > > +	bool submit = false;
> > > > > > +
> > > > > > +	if (!dev->interrupt)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	mutex_lock(&dev->interrupt_mutex);
> > > > > > +
> > > > > > +	if (force) {
> > > > > 
> > > > > That design means that interrupt_count isn't accurate if force is used.
> > > > > That is extremely ugly.
> > > > 
> > > > True; the problem here is that the URB isn't always submitted when
> > > > suspend is used.  For example, in a normal driver that doesn't need the
> > > > URB submitted all the time, interrupt_count will be 0 while !IFF_UP.
> > > > Then if the system suspends, we can't decrement interrupt_count because
> > > > it's zero.
> > > 
> > > We don't need to. You ought to understand interrupt_count as
> > > valid only while the device is not suspended.
> > 
> > Ok, so at suspend we just drop the count to zero, force-kill the URB,
> 
> No, at suspend() ignore interrupt_count. Just kill.

Isn't that what the code already does?  The suspend handler sets force
to true, which always kills the URB at suspend time.

> > and then on resume it's not re-submitted again?  That seems odd, since
> 
> On resume() evaluate interrupt_count.

Because suspend/resume doesn't touch interrupt_count (due to the problem
that interrupt_count may be 0 at suspend time if the URB is not yet
submitted), we need a flag to know whether or not to increment the
count, and that's what force is there to do.

> > the usbnet driver handles submit/resubmit internally if the interface is
> > IFF_UP, but when the interface is !IFF_UP then sub-drivers would have to
> > track whether they submitted the urb or not, and then clear that on
> > suspend?  Having separate behavior for when the sub-driver starts the
> > URB and when usbnet does seems inconsistent and error-prone.
> > 
> > What approach would you suggest here?
> 
> Religiously use interrupt_count. With one exception.
> The start/stop helpers are good. Just don't use them at suspend().

So open-code the killing at suspend()?  That's what I had in a previous
patch, but Ming suggested I use the helpers instead to make things
cleaner.  So I did.  Should I revert to the old behavior?

> [..]
> > See my questions above.  Then we'd have to have the sub-drivers
> > implement suspend/resume hooks so they'd be able to resubmit the
> > interrupt URB on resume, and the whole point of this patch was to handle
> > all that in usbnet.  The sub-drivers don't know what the core driver's
> > suspend/resume count is, because dev->suspend_count isn't exposed to
> > subdrivers, and thus they don't know whether the device is actually
> > suspended or not.
> > 
> > The core problem is this... the sub-driver submits the URB before
> > IFF_UP, and then at IFF_UP time usbnet wants to submit the driver.
> > Let's say later the sub-driver doesn't need its private interrupt URB
> > submission anymore, but it can't kill the URB because usbnet has
> > submitted it too.  Hence the refcounting.
> 
> The refcounting is very good. Just don't mess around with "force"

That's easy to do if the helpers aren't used for suspend/resume, which
is what I had previously in my v2 patches until Ming suggested that I
use the helpers there.  I can go back to that approach if you'd like, it
is a bit less complicated at the expense of sprinkling the interrupt urb
submit/kill code around more widely.

Dan

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum - April 10, 2013, 1:58 p.m.
On Wednesday 10 April 2013 08:54:43 Dan Williams wrote:
> > The refcounting is very good. Just don't mess around with "force"
> 
> That's easy to do if the helpers aren't used for suspend/resume, which
> is what I had previously in my v2 patches until Ming suggested that I
> use the helpers there.  I can go back to that approach if you'd like, it
> is a bit less complicated at the expense of sprinkling the interrupt urb
> submit/kill code around more widely.

If you introduce a third helper like "forcibly_stop_interrupt" or something,
I'll be perfectly happy. Just don't use flags which completely alter behavior.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork - April 10, 2013, 3:21 p.m.
Dan Williams <dcbw@redhat.com> writes:
> On Wed, 2013-04-10 at 15:25 +0200, Bjørn Mork wrote:
>> Dan Williams <dcbw@redhat.com> writes:
>> 
>> > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
>> > +{
>> > +	/* Only drivers that implement a status hook should call this */
>> > +	BUG_ON(dev->interrupt == NULL);
>> > 
>> I still don't think there is any reason to BUG out. See for example
>> http://article.gmane.org/gmane.linux.kernel/52102
>> 
>> > +static void __usbnet_status_stop(struct usbnet *dev, bool force)
>> > +{
>> > +	if (dev->interrupt) {
>> > +		mutex_lock(&dev->interrupt_mutex);
>> > +		if (!force)
>> > +			BUG_ON(dev->interrupt_count == 0);
>> 
>> Same here.  You can deal with this just fine.  Warn once, and go on
>> ignoring the problem.  Why kill the machine because of some minor driver
>> issue?
>
> Actually in the stop case, no, we can't deal with it, because then (due
> to the buggy sub-driver) we'd go on to decrement 0 into UINT_MAX.  It
> really is a bug if, *not* at suspend time, we're told to stop the
> interrupt URB when it's not yet submitted. 

Sure it is a bug.  All I'm saying is that you can deal with it.  Warn
about the bug and give up.  Or continue.  But don't roll over and die.
Let the user unload the buggy driver and email the author instead.

> I'm happy to add another
> if() here though, which would end up looking like this:
>
> if (dev->interrupt_count && --dev->interrupt_count == 0)
>     usb_kill_urb(dev->interrupt);
>
> which seems odd, but fine.

Yes, there are too many decision factors, so it does look odd.  You
could also decide early that the bogus dev->interrupt_count means that
nothing needs to be done, because no interrupt URB was subitted.  Like

static void __usbnet_status_stop(struct usbnet *dev, bool force)
{
	if (dev->interrupt) {
		mutex_lock(&dev->interrupt_mutex);
                if (!force && dev->interrupt_count == 0) {
                     print loud warning blaming the minidriver author;
                     goto out;
                }
		if (force || --dev->interrupt_count == 0)
			usb_kill_urb(dev->interrupt);

		dev_dbg(&dev->udev->dev,
			"decremented interrupt URB count to %d\n",
			dev->interrupt_count);
out:
		mutex_unlock(&dev->interrupt_mutex);
	}
}


But it is pretty much the same.  "force" makes a mess of it.

In any case, I believe any of those solutions are a lot nicer to any
unsuspecting user, who may not agree with you that a failing 3G modem is
important enough to kill the machine.


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 51f3192..e8b363a 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -42,7 +42,7 @@ 
 #include <linux/workqueue.h>
 #include <linux/mii.h>
 #include <linux/usb.h>
-#include <linux/usb/usbnet.h>
+#include "usbnet.h"
 #include <linux/slab.h>
 #include <linux/kernel.h>
 #include <linux/pm_runtime.h>
@@ -252,6 +252,70 @@  static int init_status (struct usbnet *dev, struct usb_interface *intf)
 	return 0;
 }
 
+/* Submit the interrupt URB if it hasn't been submitted yet */
+static int __usbnet_status_start(struct usbnet *dev, gfp_t mem_flags,
+					bool force)
+{
+	int ret = 0;
+	bool submit = false;
+
+	if (!dev->interrupt)
+		return 0;
+
+	mutex_lock(&dev->interrupt_mutex);
+
+	if (force) {
+		/* Only submit now if the URB was previously submitted */
+		if (dev->interrupt_count)
+			submit = true;
+	} else if (++dev->interrupt_count == 1)
+		submit = true;
+
+	if (submit)
+		ret = usb_submit_urb(dev->interrupt, mem_flags);
+
+	dev_dbg(&dev->udev->dev, "incremented interrupt URB count to %d\n",
+		dev->interrupt_count);
+	mutex_unlock(&dev->interrupt_mutex);
+	return ret;
+}
+
+int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
+{
+	/* Only drivers that implement a status hook should call this */
+	BUG_ON(dev->interrupt == NULL);
+
+	if (test_bit(EVENT_DEV_ASLEEP, &dev->flags))
+		return -EINVAL;
+
+	return __usbnet_status_start(dev, mem_flags, false);
+}
+EXPORT_SYMBOL_GPL(usbnet_status_start);
+
+/* Kill the interrupt URB if all submitters want it killed */
+static void __usbnet_status_stop(struct usbnet *dev, bool force)
+{
+	if (dev->interrupt) {
+		mutex_lock(&dev->interrupt_mutex);
+		if (!force)
+			BUG_ON(dev->interrupt_count == 0);
+
+		if (force || --dev->interrupt_count == 0)
+			usb_kill_urb(dev->interrupt);
+
+		dev_dbg(&dev->udev->dev,
+			"decremented interrupt URB count to %d\n",
+			dev->interrupt_count);
+		mutex_unlock(&dev->interrupt_mutex);
+	}
+}
+
+void usbnet_status_stop(struct usbnet *dev)
+{
+	__usbnet_status_stop(dev, false);
+}
+EXPORT_SYMBOL_GPL(usbnet_status_stop);
+
 /* Passes this packet up the stack, updating its accounting.
  * Some link protocols batch packets, so their rx_fixup paths
  * can return clones as well as just modify the original skb.
@@ -725,7 +789,7 @@  int usbnet_stop (struct net_device *net)
 	if (!(info->flags & FLAG_AVOID_UNLINK_URBS))
 		usbnet_terminate_urbs(dev);
 
-	usb_kill_urb(dev->interrupt);
+	usbnet_status_stop(dev);
 
 	usbnet_purge_paused_rxq(dev);
 
@@ -787,7 +851,7 @@  int usbnet_open (struct net_device *net)
 
 	/* start any status interrupt transfer */
 	if (dev->interrupt) {
-		retval = usb_submit_urb (dev->interrupt, GFP_KERNEL);
+		retval = usbnet_status_start(dev, GFP_KERNEL);
 		if (retval < 0) {
 			netif_err(dev, ifup, dev->net,
 				  "intr submit %d\n", retval);
@@ -1430,6 +1494,8 @@  usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 	dev->delay.data = (unsigned long) dev;
 	init_timer (&dev->delay);
 	mutex_init (&dev->phy_mutex);
+	mutex_init(&dev->interrupt_mutex);
+	dev->interrupt_count = 0;
 
 	dev->net = net;
 	strcpy (net->name, "usb%d");
@@ -1565,7 +1631,7 @@  int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
 		 */
 		netif_device_detach (dev->net);
 		usbnet_terminate_urbs(dev);
-		usb_kill_urb(dev->interrupt);
+		__usbnet_status_stop(dev, true);
 
 		/*
 		 * reattach so runtime management can use and
@@ -1585,9 +1651,8 @@  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);
+		/* resume interrupt URBs if they were submitted at suspend */
+		__usbnet_status_start(dev, GFP_NOIO, true);
 
 		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 0e5ac93..d71f44c 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -56,6 +56,8 @@  struct usbnet {
 	struct sk_buff_head	done;
 	struct sk_buff_head	rxq_pause;
 	struct urb		*interrupt;
+	unsigned		interrupt_count;
+	struct mutex            interrupt_mutex;
 	struct usb_anchor	deferred;
 	struct tasklet_struct	bh;
 
@@ -246,4 +248,7 @@  extern int usbnet_nway_reset(struct net_device *net);
 
 extern int usbnet_manage_power(struct usbnet *, int);
 
+int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags);
+void usbnet_status_stop(struct usbnet *dev);
+
 #endif /* __LINUX_USB_USBNET_H */