| Message ID | 1357318096.5370.15.camel@dcbw.foobar.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dan Williams <dcbw@redhat.com> writes: > On 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. 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- 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
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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. 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 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 *);
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(-)