[RFC,net-next,1/2] usb: class: cdc-wdm: add control type

Message ID 1619777783-24116-1-git-send-email-loic.poulain@linaro.org
State New
Headers show
Series
  • [RFC,net-next,1/2] usb: class: cdc-wdm: add control type
Related show

Commit Message

Loic Poulain April 30, 2021, 10:16 a.m.
Add type parameter to usb_cdc_wdm_register function in order to
specify which control protocol the cdc-wdm channel is transporting.
It will be required for exposing the channel(s) through WWAN framework.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

---
 drivers/net/usb/cdc_mbim.c       |  1 +
 drivers/net/usb/huawei_cdc_ncm.c |  1 +
 drivers/net/usb/qmi_wwan.c       |  3 ++-
 drivers/usb/class/cdc-wdm.c      | 13 +++++++++----
 include/linux/usb/cdc-wdm.h      | 16 +++++++++++++++-
 5 files changed, 28 insertions(+), 6 deletions(-)

-- 
2.7.4

Comments

Oliver Neukum April 30, 2021, 10:32 a.m. | #1
Am Freitag, den 30.04.2021, 12:16 +0200 schrieb Loic Poulain:
> Add type parameter to usb_cdc_wdm_register function in order to

> specify which control protocol the cdc-wdm channel is transporting.

> It will be required for exposing the channel(s) through WWAN framework.


Hi,

that is an interesting framework.
Some issues though.

	Regards
		Oliver

> +/**

> + * enum usb_cdc_wdm_type - CDC WDM endpoint type

> + * @USB_CDC_WDM_UNKNOWN: Unknown type

> + * @USB_CDC_WDM_MBIM: Mobile Broadband Interface Model control

> + * @USB_CDC_WDM_QMI: Qualcomm Modem Interface for modem control

> + * @USB_CDC_WDM_AT: AT commands interface

> + */

> +enum usb_cdc_wdm_type {

> +	USB_CDC_WDM_UNKNOWN,

> +	USB_CDC_WDM_MBIM,

> +	USB_CDC_WDM_QMI,

> +	USB_CDC_WDM_AT

> +};



If this is supposed to integrate CDC-WDM into a larger subsystem, what
use are private types here? If you do this the protocols need to come
from the common framework.
Oliver Neukum April 30, 2021, 10:39 a.m. | #2
Am Freitag, den 30.04.2021, 12:16 +0200 schrieb Loic Poulain:

> It would then make sense to migrate cdc-wdm to this unified framework

> and register the USB modem control endpoints as standard WWAN control

> ports.


This absolutely makes sense, but I have questions about the
implementation. I am putting comments inline.

	Regards
		Oliver

 
>  static struct usb_driver wdm_driver;

> @@ -203,7 +206,23 @@ static void wdm_in_callback(struct urb *urb)

>  	if (desc->rerr == 0 && status != -EPIPE)

>  		desc->rerr = status;

>  

> -	if (length + desc->length > desc->wMaxCommand) {

> +	if (test_bit(WDM_WWAN_IN_USE, &desc->flags)) {

> +		struct wwan_port *port = desc->wwanp;

> +		struct sk_buff *skb;

> +

> +		/* Forward data to WWAN port */

> +		skb = alloc_skb(length, GFP_ATOMIC);


You want to allocate an skb in the callback? Is that really necessary?

> +		if (skb) {

> +			memcpy(skb_put(skb, length), desc->inbuf, length);

> +			wwan_port_rx(port, skb);

> +		} else {

> +			dev_err(&desc->intf->dev,

> +				"Unable to alloc skb, response discarded\n");

> +		}

> +

> +		/* inbuf has been copied, it is safe to check for outstanding data */

> +		schedule_work(&desc->service_outs_intr);

> +	} else if (length + desc->length > desc->wMaxCommand) {

>  		/* The buffer would overflow */

>  		set_bit(WDM_OVERFLOW, &desc->flags);

>  	} else {

> @@ -699,6 +718,11 @@ static int wdm_open(struct inode *inode, struct file *file)

>  		goto out;

>  	file->private_data = desc;

>  

> +	if (test_bit(WDM_WWAN_IN_USE, &desc->flags)) {

> +		rv = -EBUSY;

> +		goto out;

> +	}

> +

>  	rv = usb_autopm_get_interface(desc->intf);

>  	if (rv < 0) {

>  		dev_err(&desc->intf->dev, "Error autopm - %d\n", rv);

> @@ -794,6 +818,146 @@ static struct usb_class_driver wdm_class = {

>  	.minor_base =	WDM_MINOR_BASE,

>  };

>  

> +/* --- WWAN framework integration --- */

> +#ifdef CONFIG_WWAN

> +static int wdm_wwan_port_start(struct wwan_port *port)

> +{

> +	struct wdm_device *desc = wwan_port_get_drvdata(port);

> +

> +	/* The interface is both exposed via the WWAN framework and as a

> +	 * legacy usbmisc chardev. If chardev is already open, just fail

> +	 * to prevent concurrent usage. Otherwise, switch to WWAN mode.

> +	 */

> +	mutex_lock(&wdm_mutex);

> +	if (desc->count) {

> +		mutex_unlock(&wdm_mutex);

> +		return -EBUSY;

> +	}

> +	set_bit(WDM_WWAN_IN_USE, &desc->flags);

> +	mutex_unlock(&wdm_mutex);

> +

> +	desc->manage_power(desc->intf, 1);

> +

> +	/* Start getting events */

> +	usb_submit_urb(desc->validity, GFP_KERNEL);

> +

> +	/* tx is allowed */

> +	wwan_port_txon(port);


Is the order here correct? This looks like you could get an
event you cannot yet respond to. And you have no error handling.
> +

> +	return 0;

> +}

> +

> +static void wdm_wwan_port_stop(struct wwan_port *port)

> +{

> +	struct wdm_device *desc = wwan_port_get_drvdata(port);

> +

> +	/* Stop all transfers and disable WWAN mode */

> +	kill_urbs(desc);

> +	desc->manage_power(desc->intf, 0);

> +	clear_bit(WDM_READ, &desc->flags);

> +	clear_bit(WDM_WWAN_IN_USE, &desc->flags);

> +}

> +

> +static void wdm_wwan_port_tx_complete(struct urb *urb)

> +{

> +	struct sk_buff *skb = urb->context;

> +	struct wwan_port *port = skb_shinfo(skb)->destructor_arg;

> +

> +	/* Allow new command transfer */

> +	wwan_port_txon(port);

> +	kfree_skb(skb);

> +}

> +

> +static int wdm_wwan_port_tx(struct wwan_port *port, struct sk_buff *skb)

> +{

> +	struct wdm_device *desc = wwan_port_get_drvdata(port);

> +	struct usb_interface *intf = desc->intf;

> +	struct usb_ctrlrequest *req = desc->orq;

> +	int rv;

> +

> +	rv = usb_autopm_get_interface(intf);

> +	if (rv)

> +		return rv;

> +

> +	usb_fill_control_urb(

> +		desc->command,

> +		interface_to_usbdev(intf),

> +		usb_sndctrlpipe(interface_to_usbdev(intf), 0),

> +		(unsigned char *)req,

> +		skb->data,

> +		skb->len,

> +		wdm_wwan_port_tx_complete,

> +		skb

> +	);

> +

> +	req->bRequestType = (USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE);

> +	req->bRequest = USB_CDC_SEND_ENCAPSULATED_COMMAND;

> +	req->wValue = 0;

> +	req->wIndex = desc->inum;

> +	req->wLength = cpu_to_le16(skb->len);

> +

> +	skb_shinfo(skb)->destructor_arg = port;

> +

> +	rv = usb_submit_urb(desc->command, GFP_KERNEL);

> +	if (!rv) /* One transfer at a time, stop TX until URB completion */

> +		wwan_port_txoff(port);

> +

> +	usb_autopm_put_interface(intf);


No, that runtime PM is broken. You have a running transmission.

> +

> +	return rv;

> +}

> +

> +static struct wwan_port_ops wdm_wwan_port_ops = {

> +	.start = wdm_wwan_port_start,

> +	.stop = wdm_wwan_port_stop,

> +	.tx = wdm_wwan_port_tx,

> +};

> +

> +static void wdm_wwan_init(struct wdm_device *desc)

> +{

> +	struct usb_interface *intf = desc->intf;

> +	struct wwan_port *port;

> +

> +	switch (desc->type) {

> +	case USB_CDC_WDM_MBIM:

> +		port = wwan_create_port(&intf->dev, WWAN_PORT_MBIM,

> +					&wdm_wwan_port_ops, desc);

> +		break;

> +	case USB_CDC_WDM_QMI:

> +		port = wwan_create_port(&intf->dev, WWAN_PORT_QMI,

> +					&wdm_wwan_port_ops, desc);

> +		break;

> +	case USB_CDC_WDM_AT:

> +		port = wwan_create_port(&intf->dev, WWAN_PORT_AT,

> +					&wdm_wwan_port_ops, desc);


Just use the common types. This is redundant.

> +		break;

> +	default:

> +		dev_info(&intf->dev, "Unknown control protocol\n");

> +		return;

> +	}

> +

> +	if (IS_ERR(port)) {

> +		dev_err(&intf->dev, "%s: Unable to create WWAN port\n",

> +			dev_name(intf->usb_dev));

> +		return;

> +	}

> +

> +	desc->wwanp = port;

> +}

> +

> +static void wdm_wwan_deinit(struct wdm_device *desc)

> +{

> +	if (!desc->wwanp)

> +		return;

> +

> +	wwan_remove_port(desc->wwanp);

> +	desc->wwanp = NULL;

> +}

> +#else /* CONFIG_WWAN */

> +static void wdm_wwan_init(struct wdm_device *desc) {}

> +static void wdm_wwan_deinit(struct wdm_device *desc) {}

> +#endif /* CONFIG_WWAN */

> +

>  /* --- error handling --- */

>  static void wdm_rxwork(struct work_struct *work)

>  {

> @@ -937,6 +1101,9 @@ static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor

>  		goto err;

>  	else

>  		dev_info(&intf->dev, "%s: USB WDM device\n", dev_name(intf->usb_dev));

> +

> +	wdm_wwan_init(desc);

> +

>  out:

>  	return rv;

>  err:

> @@ -1034,6 +1201,8 @@ static void wdm_disconnect(struct usb_interface *intf)

>  	desc = wdm_find_device(intf);

>  	mutex_lock(&wdm_mutex);

>  

> +	wdm_wwan_deinit(desc);

> +

>  	/* the spinlock makes sure no new urbs are generated in the callbacks */

>  	spin_lock_irqsave(&desc->iuspin, flags);

>  	set_bit(WDM_DISCONNECTING, &desc->flags);
Bjørn Mork May 1, 2021, 10:49 a.m. | #3
Oliver Neukum <oneukum@suse.com> writes:

> This absolutely makes sense,

+1

Thanks for working on this.


Bjørn
Loic Poulain May 3, 2021, 7:47 a.m. | #4
On Sat, 1 May 2021 at 12:49, Bjørn Mork <bjorn@mork.no> wrote:
>

> Oliver Neukum <oneukum@suse.com> writes:

>

> > This absolutely makes sense,

>

> +1


Ok, thanks, then I'll resubmit a proper patch set with comments
addressed once the merge window is closed and net-next open.

Thanks,
Loic

Patch

diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index 5db6627..63b134b 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -168,6 +168,7 @@  static int cdc_mbim_bind(struct usbnet *dev, struct usb_interface *intf)
 		subdriver = usb_cdc_wdm_register(ctx->control,
 						 &dev->status->desc,
 						 le16_to_cpu(ctx->mbim_desc->wMaxControlMessage),
+						 USB_CDC_WDM_MBIM,
 						 cdc_mbim_wdm_manage_power);
 	if (IS_ERR(subdriver)) {
 		ret = PTR_ERR(subdriver);
diff --git a/drivers/net/usb/huawei_cdc_ncm.c b/drivers/net/usb/huawei_cdc_ncm.c
index a87f0da..388a46b 100644
--- a/drivers/net/usb/huawei_cdc_ncm.c
+++ b/drivers/net/usb/huawei_cdc_ncm.c
@@ -96,6 +96,7 @@  static int huawei_cdc_ncm_bind(struct usbnet *usbnet_dev,
 		subdriver = usb_cdc_wdm_register(ctx->control,
 						 &usbnet_dev->status->desc,
 						 1024, /* wMaxCommand */
+						 USB_CDC_WDM_AT,
 						 huawei_cdc_ncm_wdm_manage_power);
 	if (IS_ERR(subdriver)) {
 		ret = PTR_ERR(subdriver);
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 17a0505..fa38471 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -724,7 +724,8 @@  static int qmi_wwan_register_subdriver(struct usbnet *dev)
 
 	/* register subdriver */
 	subdriver = usb_cdc_wdm_register(info->control, &dev->status->desc,
-					 4096, &qmi_wwan_cdc_wdm_manage_power);
+					 4096, USB_CDC_WDM_QMI,
+					 &qmi_wwan_cdc_wdm_manage_power);
 	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 508b1c3..b59f146 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -106,6 +106,8 @@  struct wdm_device {
 
 	struct list_head	device_list;
 	int			(*manage_power)(struct usb_interface *, int);
+
+	enum usb_cdc_wdm_type	type;
 };
 
 static struct usb_driver wdm_driver;
@@ -836,7 +838,8 @@  static void service_interrupt_work(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))
+		      u16 bufsize, enum usb_cdc_wdm_type type,
+		      int (*manage_power)(struct usb_interface *, int))
 {
 	int rv = -ENOMEM;
 	struct wdm_device *desc;
@@ -853,6 +856,7 @@  static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor
 	/* this will be expanded and needed in hardware endianness */
 	desc->inum = cpu_to_le16((u16)intf->cur_altsetting->desc.bInterfaceNumber);
 	desc->intf = intf;
+	desc->type = type;
 	INIT_WORK(&desc->rxwork, wdm_rxwork);
 	INIT_WORK(&desc->service_outs_intr, service_interrupt_work);
 
@@ -977,7 +981,7 @@  static int wdm_probe(struct usb_interface *intf, const struct usb_device_id *id)
 		goto err;
 	ep = &iface->endpoint[0].desc;
 
-	rv = wdm_create(intf, ep, maxcom, &wdm_manage_power);
+	rv = wdm_create(intf, ep, maxcom, USB_CDC_WDM_UNKNOWN, &wdm_manage_power);
 
 err:
 	return rv;
@@ -988,6 +992,7 @@  static int wdm_probe(struct usb_interface *intf, const struct usb_device_id *id)
  * @intf: usb interface the subdriver will associate with
  * @ep: interrupt endpoint to monitor for notifications
  * @bufsize: maximum message size to support for read/write
+ * @type: Type/protocol of the transported data (MBIM, QMI...)
  * @manage_power: call-back invoked during open and release to
  *                manage the device's power
  * Create WDM usb class character device and associate it with intf
@@ -1005,12 +1010,12 @@  static int wdm_probe(struct usb_interface *intf, const struct usb_device_id *id)
  */
 struct usb_driver *usb_cdc_wdm_register(struct usb_interface *intf,
 					struct usb_endpoint_descriptor *ep,
-					int bufsize,
+					int bufsize, enum usb_cdc_wdm_type type,
 					int (*manage_power)(struct usb_interface *, int))
 {
 	int rv;
 
-	rv = wdm_create(intf, ep, bufsize, manage_power);
+	rv = wdm_create(intf, ep, bufsize, type, manage_power);
 	if (rv < 0)
 		goto err;
 
diff --git a/include/linux/usb/cdc-wdm.h b/include/linux/usb/cdc-wdm.h
index 9b895f9..ba9702d 100644
--- a/include/linux/usb/cdc-wdm.h
+++ b/include/linux/usb/cdc-wdm.h
@@ -14,9 +14,23 @@ 
 
 #include <uapi/linux/usb/cdc-wdm.h>
 
+/**
+ * enum usb_cdc_wdm_type - CDC WDM endpoint type
+ * @USB_CDC_WDM_UNKNOWN: Unknown type
+ * @USB_CDC_WDM_MBIM: Mobile Broadband Interface Model control
+ * @USB_CDC_WDM_QMI: Qualcomm Modem Interface for modem control
+ * @USB_CDC_WDM_AT: AT commands interface
+ */
+enum usb_cdc_wdm_type {
+	USB_CDC_WDM_UNKNOWN,
+	USB_CDC_WDM_MBIM,
+	USB_CDC_WDM_QMI,
+	USB_CDC_WDM_AT
+};
+
 extern struct usb_driver *usb_cdc_wdm_register(struct usb_interface *intf,
 					struct usb_endpoint_descriptor *ep,
-					int bufsize,
+					int bufsize, enum usb_cdc_wdm_type type,
 					int (*manage_power)(struct usb_interface *, int));
 
 #endif /* __LINUX_USB_CDC_WDM_H */