strange behaviour of MC7700 with sierra_net

12 messages, 4 authors, 2013-03-28 · open the first message on its own page

strange behaviour of MC7700 with sierra_net

From: Phil Sutter <hidden>
Date: 2011-07-27 14:12:46

Hello everyone,

I am testing the above module on linux-2.6.39.2 which should contain the
latest changes to at least sierra_net.c. Although I am able to bring the
module up and then can get traffic through it, initialisation seems to
be a little picky.

After connecting the module via USB, I get the following final
initialisation message:
| Jul 27 14:01:15 (none) user.info kernel: [166371.982356] sierra_net 1-1:1.7: wwan0: register 'sierra_net' at usb-0000:00:08.2-1, Sierra Wireless USB-to-WWAN Modem, 4a:82:22:b9:05:07

Doing nothing (successfully), the driver starts printing errors after a
while:
| Jul 27 14:02:15 (none) user.err kernel: [166432.201461] sierra_net 1-1:1.7: wwan0: Submit SYNC failed -32
| Jul 27 14:02:15 (none) user.err kernel: [166432.201609] sierra_net 1-1:1.7: wwan0: Send SYNC failed, status -32
| Jul 27 14:02:15 (none) user.err kernel: [166432.205446] sierra_net 1-1:1.7: wwan0: Submit SYNC failed -32
| Jul 27 14:02:15 (none) user.err kernel: [166432.205590] sierra_net 1-1:1.7: wwan0: Send SYNC failed, status -32

The above messages are the first ones to appear (so there's a delay of
60 seconds in between), and they repeat each 2 seconds from then on.
Depending on how I continue at that point, results vary:

a) 'ip link set wwan0 up': setting the interface up stops the above error
   messages from being printed. This worked every time I tried.

b) Sending 'ATZ' on the control-tty: this makes the error-messages
   disappear for about 6 seconds, so two iterations of the sync-timer
   seem to succeed. When I then try to continue initialisation, I usually
   get to 'AT+C' (for AT+CPIN='1234'), then the control-tty dies (neither
   echoing of typed characters, nor feedback from the modem printed).

Trying a) from the state after b) indeed makes the error-messages go
away, but the tty stays dead until I power-cycle the module. Needless to
say, without the control-tty the module is completely useless.

My first thought was that the driver shouldn't try to SYNC while the
interface being down, but apparently sierra_net_send_sync() doesn't get
called anymore after setting the interface up.

So in order to prevent the module from dieing, I need to up the
interface before initialisation via AT-interface.

Another interesting aspect: the above error stays gone after the
interface has been upped once, even if it's brought down right
afterwards without doing anything else. OK, not completely - there needs
to be a little delay in which the interface stays up, but a tenth of a
second was enough.

What else can I do to track this problem down further? What additional
information do you need from me? Any advice is highly appreciated, of
course!

Greetings, Phil
--
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

[PATCH 1/2] usbnet: allow status interrupt URB to always be active

From: Dan Williams <hidden>
Date: 2013-01-04 16:46:02

Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware.  Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close.  The URB is still
killed/re-submitted for suspend/resume, as before.

Signed-off-by: Dan Williams <redacted>
---
Oliver: alternatively, is there a problem with *always*
submitting the interrupt URB, and then simply not calling
the subdriver's .status function when the netdev is
closed?  That would be a much simpler patch.

 drivers/net/usb/usbnet.c   | 43 +++++++++++++++++++++++++++++++------------
 include/linux/usb/usbnet.h |  3 +++
 2 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3d4bf01..081b685 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -206,13 +206,13 @@ static void intr_complete (struct urb *urb)
 		break;
 	}
 
-	if (!netif_running (dev->net))
-		return;
-
-	status = usb_submit_urb (urb, GFP_ATOMIC);
-	if (status != 0)
-		netif_err(dev, timer, dev->net,
-			  "intr resubmit --> %d\n", status);
+	if (dev->driver_info->flags & FLAG_INTR_ALWAYS ||
+			netif_running(dev->net)) {
+		status = usb_submit_urb(urb, GFP_ATOMIC);
+		if (status != 0)
+			netif_err(dev, timer, dev->net,
+				  "intr resubmit --> %d\n", status);
+	}
 }
 
 static int init_status (struct usbnet *dev, struct usb_interface *intf)
@@ -708,7 +708,8 @@ int usbnet_stop (struct net_device *net)
 	if (!(info->flags & FLAG_AVOID_UNLINK_URBS))
 		usbnet_terminate_urbs(dev);
 
-	usb_kill_urb(dev->interrupt);
+	if (!(info->flags & FLAG_INTR_ALWAYS))
+		usb_kill_urb(dev->interrupt);
 
 	usbnet_purge_paused_rxq(dev);
 
@@ -769,7 +770,7 @@ int usbnet_open (struct net_device *net)
 	}
 
 	/* start any status interrupt transfer */
-	if (dev->interrupt) {
+	if (dev->interrupt && !(info->flags & FLAG_INTR_ALWAYS)) {
 		retval = usb_submit_urb (dev->interrupt, GFP_KERNEL);
 		if (retval < 0) {
 			netif_err(dev, ifup, dev->net,
@@ -1469,6 +1470,15 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 	if (status < 0)
 		goto out3;
 
+	/* Submit status interrupt URB immediately if sub-driver wants */
+	if (dev->interrupt && (info->flags & FLAG_INTR_ALWAYS)) {
+		status = usb_submit_urb(dev->interrupt, GFP_KERNEL);
+		if (status < 0) {
+			dev_err(&udev->dev, "intr submit %d\n", status);
+			goto out4;
+		}
+	}
+
 	if (!dev->rx_urb_size)
 		dev->rx_urb_size = dev->hard_mtu;
 	dev->maxpacket = usb_maxpacket (dev->udev, dev->out, 1);
@@ -1480,7 +1490,7 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 
 	status = register_netdev (net);
 	if (status)
-		goto out4;
+		goto out5;
 	netif_info(dev, probe, dev->net,
 		   "register '%s' at usb-%s-%s, %s, %pM\n",
 		   udev->dev.driver->name,
@@ -1498,6 +1508,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 
 	return 0;
 
+out5:
+	usb_kill_urb(dev->interrupt);
 out4:
 	usb_free_urb(dev->interrupt);
 out3:
@@ -1559,8 +1571,15 @@ int usbnet_resume (struct usb_interface *intf)
 
 	if (!--dev->suspend_count) {
 		/* resume interrupt URBs */
-		if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
-			usb_submit_urb(dev->interrupt, GFP_NOIO);
+		if (dev->interrupt &&
+			(dev->driver_info->flags & FLAG_INTR_ALWAYS ||
+				test_bit(EVENT_DEV_OPEN, &dev->flags))) {
+			retval = usb_submit_urb(dev->interrupt, GFP_NOIO);
+			if (retval < 0) {
+				netif_err(dev, ifup, dev->net,
+					  "intr submit %d\n", retval);
+			}
+		}
 
 		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 bd45eb7..8503920 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -108,6 +108,9 @@ struct driver_info {
 #define FLAG_MULTI_PACKET	0x2000
 #define FLAG_RX_ASSEMBLE	0x4000	/* rx packets may span >1 frames */
 
+/* Indicates that the interrupt URB should not depend on netdev open/close */
+#define FLAG_INTR_ALWAYS	0x8000
+
 	/* init device ... can sleep, or cause probe() failure */
 	int	(*bind)(struct usbnet *, struct usb_interface *);
 
-- 
1.7.11.7

[PATCH 2/2] sierra_net: keep status interrupt URB active over netdev open/close

From: Dan Williams <hidden>
Date: 2013-01-04 16:49:11

The driver and firmware sync up through SYNC messages, and the
firmware's affirmative reply to these SYNC messages appears to be the
"Reset" indication received via the status interrupt endpoint.  Thus the
driver needs the status interrupt endpoint always active so that the
Reset indication can be received even if the netdev is closed, which is
the case right after device insertion.

Signed-off-by: Dan Williams <redacted>
---
diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index 18dd425..e6e2857 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -905,7 +905,7 @@ static struct sk_buff *sierra_net_tx_fixup(struct usbnet *dev,
 
 static const struct driver_info sierra_net_info_direct_ip = {
 	.description = "Sierra Wireless USB-to-WWAN Modem",
-	.flags = FLAG_WWAN | FLAG_SEND_ZLP,
+	.flags = FLAG_WWAN | FLAG_SEND_ZLP | FLAG_INTR_ALWAYS,
 	.bind = sierra_net_bind,
 	.unbind = sierra_net_unbind,
 	.status = sierra_net_status,

Re: [PATCH 1/2] usbnet: allow status interrupt URB to always be active

From: Oliver Neukum <oliver@neukum.org>
Date: 2013-01-04 22:16:29

On Friday 04 January 2013 10:48:16 Dan Williams wrote:
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware.  Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close.  The URB is still
killed/re-submitted for suspend/resume, as before.

Signed-off-by: Dan Williams <redacted>
---
Oliver: alternatively, is there a problem with *always*
submitting the interrupt URB, and then simply not calling
the subdriver's .status function when the netdev is
closed?  That would be a much simpler patch.
That is quite radical. We have no idea what a device
does when we do not react to a status update. I would
much prefer to not take the risk.
Besides, we don't use bandwidth if we don't have to.

	Regards
		Oliver

Re: [PATCH 1/2] usbnet: allow status interrupt URB to always be active

From: Dan Williams <hidden>
Date: 2013-01-05 01:24:19

On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
On Friday 04 January 2013 10:48:16 Dan Williams wrote:
quoted
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware.  Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close.  The URB is still
killed/re-submitted for suspend/resume, as before.

Signed-off-by: Dan Williams <redacted>
---
Oliver: alternatively, is there a problem with *always*
submitting the interrupt URB, and then simply not calling
the subdriver's .status function when the netdev is
closed?  That would be a much simpler patch.
That is quite radical. We have no idea what a device
does when we do not react to a status update. I would
much prefer to not take the risk.
Besides, we don't use bandwidth if we don't have to.
Ok, so scratch the alternative.  Thus, does the posted patch look like
the right course of action?

If I wasn't clear enough before, sierra_net needs to listen to the
status interrupt URB to receive the custom Restart indication as part of
the driver's device setup.  Thus for sierra_net at least, tying the
status interrupt URB submission to device open/close isn't right.

I'd previously done a patch to handle this all in sierra_net, but the
problem there is suspend/resume: without directly accessing the usbnet
structure's ->suspend_count member (icky!) sierra_net can't correctly
kill/submit the URB itself.  So I went with a flag to usbnet that Sierra
can set.

Dan

Re: [PATCH 1/2] usbnet: allow status interrupt URB to always be active

From: Oliver Neukum <oliver@neukum.org>
Date: 2013-01-05 11:01:54

On Friday 04 January 2013 19:26:33 Dan Williams wrote:
On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
quoted
On Friday 04 January 2013 10:48:16 Dan Williams wrote:
quoted
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware.  Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close.  The URB is still
killed/re-submitted for suspend/resume, as before.

Signed-off-by: Dan Williams <redacted>
---
Oliver: alternatively, is there a problem with *always*
submitting the interrupt URB, and then simply not calling
the subdriver's .status function when the netdev is
closed?  That would be a much simpler patch.
That is quite radical. We have no idea what a device
does when we do not react to a status update. I would
much prefer to not take the risk.
Besides, we don't use bandwidth if we don't have to.
Ok, so scratch the alternative.  Thus, does the posted patch look like
the right course of action?
In principle yes.
If I wasn't clear enough before, sierra_net needs to listen to the
status interrupt URB to receive the custom Restart indication as part of
the driver's device setup.  Thus for sierra_net at least, tying the
status interrupt URB submission to device open/close isn't right.
So, there seems to be an inevitable race before probe() is called.
Have you looked at FLAG_AVOID_UNLINK_URBS ?
I'd previously done a patch to handle this all in sierra_net, but the
problem there is suspend/resume: without directly accessing the usbnet
structure's ->suspend_count member (icky!) sierra_net can't correctly
kill/submit the URB itself.  So I went with a flag to usbnet that Sierra
can set.
That is absolutely the right way to do it.

	Regards
		Oliver

Re: [PATCH 1/2] usbnet: allow status interrupt URB to always be active

From: Bjørn Mork <bjorn@mork.no>
Date: 2013-01-05 11:44:40

Oliver Neukum [off-list ref] writes:
On Friday 04 January 2013 19:26:33 Dan Williams wrote:
quoted
I'd previously done a patch to handle this all in sierra_net, but the
problem there is suspend/resume: without directly accessing the usbnet
structure's ->suspend_count member (icky!) sierra_net can't correctly
kill/submit the URB itself.  So I went with a flag to usbnet that Sierra
can set.
That is absolutely the right way to do it.
Yes.

Just a comment regarding the ->suspend_count: Are you absolutely sure
you need to look at that, Dan?  usbnet uses it to handle suspend/resume
for minidrivers with an unknown number of interfaces, without knowing
whether it is the control or data interface which is suspended or
resumed first.  By using the counter it can ensure that the correct
action is taken exactly once regardless of this.

The sierra_net minidriver has the advantage of knowing that there always
is only *one* USB interface being suspended and resumed.  So you don't
have to care about ->suspend_count.  Just do whatever you need to do on
suspend and resume.


Bjørn

Re: [PATCH 1/2] usbnet: allow status interrupt URB to always be active

From: Dan Williams <hidden>
Date: 2013-01-07 15:22:46

On Sat, 2013-01-05 at 12:01 +0100, Oliver Neukum wrote:
On Friday 04 January 2013 19:26:33 Dan Williams wrote:
quoted
On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
quoted
On Friday 04 January 2013 10:48:16 Dan Williams wrote:
quoted
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware.  Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close.  The URB is still
killed/re-submitted for suspend/resume, as before.

Signed-off-by: Dan Williams <redacted>
---
Oliver: alternatively, is there a problem with *always*
submitting the interrupt URB, and then simply not calling
the subdriver's .status function when the netdev is
closed?  That would be a much simpler patch.
That is quite radical. We have no idea what a device
does when we do not react to a status update. I would
much prefer to not take the risk.
Besides, we don't use bandwidth if we don't have to.
Ok, so scratch the alternative.  Thus, does the posted patch look like
the right course of action?
In principle yes.
quoted
If I wasn't clear enough before, sierra_net needs to listen to the
status interrupt URB to receive the custom Restart indication as part of
the driver's device setup.  Thus for sierra_net at least, tying the
status interrupt URB submission to device open/close isn't right.
So, there seems to be an inevitable race before probe() is called.
Yeah, you're right, we need Sierra to send the SYNC message *after* bind
is called.    Is there an existing "after bind" hook or do we need to
create one?
Have you looked at FLAG_AVOID_UNLINK_URBS ?
Not yet, but I will.

Thanks,
Dan
quoted
I'd previously done a patch to handle this all in sierra_net, but the
problem there is suspend/resume: without directly accessing the usbnet
structure's ->suspend_count member (icky!) sierra_net can't correctly
kill/submit the URB itself.  So I went with a flag to usbnet that Sierra
can set.
That is absolutely the right way to do it.

	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

Re: [PATCH 1/2] usbnet: allow status interrupt URB to always be active

From: Dan Williams <hidden>
Date: 2013-01-14 17:49:32

On Sat, 2013-01-05 at 12:01 +0100, Oliver Neukum wrote:
On Friday 04 January 2013 19:26:33 Dan Williams wrote:
quoted
On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
quoted
On Friday 04 January 2013 10:48:16 Dan Williams wrote:
quoted
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware.  Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close.  The URB is still
killed/re-submitted for suspend/resume, as before.

Signed-off-by: Dan Williams <redacted>
---
Oliver: alternatively, is there a problem with *always*
submitting the interrupt URB, and then simply not calling
the subdriver's .status function when the netdev is
closed?  That would be a much simpler patch.
That is quite radical. We have no idea what a device
does when we do not react to a status update. I would
much prefer to not take the risk.
Besides, we don't use bandwidth if we don't have to.
Ok, so scratch the alternative.  Thus, does the posted patch look like
the right course of action?
In principle yes.
quoted
If I wasn't clear enough before, sierra_net needs to listen to the
status interrupt URB to receive the custom Restart indication as part of
the driver's device setup.  Thus for sierra_net at least, tying the
status interrupt URB submission to device open/close isn't right.
So, there seems to be an inevitable race before probe() is called.
Have you looked at FLAG_AVOID_UNLINK_URBS ?
So that looks like it only applies to the bulk URBs, what was your
suggestion here?  Sierra would want the same behavior as it currently
has (kill data urbs on stop/start) but only the interrupt urb needs to
be kept alive over stop/start.

Dan

[PATCH 1/2 v2] usbnet: allow status interrupt URB to always be active

From: Dan Williams <hidden>
Date: 2013-02-06 18:36:27

Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware.  Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close.  The URB is still
killed/re-submitted for suspend/resume, as before.

Signed-off-by: Dan Williams <redacted>
---
Note: unchanged from previous version, but rebased.

 drivers/net/usb/usbnet.c   | 43 +++++++++++++++++++++++++++++++------------
 include/linux/usb/usbnet.h |  3 +++
 2 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 5e33606..5019cc7 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -206,13 +206,13 @@ static void intr_complete (struct urb *urb)
 		break;
 	}
 
-	if (!netif_running (dev->net))
-		return;
-
-	status = usb_submit_urb (urb, GFP_ATOMIC);
-	if (status != 0)
-		netif_err(dev, timer, dev->net,
-			  "intr resubmit --> %d\n", status);
+	if (dev->driver_info->flags & FLAG_INTR_ALWAYS ||
+			netif_running(dev->net)) {
+		status = usb_submit_urb(urb, GFP_ATOMIC);
+		if (status != 0)
+			netif_err(dev, timer, dev->net,
+				  "intr resubmit --> %d\n", status);
+	}
 }
 
 static int init_status (struct usbnet *dev, struct usb_interface *intf)
@@ -725,7 +725,8 @@ int usbnet_stop (struct net_device *net)
 	if (!(info->flags & FLAG_AVOID_UNLINK_URBS))
 		usbnet_terminate_urbs(dev);
 
-	usb_kill_urb(dev->interrupt);
+	if (!(info->flags & FLAG_INTR_ALWAYS))
+		usb_kill_urb(dev->interrupt);
 
 	usbnet_purge_paused_rxq(dev);
 
@@ -786,7 +787,7 @@ int usbnet_open (struct net_device *net)
 	}
 
 	/* start any status interrupt transfer */
-	if (dev->interrupt) {
+	if (dev->interrupt && !(info->flags & FLAG_INTR_ALWAYS)) {
 		retval = usb_submit_urb (dev->interrupt, GFP_KERNEL);
 		if (retval < 0) {
 			netif_err(dev, ifup, dev->net,
@@ -1496,6 +1497,15 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 	if (status < 0)
 		goto out3;
 
+	/* Submit status interrupt URB immediately if sub-driver wants */
+	if (dev->interrupt && (info->flags & FLAG_INTR_ALWAYS)) {
+		status = usb_submit_urb(dev->interrupt, GFP_KERNEL);
+		if (status < 0) {
+			dev_err(&udev->dev, "intr submit %d\n", status);
+			goto out4;
+		}
+	}
+
 	if (!dev->rx_urb_size)
 		dev->rx_urb_size = dev->hard_mtu;
 	dev->maxpacket = usb_maxpacket (dev->udev, dev->out, 1);
@@ -1507,7 +1517,7 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 
 	status = register_netdev (net);
 	if (status)
-		goto out4;
+		goto out5;
 	netif_info(dev, probe, dev->net,
 		   "register '%s' at usb-%s-%s, %s, %pM\n",
 		   udev->dev.driver->name,
@@ -1525,6 +1535,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 
 	return 0;
 
+out5:
+	usb_kill_urb(dev->interrupt);
 out4:
 	usb_free_urb(dev->interrupt);
 out3:
@@ -1586,8 +1598,15 @@ int usbnet_resume (struct usb_interface *intf)
 
 	if (!--dev->suspend_count) {
 		/* resume interrupt URBs */
-		if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
-			usb_submit_urb(dev->interrupt, GFP_NOIO);
+		if (dev->interrupt &&
+			(dev->driver_info->flags & FLAG_INTR_ALWAYS ||
+				test_bit(EVENT_DEV_OPEN, &dev->flags))) {
+			retval = usb_submit_urb(dev->interrupt, GFP_NOIO);
+			if (retval < 0) {
+				netif_err(dev, ifup, dev->net,
+					  "intr submit %d\n", retval);
+			}
+		}
 
 		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 0de078d..b0f17f6 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -111,6 +111,9 @@ struct driver_info {
 #define FLAG_MULTI_PACKET	0x2000
 #define FLAG_RX_ASSEMBLE	0x4000	/* rx packets may span >1 frames */
 
+/* Indicates that the interrupt URB should not depend on netdev open/close */
+#define FLAG_INTR_ALWAYS	0x8000
+
 	/* init device ... can sleep, or cause probe() failure */
 	int	(*bind)(struct usbnet *, struct usb_interface *);
 
-- 
1.7.11.7

Re: [PATCH 1/2 v2] usbnet: allow status interrupt URB to always be active

From: Oliver Neukum <oliver@neukum.org>
Date: 2013-02-06 20:19:39

On Wednesday 06 February 2013 12:36:38 Dan Williams wrote:
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware.  Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close.  The URB is still
killed/re-submitted for suspend/resume, as before.
Given your description in the later patch, which uses this feature,
it seems to me that we can be more efficient if we include infrastructure
to determine whether the interrupt URB is still needed under
some circumstances. Could we put this on hold until we are clear on
the requirements of the protocol?

	Regards
		Oliver

[PATCH 1/2 v3] usbnet: allow status interrupt URB to always be active

From: Dan Williams <hidden>
Date: 2013-03-28 16:29:30

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 <redacted>
---
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 51f3192..6431a03 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -252,6 +252,43 @@ static int init_status (struct usbnet *dev, struct usb_interface *intf)
 	return 0;
 }
 
+/* Submit the interrupt URB if it hasn't been submitted yet */
+int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
+{
+	int ret = 0;
+
+	/* 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;
+
+	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);
+
+/* 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);
+		BUG_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);
+
 /* 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 +762,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 +824,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 +1467,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");
@@ -1585,9 +1624,13 @@ 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 */
+		if (dev->interrupt) {
+			mutex_lock (&dev->interrupt_mutex);
+			if (dev->interrupt_count)
+				usb_submit_urb(dev->interrupt, GFP_NOIO);
+			mutex_unlock (&dev->interrupt_mutex);
+		}
 
 		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 */
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help