Skip to contentSkip to conversation

USB: cdc: add control and state defines

PATCH In review Johan Hovold 4 participants 15 messages Jul 25, 2022

Subsystems (5)
USB Staging USB serial USB cdc Ethernet driver +1 more subsystems
lore
Share & tools
lore

Johan Hovold

author maintainer
#0 lore raw

cover USB: cdc: add control and state defines

Several drivers use the control-line and serial-state bitmap values from the CDC spec, but there were no matching defines in the global CDC (UAPI) header.

This series adds the missing defines and converts cdc-acm and f_acm to use them.

One staging driver also had an unused set of CDC defines which are removed.

The final patch by Yan Xinyu, which triggered this work, converts the usb_wwan driver to use CDC defines instead of hardcoded values in its line-control handling.

Greg, are you ok with me taking these through USB serial (where there are a few more drivers that could potentially use them) or do you want to take the lot through your tree directly?

Johan

Johan Hovold (6):
USB: cdc: add control-signal defines
USB: cdc: add serial-state defines
USB: cdc-acm: use CDC control-line defines
USB: cdc-acm: use CDC serial-state defines
staging: gdm724x: drop unused CDC defines
USB: gadget: f_acm: use CDC defines

Yan Xinyu (1):
USB: serial: usb_wwan: replace DTR/RTS magic numbers with macros

 drivers/staging/gdm724x/gdm_tty.c   |  6 -----
 drivers/usb/class/cdc-acm.c         | 42 ++++++++++++++---------------
 drivers/usb/class/cdc-acm.h         | 20 --------------
 drivers/usb/gadget/function/f_acm.c | 20 ++++----------
 drivers/usb/serial/usb_wwan.c       | 10 ++++---
 include/uapi/linux/usb/cdc.h        | 13 +++++++++
 6 files changed, 45 insertions(+), 66 deletions(-)
Signature · 1 line
2.35.1
author maintainer
#1 lore raw

PATCH 1/7 USB: cdc: add control-signal defines

Add defines for the Control Signal Bitmap Values from section 6.2.14 SetControlLineState of the CDC specification version 1.1.

Signed-off-by Johan Hovold <[email protected]>
---
 include/uapi/linux/usb/cdc.h | 4 ++++
 1 file changed, 4 insertions(+)
include/uapi/linux/usb/cdc.h +4 -0
index 6d61550959ef..372c81425cae 100644
@@ -271,6 +271,10 @@ struct usb_cdc_line_coding {
 	__u8	bDataBits;
 } __attribute__ ((packed));
 
+/* Control Signal Bitmap Values from 6.2.14 SetControlLineState */
+#define USB_CDC_CTRL_DTR			(1 << 0)
+#define USB_CDC_CTRL_RTS			(1 << 1)
+
 /* table 62; bits in multicast filter */
 #define	USB_CDC_PACKET_TYPE_PROMISCUOUS		(1 << 0)
 #define	USB_CDC_PACKET_TYPE_ALL_MULTICAST	(1 << 1) /* no filter */
Signature · 1 line
2.35.1
author maintainer
#3 lore raw

PATCH 2/7 USB: cdc: add serial-state defines

In reply to Johan Hovold

Add defines for the serial-state bitmap values from section 6.3.5 SerialState of the CDC specification version 1.1.

Note that the bTxCarrier and bRxCarrier bits have been named after their RS-232 signal equivalents DSR and DCD.

Signed-off-by Johan Hovold <[email protected]>
---
 include/uapi/linux/usb/cdc.h | 9 +++++++++
 1 file changed, 9 insertions(+)
include/uapi/linux/usb/cdc.h +9 -0
index 372c81425cae..78caa9bdc4ae 100644
@@ -306,6 +306,15 @@ struct usb_cdc_notification {
 	__le16	wLength;
 } __attribute__ ((packed));
 
+/* UART State Bitmap Values from 6.3.5 SerialState */
+#define USB_CDC_SERIAL_STATE_DCD		(1 << 0)
+#define USB_CDC_SERIAL_STATE_DSR		(1 << 1)
+#define USB_CDC_SERIAL_STATE_BREAK		(1 << 2)
+#define USB_CDC_SERIAL_STATE_RING_SIGNAL	(1 << 3)
+#define USB_CDC_SERIAL_STATE_FRAMING		(1 << 4)
+#define USB_CDC_SERIAL_STATE_PARITY		(1 << 5)
+#define USB_CDC_SERIAL_STATE_OVERRUN		(1 << 6)
+
 struct usb_cdc_speed_change {
 	__le32	DLBitRRate;	/* contains the downlink bit rate (IN pipe) */
 	__le32	ULBitRate;	/* contains the uplink bit rate (OUT pipe) */
Signature · 1 line
2.35.1
maintainer Acked-by on v1 2/7
#4 lore raw

Re: [PATCH 2/7] USB: cdc: add serial-state defines

Quoted 7 lines · Johan Hovold

Add defines for the serial-state bitmap values from section 6.3.5 SerialState of the CDC specification version 1.1.

Note that the bTxCarrier and bRxCarrier bits have been named after their RS-232 signal equivalents DSR and DCD.

Signed-off-by Johan Hovold <[email protected]>
Acked-by Oliver Neukum <[email protected]>
author maintainer
#5 lore raw

PATCH 3/7 USB: cdc-acm: use CDC control-line defines

In reply to Johan Hovold

Use the new CDC control-line defines.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/class/cdc-acm.c | 18 +++++++++---------
 drivers/usb/class/cdc-acm.h |  7 -------
 2 files changed, 9 insertions(+), 16 deletions(-)
drivers/usb/class/cdc-acm.c +9 -9
index 9b9aea24d58c..e2d80b99e074 100644
@@ -658,7 +658,7 @@ static void acm_port_dtr_rts(struct tty_port *port, int raise)
 	int res;
 
 	if (raise)
-		val = ACM_CTRL_DTR | ACM_CTRL_RTS;
+		val = USB_CDC_CTRL_DTR | USB_CDC_CTRL_RTS;
 	else
 		val = 0;
 

@@ -903,8 +903,8 @@ static int acm_tty_tiocmget(struct tty_struct *tty)
 {
 	struct acm *acm = tty->driver_data;
 
-	return (acm->ctrlout & ACM_CTRL_DTR ? TIOCM_DTR : 0) |
-	       (acm->ctrlout & ACM_CTRL_RTS ? TIOCM_RTS : 0) |
+	return (acm->ctrlout & USB_CDC_CTRL_DTR ? TIOCM_DTR : 0) |
+	       (acm->ctrlout & USB_CDC_CTRL_RTS ? TIOCM_RTS : 0) |
 	       (acm->ctrlin  & ACM_CTRL_DSR ? TIOCM_DSR : 0) |
 	       (acm->ctrlin  & ACM_CTRL_RI  ? TIOCM_RI  : 0) |
 	       (acm->ctrlin  & ACM_CTRL_DCD ? TIOCM_CD  : 0) |

@@ -918,10 +918,10 @@ static int acm_tty_tiocmset(struct tty_struct *tty,
 	unsigned int newctrl;
 
 	newctrl = acm->ctrlout;
-	set = (set & TIOCM_DTR ? ACM_CTRL_DTR : 0) |
-					(set & TIOCM_RTS ? ACM_CTRL_RTS : 0);
-	clear = (clear & TIOCM_DTR ? ACM_CTRL_DTR : 0) |
-					(clear & TIOCM_RTS ? ACM_CTRL_RTS : 0);
+	set = (set & TIOCM_DTR ? USB_CDC_CTRL_DTR : 0) |
+	      (set & TIOCM_RTS ? USB_CDC_CTRL_RTS : 0);
+	clear = (clear & TIOCM_DTR ? USB_CDC_CTRL_DTR : 0) |
+		(clear & TIOCM_RTS ? USB_CDC_CTRL_RTS : 0);
 
 	newctrl = (newctrl & ~clear) | set;
 

@@ -1068,9 +1068,9 @@ static void acm_tty_set_termios(struct tty_struct *tty,
 
 	if (C_BAUD(tty) == B0) {
 		newline.dwDTERate = acm->line.dwDTERate;
-		newctrl &= ~ACM_CTRL_DTR;
+		newctrl &= ~USB_CDC_CTRL_DTR;
 	} else if (termios_old && (termios_old->c_cflag & CBAUD) == B0) {
-		newctrl |=  ACM_CTRL_DTR;
+		newctrl |=  USB_CDC_CTRL_DTR;
 	}
 
 	if (newctrl != acm->ctrlout)
drivers/usb/class/cdc-acm.h +0 -7
index d26ecd15be60..da7e8b8aaf28 100644
@@ -22,13 +22,6 @@
 
 #define USB_RT_ACM		(USB_TYPE_CLASS | USB_RECIP_INTERFACE)
 
-/*
- * Output control lines.
- */
-
-#define ACM_CTRL_DTR		0x01
-#define ACM_CTRL_RTS		0x02
-
 /*
  * Input control lines and line errors.
  */
Signature · 1 line
2.35.1
author maintainer
#7 lore raw

PATCH 4/7 USB: cdc-acm: use CDC serial-state defines

In reply to Johan Hovold

Use the new CDC serial-state defines.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/class/cdc-acm.c | 24 ++++++++++++------------
 drivers/usb/class/cdc-acm.h | 13 -------------
 2 files changed, 12 insertions(+), 25 deletions(-)
drivers/usb/class/cdc-acm.c +12 -12
index e2d80b99e074..352df48469b2 100644
@@ -311,7 +311,7 @@ static void acm_process_notification(struct acm *acm, unsigned char *buf)
 		dev_dbg(&acm->control->dev,
 			"%s - serial state: 0x%x\n", __func__, newctrl);
 
-		if (!acm->clocal && (acm->ctrlin & ~newctrl & ACM_CTRL_DCD)) {
+		if (!acm->clocal && (acm->ctrlin & ~newctrl & USB_CDC_SERIAL_STATE_DCD)) {
 			dev_dbg(&acm->control->dev,
 				"%s - calling hangup\n", __func__);
 			tty_port_tty_hangup(&acm->port, false);

@@ -322,25 +322,25 @@ static void acm_process_notification(struct acm *acm, unsigned char *buf)
 		acm->ctrlin = newctrl;
 		acm->oldcount = acm->iocount;
 
-		if (difference & ACM_CTRL_DSR)
+		if (difference & USB_CDC_SERIAL_STATE_DSR)
 			acm->iocount.dsr++;
-		if (difference & ACM_CTRL_DCD)
+		if (difference & USB_CDC_SERIAL_STATE_DCD)
 			acm->iocount.dcd++;
-		if (newctrl & ACM_CTRL_BRK) {
+		if (newctrl & USB_CDC_SERIAL_STATE_BREAK) {
 			acm->iocount.brk++;
 			tty_insert_flip_char(&acm->port, 0, TTY_BREAK);
 		}
-		if (newctrl & ACM_CTRL_RI)
+		if (newctrl & USB_CDC_SERIAL_STATE_RING_SIGNAL)
 			acm->iocount.rng++;
-		if (newctrl & ACM_CTRL_FRAMING)
+		if (newctrl & USB_CDC_SERIAL_STATE_FRAMING)
 			acm->iocount.frame++;
-		if (newctrl & ACM_CTRL_PARITY)
+		if (newctrl & USB_CDC_SERIAL_STATE_PARITY)
 			acm->iocount.parity++;
-		if (newctrl & ACM_CTRL_OVERRUN)
+		if (newctrl & USB_CDC_SERIAL_STATE_OVERRUN)
 			acm->iocount.overrun++;
 		spin_unlock_irqrestore(&acm->read_lock, flags);
 
-		if (newctrl & ACM_CTRL_BRK)
+		if (newctrl & USB_CDC_SERIAL_STATE_BREAK)
 			tty_flip_buffer_push(&acm->port);
 
 		if (difference)

@@ -905,9 +905,9 @@ static int acm_tty_tiocmget(struct tty_struct *tty)
 
 	return (acm->ctrlout & USB_CDC_CTRL_DTR ? TIOCM_DTR : 0) |
 	       (acm->ctrlout & USB_CDC_CTRL_RTS ? TIOCM_RTS : 0) |
-	       (acm->ctrlin  & ACM_CTRL_DSR ? TIOCM_DSR : 0) |
-	       (acm->ctrlin  & ACM_CTRL_RI  ? TIOCM_RI  : 0) |
-	       (acm->ctrlin  & ACM_CTRL_DCD ? TIOCM_CD  : 0) |
+	       (acm->ctrlin  & USB_CDC_SERIAL_STATE_DSR ? TIOCM_DSR : 0) |
+	       (acm->ctrlin  & USB_CDC_SERIAL_STATE_RING_SIGNAL ? TIOCM_RI : 0) |
+	       (acm->ctrlin  & USB_CDC_SERIAL_STATE_DCD ? TIOCM_CD : 0) |
 	       TIOCM_CTS;
 }
drivers/usb/class/cdc-acm.h +0 -13
index da7e8b8aaf28..759ac15631d3 100644
@@ -22,19 +22,6 @@
 
 #define USB_RT_ACM		(USB_TYPE_CLASS | USB_RECIP_INTERFACE)
 
-/*
- * Input control lines and line errors.
- */
-
-#define ACM_CTRL_DCD		0x01
-#define ACM_CTRL_DSR		0x02
-#define ACM_CTRL_BRK		0x04
-#define ACM_CTRL_RI		0x08
-
-#define ACM_CTRL_FRAMING	0x10
-#define ACM_CTRL_PARITY		0x20
-#define ACM_CTRL_OVERRUN	0x40
-
 /*
  * Internal driver structures.
  */
Signature · 1 line
2.35.1
author maintainer
#9 lore raw

PATCH 5/7 staging: gdm724x: drop unused CDC defines

In reply to Johan Hovold

This driver has a copy of some of the CDC defines but which are currently unused.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/staging/gdm724x/gdm_tty.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/staging/gdm724x/gdm_tty.c +0 -6
index 04df6f9f5403..cc6d80554c98 100644
@@ -17,12 +17,6 @@
 #define GDM_TTY_MAJOR 0
 #define GDM_TTY_MINOR 32
 
-#define ACM_CTRL_DTR 0x01
-#define ACM_CTRL_RTS 0x02
-#define ACM_CTRL_DSR 0x02
-#define ACM_CTRL_RI  0x08
-#define ACM_CTRL_DCD 0x01
-
 #define WRITE_SIZE 2048
 
 #define MUX_TX_MAX_SIZE 2048
Signature · 1 line
2.35.1
author maintainer
#10 lore raw

PATCH 6/7 USB: gadget: f_acm: use CDC defines

In reply to Johan Hovold

Use the new CDC control-line and serial-state defines.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/gadget/function/f_acm.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)
drivers/usb/gadget/function/f_acm.c +5 -15
index 411eb489e0ff..cb523f118f04 100644
@@ -57,18 +57,8 @@ struct f_acm {
 
 	/* SetControlLineState request -- CDC 1.1 section 6.2.14 (INPUT) */
 	u16				port_handshake_bits;
-#define ACM_CTRL_RTS	(1 << 1)	/* unused with full duplex */
-#define ACM_CTRL_DTR	(1 << 0)	/* host is ready for data r/w */
-
 	/* SerialState notification -- CDC 1.1 section 6.3.5 (OUTPUT) */
 	u16				serial_state;
-#define ACM_CTRL_OVERRUN	(1 << 6)
-#define ACM_CTRL_PARITY		(1 << 5)
-#define ACM_CTRL_FRAMING	(1 << 4)
-#define ACM_CTRL_RI		(1 << 3)
-#define ACM_CTRL_BRK		(1 << 2)
-#define ACM_CTRL_DSR		(1 << 1)
-#define ACM_CTRL_DCD		(1 << 0)
 };
 
 static inline struct f_acm *func_to_acm(struct usb_function *f)

@@ -387,7 +377,7 @@ static int acm_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 		value = 0;
 
 		/* FIXME we should not allow data to flow until the
-		 * host sets the ACM_CTRL_DTR bit; and when it clears
+		 * host sets the USB_CDC_CTRL_DTR bit; and when it clears
 		 * that bit, we should return to that no-flow state.
 		 */
 		acm->port_handshake_bits = w_value;

@@ -585,7 +575,7 @@ static void acm_connect(struct gserial *port)
 {
 	struct f_acm		*acm = port_to_acm(port);
 
-	acm->serial_state |= ACM_CTRL_DSR | ACM_CTRL_DCD;
+	acm->serial_state |= USB_CDC_SERIAL_STATE_DSR | USB_CDC_SERIAL_STATE_DCD;
 	acm_notify_serial_state(acm);
 }
 

@@ -593,7 +583,7 @@ static void acm_disconnect(struct gserial *port)
 {
 	struct f_acm		*acm = port_to_acm(port);
 
-	acm->serial_state &= ~(ACM_CTRL_DSR | ACM_CTRL_DCD);
+	acm->serial_state &= ~(USB_CDC_SERIAL_STATE_DSR | USB_CDC_SERIAL_STATE_DCD);
 	acm_notify_serial_state(acm);
 }
 

@@ -603,9 +593,9 @@ static int acm_send_break(struct gserial *port, int duration)
 	u16			state;
 
 	state = acm->serial_state;
-	state &= ~ACM_CTRL_BRK;
+	state &= ~USB_CDC_SERIAL_STATE_BREAK;
 	if (duration)
-		state |= ACM_CTRL_BRK;
+		state |= USB_CDC_SERIAL_STATE_BREAK;
 
 	acm->serial_state = state;
 	return acm_notify_serial_state(acm);
Signature · 1 line
2.35.1
author maintainer
#11 lore raw

PATCH 7/7 USB: serial: usb_wwan: replace DTR/RTS magic numbers with macros

In reply to Johan Hovold

From: Yan Xinyu <[email protected]>

The usb_wwan_send_setup function generates DTR/RTS signals in compliance with CDC ACM standard. This patch changes magic numbers in this function to equivalent macros.

Signed-off-by Yan Xinyu <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
[ johan: use the new CDC control-line defines ]
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/usb_wwan.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/usb/serial/usb_wwan.c +6 -4
index dab38b63eaf7..6129a6e26f2c 100644
@@ -29,6 +29,7 @@
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
 #include <linux/usb.h>
+#include <linux/usb/cdc.h>
 #include <linux/usb/serial.h>
 #include <linux/serial.h>
 #include "usb-wwan.h"

@@ -48,9 +49,9 @@ static int usb_wwan_send_setup(struct usb_serial_port *port)
 	portdata = usb_get_serial_port_data(port);
 
 	if (portdata->dtr_state)
-		val |= 0x01;
+		val |= USB_CDC_CTRL_DTR;
 	if (portdata->rts_state)
-		val |= 0x02;
+		val |= USB_CDC_CTRL_RTS;
 
 	ifnum = serial->interface->cur_altsetting->desc.bInterfaceNumber;
 

@@ -59,8 +60,9 @@ static int usb_wwan_send_setup(struct usb_serial_port *port)
 		return res;
 
 	res = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
-				0x22, 0x21, val, ifnum, NULL, 0,
-				USB_CTRL_SET_TIMEOUT);
+				USB_CDC_REQ_SET_CONTROL_LINE_STATE,
+				USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+				val, ifnum, NULL, 0, USB_CTRL_SET_TIMEOUT);
 
 	usb_autopm_put_interface(port->serial->interface);
Signature · 1 line
2.35.1
maintainer
#12 lore raw
In reply to Johan Hovold
Quoted 10 lines · Johan Hovold

Several drivers use the control-line and serial-state bitmap values from the CDC spec, but there were no matching defines in the global CDC (UAPI) header.

This series adds the missing defines and converts cdc-acm and f_acm to use them.

One staging driver also had an unused set of CDC defines which are removed.

Quoted 7 lines

The final patch by Yan Xinyu, which triggered this work, converts the usb_wwan driver to use CDC defines instead of hardcoded values in its line-control handling.

Greg, are you ok with me taking these through USB serial (where there are a few more drivers that could potentially use them) or do you want to take the lot through your tree directly?

Many thanks for this, I'll just take this now and you can send future patches for usb-serial that can rely on these being present.

greg k-h

author maintainer
#13 lore raw
Quoted 15 lines · Greg Kroah-Hartman
Quoted 14 lines · Johan Hovold

Several drivers use the control-line and serial-state bitmap values from the CDC spec, but there were no matching defines in the global CDC (UAPI) header.

This series adds the missing defines and converts cdc-acm and f_acm to use them.

One staging driver also had an unused set of CDC defines which are removed.

The final patch by Yan Xinyu, which triggered this work, converts the usb_wwan driver to use CDC defines instead of hardcoded values in its line-control handling.

Quoted 6 lines
Quoted 3 lines

Greg, are you ok with me taking these through USB serial (where there are a few more drivers that could potentially use them) or do you want to take the lot through your tree directly?

Many thanks for this, I'll just take this now and you can send future patches for usb-serial that can rely on these being present.

Sounds good. That probably won't happen before the merge window anyway.

Thanks.

Johan

#14 lore raw
In reply to Johan Hovold
Quoted 34 lines · Johan Hovold

Several drivers use the control-line and serial-state bitmap values from the CDC spec, but there were no matching defines in the global CDC (UAPI) header.

This series adds the missing defines and converts cdc-acm and f_acm to use them.

One staging driver also had an unused set of CDC defines which are removed.

The final patch by Yan Xinyu, which triggered this work, converts the usb_wwan driver to use CDC defines instead of hardcoded values in its line-control handling.

Greg, are you ok with me taking these through USB serial (where there are a few more drivers that could potentially use them) or do you want to take the lot through your tree directly?

Johan

Johan Hovold (6):
USB: cdc: add control-signal defines
USB: cdc: add serial-state defines
USB: cdc-acm: use CDC control-line defines
USB: cdc-acm: use CDC serial-state defines
staging: gdm724x: drop unused CDC defines
USB: gadget: f_acm: use CDC defines

Yan Xinyu (1):
USB: serial: usb_wwan: replace DTR/RTS magic numbers with macros

Quoted 11 lines
drivers/staging/gdm724x/gdm_tty.c   |  6 -----
drivers/usb/class/cdc-acm.c         | 42 ++++++++++++++---------------
drivers/usb/class/cdc-acm.h         | 20 --------------
drivers/usb/gadget/function/f_acm.c | 20 ++++----------
drivers/usb/serial/usb_wwan.c       | 10 ++++---
include/uapi/linux/usb/cdc.h        | 13 +++++++++
6 files changed, 45 insertions(+), 66 deletions(-)
Signature · 1 line
2.35.1

Thank you so much for spending your time to write this patch series! And thank you and Greg for your reviews, they are very informative and educational!

sdlyyxy