Message-ID: <87y5g7nbej.fsf@nemi.mork.no>
Date:	Sat, 05 Jan 2013 11:59:16 +0100
From:	Bjørn Mork <bjorn@...k.no>
To:	Dan Williams <dcbw@...hat.com>
Cc:	Oliver Neukum <oliver@...kum.org>,
	Elina Pasheva <epasheva@...rrawireless.com>,
	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	Rory Filer <rfiler@...rrawireless.com>,
	Phil Sutter <phil@....cc>
Subject: Re: [PATCH 1/2] usbnet: allow status interrupt URB to always be active

Dan Williams <dcbw@...hat.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@...hat.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists