strange behaviour of MC7700 with sierra_net

strange behaviour of MC7700 with sierra_net

[Phil Sutter]

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

Re: strange behaviour of MC7700 with sierra_net

[Dan Williams]

On Wed, 2011-07-27 at 16:12 +0200, Phil Sutter wrote:
> 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.

I wonder if the firmware changed and Sierra hasn't updated the driver
yet.  I've got a usb306 which also uses sierra_net which doesn't have
this problem.  It looks like the driver tries to sync with the firmware
right after binding to the net interface, so that means at least a
couple of SYNC requests have already happened by the time you start
getting errors.  My first thought here is simply that the firmware on
the MC7700 doesn't work like the rest of the devices that sierra_net
expects, and for that you'd have to get Sierra to weigh in on the
required changes :(

Since the MC7700 is an LTE module it's 100% likely the firmware is
different, or at least significantly rebased, from the existing Sierra
HSPA/EVDO parts that use sierra_net and thus I wouldn't necessarily
expect it to work out of the box.

Dan

> 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 netdev" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: strange behaviour of MC7700 with sierra_net

[Phil Sutter]

Dan,

Thanks for the quick reply!

On Wed, Jul 27, 2011 at 02:40:40PM -0500, Dan Williams wrote:
> I wonder if the firmware changed and Sierra hasn't updated the driver
> yet.  I've got a usb306 which also uses sierra_net which doesn't have
> this problem.  It looks like the driver tries to sync with the firmware
> right after binding to the net interface, so that means at least a
> couple of SYNC requests have already happened by the time you start
> getting errors.  My first thought here is simply that the firmware on
> the MC7700 doesn't work like the rest of the devices that sierra_net
> expects, and for that you'd have to get Sierra to weigh in on the
> required changes :(

That's exactly the case. My printf-debugging shows that ~30 calls to
sierra_net_dosync() sycceed before it starts failing. Why the heck does
that work if I set the interface up? When doing so, the driver
immediately receives a whole bunch of restart-messages. Looks
like the number matches the number of successfully sent sync-messages.

This looks like the turned down interface is holding something back, but
looking with usbmon I see communication (this is the point when the
sync-messages start to fail):
| da9ebc40 74.842086 S Co:1:035:0 s 21 00 0000 0007 0004 4 = 00002000
| da9ebc40 74.842971 C Co:1:035:0 0 4 >
| da9ebc40 76.848151 S Co:1:035:0 s 21 00 0000 0007 0004 4 = 00002000
| da9ebc40 76.848835 C Co:1:035:0 -32 4 >

But maybe something should be done in the beginning that's done when the
interface comes up. The capture in that situation is this:
| da9ebc40 88.898257 S Co:1:035:0 s 21 00 0000 0007 0004 4 = 00002000
| da9ebc40 88.898860 C Co:1:035:0 -32 4 >
| dbe29dc0 90.548212 S Ii:1:035:8 -:64 64 <
| da9ebc40 90.548619 S Bi:1:035:9 - 8192 <
| dbe29c40 90.548632 S Bi:1:035:9 - 8192 <
| daab45c0 90.548639 S Bi:1:035:9 - 8192 <
| daac3640 90.548645 S Bi:1:035:9 - 8192 <
| da9cce40 90.548651 S Bi:1:035:9 - 8192 <
| dbe6b7c0 90.548657 S Bi:1:035:9 - 8192 <
| dab0ae40 90.548663 S Bi:1:035:9 - 8192 <
| dbe29640 90.548669 S Bi:1:035:9 - 8192 <
| dbe29340 90.548675 S Bi:1:035:9 - 8192 <
| dbe6b540 90.548681 S Bi:1:035:9 - 8192 <
| dbe6b0c0 90.548688 S Bi:1:035:9 - 8192 <
| dbe29dc0 90.550814 C Ii:1:035:8 0:64 8 = a1010000 07000000
| dbe29dc0 90.550868 S Ii:1:035:8 -:64 64 <
| daac3540 90.550980 S Ci:1:035:0 s a1 01 0000 0007 0400 1024 <
| daac3540 90.551694 C Ci:1:035:0 0 68 = 00406200 53574939 32303058 5f30332e 30302e30 322e3030 41502052 32303332
| dbe29dc0 90.558801 C Ii:1:035:8 0:64 8 = a1010000 07000000
| dbe29dc0 90.558960 S Ii:1:035:8 -:64 64 <
| daac3540 90.559054 S Ci:1:035:0 s a1 01 0000 0007 0400 1024 <
| daac3540 90.559544 C Ci:1:035:0 0 68 = 00406200 53574939 32303058 5f30332e 30302e30 322e3030 41502052 32303332
| dbe29dc0 90.566798 C Ii:1:035:8 0:64 8 = a1010000 07000000

The kernel log output of the above timeframe:
| Jul 28 17:55:36 (none) user.debug kernel: [266833.170570] usb 1-1: sierra_net_send_sync
| Jul 28 17:55:36 (none) user.info kernel: [266833.170590] sierra_net_send_sync: netif is not running
| Jul 28 17:55:36 (none) user.err kernel: [266833.171515] sierra_net 1-1:1.7: wwan0: Submit SYNC failed -32
| Jul 28 17:55:36 (none) user.err kernel: [266833.174713] sierra_net 1-1:1.7: wwan0: Send SYNC failed, status -32
| Jul 28 17:55:38 (none) user.debug kernel: [266834.823421] usb 1-1: sierra_net_status
| Jul 28 17:55:38 (none) user.debug kernel: [266834.824316] usb 1-1: sierra_net_kevent: Received status message, 0044 bytes
| Jul 28 17:55:38 (none) user.debug kernel: [266834.824475] usb 1-1: Restart reported: 0, stopping sync timer
| Jul 28 17:55:38 (none) user.debug kernel: [266834.831400] usb 1-1: sierra_net_status
| Jul 28 17:55:38 (none) user.debug kernel: [266834.832268] usb 1-1: sierra_net_kevent: Received status message, 0044 bytes
| Jul 28 17:55:38 (none) user.debug kernel: [266834.832426] usb 1-1: Restart reported: 0, stopping sync timer

> Since the MC7700 is an LTE module it's 100% likely the firmware is
> different, or at least significantly rebased, from the existing Sierra
> HSPA/EVDO parts that use sierra_net and thus I wouldn't necessarily
> expect it to work out of the box.

Well, it's obviously not that bad. Setting wwan0 up as a workaround,
everything's quite fine. Also, if I'm fast enough initialising the modem
via AT-commands before the control-tty dies, I can dial in even with
wwan0 being down, set it up and receive an address via DHCP. 

Since the MC7700 has the same USB product and vendor IDs as any other
direct-IP device (1199:68a3), Sierra may indeed have meant this device
to act identical to the other ones. Or their driver-developers are just
a little bit masochistic. ;)

Hopefully Elina Pasheva can provide some helpful insights here.

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

Re: strange behaviour of MC7700 with sierra_net

[Phil Sutter]

Hello Elina,

(Added the lists to CC again since others might benefit from the
provided information.)

On Tue, Aug 02, 2011 at 03:48:42PM -0700, Elina Pasheva wrote:
> Please note that we publish updates to sierra and sierra_net Linux drivers on Sierra Wireless Knowledge Base.
> http://www.sierrawireless.com/Support/knowledgebase.aspx
> 
> 
> Please find attached the latest published drivers (for kernel-2.6.38, but sierra_net may be ported to kernel-2.6.39 with almost no changes).
> 
> Would you please let me know if these work for you?
> 
> I have tried MC7700 device on kernel-2.6.38 with sierra v.1.7.40 and sierra_net v.3.0 Linux drivers.
> 
> Using a commercial sim card I do not see problems with sync as you described it.
> 
> Please let me know if it works better for you.

Thanks for the pointer! I've downloaded the DirectIP driver v3.0, which
indeed compiles against 2.6.39.2 without any changes at all. Also, the
SYNC problem is gone. Looking at the driver, it seems to be related to
the new function sierra_net_open() calling sierra_net_dosync() instead
of sierra_net_bind(). 

Other than that and the autopm-support, there are not many changes to
the driver. Are there concrete plans already when this will go mainline?

Greetings and again many thanks,
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

[Dan Williams]

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 <dcbw@redhat.com>
---
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

[Dan Williams]

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 <dcbw@redhat.com>
---
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,

[PATCH 2/2 v2] sierra_net: fix issues with SYNC/RESTART messages and interrupt pipe setup

[Dan Williams]

As part of the initialization sequence, the driver sends a SYNC message
via the control pipe to the firmware, which appears to request a
firmware restart.  The firmware responds with an indication via the
interrupt pipe set up by usbnet.  If the driver does not receive a
RESTART indication within a certain amount of time, it will periodically
send additional SYNC messages until it receives the RESTART indication.

Unfortunately, the interrupt URB is only submitted while the netdev
is open, which is usually not the case during initialization, and thus
the firmware's RESTART indication is lost.  So the driver continues
sending SYNC messages, and eventually the firmware crashes when it
receives too many.  This leads to a wedged netdev.

To ensure the firmware's RESTART indications can be received by the
driver, request that usbnet keep the interrupt URB active via
FLAG_INTR_ALWAYS.

Second, move the code that sends the SYNC message out of the
bind() hook and after usbnet_probe() to ensure the interrupt URB
is set up before trying to use it.

Signed-off-by: Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/net/usb/sierra_net.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index 18dd425..185ffec 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -427,6 +427,13 @@ static void sierra_net_dosync(struct usbnet *dev)
 
 	dev_dbg(&dev->udev->dev, "%s", __func__);
 
+	/* The SIERRA_NET_HIP_MSYNC_ID command appears to request that the
+	 * firmware restart itself.  After restarting, the modem will respond
+	 * with the SIERRA_NET_HIP_RESTART_ID indication.  The driver continues
+	 * sending MSYNC commands every few seconds until it receives the
+	 * RESTART event from the firmware
+	 */
+
 	/* tell modem we are ready */
 	status = sierra_net_send_sync(dev);
 	if (status < 0)
@@ -709,6 +716,9 @@ static int sierra_net_bind(struct usbnet *dev, struct usb_interface *intf)
 	/* set context index initially to 0 - prepares tx hdr template */
 	sierra_net_set_ctx_index(priv, 0);
 
+	/* prepare sync message template */
+	memcpy(priv->sync_msg, sync_tmplate, sizeof(priv->sync_msg));
+
 	/* decrease the rx_urb_size and max_tx_size to 4k on USB 1.1 */
 	dev->rx_urb_size  = SIERRA_NET_RX_URB_SIZE;
 	if (dev->udev->speed != USB_SPEED_HIGH)
@@ -744,11 +754,6 @@ static int sierra_net_bind(struct usbnet *dev, struct usb_interface *intf)
 		kfree(priv);
 		return -ENODEV;
 	}
-	/* prepare sync message from template */
-	memcpy(priv->sync_msg, sync_tmplate, sizeof(priv->sync_msg));
-
-	/* initiate the sync sequence */
-	sierra_net_dosync(dev);
 
 	return 0;
 }
@@ -905,7 +910,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,
@@ -913,6 +918,20 @@ static const struct driver_info sierra_net_info_direct_ip = {
 	.tx_fixup = sierra_net_tx_fixup,
 };
 
+static int
+sierra_net_probe (struct usb_interface *udev, const struct usb_device_id *prod)
+{
+	int ret;
+
+	ret = usbnet_probe (udev, prod);
+	if (ret == 0) {
+		/* Interrupt URB now set up; initiate sync sequence */
+		sierra_net_dosync(usb_get_intfdata(udev));
+	}
+
+	return ret;
+}
+
 #define DIRECT_IP_DEVICE(vend, prod) \
 	{USB_DEVICE_INTERFACE_NUMBER(vend, prod, 7), \
 	.driver_info = (unsigned long)&sierra_net_info_direct_ip}, \
@@ -935,7 +954,7 @@ MODULE_DEVICE_TABLE(usb, products);
 static struct usb_driver sierra_net_driver = {
 	.name = "sierra_net",
 	.id_table = products,
-	.probe = usbnet_probe,
+	.probe = sierra_net_probe,
 	.disconnect = usbnet_disconnect,
 	.suspend = usbnet_suspend,
 	.resume = usbnet_resume,
-- 
1.7.11.7


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v2] sierra_net: fix issues with SYNC/RESTART messages and interrupt pipe setup

[Oliver Neukum]

On Wednesday 06 February 2013 12:42:56 Dan Williams wrote:
> As part of the initialization sequence, the driver sends a SYNC message
> via the control pipe to the firmware, which appears to request a
> firmware restart.  The firmware responds with an indication via the
> interrupt pipe set up by usbnet.  If the driver does not receive a
> RESTART indication within a certain amount of time, it will periodically
> send additional SYNC messages until it receives the RESTART indication.
> 
> Unfortunately, the interrupt URB is only submitted while the netdev
> is open, which is usually not the case during initialization, and thus
> the firmware's RESTART indication is lost.  So the driver continues
> sending SYNC messages, and eventually the firmware crashes when it
> receives too many.  This leads to a wedged netdev.

If I understand this correctly we should stop the interrupt pipe once
RESTART has been recieved. I am afraid this patch is a bit inefficient.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v2] sierra_net: fix issues with SYNC/RESTART messages and interrupt pipe setup

[Dan Williams]

On Wed, 2013-02-06 at 21:17 +0100, Oliver Neukum wrote:
> On Wednesday 06 February 2013 12:42:56 Dan Williams wrote:
> > As part of the initialization sequence, the driver sends a SYNC message
> > via the control pipe to the firmware, which appears to request a
> > firmware restart.  The firmware responds with an indication via the
> > interrupt pipe set up by usbnet.  If the driver does not receive a
> > RESTART indication within a certain amount of time, it will periodically
> > send additional SYNC messages until it receives the RESTART indication.
> > 
> > Unfortunately, the interrupt URB is only submitted while the netdev
> > is open, which is usually not the case during initialization, and thus
> > the firmware's RESTART indication is lost.  So the driver continues
> > sending SYNC messages, and eventually the firmware crashes when it
> > receives too many.  This leads to a wedged netdev.
> 
> If I understand this correctly we should stop the interrupt pipe once
> RESTART has been recieved. I am afraid this patch is a bit inefficient.

The firmware may send other indications to the host over the interrupt
pipe at any time, apparently.  However, I have not seen firmware send
these indications in practice, so maybe we can ignore this and do as you
suggest.

So how would you suggest to handle this given the constraints?

Basically, we need to allow sierra_net to submit dev->interrupt at
probe/bind and then after a period of time kill it when it's no longer
needed.  The problem is that if the interface is set IFF_UP right after
bind but before sierra_net has finished its SYNC/Restart dance, we need
to ensure that sierra_net doesn't kill the URB unconditionally.

One way to do this would be a new usbnet function to subdrivers could
call to submit the URB which would be a NOP if the URB was already
submitted.  It would be paired with a function to kill the URB which
would be a NOP if (URB already killed || (IFF_UP and subdriver has
status() hook)).  Kinda like refcounting the interrupt URB submission
but not.

Does that sound like a worthwhile approach?

Dan

Re: [PATCH 2/2 v2] sierra_net: fix issues with SYNC/RESTART messages and interrupt pipe setup

[Bjørn Mork]

Dan Williams <dcbw@redhat.com> writes:

> As part of the initialization sequence, the driver sends a SYNC message
> via the control pipe to the firmware, which appears to request a
> firmware restart.  The firmware responds with an indication via the
> interrupt pipe set up by usbnet.  If the driver does not receive a
> RESTART indication within a certain amount of time, it will periodically
> send additional SYNC messages until it receives the RESTART indication.
>
> Unfortunately, the interrupt URB is only submitted while the netdev
> is open, which is usually not the case during initialization, and thus
> the firmware's RESTART indication is lost.  So the driver continues
> sending SYNC messages, and eventually the firmware crashes when it
> receives too many.  This leads to a wedged netdev.
>
> To ensure the firmware's RESTART indications can be received by the
> driver, request that usbnet keep the interrupt URB active via
> FLAG_INTR_ALWAYS.
>
> Second, move the code that sends the SYNC message out of the
> bind() hook and after usbnet_probe() to ensure the interrupt URB
> is set up before trying to use it.

Given this description I am wondering if you couldn't just move the
whole SYNC thing to a new reset() hook, using some private flag to make
sure it only runs once?  Does it really need to start at probe time?


Bjørn

Re: [PATCH 2/2 v2] sierra_net: fix issues with SYNC/RESTART messages and interrupt pipe setup

[Dan Williams]

On Wed, 2013-02-06 at 22:11 +0100, Bjørn Mork wrote:
> Dan Williams <dcbw@redhat.com> writes:
> 
> > As part of the initialization sequence, the driver sends a SYNC message
> > via the control pipe to the firmware, which appears to request a
> > firmware restart.  The firmware responds with an indication via the
> > interrupt pipe set up by usbnet.  If the driver does not receive a
> > RESTART indication within a certain amount of time, it will periodically
> > send additional SYNC messages until it receives the RESTART indication.
> >
> > Unfortunately, the interrupt URB is only submitted while the netdev
> > is open, which is usually not the case during initialization, and thus
> > the firmware's RESTART indication is lost.  So the driver continues
> > sending SYNC messages, and eventually the firmware crashes when it
> > receives too many.  This leads to a wedged netdev.
> >
> > To ensure the firmware's RESTART indications can be received by the
> > driver, request that usbnet keep the interrupt URB active via
> > FLAG_INTR_ALWAYS.
> >
> > Second, move the code that sends the SYNC message out of the
> > bind() hook and after usbnet_probe() to ensure the interrupt URB
> > is set up before trying to use it.
> 
> Given this description I am wondering if you couldn't just move the
> whole SYNC thing to a new reset() hook, using some private flag to make
> sure it only runs once?  Does it really need to start at probe time?

It doesn't need to run exactly at probe, but it appears to need to be
the first thing the driver does when communicating with the firmware to
ensure clear state and whatnot.  Possibly like the QMI SYNC message that
clears all the client IDs and resets the internal stack.  (the driver
also sends a "shutdown" message to the firmware when unbinding).

So I do think that somewhere around probe() is the best time to do this,
because it's best to initialize the device when the driver binds to it
and react to errors as soon as possible, rather than trying to set
everything up on open/IFF_UP and then fail right before you want to
actually use the device.  Late-fail is quite unhelpful for applications.

I don't really care if it happens in probe() or somewhere else right
after the driver is bound to the device, but it should be part of the
initialization process.

Dan

Re: [PATCH 2/2 v2] sierra_net: fix issues with SYNC/RESTART messages and interrupt pipe setup

[Bjørn Mork]

Dan Williams <dcbw@redhat.com> writes:

> It doesn't need to run exactly at probe, but it appears to need to be
> the first thing the driver does when communicating with the firmware to
> ensure clear state and whatnot.  Possibly like the QMI SYNC message that
> clears all the client IDs and resets the internal stack.  (the driver
> also sends a "shutdown" message to the firmware when unbinding).
>
> So I do think that somewhere around probe() is the best time to do this,
> because it's best to initialize the device when the driver binds to it
> and react to errors as soon as possible, rather than trying to set
> everything up on open/IFF_UP and then fail right before you want to
> actually use the device.  Late-fail is quite unhelpful for applications.
>
> I don't really care if it happens in probe() or somewhere else right
> after the driver is bound to the device, but it should be part of the
> initialization process.

I was looking for something else in the rndis_host driver right now and
noticed that it has the same sort of problem.  The generic_rndis_bind()
function will call rndis_command() which does

         usb_control_msg(.., USB_CDC_SEND_ENCAPSULATED_COMMAND, ..);
         usb_interrupt_msg(.., dev->status->desc.bEndpointAddress, .. );
         for (count = 0; count < 10; count++) {
             usb_control_msg(.., USB_CDC_GET_ENCAPSULATED_RESPONSE, ..);
             if (ok)
                return 0;
             msleep(20);
         }

Somewhat ugly, but it is a way to send and receive commands while
probing.  This driver also sends a command in unbind().

Looks like your patch would allow this to be solved a lot cleaner,
replacing the loop over USB_CDC_GET_ENCAPSULATED_RESPONSE with proper
interrupt endpoint handling.


Bjørn

Re: [PATCH 2/2 v2] sierra_net: fix issues with SYNC/RESTART messages and interrupt pipe setup

[Dan Williams]

On Wed, 2013-02-13 at 12:44 +0100, Bjørn Mork wrote:
> Dan Williams <dcbw@redhat.com> writes:
> 
> > It doesn't need to run exactly at probe, but it appears to need to be
> > the first thing the driver does when communicating with the firmware to
> > ensure clear state and whatnot.  Possibly like the QMI SYNC message that
> > clears all the client IDs and resets the internal stack.  (the driver
> > also sends a "shutdown" message to the firmware when unbinding).
> >
> > So I do think that somewhere around probe() is the best time to do this,
> > because it's best to initialize the device when the driver binds to it
> > and react to errors as soon as possible, rather than trying to set
> > everything up on open/IFF_UP and then fail right before you want to
> > actually use the device.  Late-fail is quite unhelpful for applications.
> >
> > I don't really care if it happens in probe() or somewhere else right
> > after the driver is bound to the device, but it should be part of the
> > initialization process.
> 
> I was looking for something else in the rndis_host driver right now and
> noticed that it has the same sort of problem.  The generic_rndis_bind()
> function will call rndis_command() which does
> 
>          usb_control_msg(.., USB_CDC_SEND_ENCAPSULATED_COMMAND, ..);
>          usb_interrupt_msg(.., dev->status->desc.bEndpointAddress, .. );
>          for (count = 0; count < 10; count++) {
>              usb_control_msg(.., USB_CDC_GET_ENCAPSULATED_RESPONSE, ..);
>              if (ok)
>                 return 0;
>              msleep(20);
>          }
> 
> Somewhat ugly, but it is a way to send and receive commands while
> probing.  This driver also sends a command in unbind().
> 
> Looks like your patch would allow this to be solved a lot cleaner,
> replacing the loop over USB_CDC_GET_ENCAPSULATED_RESPONSE with proper
> interrupt endpoint handling.

It would end up being more "correct" but more complicated, because you'd
need to have rndis_command() block on a semaphore or something until the
response was processed by a "status" handler, which neither rndis_host.c
or rndis_wlan.c actually implement.  The status handler would have to
know that something was waiting on it, and then package up the response
in some way that the rndis_command() (which is now blocking on the
status interrupt) can read it and return it to the caller.

More correct, more code, more complicated...  but probably still
worthwhile?

Dan

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

[Oliver Neukum]

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 <dcbw@redhat.com>
> ---
> 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

[Dan Williams]

On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
> 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 <dcbw@redhat.com>
> > ---
> > 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

[Oliver Neukum]

On Friday 04 January 2013 19:26:33 Dan Williams wrote:
> On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
> > 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 <dcbw@redhat.com>
> > > ---
> > > 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

[Bjørn Mork]

Oliver Neukum <oliver@neukum.org> writes:
> On Friday 04 January 2013 19:26:33 Dan Williams wrote:
>
>> 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

[Dan Williams]

On Sat, 2013-01-05 at 12:01 +0100, Oliver Neukum wrote:
> On Friday 04 January 2013 19:26:33 Dan Williams wrote:
> > On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
> > > 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 <dcbw@redhat.com>
> > > > ---
> > > > 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.

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

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

[Dan Williams]

On Sat, 2013-01-05 at 12:01 +0100, Oliver Neukum wrote:
> On Friday 04 January 2013 19:26:33 Dan Williams wrote:
> > On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
> > > 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 <dcbw@redhat.com>
> > > > ---
> > > > 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 ?

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

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

[Bjørn Mork]

Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
>> 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 <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> > 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.

Just a few random thoughts...

The sierra_net driver would have been an excellent candidate for the
cdc-wdm subdriver model if that had existed when sierra_net was written.
I assume the current driver only implements a minimal subset of the
DirectIP HIP protocol embedded in CDC, and exporting this to userspace
instead would have made it possible to extend that support without
changing the driver, in addtion to making the driver much more robust
against firmware differences.  It would have eliminated the problem you
are facing and removed the minidriver workqueue complexity.

But I guess it's too late to change this now.  In theory we could
implement the cdc-wdm hooks that were initially proposed for cdc_mbim
and rewrite sierra_net to use it while being backwards compatible.
Don't think anyone wants to do either, so forget about it...

You can still use a trick similar to what qmi_wwan and cdc_mbim does to
take over the status endpoint from usbnet: By not implementing .status,
and possibly setting dev->status to NULL in .bind, you are free to
handle the status endpoint entirely inside the minidriver.  Not sure if
that is smart though.  You would have to reimplement init_status and
intr_complete from usbnet, and kill or resubmit the interrupt urb on
suspend/resume/disconnect yourself.

The new usbnet flag is probably a better solution.

FWIW, I agree with Oliver that always submitting the interrupt URB is
both risky and will cause too much unnecessary USB activity for most
usbnet devices.


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

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

[Oliver Neukum]

On Saturday 05 January 2013 11:59:16 Bjørn Mork wrote:

Hi,

> You can still use a trick similar to what qmi_wwan and cdc_mbim does to
> take over the status endpoint from usbnet: By not implementing .status,
> and possibly setting dev->status to NULL in .bind, you are free to
> handle the status endpoint entirely inside the minidriver.  Not sure if
> that is smart though.  You would have to reimplement init_status and
> intr_complete from usbnet, and kill or resubmit the interrupt urb on
> suspend/resume/disconnect yourself.

Please no. We don't look for ways to circumvent frameworks. We alter
frameworks to act as we need them to act.

> The new usbnet flag is probably a better solution.

Indeed.

> FWIW, I agree with Oliver that always submitting the interrupt URB is
> both risky and will cause too much unnecessary USB activity for most
> usbnet devices.

Good.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Dan Williams]

On Sat, 2013-01-05 at 11:59 +0100, Bjørn Mork wrote:
> Dan Williams <dcbw@redhat.com> writes:
> > On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
> >> 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 <dcbw@redhat.com>
> >> > ---
> >> > 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.
> 
> Just a few random thoughts...
> 
> The sierra_net driver would have been an excellent candidate for the
> cdc-wdm subdriver model if that had existed when sierra_net was written.
> I assume the current driver only implements a minimal subset of the
> DirectIP HIP protocol embedded in CDC, and exporting this to userspace
> instead would have made it possible to extend that support without
> changing the driver, in addtion to making the driver much more robust
> against firmware differences.  It would have eliminated the problem you
> are facing and removed the minidriver workqueue complexity.
> 
> But I guess it's too late to change this now.  In theory we could
> implement the cdc-wdm hooks that were initially proposed for cdc_mbim
> and rewrite sierra_net to use it while being backwards compatible.
> Don't think anyone wants to do either, so forget about it...
> 
> You can still use a trick similar to what qmi_wwan and cdc_mbim does to
> take over the status endpoint from usbnet: By not implementing .status,
> and possibly setting dev->status to NULL in .bind, you are free to
> handle the status endpoint entirely inside the minidriver.  Not sure if
> that is smart though.  You would have to reimplement init_status and
> intr_complete from usbnet, and kill or resubmit the interrupt urb on
> suspend/resume/disconnect yourself.

That was exactly the approach of my first patch.  But that was icky
because we need to also track suspend/resume state by looking at
internal members of usbnet's structure.  Yes, it's specific to Sierra,
but it's pretty much a layer violation.  So I went with the flag
approach.

Dan

> The new usbnet flag is probably a better solution.
> 
> FWIW, I agree with Oliver that always submitting the interrupt URB is
> both risky and will cause too much unnecessary USB activity for most
> usbnet devices.
> 
> 
> Bjørn
> --
> 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

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

[Ming Lei]

On Sat, Jan 5, 2013 at 9:26 AM, Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
>> 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 <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> > 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

I am curious who are interested in the 'custom Restart indication'
information after the interface is closed.

If sierra_net provides ways(such as read registers) to query the
indication event, you can just query the information and setup
the device in driver_info->reset() during device open, so you can
avoid submitting interrupt URB always.

> status interrupt URB submission to device open/close isn't right.

In theory, drivers should support to report its link status
even it is closed, but looks no much actual usage, so guys opt to
submit the interrupt URB only after it is opened.


Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Dan Williams]

On Fri, 2013-01-11 at 11:06 +0800, Ming Lei wrote:
> On Sat, Jan 5, 2013 at 9:26 AM, Dan Williams <dcbw@redhat.com> wrote:
> > On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
> >> 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 <dcbw@redhat.com>
> >> > ---
> >> > 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
> 
> I am curious who are interested in the 'custom Restart indication'
> information after the interface is closed.

It's actually before the interface is even opened.  It's really just a
sync signal that's part of the driver's setup/initialization of the
device.

> If sierra_net provides ways(such as read registers) to query the
> indication event, you can just query the information and setup
> the device in driver_info->reset() during device open, so you can
> avoid submitting interrupt URB always.

As far as I know, it does not, or at least Sierra hasn't released such
information about the firmware API of their devices.

Dan

> > status interrupt URB submission to device open/close isn't right.
> 
> In theory, drivers should support to report its link status
> even it is closed, but looks no much actual usage, so guys opt to
> submit the interrupt URB only after it is opened.
> 
> 
> Thanks,
> --
> Ming Lei
> --
> 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

[Ming Lei]

On Tue, Jan 15, 2013 at 1:23 AM, Dan Williams <dcbw@redhat.com> wrote:
> On Fri, 2013-01-11 at 11:06 +0800, Ming Lei wrote:
>> I am curious who are interested in the 'custom Restart indication'
>> information after the interface is closed.
>
> It's actually before the interface is even opened.  It's really just a
> sync signal that's part of the driver's setup/initialization of the
> device.

OK, got it.

Considered that it is only required by Sierra, could you submit
the status URB in driver->bind and cancel it in driver->reset so
we can avoid touching usbnet core code(one simple change
might be to move init_status() before calling driver->info()
inside usbnet_probe())?

>
>> If sierra_net provides ways(such as read registers) to query the
>> indication event, you can just query the information and setup
>> the device in driver_info->reset() during device open, so you can
>> avoid submitting interrupt URB always.
>
> As far as I know, it does not, or at least Sierra hasn't released such
> information about the firmware API of their devices.

You are unlucky, :-)

In fact, many usbnet devices provide read command to query this kind of
information.


Thanks,
--
Ming Lei

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

[Dan Williams]

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 <dcbw@redhat.com>
---
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

[Oliver Neukum]

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

[Dan Williams]

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

[PATCH 2/2 v3] sierra_net: keep status interrupt URB active

[Dan Williams]

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 <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index 79ab243..c707225 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -427,6 +427,13 @@ static void sierra_net_dosync(struct usbnet *dev)
 
 	dev_dbg(&dev->udev->dev, "%s", __func__);
 
+	/* The SIERRA_NET_HIP_MSYNC_ID command appears to request that the
+	 * firmware restart itself.  After restarting, the modem will respond
+	 * with the SIERRA_NET_HIP_RESTART_ID indication.  The driver continues
+	 * sending MSYNC commands every few seconds until it receives the
+	 * RESTART event from the firmware
+	 */
+
 	/* tell modem we are ready */
 	status = sierra_net_send_sync(dev);
 	if (status < 0)
@@ -705,6 +712,9 @@ static int sierra_net_bind(struct usbnet *dev, struct usb_interface *intf)
 	/* set context index initially to 0 - prepares tx hdr template */
 	sierra_net_set_ctx_index(priv, 0);
 
+	/* prepare sync message template */
+	memcpy(priv->sync_msg, sync_tmplate, sizeof(priv->sync_msg));
+
 	/* decrease the rx_urb_size and max_tx_size to 4k on USB 1.1 */
 	dev->rx_urb_size  = SIERRA_NET_RX_URB_SIZE;
 	if (dev->udev->speed != USB_SPEED_HIGH)
@@ -740,11 +750,6 @@ static int sierra_net_bind(struct usbnet *dev, struct usb_interface *intf)
 		kfree(priv);
 		return -ENODEV;
 	}
-	/* prepare sync message from template */
-	memcpy(priv->sync_msg, sync_tmplate, sizeof(priv->sync_msg));
-
-	/* initiate the sync sequence */
-	sierra_net_dosync(dev);
 
 	return 0;
 }
@@ -767,8 +772,9 @@ static void sierra_net_unbind(struct usbnet *dev, struct usb_interface *intf)
 		netdev_err(dev->net,
 			"usb_control_msg failed, status %d\n", status);
 
-	sierra_net_set_private(dev, NULL);
+	usbnet_status_stop(dev);
 
+	sierra_net_set_private(dev, NULL);
 	kfree(priv);
 }
 
@@ -909,6 +915,24 @@ static const struct driver_info sierra_net_info_direct_ip = {
 	.tx_fixup = sierra_net_tx_fixup,
 };
 
+static int
+sierra_net_probe (struct usb_interface *udev, const struct usb_device_id *prod)
+{
+	int ret;
+
+	ret = usbnet_probe(udev, prod);
+	if (ret == 0) {
+		struct usbnet *dev = usb_get_intfdata(udev);
+
+		ret = usbnet_status_start(dev, GFP_KERNEL);
+		if (ret == 0) {
+			/* Interrupt URB now set up; initiate sync sequence */
+			sierra_net_dosync(dev);
+		}
+	}
+	return ret;
+}
+
 #define DIRECT_IP_DEVICE(vend, prod) \
 	{USB_DEVICE_INTERFACE_NUMBER(vend, prod, 7), \
 	.driver_info = (unsigned long)&sierra_net_info_direct_ip}, \
@@ -931,7 +955,7 @@ MODULE_DEVICE_TABLE(usb, products);
 static struct usb_driver sierra_net_driver = {
 	.name = "sierra_net",
 	.id_table = products,
-	.probe = usbnet_probe,
+	.probe = sierra_net_probe,
 	.disconnect = usbnet_disconnect,
 	.suspend = usbnet_suspend,
 	.resume = usbnet_resume,


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v3] sierra_net: keep status interrupt URB active

[David Miller]

Please respin this along with patch #1 when you fix the coding
style issues I mentioned, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Miller]

From: Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date: Thu, 28 Mar 2013 11:30:07 -0500

> +	if (test_bit (EVENT_DEV_ASLEEP, &dev->flags))
> +		return -EINVAL;
> +
> +	mutex_lock (&dev->interrupt_mutex);

Please do not put a space between a function name and the openning
parenthesis in the call.

These are not C language primitives (where the space would be
appropriate, f.e. "if (") they are C functions.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Dan Williams]

On Sat, 2013-03-30 at 22:11 +0800, Ming Lei wrote:
> On Fri, Mar 29, 2013 at 12:30 AM, Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> > 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);
> 
> It isn't worth BUG_ON() and returning 0 simply should be OK.

The code is attempting to ensure that modules never, ever call
usbnet_status_start() unless they implement the "status" subdriver hook
which actually handles the interrupt URB messages.  If they don't
implement the hook, that means the driver is doing nothing special with
the interrupt URB, and thus it shouldn't be calling these functions.

> > +
> > +       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)
> 
> Check on dev->interrupt_count isn't necessary.

Fixed.

> > +                       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);
> > +               }
> 
> Given the introduced usbnet_status_start/usbnet_status_sop, it is
> better to apply them with one extra 'force' parameter in suspend()
> and resume() too.

Except that I'd rather the 'force' parameter never leak out of usbnet.c,
since that's the only place it should ever be used.  Instead, I'll
create an internal _usbnet_status_start()/_usbnet_status_stop() function
that has these parameters, while the public ones just call these with
'false'.  Sound OK?

Thanks,
Dan

> >
> >                 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 */
> >
> >
> > --
> > 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
> 
> Thanks,
> --
> Ming Lei


--
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 v4] usbnet: allow status interrupt URB to always be active

[Dan Williams]

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) {
+		/* 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 */
-- 
1.8.1.4

[PATCH 2/2 v4] sierra_net: keep status interrupt URB active

[Dan Williams]

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 <dcbw@redhat.com>
---
 drivers/net/usb/sierra_net.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index 79ab243..c707225 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -44,7 +44,7 @@ static const char driver_name[] = "sierra_net";
 #include <net/ip.h>
 #include <net/udp.h>
 #include <asm/unaligned.h>
-#include <linux/usb/usbnet.h>
+#include "usbnet.h"
 
 #define SWI_USB_REQUEST_GET_FW_ATTR	0x06
 #define SWI_GET_FW_ATTR_MASK		0x08
@@ -427,6 +427,13 @@ static void sierra_net_dosync(struct usbnet *dev)
 
 	dev_dbg(&dev->udev->dev, "%s", __func__);
 
+	/* The SIERRA_NET_HIP_MSYNC_ID command appears to request that the
+	 * firmware restart itself.  After restarting, the modem will respond
+	 * with the SIERRA_NET_HIP_RESTART_ID indication.  The driver continues
+	 * sending MSYNC commands every few seconds until it receives the
+	 * RESTART event from the firmware
+	 */
+
 	/* tell modem we are ready */
 	status = sierra_net_send_sync(dev);
 	if (status < 0)
@@ -705,6 +712,9 @@ static int sierra_net_bind(struct usbnet *dev, struct usb_interface *intf)
 	/* set context index initially to 0 - prepares tx hdr template */
 	sierra_net_set_ctx_index(priv, 0);
 
+	/* prepare sync message template */
+	memcpy(priv->sync_msg, sync_tmplate, sizeof(priv->sync_msg));
+
 	/* decrease the rx_urb_size and max_tx_size to 4k on USB 1.1 */
 	dev->rx_urb_size  = SIERRA_NET_RX_URB_SIZE;
 	if (dev->udev->speed != USB_SPEED_HIGH)
@@ -740,11 +750,6 @@ static int sierra_net_bind(struct usbnet *dev, struct usb_interface *intf)
 		kfree(priv);
 		return -ENODEV;
 	}
-	/* prepare sync message from template */
-	memcpy(priv->sync_msg, sync_tmplate, sizeof(priv->sync_msg));
-
-	/* initiate the sync sequence */
-	sierra_net_dosync(dev);
 
 	return 0;
 }
@@ -767,8 +772,9 @@ static void sierra_net_unbind(struct usbnet *dev, struct usb_interface *intf)
 		netdev_err(dev->net,
 			"usb_control_msg failed, status %d\n", status);
 
-	sierra_net_set_private(dev, NULL);
+	usbnet_status_stop(dev);
 
+	sierra_net_set_private(dev, NULL);
 	kfree(priv);
 }
 
@@ -909,6 +915,24 @@ static const struct driver_info sierra_net_info_direct_ip = {
 	.tx_fixup = sierra_net_tx_fixup,
 };
 
+static int
+sierra_net_probe(struct usb_interface *udev, const struct usb_device_id *prod)
+{
+	int ret;
+
+	ret = usbnet_probe(udev, prod);
+	if (ret == 0) {
+		struct usbnet *dev = usb_get_intfdata(udev);
+
+		ret = usbnet_status_start(dev, GFP_KERNEL);
+		if (ret == 0) {
+			/* Interrupt URB now set up; initiate sync sequence */
+			sierra_net_dosync(dev);
+		}
+	}
+	return ret;
+}
+
 #define DIRECT_IP_DEVICE(vend, prod) \
 	{USB_DEVICE_INTERFACE_NUMBER(vend, prod, 7), \
 	.driver_info = (unsigned long)&sierra_net_info_direct_ip}, \
@@ -931,7 +955,7 @@ MODULE_DEVICE_TABLE(usb, products);
 static struct usb_driver sierra_net_driver = {
 	.name = "sierra_net",
 	.id_table = products,
-	.probe = usbnet_probe,
+	.probe = sierra_net_probe,
 	.disconnect = usbnet_disconnect,
 	.suspend = usbnet_suspend,
 	.resume = usbnet_resume,
-- 
1.8.1.4

Re: [PATCH 2/2 v4] sierra_net: keep status interrupt URB active

[Oliver Neukum]

On Tuesday 09 April 2013 18:05:51 Dan Williams wrote:
> 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.

WHat about suspend/resume?

	Regards
		Oliver

Re: [PATCH 2/2 v4] sierra_net: keep status interrupt URB active

[Dan Williams]

On Wed, 2013-04-10 at 09:15 +0200, Oliver Neukum wrote:
> On Tuesday 09 April 2013 18:05:51 Dan Williams wrote:
> > 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.
> 
> WHat about suspend/resume?

usbnet should take care of that transparently from the sub-drivers
through it's own suspend/resume logic to kill/resubmit the URB if
required.  Though is there something I'm missing here?

Dan

Re: [PATCH 2/2 v4] sierra_net: keep status interrupt URB active

[Oliver Neukum]

On Wednesday 10 April 2013 09:57:50 Dan Williams wrote:
> On Wed, 2013-04-10 at 09:15 +0200, Oliver Neukum wrote:
> > On Tuesday 09 April 2013 18:05:51 Dan Williams wrote:
> > > 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.
> > 
> > WHat about suspend/resume?
> 
> usbnet should take care of that transparently from the sub-drivers
> through it's own suspend/resume logic to kill/resubmit the URB if
> required.  Though is there something I'm missing here?

Sorry, I read mails in antichronological order.

	Regards
		Oliver

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

[Oliver Neukum]

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

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

[Dan Williams]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

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

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

[Dan Williams]

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

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

[Oliver Neukum]

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

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

[Dan Williams]

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 linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

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 linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Dan Williams]

On Wed, 2013-04-10 at 15:58 +0200, Oliver Neukum wrote:
> 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

Do you mean something more like this?  If so, I'll go ahead and do the
formal submission.  Thanks!

Dan
---
 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..f903beb 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)
+{
+	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);
+	}
+}
+
 /* 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

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

[Oliver Neukum]

On Wednesday 10 April 2013 10:01:20 Dan Williams wrote:
> On Wed, 2013-04-10 at 15:58 +0200, Oliver Neukum wrote:
> > 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
> 
> Do you mean something more like this?  If so, I'll go ahead and do the
> formal submission.  Thanks!

That looks good and ready.

	Regards
		Oliver

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

[Dan Williams]

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)
+{
+	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);
+	}
+}
+
 /* 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

[PATCH 2/2 v5] sierra_net: keep status interrupt URB active

[Dan Williams]

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 <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/net/usb/sierra_net.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index 79ab243..d0fa5c18 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -427,6 +427,13 @@ static void sierra_net_dosync(struct usbnet *dev)
 
 	dev_dbg(&dev->udev->dev, "%s", __func__);
 
+	/* The SIERRA_NET_HIP_MSYNC_ID command appears to request that the
+	 * firmware restart itself.  After restarting, the modem will respond
+	 * with the SIERRA_NET_HIP_RESTART_ID indication.  The driver continues
+	 * sending MSYNC commands every few seconds until it receives the
+	 * RESTART event from the firmware
+	 */
+
 	/* tell modem we are ready */
 	status = sierra_net_send_sync(dev);
 	if (status < 0)
@@ -705,6 +712,9 @@ static int sierra_net_bind(struct usbnet *dev, struct usb_interface *intf)
 	/* set context index initially to 0 - prepares tx hdr template */
 	sierra_net_set_ctx_index(priv, 0);
 
+	/* prepare sync message template */
+	memcpy(priv->sync_msg, sync_tmplate, sizeof(priv->sync_msg));
+
 	/* decrease the rx_urb_size and max_tx_size to 4k on USB 1.1 */
 	dev->rx_urb_size  = SIERRA_NET_RX_URB_SIZE;
 	if (dev->udev->speed != USB_SPEED_HIGH)
@@ -740,11 +750,6 @@ static int sierra_net_bind(struct usbnet *dev, struct usb_interface *intf)
 		kfree(priv);
 		return -ENODEV;
 	}
-	/* prepare sync message from template */
-	memcpy(priv->sync_msg, sync_tmplate, sizeof(priv->sync_msg));
-
-	/* initiate the sync sequence */
-	sierra_net_dosync(dev);
 
 	return 0;
 }
@@ -767,8 +772,9 @@ static void sierra_net_unbind(struct usbnet *dev, struct usb_interface *intf)
 		netdev_err(dev->net,
 			"usb_control_msg failed, status %d\n", status);
 
-	sierra_net_set_private(dev, NULL);
+	usbnet_status_stop(dev);
 
+	sierra_net_set_private(dev, NULL);
 	kfree(priv);
 }
 
@@ -909,6 +915,24 @@ static const struct driver_info sierra_net_info_direct_ip = {
 	.tx_fixup = sierra_net_tx_fixup,
 };
 
+static int
+sierra_net_probe(struct usb_interface *udev, const struct usb_device_id *prod)
+{
+	int ret;
+
+	ret = usbnet_probe(udev, prod);
+	if (ret == 0) {
+		struct usbnet *dev = usb_get_intfdata(udev);
+
+		ret = usbnet_status_start(dev, GFP_KERNEL);
+		if (ret == 0) {
+			/* Interrupt URB now set up; initiate sync sequence */
+			sierra_net_dosync(dev);
+		}
+	}
+	return ret;
+}
+
 #define DIRECT_IP_DEVICE(vend, prod) \
 	{USB_DEVICE_INTERFACE_NUMBER(vend, prod, 7), \
 	.driver_info = (unsigned long)&sierra_net_info_direct_ip}, \
@@ -931,7 +955,7 @@ MODULE_DEVICE_TABLE(usb, products);
 static struct usb_driver sierra_net_driver = {
 	.name = "sierra_net",
 	.id_table = products,
-	.probe = usbnet_probe,
+	.probe = sierra_net_probe,
 	.disconnect = usbnet_disconnect,
 	.suspend = usbnet_suspend,
 	.resume = usbnet_resume,
-- 
1.8.1.4


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ming Lei]

On Thu, Apr 11, 2013 at 4:30 AM, Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

> +{
> +       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.

> +
>  /* 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,
-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

On Thursday 11 April 2013 10:31:31 Ming Lei wrote:
 
> '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.

The automatic system goes a long way, but there are corner cases, for example
work queues, which still need mem_flags.

	Regards
		Oliver

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

[Bjørn Mork]

Oliver Neukum <oliver@neukum.org> writes:
> On Thursday 11 April 2013 10:31:31 Ming Lei wrote:
>  
>> '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.
>
> The automatic system goes a long way, but there are corner cases, for example
> work queues, which still need mem_flags.


My immediate thought was that someone also might want to use this new
API from atomic context, e.g. calling it directly from an URB callback.
But that is of course not possible taking a mutex.  Could the lock
preventing interrupt_count maybe be a spinlock instead?  Or am I on the
completely wrong track here?

In any case, I don't see the point unnecessarily limiting the API by
dropping the memflags.  What possible problem would that solve?


Bjørn

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

[Ming Lei]

On Thu, Apr 11, 2013 at 4:06 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Oliver Neukum <oliver@neukum.org> writes:
>
> My immediate thought was that someone also might want to use this new
> API from atomic context, e.g. calling it directly from an URB callback.

I am wondering it is a valid use case, and if there is one URB submitted,
the interrupt URB for status has been submitted already, hasn't it?

> But that is of course not possible taking a mutex.  Could the lock
> preventing interrupt_count maybe be a spinlock instead?  Or am I on the
> completely wrong track here?

Also it is a bit odd that the 'start' API is allowed in atomic context, but
the 'stop' API isn't allowed, and it is very easy to cause unbalanced counter.

>
> In any case, I don't see the point unnecessarily limiting the API by
> dropping the memflags.  What possible problem would that solve?

If you think 'start' API should be called in atomic context, the memflags
should be always 'GFP_ATOMIC'. I let Oliver explain why GFP_NOIO
is needed in other cases.

Thanks
-- 
Ming Lei

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

[Oliver Neukum]

On Thursday 11 April 2013 16:37:53 Ming Lei wrote:
> On Thu, Apr 11, 2013 at 4:06 PM, Bjørn Mork <bjorn@mork.no> wrote:
> > Oliver Neukum <oliver@neukum.org> writes:
> >
> > My immediate thought was that someone also might want to use this new
> > API from atomic context, e.g. calling it directly from an URB callback.
> 
> I am wondering it is a valid use case, and if there is one URB submitted,
> the interrupt URB for status has been submitted already, hasn't it?

That is the point of this patch. There are multiple reasons to keep
the status urb submitted. The generic layer has to count them and
react to the count.

> > But that is of course not possible taking a mutex.  Could the lock
> > preventing interrupt_count maybe be a spinlock instead?  Or am I on the
> > completely wrong track here?
> 
> Also it is a bit odd that the 'start' API is allowed in atomic context, but
> the 'stop' API isn't allowed, and it is very easy to cause unbalanced counter.

It simply is easier to submit an URB in an atomic context than to kill it.
The code allowing doing it under a spinlock would be complex.

> > In any case, I don't see the point unnecessarily limiting the API by
> > dropping the memflags.  What possible problem would that solve?
> 
> If you think 'start' API should be called in atomic context, the memflags

It may be called. It doesn't have to be. usbnet needs a certain amount
of genericness in the API. Passing a flag does that and is simple.

	Regards
		Oliver

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

[Bjørn Mork]

Ming Lei <tom.leiming@gmail.com> writes:

> On Thu, Apr 11, 2013 at 4:06 PM, Bjørn Mork <bjorn@mork.no> wrote:
>> Oliver Neukum <oliver@neukum.org> writes:
>>
>> My immediate thought was that someone also might want to use this new
>> API from atomic context, e.g. calling it directly from an URB callback.
>
> I am wondering it is a valid use case, and if there is one URB submitted,
> the interrupt URB for status has been submitted already, hasn't it?

It might not be valid.

>> But that is of course not possible taking a mutex.  Could the lock
>> preventing interrupt_count maybe be a spinlock instead?  Or am I on the
>> completely wrong track here?
>
> Also it is a bit odd that the 'start' API is allowed in atomic context, but
> the 'stop' API isn't allowed, and it is very easy to cause unbalanced counter.

Yes, that's a valid point.  Just a random thought popping out :)

For the record: I believe the v5 patch as posted really is fine without
any changes.

>> In any case, I don't see the point unnecessarily limiting the API by
>> dropping the memflags.  What possible problem would that solve?
>
> If you think 'start' API should be called in atomic context, the memflags
> should be always 'GFP_ATOMIC'. I let Oliver explain why GFP_NOIO
> is needed in other cases.

Again: What problem are you attempting to solve by removing the
mem_flags from the API?

I think you are turning this the wrong way around. Please explain why
there are no use cases where different flags are needed.  You seem to be
only concerned about the resume case. This API is not limited to
resuming. We pass mem_flags around all the time. It's the common thing
to do in any API where allocations may be required.


Bjørn

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

[Ming Lei]

On Thu, Apr 11, 2013 at 6:04 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <tom.leiming@gmail.com> writes:
>
> I think you are turning this the wrong way around. Please explain why
> there are no use cases where different flags are needed.  You seem to be
> only concerned about the resume case. This API is not limited to
> resuming. We pass mem_flags around all the time. It's the common thing
> to do in any API where allocations may be required.

No always, we can find many many APIs which doesn't have the memflags
parameter but may allocate memory.

My point is very simple: if GFP_KERNEL is enough, why bother to pass one
memflag parameter?

Thanks,
-- 
Ming Lei

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

[Ming Lei]

On Thu, Apr 11, 2013 at 6:04 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <tom.leiming@gmail.com> writes:
>
> Again: What problem are you attempting to solve by removing the
> mem_flags from the API?

It is not about removing anything, we are discussing one new API
(include the parameters) to be introduced.

Thanks,
-- 
Ming Lei

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

[Bjørn Mork]

Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> On Thu, Apr 11, 2013 at 6:04 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
>> Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>
>> Again: What problem are you attempting to solve by removing the
>> mem_flags from the API?
>
> It is not about removing anything, we are discussing one new API
> (include the parameters) to be introduced.

Yes.  Sure.  And the original proposal was to add a new API with a
mem_flags parameter.  You proposed to add the same API, but without the
mem_flags parameter.  You did not explain why.  I still assumed that you
have some reason to propose it.  I assumed that reason must be some
problem which would be introduced by having the mem_flags parameter, and
which would be solved if we instead drop it.

It seems that you are either unable or unwilling to explain your
reasons, so I'll just go ahead and drop my assumptions.  You never had
any reason and there never would be any problem.

Thanks.


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

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

[Ming Lei]

On Thu, Apr 11, 2013 at 6:26 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <tom.leiming@gmail.com> writes:
>
>> On Thu, Apr 11, 2013 at 6:04 PM, Bjørn Mork <bjorn@mork.no> wrote:
>>> Ming Lei <tom.leiming@gmail.com> writes:
>>>
>>> Again: What problem are you attempting to solve by removing the
>>> mem_flags from the API?
>>
>> It is not about removing anything, we are discussing one new API
>> (include the parameters) to be introduced.
>
> Yes.  Sure.  And the original proposal was to add a new API with a
> mem_flags parameter.  You proposed to add the same API, but without the
> mem_flags parameter.  You did not explain why.  I still assumed that you
> have some reason to propose it.  I assumed that reason must be some
> problem which would be introduced by having the mem_flags parameter, and
> which would be solved if we instead drop it.
>
> It seems that you are either unable or unwilling to explain your
> reasons, so I'll just go ahead and drop my assumptions.  You never had
> any reason and there never would be any problem.

OK, I say it again, GFP_KERNEL is enough to cover all cases, and the
mem_flags parameter is redundant.


Thanks,
-- 
Ming Lei

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

[Bjørn Mork]

Ming Lei <tom.leiming@gmail.com> writes:

> OK, I say it again, GFP_KERNEL is enough to cover all cases, and the
> mem_flags parameter is redundant.

The docs for usb_submit_urb() in drivers/usb/core/urb.c lists some
possible mem_flags use cases. Among these are (where (b) and (c) are
cases needing GFP_ATOMIC and not applicable here):

<quote>
 *  (3) If you use a kernel thread with a network driver you must use
 *      GFP_NOIO, unless (b) or (c) apply;
</quote>

Is this example 
 a) wrong, or
 b) not applicable, or 
 c) to be excluded from the new API?


Bjørn

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

[Ming Lei]

On Thu, Apr 11, 2013 at 7:08 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
>
> The docs for usb_submit_urb() in drivers/usb/core/urb.c lists some
> possible mem_flags use cases. Among these are (where (b) and (c) are
> cases needing GFP_ATOMIC and not applicable here):
>
> <quote>
>  *  (3) If you use a kernel thread with a network driver you must use
>  *      GFP_NOIO, unless (b) or (c) apply;
> </quote>
>
> Is this example
>  a) wrong, or
>  b) not applicable, or
>  c) to be excluded from the new API?

IMO, it may be a) or b), and we can find many GFP_KERNEL usage
inside usbnet(kevent(), ...).

Also (3) doesn't explain the cause. Oliver, could you give a hit?

Wrt. usbnet, except for in xmit & receive handler, the rule should be
same with other usb driver.

Thanks,
-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

On Thursday 11 April 2013 19:42:53 Ming Lei wrote:
> On Thu, Apr 11, 2013 at 7:08 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
> >
> > The docs for usb_submit_urb() in drivers/usb/core/urb.c lists some
> > possible mem_flags use cases. Among these are (where (b) and (c) are
> > cases needing GFP_ATOMIC and not applicable here):
> >
> > <quote>
> >  *  (3) If you use a kernel thread with a network driver you must use
> >  *      GFP_NOIO, unless (b) or (c) apply;
> > </quote>
> >
> > Is this example
> >  a) wrong, or
> >  b) not applicable, or
> >  c) to be excluded from the new API?
> 
> IMO, it may be a) or b), and we can find many GFP_KERNEL usage
> inside usbnet(kevent(), ...).

Only in the rx path.

> Also (3) doesn't explain the cause. Oliver, could you give a hit?

IIRC this was to cover networked file systems.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ming Lei]

On Thu, Apr 11, 2013 at 7:54 PM, Oliver Neukum <oliver@neukum.org> wrote:
>> > Is this example
>> >  a) wrong, or
>> >  b) not applicable, or
>> >  c) to be excluded from the new API?
>>
>> IMO, it may be a) or b), and we can find many GFP_KERNEL usage
>> inside usbnet(kevent(), ...).
>
> Only in the rx path.

So it is b) since usbnet_status_start() can't be in rx path, :-)

>
>> Also (3) doesn't explain the cause. Oliver, could you give a hit?
>
> IIRC this was to cover networked file systems.

Given atomic allocation is used in rx/tx path already, and suppose
no other contexts might block rx/tx path, so is the rule still valid now?

Thanks,
-- 
Ming Lei

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

[Ming Lei]

On Thu, Apr 11, 2013 at 2:50 PM, Oliver Neukum <oliver@neukum.org> wrote:
> On Thursday 11 April 2013 10:31:31 Ming Lei wrote:
>
>> '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.
>
> The automatic system goes a long way, but there are corner cases, for example
> work queues, which still need mem_flags.

Could you explain why work queue need GFP_NOIO? and the use case for
usbnet?

Thanks,
-- 
Ming Lei

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

[Oliver Neukum]

On Thursday 11 April 2013 16:09:16 Ming Lei wrote:
> On Thu, Apr 11, 2013 at 2:50 PM, Oliver Neukum <oliver@neukum.org> wrote:
> > On Thursday 11 April 2013 10:31:31 Ming Lei wrote:
> >
> >> '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.
> >
> > The automatic system goes a long way, but there are corner cases, for example
> > work queues, which still need mem_flags.
> 
> Could you explain why work queue need GFP_NOIO?

Your fix for the memory allocation depends on it happening in the same
context. If you execute code on a work queue this happens in the context
of a kernel thread.

> and the use case for
> usbnet?

Processing your response from a work queue.

	Regards
		Oliver

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

[Ming Lei]

On Thu, Apr 11, 2013 at 5:53 PM, Oliver Neukum <oliver@neukum.org> wrote:
> On Thursday 11 April 2013 16:09:16 Ming Lei wrote:
>>
>> Could you explain why work queue need GFP_NOIO?
>
> Your fix for the memory allocation depends on it happening in the same
> context. If you execute code on a work queue this happens in the context
> of a kernel thread.

I understand the interface might be called from workqueue, and my question
is why GFP_NOIO is needed in the work queue context. Generally speaking,
GFP_KERNEL is enough for work queue context.

As we discussed before, GFP_NOIO is required in runtime resume context
and reset context, and the two contexts have been addressed automatically.
So looks you didn't answer my question, :-)

I mean if GFP_NOIO isn't needed, we can use GFP_KERNEL directly, and
the extra parameter isn't need.

Thanks,
-- 
Ming Lei

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

[Oliver Neukum]

On Thursday 11 April 2013 18:03:09 Ming Lei wrote:
> On Thu, Apr 11, 2013 at 5:53 PM, Oliver Neukum <oliver@neukum.org> wrote:
> > On Thursday 11 April 2013 16:09:16 Ming Lei wrote:
> >>
> >> Could you explain why work queue need GFP_NOIO?
> >
> > Your fix for the memory allocation depends on it happening in the same
> > context. If you execute code on a work queue this happens in the context
> > of a kernel thread.
> 
> I understand the interface might be called from workqueue, and my question
> is why GFP_NOIO is needed in the work queue context. Generally speaking,
> GFP_KERNEL is enough for work queue context.
> 
> As we discussed before, GFP_NOIO is required in runtime resume context
> and reset context, and the two contexts have been addressed automatically.
> So looks you didn't answer my question, :-)

Sorry, I misunderstood.

Task A					Task B				queue

queue work
						request a reset
											allocate memory and block
						cancel the work
											shit happened

	Regards
		Oliver

		

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

[Ming Lei]

On Thu, Apr 11, 2013 at 7:14 PM, Oliver Neukum <oliver@neukum.org> wrote:
>
> Sorry, I misunderstood.

No problem, :-)

>
> Task A                                  Task B                          queue
>
> queue work
>                                                 request a reset
>                                                                                         allocate memory and block
>                                                 cancel the work
>                                                                                         shit happened

If I understand the case correctly, the above deadlock can be avoided
by canceling rx/tx URBs at the end of pre_reset() or usbnet_disconnect(),
then memory can still be reclaimed in queue context since rx/tx is still
workable. Right?


Thanks,
-- 
Ming Lei

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

[Oliver Neukum]

On Thursday 11 April 2013 20:11:13 Ming Lei wrote:
> On Thu, Apr 11, 2013 at 7:14 PM, Oliver Neukum <oliver@neukum.org> wrote:
> >
> > Sorry, I misunderstood.
> 
> No problem, :-)
> 
> >
> > Task A                                  Task B                          queue
> >
> > queue work
> >                                                 request a reset
> >                                                                                         allocate memory and block
> >                                                 cancel the work
> >                                                                                         shit happened
> 
> If I understand the case correctly, the above deadlock can be avoided
> by canceling rx/tx URBs at the end of pre_reset() or usbnet_disconnect(),

No. cancel_work_sync() must wait for the work. The work will not finish.

	Regards
		Oliver

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

[Ming Lei]

On Thu, Apr 11, 2013 at 8:28 PM, Oliver Neukum <oliver@neukum.org> wrote:
> On Thursday 11 April 2013 20:11:13 Ming Lei wrote:
>> On Thu, Apr 11, 2013 at 7:14 PM, Oliver Neukum <oliver@neukum.org> wrote:
>> >
>> > Sorry, I misunderstood.
>>
>> No problem, :-)
>>
>> >
>> > Task A                                  Task B                          queue
>> >
>> > queue work
>> >                                                 request a reset
>> >                                                                                         allocate memory and block
>> >                                                 cancel the work
>> >                                                                                         shit happened
>>
>> If I understand the case correctly, the above deadlock can be avoided
>> by canceling rx/tx URBs at the end of pre_reset() or usbnet_disconnect(),
>
> No. cancel_work_sync() must wait for the work. The work will not finish.

The work will complete when memory is reclaimed, and the rx/tx path is
still working, so memory reclaim can continue and the deadlock may not
be caused, may it?


Thanks,
-- 
Ming Lei

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

[Oliver Neukum]

On Thursday 11 April 2013 20:59:05 Ming Lei wrote:
> On Thu, Apr 11, 2013 at 8:28 PM, Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org> wrote:
> > On Thursday 11 April 2013 20:11:13 Ming Lei wrote:
> >> On Thu, Apr 11, 2013 at 7:14 PM, Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org> wrote:
> >> >
> >> > Sorry, I misunderstood.
> >>
> >> No problem, :-)
> >>
> >> >
> >> > Task A                                  Task B                          queue
> >> >
> >> > queue work
> >> >                                                 request a reset
> >> >                                                                                         allocate memory and block
> >> >                                                 cancel the work
> >> >                                                                                         shit happened
> >>
> >> If I understand the case correctly, the above deadlock can be avoided
> >> by canceling rx/tx URBs at the end of pre_reset() or usbnet_disconnect(),
> >
> > No. cancel_work_sync() must wait for the work. The work will not finish.
> 
> The work will complete when memory is reclaimed, and the rx/tx path is
> still working, so memory reclaim can continue and the deadlock may not
> be caused, may it?

Only if the memory allocation goes to the same interface. If the blocking interface
is storage, something bad happens (data loss not deadlock)

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ming Lei]

On Fri, Apr 12, 2013 at 2:37 PM, Oliver Neukum <oliver@neukum.org> wrote:
>> The work will complete when memory is reclaimed, and the rx/tx path is
>> still working, so memory reclaim can continue and the deadlock may not
>> be caused, may it?
>
> Only if the memory allocation goes to the same interface. If the blocking interface
> is storage, something bad happens (data loss not deadlock)

OK, got it, it should be both since reset can't move on, so
memory reclaim can't complete to satisfy the allocation.

But I am wondering if it is a case which is worth the consideration.

Almost all USB probe() allocate memory with GFP_KERNEL, then
the similar scenario(data loss only this time) might happen too. Do we
need to fix all USB drivers?

Wrt. the usbnet_status_start() API, looks no good reason to call
it in another queue context, it should be enough to call it in probe() or
bind() if init_status() can be put before info->bind() in usbnet_probe().

Then looks the mem_flags isn't needed in both cases, and we should
always take GFP_NOIO inside the API?

Thanks,
-- 
Ming Lei

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

[Oliver Neukum]

On Friday 12 April 2013 17:42:46 Ming Lei wrote:
> On Fri, Apr 12, 2013 at 2:37 PM, Oliver Neukum <oliver@neukum.org> wrote:
> >> The work will complete when memory is reclaimed, and the rx/tx path is
> >> still working, so memory reclaim can continue and the deadlock may not
> >> be caused, may it?
> >
> > Only if the memory allocation goes to the same interface. If the blocking interface
> > is storage, something bad happens (data loss not deadlock)
> 
> OK, got it, it should be both since reset can't move on, so
> memory reclaim can't complete to satisfy the allocation.
> 
> But I am wondering if it is a case which is worth the consideration.

The remedy is close to trivial, so yes.

> Almost all USB probe() allocate memory with GFP_KERNEL, then
> the similar scenario(data loss only this time) might happen too. Do we
> need to fix all USB drivers?

No. We assume that probing happens before the interfaces are used.
Furthermore, if this turns out to be real, we can reuse your fix for the
general (w.o. work queue) reset case.

> Wrt. the usbnet_status_start() API, looks no good reason to call
> it in another queue context, it should be enough to call it in probe() or
> bind() if init_status() can be put before info->bind() in usbnet_probe().

We are talking about a generic helper. And the fix is really simple.
We just pass mem_flags and all is well. If the API can be made better
at next to no cost, we do it.

> Then looks the mem_flags isn't needed in both cases, and we should
> always take GFP_NOIO inside the API?

We cannot. GFP_ATOMIC may be needed. And no, we atre not going to
unconditionally use GFP_ATOMIC :-;

	Regards
		Oliver

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

[Ming Lei]

On Fri, Apr 12, 2013 at 6:02 PM, Oliver Neukum <oliver@neukum.org> wrote:
> On Friday 12 April 2013 17:42:46 Ming Lei wrote:
>> On Fri, Apr 12, 2013 at 2:37 PM, Oliver Neukum <oliver@neukum.org> wrote:
>> >> The work will complete when memory is reclaimed, and the rx/tx path is
>> >> still working, so memory reclaim can continue and the deadlock may not
>> >> be caused, may it?
>> >
>> > Only if the memory allocation goes to the same interface. If the blocking interface
>> > is storage, something bad happens (data loss not deadlock)
>>
>> OK, got it, it should be both since reset can't move on, so
>> memory reclaim can't complete to satisfy the allocation.
>>
>> But I am wondering if it is a case which is worth the consideration.
>
> The remedy is close to trivial, so yes.
>
>> Almost all USB probe() allocate memory with GFP_KERNEL, then
>> the similar scenario(data loss only this time) might happen too. Do we
>> need to fix all USB drivers?
>
> No. We assume that probing happens before the interfaces are used.
> Furthermore, if this turns out to be real, we can reuse your fix for the
> general (w.o. work queue) reset case.

See driver load/bind path(driver_attach()/driver_bound()), both may
happen anytime on unbind interface.

Yes, we can easily address it with the PF_MEMALLOC_NOIO trick,
but can we afford NOIO allocation in probe() path for all drivers?

>
>> Wrt. the usbnet_status_start() API, looks no good reason to call
>> it in another queue context, it should be enough to call it in probe() or
>> bind() if init_status() can be put before info->bind() in usbnet_probe().
>
> We are talking about a generic helper. And the fix is really simple.
> We just pass mem_flags and all is well. If the API can be made better
> at next to no cost, we do it.
>
>> Then looks the mem_flags isn't needed in both cases, and we should
>> always take GFP_NOIO inside the API?
>
> We cannot. GFP_ATOMIC may be needed. And no, we atre not going to
> unconditionally use GFP_ATOMIC :-;

It depend on the probe() case, :-)

Thanks,
-- 
Ming Lei

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

[Bjørn Mork]

Oliver Neukum <oliver@neukum.org> writes:
> On Friday 12 April 2013 17:42:46 Ming Lei wrote:
>
>> Wrt. the usbnet_status_start() API, looks no good reason to call
>> it in another queue context, it should be enough to call it in probe() or
>> bind() if init_status() can be put before info->bind() in usbnet_probe().
>
> We are talking about a generic helper. And the fix is really simple.
> We just pass mem_flags and all is well. If the API can be made better
> at next to no cost, we do it.

I absolutely agree with this.

>> Then looks the mem_flags isn't needed in both cases, and we should
>> always take GFP_NOIO inside the API?
>
> We cannot. GFP_ATOMIC may be needed. And no, we atre not going to
> unconditionally use GFP_ATOMIC :-;

But I don't think we ever will need GFP_ATOMIC as long as we take a
mutex lock anyway.


Bjørn

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

[Dan Williams]

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,

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

[Oliver Neukum]

On Thursday 11 April 2013 10:28:33 Dan Williams wrote:
> > 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.

We don't do functions which do a different thing based on flags.
Linus chewed our posteriors for that.

	Regards
		Oliver

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

[Oliver Neukum]

On Wednesday 10 April 2013 15:30:50 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>
Acked-by: Oliver Neukum <oliver@neukum.org>

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

[Dan Williams]

On Wed, 2013-04-10 at 15:30 -0500, 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>

So, what was the general consensus on this one?  I know Oliver signed
off on it, but the discussion about memflags seemed to die out without a
specific conclusion.  davem might be looking for that conclusion before
moving forward with the series :)

Dan

> ---
>  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)
> +{
> +	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);
> +	}
> +}
> +
>  /* 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 */

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

[Ming Lei]

On Mon, Apr 15, 2013 at 11:59 PM, Dan Williams <dcbw@redhat.com> wrote:
>
> So, what was the general consensus on this one?  I know Oliver signed
> off on it, but the discussion about memflags seemed to die out without a
> specific conclusion.  davem might be looking for that conclusion before
> moving forward with the series :)

Suggest to remove the memflags parameter, because:

- the probable issue addressed by introducing memflags is a general issue
of all USB drivers, also very corener one, not a specific one on usbnet, and
the issue only exists on devices with at least one mass storage interface and
another non-mass-storage interface, and it is not considered by other USB
drivers now.

- usbnet_status_start() is called from either probe() or work queue scheduled
from probe(), if we want to address the probable issue, the memflags should
always be GFP_NOIO under the two situations, __or__ GFP_KERNEL if we
choose to ignore the very corner case like other USB drivers.  So hardcoded
GFP_NOIO or GFP_KERNEL should be accepted.

Oliver, do you have objections on not adding the memflags parameter now?

Williams, looks there is another problem in your patch, sorry for not
finding it previously.  usb_autopm_get_interface() need to be called before
submitting URB inside usbnet_status_start(), and usb_autopm_put_interface()
need to be called after killing URB inside usbnet_status_stop(). Otherwise
your patch doesn't work as you expected under runtime PM situation.


Thanks,
-- 
Ming Lei

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

[Oliver Neukum]

On Tuesday 16 April 2013 09:15:45 Ming Lei wrote:
> On Mon, Apr 15, 2013 at 11:59 PM, Dan Williams <dcbw@redhat.com> wrote:
> >
> > So, what was the general consensus on this one?  I know Oliver signed
> > off on it, but the discussion about memflags seemed to die out without a
> > specific conclusion.  davem might be looking for that conclusion before
> > moving forward with the series :)
> 
> Suggest to remove the memflags parameter, because:
> 
> - the probable issue addressed by introducing memflags is a general issue
> of all USB drivers, also very corener one, not a specific one on usbnet, and
> the issue only exists on devices with at least one mass storage interface and
> another non-mass-storage interface, and it is not considered by other USB
> drivers now.

Generally, saying that somebody else has a problem is not an argument
as long as the fix is very simple. Of course it is a corner case, but why
fail to solve it as long as the cost is extremely low?

> - usbnet_status_start() is called from either probe() or work queue scheduled
> from probe(), if we want to address the probable issue, the memflags should

No, we export this symbol. So we keep it generic if that is possible at low cost.
We cannot assume that the conditions it would be called in now, remain so.
Of course we don't add stuff needlessly, but here the fix is trivial.

> always be GFP_NOIO under the two situations, __or__ GFP_KERNEL if we
> choose to ignore the very corner case like other USB drivers.  So hardcoded
> GFP_NOIO or GFP_KERNEL should be accepted.
> 
> Oliver, do you have objections on not adding the memflags parameter now?

Yes, it is a change of almost no gain and a known problem.
It should be added with mem_flags.
 
> Williams, looks there is another problem in your patch, sorry for not
> finding it previously.  usb_autopm_get_interface() need to be called before
> submitting URB inside usbnet_status_start(), and usb_autopm_put_interface()
> need to be called after killing URB inside usbnet_status_stop(). Otherwise
> your patch doesn't work as you expected under runtime PM situation.

Again, no. This is a generic API. It may be called for devices which need
their status polled forever and we cannot block them from sleeping.
If a driver wants to block suspend while a status URB is on, it should
call the autopm method. This is the way we do it while the connection
is up.

	Regards
		Oliver

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

[Ming Lei]

On Tue, Apr 16, 2013 at 3:57 PM, Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org> wrote:
> On Tuesday 16 April 2013 09:15:45 Ming Lei wrote:
>> On Mon, Apr 15, 2013 at 11:59 PM, Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >
>> > So, what was the general consensus on this one?  I know Oliver signed
>> > off on it, but the discussion about memflags seemed to die out without a
>> > specific conclusion.  davem might be looking for that conclusion before
>> > moving forward with the series :)
>>
>> Suggest to remove the memflags parameter, because:
>>
>> - the probable issue addressed by introducing memflags is a general issue
>> of all USB drivers, also very corener one, not a specific one on usbnet, and
>> the issue only exists on devices with at least one mass storage interface and
>> another non-mass-storage interface, and it is not considered by other USB
>> drivers now.
>
> Generally, saying that somebody else has a problem is not an argument
> as long as the fix is very simple. Of course it is a corner case, but why
> fail to solve it as long as the cost is extremely low?

The cost isn't low because users have to decide(learn) when to use GFP_NOIO
and when to use GFP_KERNEL, and GFP_NOIO isn't easy to use,
especially in the corner case which isn't easy to understand too. I bet few of
guys can think of the case and the usage if they don't recall previous threads.

If you want to avoid the corner case, just hardcode GFP_NOIO in the API. As
we see, both current usage of the API may have the corner case.

>
>> - usbnet_status_start() is called from either probe() or work queue scheduled
>> from probe(), if we want to address the probable issue, the memflags should
>
> No, we export this symbol. So we keep it generic if that is possible at low cost.
> We cannot assume that the conditions it would be called in now, remain so.
> Of course we don't add stuff needlessly, but here the fix is trivial.

Why we can't assume the conditions?  The API is introduced just for solving
one specific problem of sierra net, and shouldn't apply to general situations
and its usage is __not__ encouraged since it introduces extra power
consumption. If you have other use cases which need the API, please post
them out for discussion.

In fact, it is sort of a workaround for sierra device only, so we don't
need to make it general in the extreme, IMO.

The fix isn't trivial since GFP_NOIO isn't trivial and the corner case
isn't easy
to understand, as I said above.

>
>> always be GFP_NOIO under the two situations, __or__ GFP_KERNEL if we
>> choose to ignore the very corner case like other USB drivers.  So hardcoded
>> GFP_NOIO or GFP_KERNEL should be accepted.
>>
>> Oliver, do you have objections on not adding the memflags parameter now?
>
> Yes, it is a change of almost no gain and a known problem.
> It should be added with mem_flags.
>> Williams, looks there is another problem in your patch, sorry for not
>> finding it previously.  usb_autopm_get_interface() need to be called before
>> submitting URB inside usbnet_status_start(), and usb_autopm_put_interface()
>> need to be called after killing URB inside usbnet_status_stop(). Otherwise
>> your patch doesn't work as you expected under runtime PM situation.
>
> Again, no. This is a generic API. It may be called for devices which need
> their status polled forever and we cannot block them from sleeping.
> If a driver wants to block suspend while a status URB is on, it should
> call the autopm method. This is the way we do it while the connection
> is up.

The API can't be called in atomic context at all since mutex is to be held.

Also I am wondering there is valid use case for such usage, if you have,
please post them out.


Thanks,
-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

On Wednesday 17 April 2013 09:56:33 Ming Lei wrote:
> On Tue, Apr 16, 2013 at 3:57 PM, Oliver Neukum <oliver@neukum.org> wrote:

> > Generally, saying that somebody else has a problem is not an argument
> > as long as the fix is very simple. Of course it is a corner case, but why
> > fail to solve it as long as the cost is extremely low?
> 
> The cost isn't low because users have to decide(learn) when to use GFP_NOIO
> and when to use GFP_KERNEL, and GFP_NOIO isn't easy to use,
> especially in the corner case which isn't easy to understand too. I bet few of
> guys can think of the case and the usage if they don't recall previous threads.

1. Kernel coders generally need to learn when to use which GFP
2. We have code review

> If you want to avoid the corner case, just hardcode GFP_NOIO in the API. As
> we see, both current usage of the API may have the corner case.

That would make it a worse API for little gain.

> >> - usbnet_status_start() is called from either probe() or work queue scheduled
> >> from probe(), if we want to address the probable issue, the memflags should
> >
> > No, we export this symbol. So we keep it generic if that is possible at low cost.
> > We cannot assume that the conditions it would be called in now, remain so.
> > Of course we don't add stuff needlessly, but here the fix is trivial.
> 
> Why we can't assume the conditions?  The API is introduced just for solving
> one specific problem of sierra net, and shouldn't apply to general situations
> and its usage is __not__ encouraged since it introduces extra power
> consumption. If you have other use cases which need the API, please post
> them out for discussion.

If we have a function for starting a status URB we want to use it whenever
it applies, that is also when we need to poll status for internal reason while
an interface is up.

> In fact, it is sort of a workaround for sierra device only, so we don't
> need to make it general in the extreme, IMO.

As soon as we export it, it is available to general use.

> The fix isn't trivial since GFP_NOIO isn't trivial and the corner case
> isn't easy
> to understand, as I said above.

You don't need to understand it any more than you need to understand
the rule for usb_submit_urb(). The rules are the very same. There is no
special effort here. 

> > Again, no. This is a generic API. It may be called for devices which need
> > their status polled forever and we cannot block them from sleeping.
> > If a driver wants to block suspend while a status URB is on, it should
> > call the autopm method. This is the way we do it while the connection
> > is up.
> 
> The API can't be called in atomic context at all since mutex is to be held.

???

I meant block suspend in the sense of disallowing it. Which is very problematic.
The CDC protocols generally support remote wakeup for status information,
so we need to be able to sleep while status is running to accomodate devices
which are intended to be always online.

	Regards
		Oliver

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

[Ming Lei]

On Wed, Apr 17, 2013 at 2:55 PM, Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org> wrote:
> On Wednesday 17 April 2013 09:56:33 Ming Lei wrote:
>> On Tue, Apr 16, 2013 at 3:57 PM, Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org> wrote:
>
>> > Generally, saying that somebody else has a problem is not an argument
>> > as long as the fix is very simple. Of course it is a corner case, but why
>> > fail to solve it as long as the cost is extremely low?
>>
>> The cost isn't low because users have to decide(learn) when to use GFP_NOIO
>> and when to use GFP_KERNEL, and GFP_NOIO isn't easy to use,
>> especially in the corner case which isn't easy to understand too. I bet few of
>> guys can think of the case and the usage if they don't recall previous threads.
>
> 1. Kernel coders generally need to learn when to use which GFP
> 2. We have code review
>
>> If you want to avoid the corner case, just hardcode GFP_NOIO in the API. As
>> we see, both current usage of the API may have the corner case.
>
> That would make it a worse API for little gain.
>
>> >> - usbnet_status_start() is called from either probe() or work queue scheduled
>> >> from probe(), if we want to address the probable issue, the memflags should
>> >
>> > No, we export this symbol. So we keep it generic if that is possible at low cost.
>> > We cannot assume that the conditions it would be called in now, remain so.
>> > Of course we don't add stuff needlessly, but here the fix is trivial.
>>
>> Why we can't assume the conditions?  The API is introduced just for solving
>> one specific problem of sierra net, and shouldn't apply to general situations
>> and its usage is __not__ encouraged since it introduces extra power
>> consumption. If you have other use cases which need the API, please post
>> them out for discussion.
>
> If we have a function for starting a status URB we want to use it whenever
> it applies, that is also when we need to poll status for internal reason while
> an interface is up.

For other non-sierra usbnet devices, when an interface is up, the status URB
is scheduled in open() and needn't the API.

>
>> In fact, it is sort of a workaround for sierra device only, so we don't
>> need to make it general in the extreme, IMO.
>
> As soon as we export it, it is available to general use.

It doesn't mean there are general uses, looks you don't provide
one valid general one too, :-)

>
>> The fix isn't trivial since GFP_NOIO isn't trivial and the corner case
>> isn't easy
>> to understand, as I said above.
>
> You don't need to understand it any more than you need to understand
> the rule for usb_submit_urb(). The rules are the very same. There is no
> special effort here.

No, there isn't one rule for the corner case here, and the corner case should
have existed in probe() or cancel queue with reset of all USB drivers, instead
of usbnet only.

Also the rule 3 is a bit obscure, maybe not correct, at least there are much
GFP_KERNEL usages in kthread of usbnet. I am wondering if there are
other usbnet specific memflags rules except for 1 & 6.

Strictly speaking, the rule 5 isn't correct, since it might trigger the corner
case you mentioned, right?

I think we need to review the memflags part of usb_submit_urb() doc now.

>
>> > Again, no. This is a generic API. It may be called for devices which need
>> > their status polled forever and we cannot block them from sleeping.
>> > If a driver wants to block suspend while a status URB is on, it should
>> > call the autopm method. This is the way we do it while the connection
>> > is up.
>>
>> The API can't be called in atomic context at all since mutex is to be held.
>
> ???

+int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
+{
+       int ret = 0;
+
+       WARN_ON_ONCE(dev->interrupt == NULL);
+       if (dev->interrupt) {
+               mutex_lock(&dev->interrupt_mutex);

....

+}

Obviously, the API can't be called in atomic context, and putting runtime PM
inside is reasonable and correct.

>
> I meant block suspend in the sense of disallowing it. Which is very problematic.
> The CDC protocols generally support remote wakeup for status information,
> so we need to be able to sleep while status is running to accomodate devices
> which are intended to be always online.

At least now, for non-sierra drivers, it needn't the API to schedule status URB
which will be started in normal open() path, so won't power on device runtime
unnecessarily. That is what I say the API shouldn't be for a general usage, :-)


Thanks,
-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

On Thursday 18 April 2013 11:51:25 Ming Lei wrote:
> On Wed, Apr 17, 2013 at 2:55 PM, Oliver Neukum <oliver@neukum.org> wrote:
>
> > If we have a function for starting a status URB we want to use it whenever
> > it applies, that is also when we need to poll status for internal reason while
> > an interface is up.
> 
> For other non-sierra usbnet devices, when an interface is up, the status URB
> is scheduled in open() and needn't the API.

But that is the very point. This API is used from _within_ open.
We cannot make every open() use GFP_NOIO

> > You don't need to understand it any more than you need to understand
> > the rule for usb_submit_urb(). The rules are the very same. There is no
> > special effort here.
> 
> No, there isn't one rule for the corner case here, and the corner case should
> have existed in probe() or cancel queue with reset of all USB drivers, instead
> of usbnet only.

The same rule applies to usb_submit_urb(), too.
 
> Also the rule 3 is a bit obscure, maybe not correct, at least there are much
> GFP_KERNEL usages in kthread of usbnet. I am wondering if there are
> other usbnet specific memflags rules except for 1 & 6.
> 
> Strictly speaking, the rule 5 isn't correct, since it might trigger the corner
> case you mentioned, right?
> 
> I think we need to review the memflags part of usb_submit_urb() doc now.

Yes.

> +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> +{
> +       int ret = 0;
> +
> +       WARN_ON_ONCE(dev->interrupt == NULL);
> +       if (dev->interrupt) {
> +               mutex_lock(&dev->interrupt_mutex);
> 
> ....
> 
> +}
> 
> Obviously, the API can't be called in atomic context, and putting runtime PM
> inside is reasonable and correct.

Yes, but how is it relevant. What allows us to conclude that a driver does not want
runtime PM while a status URB is running?

> > I meant block suspend in the sense of disallowing it. Which is very problematic.
> > The CDC protocols generally support remote wakeup for status information,
> > so we need to be able to sleep while status is running to accomodate devices
> > which are intended to be always online.
> 
> At least now, for non-sierra drivers, it needn't the API to schedule status URB
> which will be started in normal open() path, so won't power on device runtime
> unnecessarily. That is what I say the API shouldn't be for a general usage, :-)

But many drivers, e.g. cdc-ether, cdc-ncm and cdc-mbim want to be able
to runtime suspend while the device is open.

	Regards
		Oliver

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

[Dan Williams]

On Fri, 2013-04-19 at 10:25 +0200, Oliver Neukum wrote:
> On Thursday 18 April 2013 11:51:25 Ming Lei wrote:
> > On Wed, Apr 17, 2013 at 2:55 PM, Oliver Neukum <oliver@neukum.org> wrote:
> >
> > > If we have a function for starting a status URB we want to use it whenever
> > > it applies, that is also when we need to poll status for internal reason while
> > > an interface is up.
> > 
> > For other non-sierra usbnet devices, when an interface is up, the status URB
> > is scheduled in open() and needn't the API.
> 
> But that is the very point. This API is used from _within_ open.
> We cannot make every open() use GFP_NOIO
> 
> > > You don't need to understand it any more than you need to understand
> > > the rule for usb_submit_urb(). The rules are the very same. There is no
> > > special effort here.
> > 
> > No, there isn't one rule for the corner case here, and the corner case should
> > have existed in probe() or cancel queue with reset of all USB drivers, instead
> > of usbnet only.
> 
> The same rule applies to usb_submit_urb(), too.
>  
> > Also the rule 3 is a bit obscure, maybe not correct, at least there are much
> > GFP_KERNEL usages in kthread of usbnet. I am wondering if there are
> > other usbnet specific memflags rules except for 1 & 6.
> > 
> > Strictly speaking, the rule 5 isn't correct, since it might trigger the corner
> > case you mentioned, right?
> > 
> > I think we need to review the memflags part of usb_submit_urb() doc now.
> 
> Yes.
> 
> > +int usbnet_status_start(struct usbnet *dev, gfp_t mem_flags)
> > +{
> > +       int ret = 0;
> > +
> > +       WARN_ON_ONCE(dev->interrupt == NULL);
> > +       if (dev->interrupt) {
> > +               mutex_lock(&dev->interrupt_mutex);
> > 
> > ....
> > 
> > +}
> > 
> > Obviously, the API can't be called in atomic context, and putting runtime PM
> > inside is reasonable and correct.
> 
> Yes, but how is it relevant. What allows us to conclude that a driver does not want
> runtime PM while a status URB is running?
> 
> > > I meant block suspend in the sense of disallowing it. Which is very problematic.
> > > The CDC protocols generally support remote wakeup for status information,
> > > so we need to be able to sleep while status is running to accomodate devices
> > > which are intended to be always online.
> > 
> > At least now, for non-sierra drivers, it needn't the API to schedule status URB
> > which will be started in normal open() path, so won't power on device runtime
> > unnecessarily. That is what I say the API shouldn't be for a general usage, :-)
> 
> But many drivers, e.g. cdc-ether, cdc-ncm and cdc-mbim want to be able
> to runtime suspend while the device is open.

So how do we go forward from here...  I'm happy to update the patchset
with anything needed to get davem to apply it.  Have we decided to keep
the memflags for now, and thus should I just resubmit with a summary of
the discussion under the ---?

If for whatever reason we do find out the memflags aren't really needed,
they can always be removed later since this isn't userspace API.  But I
think they're useful at this point.

Dan

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

[Oliver Neukum]

On Wednesday 24 April 2013 07:31:25 Dan Williams wrote:

> So how do we go forward from here...  I'm happy to update the patchset
> with anything needed to get davem to apply it.  Have we decided to keep
> the memflags for now, and thus should I just resubmit with a summary of
> the discussion under the ---?
> 
> If for whatever reason we do find out the memflags aren't really needed,
> they can always be removed later since this isn't userspace API.  But I
> think they're useful at this point.

I suggest if Ming Lei doesn't comment till Friday, please resubmit as is.

	Regards
		Oliver

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

[Bjørn Mork]

Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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 linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

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

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

[Bjørn Mork]

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