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

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

[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.

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(-)

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 */
-- 
2.7.4

[RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration

[Loic Poulain]

The WWAN framework provides a unified way to handle WWAN/modems and
control port(s). It has initially been introduced to support MHI/PCI
modems, offering the same control protocols as the USB variants,
such as MBIM, QMI, AT... The WWAN framework exposes these control
protocols as character devices, similarly to cdc-wdm, but in a
bus agnostic fashion.

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.

Exposing cdc-wdm through WWAN framework normally maintains backward
compatibility, e.g:
    $ qmicli --device-open-qmi -d /dev/wwan0p1QMI --dms-get-ids
instead of
    $ qmicli --device-open-qmi -d /dev/cdc-wdm0 --dms-get-ids

However, some tools may rely on cdc-wdm driver/device name for device
detection. It is then safer to keep the 'legacy' cdc-wdm character
device to prevent any breakage. This is handled in this change by
API mutual exclusion, only one access method can be used at a time,
either cdc-wdm chardev or WWAN API.

Note that unknown channel types (other than MBIM, AT or MBIM) are not
registered to the WWAN framework.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/usb/class/cdc-wdm.c | 171 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 170 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index b59f146..7b798b5 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -23,6 +23,7 @@
 #include <linux/poll.h>
 #include <linux/usb.h>
 #include <linux/usb/cdc.h>
+#include <linux/wwan.h>
 #include <asm/byteorder.h>
 #include <asm/unaligned.h>
 #include <linux/usb/cdc-wdm.h>
@@ -55,6 +56,7 @@ MODULE_DEVICE_TABLE (usb, wdm_ids);
 #define WDM_SUSPENDING		8
 #define WDM_RESETTING		9
 #define WDM_OVERFLOW		10
+#define WDM_WWAN_IN_USE		11
 
 #define WDM_MAX			16
 
@@ -108,6 +110,7 @@ struct wdm_device {
 	int			(*manage_power)(struct usb_interface *, int);
 
 	enum usb_cdc_wdm_type	type;
+	struct wwan_port	*wwanp;
 };
 
 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);
+		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);
+
+	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);
+
+	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);
+		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);
-- 
2.7.4

Re: [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration

[Oliver Neukum]

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);

Re: [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:

> This absolutely makes sense,

+1

Thanks for working on this.


Bjørn

Re: [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration

[Loic Poulain]

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

Re: [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4553 bytes --]

Hi Loic,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Loic-Poulain/usb-class-cdc-wdm-add-control-type/20210430-180831
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 4a52dd8fefb45626dace70a63c0738dbd83b7edb
config: x86_64-randconfig-s022-20210430 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/e5bc3ddfc2b8b4727830f2d4a131d4731e1a0e27
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Loic-Poulain/usb-class-cdc-wdm-add-control-type/20210430-180831
        git checkout e5bc3ddfc2b8b4727830f2d4a131d4731e1a0e27
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: drivers/usb/class/cdc-wdm.o: in function `wdm_in_callback':
>> drivers/usb/class/cdc-wdm.c:217: undefined reference to `wwan_port_rx'


vim +217 drivers/usb/class/cdc-wdm.c

   164	
   165	static void wdm_in_callback(struct urb *urb)
   166	{
   167		unsigned long flags;
   168		struct wdm_device *desc = urb->context;
   169		int status = urb->status;
   170		int length = urb->actual_length;
   171	
   172		spin_lock_irqsave(&desc->iuspin, flags);
   173		clear_bit(WDM_RESPONDING, &desc->flags);
   174	
   175		if (status) {
   176			switch (status) {
   177			case -ENOENT:
   178				dev_dbg(&desc->intf->dev,
   179					"nonzero urb status received: -ENOENT\n");
   180				goto skip_error;
   181			case -ECONNRESET:
   182				dev_dbg(&desc->intf->dev,
   183					"nonzero urb status received: -ECONNRESET\n");
   184				goto skip_error;
   185			case -ESHUTDOWN:
   186				dev_dbg(&desc->intf->dev,
   187					"nonzero urb status received: -ESHUTDOWN\n");
   188				goto skip_error;
   189			case -EPIPE:
   190				dev_err(&desc->intf->dev,
   191					"nonzero urb status received: -EPIPE\n");
   192				break;
   193			default:
   194				dev_err(&desc->intf->dev,
   195					"Unexpected error %d\n", status);
   196				break;
   197			}
   198		}
   199	
   200		/*
   201		 * only set a new error if there is no previous error.
   202		 * Errors are only cleared during read/open
   203		 * Avoid propagating -EPIPE (stall) to userspace since it is
   204		 * better handled as an empty read
   205		 */
   206		if (desc->rerr == 0 && status != -EPIPE)
   207			desc->rerr = status;
   208	
   209		if (test_bit(WDM_WWAN_IN_USE, &desc->flags)) {
   210			struct wwan_port *port = desc->wwanp;
   211			struct sk_buff *skb;
   212	
   213			/* Forward data to WWAN port */
   214			skb = alloc_skb(length, GFP_ATOMIC);
   215			if (skb) {
   216				memcpy(skb_put(skb, length), desc->inbuf, length);
 > 217				wwan_port_rx(port, skb);
   218			} else {
   219				dev_err(&desc->intf->dev,
   220					"Unable to alloc skb, response discarded\n");
   221			}
   222	
   223			/* inbuf has been copied, it is safe to check for outstanding data */
   224			schedule_work(&desc->service_outs_intr);
   225		} else if (length + desc->length > desc->wMaxCommand) {
   226			/* The buffer would overflow */
   227			set_bit(WDM_OVERFLOW, &desc->flags);
   228		} else {
   229			/* we may already be in overflow */
   230			if (!test_bit(WDM_OVERFLOW, &desc->flags)) {
   231				memmove(desc->ubuf + desc->length, desc->inbuf, length);
   232				desc->length += length;
   233				desc->reslength = length;
   234			}
   235		}
   236	skip_error:
   237	
   238		if (desc->rerr) {
   239			/*
   240			 * Since there was an error, userspace may decide to not read
   241			 * any data after poll'ing.
   242			 * We should respond to further attempts from the device to send
   243			 * data, so that we can get unstuck.
   244			 */
   245			schedule_work(&desc->service_outs_intr);
   246		} else {
   247			set_bit(WDM_READ, &desc->flags);
   248			wake_up(&desc->wait);
   249		}
   250		spin_unlock_irqrestore(&desc->iuspin, flags);
   251	}
   252	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35627 bytes --]

Re: [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

Hi Loic,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Loic-Poulain/usb-class-cdc-wdm-add-control-type/20210430-180831
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 4a52dd8fefb45626dace70a63c0738dbd83b7edb
config: parisc-randconfig-r026-20210430 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e5bc3ddfc2b8b4727830f2d4a131d4731e1a0e27
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Loic-Poulain/usb-class-cdc-wdm-add-control-type/20210430-180831
        git checkout e5bc3ddfc2b8b4727830f2d4a131d4731e1a0e27
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

ERROR: modpost: "wwan_port_rx" [drivers/usb/class/cdc-wdm.ko] undefined!
>> ERROR: modpost: "skb_put" [drivers/usb/class/cdc-wdm.ko] undefined!
>> ERROR: modpost: "__alloc_skb" [drivers/usb/class/cdc-wdm.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 30452 bytes --]

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

[Oliver Neukum]

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.