From: Phil Sutter <hidden> Date: 2011-07-27 14:12:46
Hello everyone,
I am testing the above module on linux-2.6.39.2 which should contain the
latest changes to at least sierra_net.c. Although I am able to bring the
module up and then can get traffic through it, initialisation seems to
be a little picky.
After connecting the module via USB, I get the following final
initialisation message:
| Jul 27 14:01:15 (none) user.info kernel: [166371.982356] sierra_net 1-1:1.7: wwan0: register 'sierra_net' at usb-0000:00:08.2-1, Sierra Wireless USB-to-WWAN Modem, 4a:82:22:b9:05:07
Doing nothing (successfully), the driver starts printing errors after a
while:
| Jul 27 14:02:15 (none) user.err kernel: [166432.201461] sierra_net 1-1:1.7: wwan0: Submit SYNC failed -32
| Jul 27 14:02:15 (none) user.err kernel: [166432.201609] sierra_net 1-1:1.7: wwan0: Send SYNC failed, status -32
| Jul 27 14:02:15 (none) user.err kernel: [166432.205446] sierra_net 1-1:1.7: wwan0: Submit SYNC failed -32
| Jul 27 14:02:15 (none) user.err kernel: [166432.205590] sierra_net 1-1:1.7: wwan0: Send SYNC failed, status -32
The above messages are the first ones to appear (so there's a delay of
60 seconds in between), and they repeat each 2 seconds from then on.
Depending on how I continue at that point, results vary:
a) 'ip link set wwan0 up': setting the interface up stops the above error
messages from being printed. This worked every time I tried.
b) Sending 'ATZ' on the control-tty: this makes the error-messages
disappear for about 6 seconds, so two iterations of the sync-timer
seem to succeed. When I then try to continue initialisation, I usually
get to 'AT+C' (for AT+CPIN='1234'), then the control-tty dies (neither
echoing of typed characters, nor feedback from the modem printed).
Trying a) from the state after b) indeed makes the error-messages go
away, but the tty stays dead until I power-cycle the module. Needless to
say, without the control-tty the module is completely useless.
My first thought was that the driver shouldn't try to SYNC while the
interface being down, but apparently sierra_net_send_sync() doesn't get
called anymore after setting the interface up.
So in order to prevent the module from dieing, I need to up the
interface before initialisation via AT-interface.
Another interesting aspect: the above error stays gone after the
interface has been upped once, even if it's brought down right
afterwards without doing anything else. OK, not completely - there needs
to be a little delay in which the interface stays up, but a tenth of a
second was enough.
What else can I do to track this problem down further? What additional
information do you need from me? Any advice is highly appreciated, of
course!
Greetings, Phil
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Dan Williams <hidden> Date: 2013-01-04 16:46:02
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware. Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close. The URB is still
killed/re-submitted for suspend/resume, as before.
Signed-off-by: Dan Williams <redacted>
---
Oliver: alternatively, is there a problem with *always*
submitting the interrupt URB, and then simply not calling
the subdriver's .status function when the netdev is
closed? That would be a much simpler patch.
drivers/net/usb/usbnet.c | 43 +++++++++++++++++++++++++++++++------------
include/linux/usb/usbnet.h | 3 +++
2 files changed, 34 insertions(+), 12 deletions(-)
@@ -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,
@@ -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)(structusbnet*,structusb_interface*);
From: Dan Williams <hidden> Date: 2013-01-04 16:49:11
The driver and firmware sync up through SYNC messages, and the
firmware's affirmative reply to these SYNC messages appears to be the
"Reset" indication received via the status interrupt endpoint. Thus the
driver needs the status interrupt endpoint always active so that the
Reset indication can be received even if the netdev is closed, which is
the case right after device insertion.
Signed-off-by: Dan Williams <redacted>
---
From: Oliver Neukum <oliver@neukum.org> Date: 2013-01-04 22:16:29
On Friday 04 January 2013 10:48:16 Dan Williams wrote:
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware. Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close. The URB is still
killed/re-submitted for suspend/resume, as before.
Signed-off-by: Dan Williams <redacted>
---
Oliver: alternatively, is there a problem with *always*
submitting the interrupt URB, and then simply not calling
the subdriver's .status function when the netdev is
closed? That would be a much simpler patch.
That is quite radical. We have no idea what a device
does when we do not react to a status update. I would
much prefer to not take the risk.
Besides, we don't use bandwidth if we don't have to.
Regards
Oliver
From: Dan Williams <hidden> Date: 2013-01-05 01:24:19
On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
On Friday 04 January 2013 10:48:16 Dan Williams wrote:
quoted
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware. Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close. The URB is still
killed/re-submitted for suspend/resume, as before.
Signed-off-by: Dan Williams <redacted>
---
Oliver: alternatively, is there a problem with *always*
submitting the interrupt URB, and then simply not calling
the subdriver's .status function when the netdev is
closed? That would be a much simpler patch.
That is quite radical. We have no idea what a device
does when we do not react to a status update. I would
much prefer to not take the risk.
Besides, we don't use bandwidth if we don't have to.
Ok, so scratch the alternative. Thus, does the posted patch look like
the right course of action?
If I wasn't clear enough before, sierra_net needs to listen to the
status interrupt URB to receive the custom Restart indication as part of
the driver's device setup. Thus for sierra_net at least, tying the
status interrupt URB submission to device open/close isn't right.
I'd previously done a patch to handle this all in sierra_net, but the
problem there is suspend/resume: without directly accessing the usbnet
structure's ->suspend_count member (icky!) sierra_net can't correctly
kill/submit the URB itself. So I went with a flag to usbnet that Sierra
can set.
Dan
From: Oliver Neukum <oliver@neukum.org> Date: 2013-01-05 11:01:54
On Friday 04 January 2013 19:26:33 Dan Williams wrote:
On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
quoted
On Friday 04 January 2013 10:48:16 Dan Williams wrote:
quoted
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware. Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close. The URB is still
killed/re-submitted for suspend/resume, as before.
Signed-off-by: Dan Williams <redacted>
---
Oliver: alternatively, is there a problem with *always*
submitting the interrupt URB, and then simply not calling
the subdriver's .status function when the netdev is
closed? That would be a much simpler patch.
That is quite radical. We have no idea what a device
does when we do not react to a status update. I would
much prefer to not take the risk.
Besides, we don't use bandwidth if we don't have to.
Ok, so scratch the alternative. Thus, does the posted patch look like
the right course of action?
In principle yes.
If I wasn't clear enough before, sierra_net needs to listen to the
status interrupt URB to receive the custom Restart indication as part of
the driver's device setup. Thus for sierra_net at least, tying the
status interrupt URB submission to device open/close isn't right.
So, there seems to be an inevitable race before probe() is called.
Have you looked at FLAG_AVOID_UNLINK_URBS ?
I'd previously done a patch to handle this all in sierra_net, but the
problem there is suspend/resume: without directly accessing the usbnet
structure's ->suspend_count member (icky!) sierra_net can't correctly
kill/submit the URB itself. So I went with a flag to usbnet that Sierra
can set.
That is absolutely the right way to do it.
Regards
Oliver
On Friday 04 January 2013 19:26:33 Dan Williams wrote:
quoted
I'd previously done a patch to handle this all in sierra_net, but the
problem there is suspend/resume: without directly accessing the usbnet
structure's ->suspend_count member (icky!) sierra_net can't correctly
kill/submit the URB itself. So I went with a flag to usbnet that Sierra
can set.
That is absolutely the right way to do it.
Yes.
Just a comment regarding the ->suspend_count: Are you absolutely sure
you need to look at that, Dan? usbnet uses it to handle suspend/resume
for minidrivers with an unknown number of interfaces, without knowing
whether it is the control or data interface which is suspended or
resumed first. By using the counter it can ensure that the correct
action is taken exactly once regardless of this.
The sierra_net minidriver has the advantage of knowing that there always
is only *one* USB interface being suspended and resumed. So you don't
have to care about ->suspend_count. Just do whatever you need to do on
suspend and resume.
Bjørn
From: Dan Williams <hidden> Date: 2013-01-07 15:22:46
On Sat, 2013-01-05 at 12:01 +0100, Oliver Neukum wrote:
On Friday 04 January 2013 19:26:33 Dan Williams wrote:
quoted
On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
quoted
On Friday 04 January 2013 10:48:16 Dan Williams wrote:
quoted
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware. Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close. The URB is still
killed/re-submitted for suspend/resume, as before.
Signed-off-by: Dan Williams <redacted>
---
Oliver: alternatively, is there a problem with *always*
submitting the interrupt URB, and then simply not calling
the subdriver's .status function when the netdev is
closed? That would be a much simpler patch.
That is quite radical. We have no idea what a device
does when we do not react to a status update. I would
much prefer to not take the risk.
Besides, we don't use bandwidth if we don't have to.
Ok, so scratch the alternative. Thus, does the posted patch look like
the right course of action?
In principle yes.
quoted
If I wasn't clear enough before, sierra_net needs to listen to the
status interrupt URB to receive the custom Restart indication as part of
the driver's device setup. Thus for sierra_net at least, tying the
status interrupt URB submission to device open/close isn't right.
So, there seems to be an inevitable race before probe() is called.
Yeah, you're right, we need Sierra to send the SYNC message *after* bind
is called. Is there an existing "after bind" hook or do we need to
create one?
Have you looked at FLAG_AVOID_UNLINK_URBS ?
Not yet, but I will.
Thanks,
Dan
quoted
I'd previously done a patch to handle this all in sierra_net, but the
problem there is suspend/resume: without directly accessing the usbnet
structure's ->suspend_count member (icky!) sierra_net can't correctly
kill/submit the URB itself. So I went with a flag to usbnet that Sierra
can set.
That is absolutely the right way to do it.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Dan Williams <hidden> Date: 2013-01-14 17:49:32
On Sat, 2013-01-05 at 12:01 +0100, Oliver Neukum wrote:
On Friday 04 January 2013 19:26:33 Dan Williams wrote:
quoted
On Fri, 2013-01-04 at 23:16 +0100, Oliver Neukum wrote:
quoted
On Friday 04 January 2013 10:48:16 Dan Williams wrote:
quoted
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware. Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close. The URB is still
killed/re-submitted for suspend/resume, as before.
Signed-off-by: Dan Williams <redacted>
---
Oliver: alternatively, is there a problem with *always*
submitting the interrupt URB, and then simply not calling
the subdriver's .status function when the netdev is
closed? That would be a much simpler patch.
That is quite radical. We have no idea what a device
does when we do not react to a status update. I would
much prefer to not take the risk.
Besides, we don't use bandwidth if we don't have to.
Ok, so scratch the alternative. Thus, does the posted patch look like
the right course of action?
In principle yes.
quoted
If I wasn't clear enough before, sierra_net needs to listen to the
status interrupt URB to receive the custom Restart indication as part of
the driver's device setup. Thus for sierra_net at least, tying the
status interrupt URB submission to device open/close isn't right.
So, there seems to be an inevitable race before probe() is called.
Have you looked at FLAG_AVOID_UNLINK_URBS ?
So that looks like it only applies to the bulk URBs, what was your
suggestion here? Sierra would want the same behavior as it currently
has (kill data urbs on stop/start) but only the interrupt urb needs to
be kept alive over stop/start.
Dan
From: Dan Williams <hidden> Date: 2013-02-06 18:36:27
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware. Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close. The URB is still
killed/re-submitted for suspend/resume, as before.
Signed-off-by: Dan Williams <redacted>
---
Note: unchanged from previous version, but rebased.
drivers/net/usb/usbnet.c | 43 +++++++++++++++++++++++++++++++------------
include/linux/usb/usbnet.h | 3 +++
2 files changed, 34 insertions(+), 12 deletions(-)
@@ -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,
@@ -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)(structusbnet*,structusb_interface*);
From: Oliver Neukum <oliver@neukum.org> Date: 2013-02-06 20:19:39
On Wednesday 06 February 2013 12:36:38 Dan Williams wrote:
Some drivers (ex sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware. Allow sub-drivers to set
a flag that submits the status interrupt URB on probe and
keeps the URB alive over device open/close. The URB is still
killed/re-submitted for suspend/resume, as before.
Given your description in the later patch, which uses this feature,
it seems to me that we can be more efficient if we include infrastructure
to determine whether the interrupt URB is still needed under
some circumstances. Could we put this on hold until we are clear on
the requirements of the protocol?
Regards
Oliver
From: Dan Williams <hidden> Date: 2013-03-28 16:29:30
Some drivers (sierra_net) need the status interrupt URB
active even when the device is closed, because they receive
custom indications from firmware. Add functions to refcount
the status interrupt URB submit/kill operation so that
sub-drivers and the generic driver don't fight over whether
the status interrupt URB is active or not.
A sub-driver can call usbnet_status_start() at any time, but
the URB is only submitted the first time the function is
called. Likewise, when the sub-driver is done with the URB,
it calls usbnet_status_stop() but the URB is only killed when
all users have stopped it. The URB is still killed and
re-submitted for suspend/resume, as before, with the same
refcount it had at suspend.
Signed-off-by: Dan Williams <redacted>
---
@@ -252,6 +252,43 @@ static int init_status (struct usbnet *dev, struct usb_interface *intf)return0;}+/* Submit the interrupt URB if it hasn't been submitted yet */+intusbnet_status_start(structusbnet*dev,gfp_tmem_flags)+{+intret=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);+returnret;+}+EXPORT_SYMBOL_GPL(usbnet_status_start);++/* Kill the interrupt URB if all submitters want it killed */+voidusbnet_status_stop(structusbnet*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.*Somelinkprotocolsbatchpackets,sotheirrx_fixuppaths*canreturnclonesaswellasjustmodifytheoriginalskb.
@@ -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);
@@ -1585,9 +1624,13 @@ int usbnet_resume (struct usb_interface *intf)intretval;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))){