Re: [RFC] USB: cdc-wdm: Extend and improve subdriver interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Oliver Neukum <oneukum@xxxxxxx> writes:

> On Tuesday 04 September 2012 11:13:52 Bjørn Mork wrote:
>
> Hi,
>
>> there is some work going on trying to support CDC MBIM devices in
>> Linux ( http://www.usb.org/developers/devclass_docs/MBIM10.zip )
>
> Yet another protocol.

Yes.  But this time with some hope of multi-vendor support, given that
Microsoft points to it for Windows 8 Mobile Broadband device support:
http://msdn.microsoft.com/en-us/library/windows/hardware/mbim-based-mobile-broadband-requirements-for-windows.aspx


>> The protocol is based on CDC NCM with a rather complex control protocol
>> ecapsulated in CDC using SendEncapsulatedCommand and
>> GetEncapsulatedResponse, similar to how QMI/wwan devices combine CDC ECM
>> with the Qualcomm QMI control protocol.
>> 
>> Based on these similarities, I proposed to reuse as much as possible of
>
> I am afraid cdc_ncm is not very clean at present. The bundling of network
> packets into larger units should be done so that usbnet is aware of what
> is happening. In particular starting a timer inside a subdriver is very dirty.

Yes, I expect that major changes to cdc_ncm will be necessary, and
pointing to it could be wrong from my side? It just seemed natural to
try to reuse any existing code.

>> the cdc-ncm and cdc-wdm drivers, creating a construct similar to that
>> provided by qmi_wwan: Exporting the control protocol as a /dev/cdc-wdmX
>> character device to some userspace application doing all the complex
>> policy decisions. Greg Suarez has started out on this track, but has
>
> Is code available to be seen?

Dunno how far Greg is on that.  I haven't seen it yet, in any case.

>> found that the cdc-wdm subdriver interface must be extended somewhat for
>> this to be possible.  At the very least, the main driver need to handle
>> some notifications which are ignored by cdc-wdm.  And depending on which
>
> Which ones?

The ones which are already handled by cdc_ncm I believe:

USB_CDC_NOTIFY_NETWORK_CONNECTION and USB_CDC_NOTIFY_SPEED_CHANGE.
cdc-wdm will just debug print USB_CDC_NOTIFY_NETWORK_CONNECTION and
ignore it, but will log an error if it sees USB_CDC_NOTIFY_SPEED_CHANGE

(This point should in theory apply to qmi_wwan as well, but I have not
yet seen a QMI device sending either of these notifications so the
theoretical problem is currently ignored)


>> CDC MBIM features we are going to support, there is also a possibility
>> that the main driver will have to intercept messages between the
>> userspace and the cdc-wdm subdriver.
>
> Ouch.

yes.  Reading the spec terrified me.  Among other issues, it allows
multiplexing several IP connections over a single set of bulk in/out
endpoints, where an "IP connection" probably will have to map logically
to a network device.

>> The following patch is meant as a RFC for a basic interface change,
>> adding the flexibility the new driver will need.  I would very much
>> welcome comments on
>>  - the whole concept of doing the same for MBIM as for QMI
>
> Sensible.
>
>>  - whether data interception may be acceptable at all
>
> in one direction yes, the other is very problematic

I believe we can get away with the main driver only looking at messages
received from the device, without changing or filtering them.

>>  - the proposed symbol change, including restricting it to GPL
>>    drivers
>
> The changes are mainly sensible. Simply changing the license is not nice.
> It seems to me, once something has been allowed, it should stay allowed.
>
>>  - anything else that pops up in your head while reading this :-)
>> 
>> The RFC patch also includes the oneline change for qmi_wwan to use the
>> new API. If this approach is acceptable, I would like to push these
>> changes through net-next to reduce the cross-tree dependencies.
>
> This is the brute force approach:
>
> @@ -224,6 +224,11 @@ static void wdm_int_callback(struct urb *urb)
>  		goto exit;
>  	}
>  
> +	/* check for callback function */
> +	if (desc->driver_info->status &&
> +	    desc->driver_info->status(desc->intf, urb))
> +	        goto exit;
> +
>
> And it breaks the status model. This particular approach is not good.
> Even with MBIM you want the driver to retrieve the responses. So the
> correct approach would be to intercept the responses to GET_ENCAPSULATED_RESPONSE
> and filter them, more or less like a line discipline.

I expected the desc->driver_info->status() to return true only for the
notifications it handled, which cannot include
USB_CDC_NOTIFY_RESPONSE_AVAILABLE as you note.

I am not sure I understand how else you would filter the notifications?

Note: This was not meant for the data interception I mentioned above.
That was just a pre-warning for things chich may be required.  Greg did
include it in an early patch, but I made him drop it for now as I wasn't
convinced it was needed.  Reading the MBIM standard made me more
unsure.  You are of course right that we want cdc_wdm to *always* deal
with USB_CDC_NOTIFY_RESPONSE_AVAILABLE, sending
GET_ENCAPSULATED_RESPONSE, and then possibly forward the data to the
main driver for filtering/inspection.

I see that I should have included the data interception hook, or not
have mentioned it at all. Sorry for any confusion.


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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux