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
