| Message ID | 1365548547.23372.2.camel@dcbw.foobar.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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 */
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(-)