Hello,
there is some work going on trying to support CDC MBIM devices in
Linux ( http://www.usb.org/developers/devclass_docs/MBIM10.zip )
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
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
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
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.
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
- whether data interception may be acceptable at all
- the proposed symbol change, including restricting it to GPL
drivers
- 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.
>From fda3389d16e94ac358aff9f7af72b14320a3539d Mon Sep 17 00:00:00 2001
From: Greg Suarez <gpsuarez2512@xxxxxxxxx>
Date: Thu, 16 Aug 2012 10:49:03 +0200
Subject: [RFC] USB: cdc-wdm: Extend and improve subdriver interface
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
New users of the subdriver interface need to
intercept the USB_CDC_NOTIFY_NETWORK_CONNECTION and
USB_CDC_NOTIFY_SPEED_CHANGE events. Adding a status
callback to enable this.
There are use cases where data interception is required
as well, and we anticipate callbacks enabling this may be
added at a later stage. Wrapping the subdriver callbacks
in a struct to reduce the number of arguments to the
register function and to enable such future extentions
without having to update all existing users.
The old subdriver interface provided no more than a full
cdc-wdm driver with few possibilities for the registering
driver to modify or inspect the data transmitted between
the device and the subdriver. Exporting this interface to
non GPL users made sense as it provided no more access to
the driver internals than any device with CDC WDM
descriptors would have. The new interface is more
powerful, allowing user specific handling of some events,
and it may even be extended with data interception
callbacks. This makes the interface unsuitable for non
GPL users. Tightening the symbol license now, before
there are any such users, prevents future problems.
Updating the only in-kernel user of this interface,
qmi_wwan, at the same time to make this a one step
transition.
Cc: Oliver Neukum <oneukum@xxxxxxx>
Signed-off-by: Greg Suarez <gpsuarez2512@xxxxxxxxx>
[bjorn@xxxxxxx: code style, qmi_wwan, and symbol license]
Signed-off-by: Bjørn Mork <bjorn@xxxxxxx>
---
drivers/net/usb/qmi_wwan.c | 6 ++++-
drivers/usb/class/cdc-wdm.c | 57 ++++++++++++++++++++++++++-----------------
include/linux/usb/cdc-wdm.h | 9 ++++++-
3 files changed, 47 insertions(+), 25 deletions(-)
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 328397c..d9586b3 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -82,6 +82,10 @@ static int qmi_wwan_cdc_wdm_manage_power(struct usb_interface *intf, int on)
return qmi_wwan_manage_power(dev, on);
}
+static const struct wdm_driver_info wdm_drv_info = {
+ .manage_power = &qmi_wwan_cdc_wdm_manage_power,
+};
+
/* collect all three endpoints and register subdriver */
static int qmi_wwan_register_subdriver(struct usbnet *dev)
{
@@ -108,7 +112,7 @@ static int qmi_wwan_register_subdriver(struct usbnet *dev)
atomic_set(&info->pmcount, 0);
/* register subdriver */
- subdriver = usb_cdc_wdm_register(info->control, &dev->status->desc, 512, &qmi_wwan_cdc_wdm_manage_power);
+ subdriver = usb_cdc_wdm_register(info->control, &dev->status->desc, 512, &wdm_drv_info);
if (IS_ERR(subdriver)) {
dev_err(&info->control->dev, "subdriver registration failed\n");
rv = PTR_ERR(subdriver);
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 65a55ab..627574b 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -101,7 +101,7 @@ struct wdm_device {
int rerr;
struct list_head device_list;
- int (*manage_power)(struct usb_interface *, int);
+ const struct wdm_driver_info *driver_info;
};
static struct usb_driver wdm_driver;
@@ -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;
+
switch (dr->bNotificationType) {
case USB_CDC_NOTIFY_RESPONSE_AVAILABLE:
dev_dbg(&desc->intf->dev,
@@ -590,8 +595,8 @@ static int wdm_open(struct inode *inode, struct file *file)
rv = 0;
}
mutex_unlock(&desc->wlock);
- if (desc->count == 1)
- desc->manage_power(intf, 1);
+ if (desc->count == 1 && desc->driver_info->manage_power)
+ desc->driver_info->manage_power(intf, 1);
usb_autopm_put_interface(desc->intf);
out:
mutex_unlock(&wdm_mutex);
@@ -613,7 +618,8 @@ static int wdm_release(struct inode *inode, struct file *file)
if (!test_bit(WDM_DISCONNECTING, &desc->flags)) {
dev_dbg(&desc->intf->dev, "wdm_release: cleanup");
kill_urbs(desc);
- desc->manage_power(desc->intf, 0);
+ if (desc->driver_info->manage_power)
+ desc->driver_info->manage_power(desc->intf, 0);
} else {
/* must avoid dev_printk here as desc->intf is invalid */
pr_debug(KBUILD_MODNAME " %s: device gone - cleaning up\n", __func__);
@@ -624,6 +630,23 @@ static int wdm_release(struct inode *inode, struct file *file)
return 0;
}
+static int wdm_manage_power(struct usb_interface *intf, int on)
+{
+ /* need autopm_get/put here to ensure the usbcore sees the new value */
+ int rv = usb_autopm_get_interface(intf);
+ if (rv < 0)
+ goto err;
+
+ intf->needs_remote_wakeup = on;
+ usb_autopm_put_interface(intf);
+err:
+ return rv;
+}
+
+static const struct wdm_driver_info wdm_drv_info = {
+ .manage_power = &wdm_manage_power,
+};
+
static const struct file_operations wdm_fops = {
.owner = THIS_MODULE,
.read = wdm_read,
@@ -666,7 +689,7 @@ static void wdm_rxwork(struct work_struct *work)
/* --- hotplug --- */
static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor *ep,
- u16 bufsize, int (*manage_power)(struct usb_interface *, int))
+ int bufsize, const struct wdm_driver_info *drv)
{
int rv = -ENOMEM;
struct wdm_device *desc;
@@ -751,7 +774,7 @@ static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor
desc
);
- desc->manage_power = manage_power;
+ desc->driver_info = drv;
spin_lock(&wdm_device_list_lock);
list_add(&desc->device_list, &wdm_device_list);
@@ -772,19 +795,6 @@ err:
return rv;
}
-static int wdm_manage_power(struct usb_interface *intf, int on)
-{
- /* need autopm_get/put here to ensure the usbcore sees the new value */
- int rv = usb_autopm_get_interface(intf);
- if (rv < 0)
- goto err;
-
- intf->needs_remote_wakeup = on;
- usb_autopm_put_interface(intf);
-err:
- return rv;
-}
-
static int wdm_probe(struct usb_interface *intf, const struct usb_device_id *id)
{
int rv = -EINVAL;
@@ -828,7 +838,7 @@ next_desc:
goto err;
ep = &iface->endpoint[0].desc;
- rv = wdm_create(intf, ep, maxcom, &wdm_manage_power);
+ rv = wdm_create(intf, ep, maxcom, &wdm_drv_info);
err:
return rv;
@@ -839,6 +849,7 @@ err:
* @intf: usb interface the subdriver will associate with
* @ep: interrupt endpoint to monitor for notifications
* @bufsize: maximum message size to support for read/write
+ * @drv: subdriver callbacks
*
* Create WDM usb class character device and associate it with intf
* without binding, allowing another driver to manage the interface.
@@ -856,11 +867,11 @@ err:
struct usb_driver *usb_cdc_wdm_register(struct usb_interface *intf,
struct usb_endpoint_descriptor *ep,
int bufsize,
- int (*manage_power)(struct usb_interface *, int))
+ const struct wdm_driver_info *drv)
{
int rv = -EINVAL;
- rv = wdm_create(intf, ep, bufsize, manage_power);
+ rv = wdm_create(intf, ep, bufsize, drv);
if (rv < 0)
goto err;
@@ -868,7 +879,7 @@ struct usb_driver *usb_cdc_wdm_register(struct usb_interface *intf,
err:
return ERR_PTR(rv);
}
-EXPORT_SYMBOL(usb_cdc_wdm_register);
+EXPORT_SYMBOL_GPL(usb_cdc_wdm_register);
static void wdm_disconnect(struct usb_interface *intf)
{
diff --git a/include/linux/usb/cdc-wdm.h b/include/linux/usb/cdc-wdm.h
index 719c332..00d1f55 100644
--- a/include/linux/usb/cdc-wdm.h
+++ b/include/linux/usb/cdc-wdm.h
@@ -11,9 +11,16 @@
#ifndef __LINUX_USB_CDC_WDM_H
#define __LINUX_USB_CDC_WDM_H
+#include <linux/usb.h>
+
+struct wdm_driver_info {
+ int (*manage_power)(struct usb_interface *, int);
+ int (*status)(struct usb_interface *, struct urb *);
+};
+
extern struct usb_driver *usb_cdc_wdm_register(struct usb_interface *intf,
struct usb_endpoint_descriptor *ep,
int bufsize,
- int (*manage_power)(struct usb_interface *, int));
+ const struct wdm_driver_info *drv);
#endif /* __LINUX_USB_CDC_WDM_H */
--
1.7.10.4
Thanks,
Bjørn
