| Message ID | 1619777783-24116-2-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next,1/2] usb: class: cdc-wdm: add control type | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 4 maintainers not CCed: oneukum@suse.com gregkh@linuxfoundation.org lee.jones@linaro.org penguin-kernel@i-love.sakura.ne.jp |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Lines should not end with a '(' WARNING: line length of 81 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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);
Oliver Neukum <oneukum@suse.com> writes:
> This absolutely makes sense,
+1
Thanks for working on this.
Bjørn
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
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);
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(-)