* [RFC net-next 1/2] usb: class: cdc-wdm: add control type
@ 2021-04-30 10:16 Loic Poulain
2021-04-30 10:16 ` [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration Loic Poulain
2021-04-30 10:32 ` [RFC net-next 1/2] usb: class: cdc-wdm: add control type Oliver Neukum
0 siblings, 2 replies; 6+ messages in thread
From: Loic Poulain @ 2021-04-30 10:16 UTC (permalink / raw)
To: oliver; +Cc: netdev, linux-usb, kuba, bjorn, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration
2021-04-30 10:16 [RFC net-next 1/2] usb: class: cdc-wdm: add control type Loic Poulain
@ 2021-04-30 10:16 ` Loic Poulain
2021-04-30 10:39 ` Oliver Neukum
2021-04-30 10:32 ` [RFC net-next 1/2] usb: class: cdc-wdm: add control type Oliver Neukum
1 sibling, 1 reply; 6+ messages in thread
From: Loic Poulain @ 2021-04-30 10:16 UTC (permalink / raw)
To: oliver; +Cc: netdev, linux-usb, kuba, bjorn, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC net-next 1/2] usb: class: cdc-wdm: add control type
2021-04-30 10:16 [RFC net-next 1/2] usb: class: cdc-wdm: add control type Loic Poulain
2021-04-30 10:16 ` [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration Loic Poulain
@ 2021-04-30 10:32 ` Oliver Neukum
1 sibling, 0 replies; 6+ messages in thread
From: Oliver Neukum @ 2021-04-30 10:32 UTC (permalink / raw)
To: Loic Poulain; +Cc: netdev, linux-usb, kuba, bjorn
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration
2021-04-30 10:16 ` [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration Loic Poulain
@ 2021-04-30 10:39 ` Oliver Neukum
2021-05-01 10:49 ` Bjørn Mork
0 siblings, 1 reply; 6+ messages in thread
From: Oliver Neukum @ 2021-04-30 10:39 UTC (permalink / raw)
To: Loic Poulain; +Cc: netdev, linux-usb, kuba, bjorn
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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration
2021-04-30 10:39 ` Oliver Neukum
@ 2021-05-01 10:49 ` Bjørn Mork
2021-05-03 7:47 ` Loic Poulain
0 siblings, 1 reply; 6+ messages in thread
From: Bjørn Mork @ 2021-05-01 10:49 UTC (permalink / raw)
To: Oliver Neukum; +Cc: Loic Poulain, netdev, linux-usb, kuba
Oliver Neukum <oneukum@suse.com> writes:
> This absolutely makes sense,
+1
Thanks for working on this.
Bjørn
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration
2021-05-01 10:49 ` Bjørn Mork
@ 2021-05-03 7:47 ` Loic Poulain
0 siblings, 0 replies; 6+ messages in thread
From: Loic Poulain @ 2021-05-03 7:47 UTC (permalink / raw)
To: Bjørn Mork, Oliver Neukum; +Cc: Network Development, USB, Jakub Kicinski
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-03 7:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-30 10:16 [RFC net-next 1/2] usb: class: cdc-wdm: add control type Loic Poulain
2021-04-30 10:16 ` [RFC net-next 2/2] usb: class: cdc-wdm: WWAN framework integration Loic Poulain
2021-04-30 10:39 ` Oliver Neukum
2021-05-01 10:49 ` Bjørn Mork
2021-05-03 7:47 ` Loic Poulain
2021-04-30 10:32 ` [RFC net-next 1/2] usb: class: cdc-wdm: add control type Oliver Neukum
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).