From: Dan Williams <dcbw@redhat•com>
To: Ming Lei <tom.leiming@gmail•com>
Cc: Oliver Neukum <oliver@neukum•org>,
Elina Pasheva <epasheva@sierrawireless•com>,
Network Development <netdev@vger•kernel.org>,
linux-usb <linux-usb@vger•kernel.org>,
Rory Filer <rfiler@sierrawireless•com>, Phil Sutter <phil@nwl•cc>
Subject: Re: [PATCH 1/2 v5] usbnet: allow status interrupt URB to always be active
Date: Thu, 11 Apr 2013 10:28:33 -0500 [thread overview]
Message-ID: <1365694113.22668.4.camel@dcbw.foobar.com> (raw)
In-Reply-To: <CACVXFVNKSszw+3VnM3hwepbTHh3gvjC+pG2SL_OsVm+1SqrNzQ@mail.gmail.com>
On Thu, 2013-04-11 at 10:31 +0800, Ming Lei wrote:
> On Thu, Apr 11, 2013 at 4:30 AM, Dan Williams <dcbw@redhat•com> 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..b71ce36 100644
> > --- a/drivers/net/usb/usbnet.c
> > +++ b/drivers/net/usb/usbnet.c
> > @@ -252,6 +252,70 @@ static int init_status (struct usbnet *dev, struct usb_interface *intf)
> > return 0;
> > }
> >
> > +/* Submit the interrupt URB if not previously submitted, increasing refcount */
> > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
>
> 'mem_flags' isn't needed any more since we can apply allocation
> of GFP_NOIO automatically in resume path now, and you can always
> use GFP_KERNEL safely. Considered that it is a API, please don't
> introduce it.
>
> After removing it, you can add
>
> Acked-by: Ming Lei <ming.lei@canonical•com>
>
> > +{
> > + int ret = 0;
> > +
> > + WARN_ON_ONCE(dev->interrupt == NULL);
> > + if (dev->interrupt) {
> > + mutex_lock(&dev->interrupt_mutex);
> > +
> > + if (++dev->interrupt_count == 1)
> > + 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;
> > +}
> > +EXPORT_SYMBOL_GPL(usbnet_status_start);
> > +
> > +/* For resume; submit interrupt URB if previously submitted */
> > +static int __usbnet_status_start_force(struct usbnet *dev, gfp_t mem_flags)
> > +{
> > + int ret = 0;
> > +
> > + mutex_lock(&dev->interrupt_mutex);
> > + if (dev->interrupt_count) {
> > + ret = usb_submit_urb(dev->interrupt, mem_flags);
> > + dev_dbg(&dev->udev->dev,
> > + "submitted interrupt URB for resume\n");
> > + }
> > + mutex_unlock(&dev->interrupt_mutex);
> > + return ret;
> > +}
> > +
> > +/* Kill the interrupt URB if all submitters want it killed */
> > +void usbnet_status_stop(struct usbnet *dev)
> > +{
> > + if (dev->interrupt) {
> > + mutex_lock(&dev->interrupt_mutex);
> > + WARN_ON(dev->interrupt_count == 0);
> > +
> > + if (dev->interrupt_count && --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);
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(usbnet_status_stop);
> > +
> > +/* For suspend; always kill interrupt URB */
> > +static void __usbnet_status_stop_force(struct usbnet *dev)
> > +{
> > + if (dev->interrupt) {
> > + mutex_lock(&dev->interrupt_mutex);
> > + usb_kill_urb(dev->interrupt);
> > + dev_dbg(&dev->udev->dev, "killed interrupt URB for suspend\n");
> > + mutex_unlock(&dev->interrupt_mutex);
> > + }
> > +}
>
> Looks it isn't a good practice to duplicate code in above four functions, but
> it should be OK to merge first.
Oliver requested this approach. I'd originally taken your suggestion to
merge them, but this proved somewhat more complicated and Oliver
rejected that approach.
Dan
> > +
> > /* 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_force(dev);
> >
> > /*
> > * 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 URB if it was previously submitted */
> > + __usbnet_status_start_force(dev, 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 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 */
> > --
> > 1.8.1.4
> >
> >
>
>
> Thanks,
next prev parent reply other threads:[~2013-04-11 15:27 UTC|newest]
Thread overview: 89+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-07-27 14:12 strange behaviour of MC7700 with sierra_net Phil Sutter
[not found] ` <20110727141246.GC29616-2COwY06ShZsYt6otZODPyA@public.gmane.org>
2011-07-27 19:40 ` Dan Williams
[not found] ` <1311795643.17655.13.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
2011-07-28 16:32 ` Phil Sutter
[not found] ` <8F90F944E50427428C60E12A34A309D23424BB0F25@carmd-exchmb01.sierrawireless.local>
[not found] ` <8F90F944E50427428C60E12A34A309D23424BB0F25-3qf8vkpM5jTbmvMHnzRVpW3Pdy6AhKVLXbPIYa3/oNjDOqzlkpFKJg@public.gmane.org>
2011-08-04 16:38 ` Phil Sutter
2013-01-04 16:48 ` [PATCH 1/2] usbnet: allow status interrupt URB to always be active Dan Williams
2013-01-04 16:51 ` [PATCH 2/2] sierra_net: keep status interrupt URB active over netdev open/close Dan Williams
[not found] ` <1357318289.5370.19.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
2013-02-06 18:42 ` [PATCH 2/2 v2] sierra_net: fix issues with SYNC/RESTART messages and interrupt pipe setup Dan Williams
[not found] ` <1360176176.11742.16.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
2013-02-06 20:17 ` Oliver Neukum
2013-02-07 21:09 ` Dan Williams
2013-02-06 21:11 ` Bjørn Mork
2013-02-07 17:06 ` Dan Williams
2013-02-13 11:44 ` Bjørn Mork
2013-02-13 16:34 ` Dan Williams
2013-01-04 22:16 ` [PATCH 1/2] usbnet: allow status interrupt URB to always be active Oliver Neukum
2013-01-05 1:26 ` Dan Williams
2013-01-05 11:01 ` Oliver Neukum
2013-01-05 11:44 ` Bjørn Mork
2013-01-07 15:25 ` Dan Williams
2013-01-14 17:52 ` Dan Williams
[not found] ` <1357349193.19684.3.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
2013-01-05 10:59 ` Bjørn Mork
[not found] ` <87y5g7nbej.fsf-lbf33ChDnrE/G1V5fR+Y7Q@public.gmane.org>
2013-01-07 7:40 ` Oliver Neukum
2013-01-07 15:21 ` Dan Williams
2013-01-11 3:06 ` Ming Lei
2013-01-14 17:23 ` Dan Williams
2013-01-15 1:13 ` Ming Lei
2013-02-06 18:36 ` [PATCH 1/2 v2] " Dan Williams
2013-02-06 20:19 ` Oliver Neukum
2013-03-28 16:30 ` [PATCH 1/2 v3] " Dan Williams
[not found] ` <1364488207.1877.20.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
2013-03-28 16:32 ` [PATCH 2/2 v3] sierra_net: keep status interrupt URB active Dan Williams
[not found] ` <1364488327.1877.22.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
2013-03-29 19:21 ` David Miller
2013-03-29 19:20 ` [PATCH 1/2 v3] usbnet: allow status interrupt URB to always be active David Miller
[not found] ` <CACVXFVMBAzTYZKiE_uSTqr_yB4f7c5_PSnK=LBP6=oWdWwYHfg@mail.gmail.com>
[not found] ` <CACVXFVMBAzTYZKiE_uSTqr_yB4f7c5_PSnK=LBP6=oWdWwYHfg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-04-01 16:04 ` Dan Williams
2013-04-09 23:02 ` [PATCH 1/2 v4] " Dan Williams
2013-04-09 23:05 ` [PATCH 2/2 v4] sierra_net: keep status interrupt URB active Dan Williams
2013-04-10 7:15 ` Oliver Neukum
2013-04-10 14:57 ` Dan Williams
2013-04-10 15:01 ` Oliver Neukum
2013-04-10 7:23 ` [PATCH 1/2 v4] usbnet: allow status interrupt URB to always be active Oliver Neukum
[not found] ` <2328167.HJRXSHNhKT-7ztolUikljGernLeA6q8OA@public.gmane.org>
2013-04-10 12:49 ` Dan Williams
2013-04-10 13:06 ` Oliver Neukum
2013-04-10 13:18 ` Dan Williams
2013-04-10 13:29 ` Oliver Neukum
[not found] ` <1542762.9EJvNHlBNo-7ztolUikljGernLeA6q8OA@public.gmane.org>
2013-04-10 13:54 ` Dan Williams
[not found] ` <1365602083.28888.40.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
2013-04-10 13:58 ` Oliver Neukum
2013-04-10 15:01 ` Dan Williams
2013-04-10 18:10 ` Oliver Neukum
2013-04-10 20:30 ` [PATCH 1/2 v5] " Dan Williams
[not found] ` <1365625850.22411.1.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
2013-04-10 20:33 ` [PATCH 2/2 v5] sierra_net: keep status interrupt URB active Dan Williams
2013-04-11 2:31 ` [PATCH 1/2 v5] usbnet: allow status interrupt URB to always be active Ming Lei
2013-04-11 6:50 ` Oliver Neukum
2013-04-11 8:06 ` Bjørn Mork
2013-04-11 8:37 ` Ming Lei
2013-04-11 9:59 ` Oliver Neukum
2013-04-11 10:04 ` Bjørn Mork
2013-04-11 10:09 ` Ming Lei
2013-04-11 10:19 ` Ming Lei
[not found] ` <CACVXFVNn9MQr6JLdin=u642Jb-2ZPfVk8YaBmNdhU0_2e7WJqQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-04-11 10:26 ` Bjørn Mork
2013-04-11 10:30 ` Ming Lei
2013-04-11 11:08 ` Bjørn Mork
[not found] ` <87d2u1wci6.fsf-lbf33ChDnrE/G1V5fR+Y7Q@public.gmane.org>
2013-04-11 11:42 ` Ming Lei
[not found] ` <CACVXFVO13tG606nDHth5rEA9jexj1iFHnry5ktrMpm_aGGTSvw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-04-11 11:54 ` Oliver Neukum
2013-04-11 12:16 ` Ming Lei
2013-04-11 8:09 ` Ming Lei
2013-04-11 9:53 ` Oliver Neukum
2013-04-11 10:03 ` Ming Lei
2013-04-11 11:14 ` Oliver Neukum
2013-04-11 12:11 ` Ming Lei
2013-04-11 12:28 ` Oliver Neukum
2013-04-11 12:59 ` Ming Lei
[not found] ` <CACVXFVPPzva+EwjNdxWsg4FufCc0zzzzvhGH_nLv479vT8VcxQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-04-12 6:37 ` Oliver Neukum
2013-04-12 9:42 ` Ming Lei
2013-04-12 10:02 ` Oliver Neukum
2013-04-12 10:08 ` Ming Lei
2013-04-12 10:13 ` Bjørn Mork
2013-04-11 15:28 ` Dan Williams [this message]
2013-04-12 7:28 ` Oliver Neukum
2013-04-10 21:35 ` Oliver Neukum
2013-04-15 15:59 ` Dan Williams
2013-04-16 1:15 ` Ming Lei
2013-04-16 7:57 ` Oliver Neukum
[not found] ` <1637650.tuYfvStuVJ-7ztolUikljGernLeA6q8OA@public.gmane.org>
2013-04-17 1:56 ` Ming Lei
2013-04-17 6:55 ` Oliver Neukum
[not found] ` <5581442.KzM2iaDSQ0-7ztolUikljGernLeA6q8OA@public.gmane.org>
2013-04-18 3:51 ` Ming Lei
2013-04-19 8:25 ` Oliver Neukum
2013-04-24 12:31 ` Dan Williams
2013-04-25 13:00 ` Oliver Neukum
[not found] ` <1365548547.23372.2.camel-wKZy7rqYPVb5EHUCmHmTqw@public.gmane.org>
2013-04-10 13:25 ` [PATCH 1/2 v4] " Bjørn Mork
2013-04-10 13:31 ` Oliver Neukum
[not found] ` <1365604263.28888.56.camel@dcbw.foobar.com>
2013-04-10 15:21 ` Bjørn Mork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1365694113.22668.4.camel@dcbw.foobar.com \
--to=dcbw@redhat$(echo .)com \
--cc=epasheva@sierrawireless$(echo .)com \
--cc=linux-usb@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=oliver@neukum$(echo .)org \
--cc=phil@nwl$(echo .)cc \
--cc=rfiler@sierrawireless$(echo .)com \
--cc=tom.leiming@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox