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

login
register
mail settings
Submitter Dan Williams
Date Jan. 4, 2013, 4:48 p.m.
Message ID <1357318096.5370.15.camel@dcbw.foobar.com>
Download mbox | patch
Permalink /patch/209489/
State Rejected
Delegated to: David Miller
Headers show

Comments

Dan Williams - Jan. 4, 2013, 4:48 p.m.
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(-)
Oliver Neukum - Jan. 4, 2013, 10:16 p.m.
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
Dan Williams - Jan. 5, 2013, 1:26 a.m.
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
Bjørn Mork - Jan. 5, 2013, 10:59 a.m.
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
Oliver Neukum - Jan. 5, 2013, 11:01 a.m.
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
Bjørn Mork - Jan. 5, 2013, 11:44 a.m.
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
Oliver Neukum - Jan. 7, 2013, 7:40 a.m.
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
Dan Williams - Jan. 7, 2013, 3:21 p.m.
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
Dan Williams - Jan. 7, 2013, 3:25 p.m.
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
Ming Lei - Jan. 11, 2013, 3:06 a.m.
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
Dan Williams - Jan. 14, 2013, 5:23 p.m.
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
Dan Williams - Jan. 14, 2013, 5:52 p.m.
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
Ming Lei - Jan. 15, 2013, 1:13 a.m.
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

Patch

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 *);