Skip to contentSkip to conversation

USB: serial: return errors from break handling

PATCH In review Johan Hovold 3 participants 10 messages Jun 4, 2023 – Jun 6, 2023

Subsystems (3)
USB USB serial USB ACM driver
lore
Share & tools
lore

Johan Hovold

author maintainer
#0 lore raw

v2 cover USB: serial: return errors from break handling

This series starts returning errors from break handling and also uses that mechanism to report to user space when break signalling is not supported (e.g. when device or driver support is missing).

Note that the tty layer currently returns early but without reporting errors when a tty driver does not support break signalling. The intent expressed in commit 9e98966c7bb9 ("tty: rework break handling") from 2008 appears to be to allow missing support to be reported to user space however.

Johan

Changes in v2
- fix return of potentially uninitialised status variable in io_edgeport as reported by kernel test robot <[email protected]> and Dan Carpenter:

https://lore.kernel.org/all/[email protected]/

Johan Hovold (3):
USB: serial: return errors from break handling
USB: serial: cp210x: disable break signalling on CP2105 SCI
USB: serial: report unsupported break signalling

 drivers/usb/serial/ark3116.c          |  7 +++--
 drivers/usb/serial/belkin_sa.c        | 12 ++++++---
 drivers/usb/serial/ch341.c            | 37 +++++++++++++++++----------
 drivers/usb/serial/cp210x.c           | 14 +++++++---
 drivers/usb/serial/digi_acceleport.c  |  7 ++---
 drivers/usb/serial/f81232.c           |  4 ++-
 drivers/usb/serial/f81534.c           |  4 ++-
 drivers/usb/serial/ftdi_sio.c         | 10 +++++---
 drivers/usb/serial/io_edgeport.c      |  6 +++--
 drivers/usb/serial/io_ti.c            |  9 +++++--
 drivers/usb/serial/keyspan.c          |  5 +++-
 drivers/usb/serial/keyspan_pda.c      |  8 ++++--
 drivers/usb/serial/mct_u232.c         |  6 ++---
 drivers/usb/serial/mos7720.c          |  9 ++++---
 drivers/usb/serial/mos7840.c          |  7 ++---
 drivers/usb/serial/mxuport.c          |  6 ++---
 drivers/usb/serial/pl2303.c           | 14 ++++++----
 drivers/usb/serial/quatech2.c         |  8 ++++--
 drivers/usb/serial/ti_usb_3410_5052.c | 10 +++++---
 drivers/usb/serial/upd78f0730.c       |  7 +++--
 drivers/usb/serial/usb-serial.c       |  4 +--
 drivers/usb/serial/usb_debug.c        | 13 +++++++---
 drivers/usb/serial/whiteheat.c        |  7 ++---
 drivers/usb/serial/xr_serial.c        |  4 +--
 include/linux/usb/serial.h            |  2 +-
 25 files changed, 147 insertions(+), 73 deletions(-)
Signature · 1 line
2.39.3
author maintainer
#1 lore raw

PATCH v2 1/3 USB: serial: return errors from break handling

Start propagating errors to user space when setting the break state fails.

This will be used by follow-on changes to also report when a driver or device does not support break control.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/ark3116.c          |  7 +++--
 drivers/usb/serial/belkin_sa.c        | 12 ++++++---
 drivers/usb/serial/ch341.c            | 37 +++++++++++++++++----------
 drivers/usb/serial/cp210x.c           |  8 +++---
 drivers/usb/serial/digi_acceleport.c  |  7 ++---
 drivers/usb/serial/f81232.c           |  4 ++-
 drivers/usb/serial/f81534.c           |  4 ++-
 drivers/usb/serial/ftdi_sio.c         | 10 +++++---
 drivers/usb/serial/io_edgeport.c      |  6 +++--
 drivers/usb/serial/io_ti.c            |  9 +++++--
 drivers/usb/serial/keyspan.c          |  5 +++-
 drivers/usb/serial/keyspan_pda.c      |  8 ++++--
 drivers/usb/serial/mct_u232.c         |  6 ++---
 drivers/usb/serial/mos7720.c          |  9 ++++---
 drivers/usb/serial/mos7840.c          |  7 ++---
 drivers/usb/serial/mxuport.c          |  6 ++---
 drivers/usb/serial/pl2303.c           | 14 ++++++----
 drivers/usb/serial/quatech2.c         |  8 ++++--
 drivers/usb/serial/ti_usb_3410_5052.c | 10 +++++---
 drivers/usb/serial/upd78f0730.c       |  7 +++--
 drivers/usb/serial/usb-serial.c       |  2 +-
 drivers/usb/serial/usb_debug.c        | 13 +++++++---
 drivers/usb/serial/whiteheat.c        |  7 ++---
 drivers/usb/serial/xr_serial.c        |  4 +--
 include/linux/usb/serial.h            |  2 +-
 25 files changed, 140 insertions(+), 72 deletions(-)
drivers/usb/serial/ark3116.c +5 -2
index 9452291f1703..67a07cc007f0 100644
@@ -433,10 +433,11 @@ static int ark3116_tiocmset(struct tty_struct *tty,
 	return 0;
 }
 
-static void ark3116_break_ctl(struct tty_struct *tty, int break_state)
+static int ark3116_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct ark3116_private *priv = usb_get_serial_port_data(port);
+	int ret;
 
 	/* LCR is also used for other things: protect access */
 	mutex_lock(&priv->hw_lock);

@@ -446,9 +447,11 @@ static void ark3116_break_ctl(struct tty_struct *tty, int break_state)
 	else
 		priv->lcr &= ~UART_LCR_SBC;
 
-	ark3116_write_reg(port->serial, UART_LCR, priv->lcr);
+	ret = ark3116_write_reg(port->serial, UART_LCR, priv->lcr);
 
 	mutex_unlock(&priv->hw_lock);
+
+	return ret;
 }
 
 static void ark3116_update_msr(struct usb_serial_port *port, __u8 msr)
drivers/usb/serial/belkin_sa.c +9 -3
index 9331a562dac0..cf47ee4ae5d3 100644
@@ -46,7 +46,7 @@ static void belkin_sa_process_read_urb(struct urb *urb);
 static void belkin_sa_set_termios(struct tty_struct *tty,
 				  struct usb_serial_port *port,
 				  const struct ktermios *old_termios);
-static void belkin_sa_break_ctl(struct tty_struct *tty, int break_state);
+static int belkin_sa_break_ctl(struct tty_struct *tty, int break_state);
 static int  belkin_sa_tiocmget(struct tty_struct *tty);
 static int  belkin_sa_tiocmset(struct tty_struct *tty,
 					unsigned int set, unsigned int clear);

@@ -399,13 +399,19 @@ static void belkin_sa_set_termios(struct tty_struct *tty,
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
-static void belkin_sa_break_ctl(struct tty_struct *tty, int break_state)
+static int belkin_sa_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct usb_serial *serial = port->serial;
+	int ret;
 
-	if (BSA_USB_CMD(BELKIN_SA_SET_BREAK_REQUEST, break_state ? 1 : 0) < 0)
+	ret = BSA_USB_CMD(BELKIN_SA_SET_BREAK_REQUEST, break_state ? 1 : 0);
+	if (ret < 0) {
 		dev_err(&port->dev, "Set break_ctl %d\n", break_state);
+		return ret;
+	}
+
+	return 0;
 }
 
 static int belkin_sa_tiocmget(struct tty_struct *tty)
drivers/usb/serial/ch341.c +23 -14
index 6e1b87e67304..612bea504d7a 100644
@@ -562,12 +562,12 @@ static void ch341_set_termios(struct tty_struct *tty,
  * TCSBRKP. Due to how the simulation is implemented the duration can't be
  * controlled. The duration is always about (1s / 46bd * 9bit) = 196ms.
  */
-static void ch341_simulate_break(struct tty_struct *tty, int break_state)
+static int ch341_simulate_break(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct ch341_private *priv = usb_get_serial_port_data(port);
 	unsigned long now, delay;
-	int r;
+	int r, r2;
 
 	if (break_state != 0) {
 		dev_dbg(&port->dev, "enter break state requested\n");

@@ -599,7 +599,7 @@ static void ch341_simulate_break(struct tty_struct *tty, int break_state)
 		 */
 		priv->break_end = jiffies + (11 * HZ / CH341_MIN_BPS);
 
-		return;
+		return 0;
 	}
 
 	dev_dbg(&port->dev, "leave break state requested\n");

@@ -615,17 +615,22 @@ static void ch341_simulate_break(struct tty_struct *tty, int break_state)
 		schedule_timeout_interruptible(delay);
 	}
 
+	r = 0;
 restore:
 	/* Restore original baud rate */
-	r = ch341_set_baudrate_lcr(port->serial->dev, priv, priv->baud_rate,
-				   priv->lcr);
-	if (r < 0)
+	r2 = ch341_set_baudrate_lcr(port->serial->dev, priv, priv->baud_rate,
+			priv->lcr);
+	if (r2 < 0) {
 		dev_err(&port->dev,
 			"restoring original baud rate of %u failed: %d\n",
-			priv->baud_rate, r);
+			priv->baud_rate, r2);
+		return r2;
+	}
+
+	return r;
 }
 
-static void ch341_break_ctl(struct tty_struct *tty, int break_state)
+static int ch341_break_ctl(struct tty_struct *tty, int break_state)
 {
 	const uint16_t ch341_break_reg =
 			((uint16_t) CH341_REG_LCR << 8) | CH341_REG_BREAK;

@@ -635,17 +640,17 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
 	uint16_t reg_contents;
 	uint8_t break_reg[2];
 
-	if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK) {
-		ch341_simulate_break(tty, break_state);
-		return;
-	}
+	if (priv->quirks & CH341_QUIRK_SIMULATE_BREAK)
+		return ch341_simulate_break(tty, break_state);
 
 	r = ch341_control_in(port->serial->dev, CH341_REQ_READ_REG,
 			ch341_break_reg, 0, break_reg, 2);
 	if (r) {
 		dev_err(&port->dev, "%s - USB control read error (%d)\n",
 				__func__, r);
-		return;
+		if (r > 0)
+			r = -EIO;
+		return r;
 	}
 	dev_dbg(&port->dev, "%s - initial ch341 break register contents - reg1: %x, reg2: %x\n",
 		__func__, break_reg[0], break_reg[1]);

@@ -663,9 +668,13 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
 	reg_contents = get_unaligned_le16(break_reg);
 	r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
 			ch341_break_reg, reg_contents);
-	if (r < 0)
+	if (r < 0) {
 		dev_err(&port->dev, "%s - USB control write error (%d)\n",
 				__func__, r);
+		return r;
+	}
+
+	return 0;
 }
 
 static int ch341_tiocmset(struct tty_struct *tty,
drivers/usb/serial/cp210x.c +5 -3
index cdea1bff3b70..81e49ed9d147 100644
@@ -39,7 +39,7 @@ static int cp210x_tiocmget(struct tty_struct *);
 static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int);
 static int cp210x_tiocmset_port(struct usb_serial_port *port,
 		unsigned int, unsigned int);
-static void cp210x_break_ctl(struct tty_struct *, int);
+static int cp210x_break_ctl(struct tty_struct *, int);
 static int cp210x_attach(struct usb_serial *);
 static void cp210x_disconnect(struct usb_serial *);
 static void cp210x_release(struct usb_serial *);

@@ -1434,7 +1434,7 @@ static int cp210x_tiocmget(struct tty_struct *tty)
 	return result;
 }
 
-static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
+static int cp210x_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	u16 state;

@@ -1443,9 +1443,11 @@ static void cp210x_break_ctl(struct tty_struct *tty, int break_state)
 		state = BREAK_OFF;
 	else
 		state = BREAK_ON;
+
 	dev_dbg(&port->dev, "%s - turning break %s\n", __func__,
 		state == BREAK_OFF ? "off" : "on");
-	cp210x_write_u16_reg(port, CP210X_SET_BREAK, state);
+
+	return cp210x_write_u16_reg(port, CP210X_SET_BREAK, state);
 }
 
 #ifdef CONFIG_GPIOLIB
drivers/usb/serial/digi_acceleport.c +4 -3
index 45d688e9b93f..d1dea3850576 100644
@@ -217,7 +217,7 @@ static void digi_rx_unthrottle(struct tty_struct *tty);
 static void digi_set_termios(struct tty_struct *tty,
 			     struct usb_serial_port *port,
 			     const struct ktermios *old_termios);
-static void digi_break_ctl(struct tty_struct *tty, int break_state);
+static int digi_break_ctl(struct tty_struct *tty, int break_state);
 static int digi_tiocmget(struct tty_struct *tty);
 static int digi_tiocmset(struct tty_struct *tty, unsigned int set,
 		unsigned int clear);

@@ -839,7 +839,7 @@ static void digi_set_termios(struct tty_struct *tty,
 }
 
 
-static void digi_break_ctl(struct tty_struct *tty, int break_state)
+static int digi_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	unsigned char buf[4];

@@ -848,7 +848,8 @@ static void digi_break_ctl(struct tty_struct *tty, int break_state)
 	buf[1] = 2;				/* length */
 	buf[2] = break_state ? 1 : 0;
 	buf[3] = 0;				/* pad */
-	digi_write_inb_command(port, buf, 4, 0);
+
+	return digi_write_inb_command(port, buf, 4, 0);
 }
drivers/usb/serial/f81232.c +3 -1
index 891fb1fe69df..5f7a46bcace6 100644
@@ -448,7 +448,7 @@ static void f81534a_process_read_urb(struct urb *urb)
 	tty_flip_buffer_push(&port->port);
 }
 
-static void f81232_break_ctl(struct tty_struct *tty, int break_state)
+static int f81232_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct f81232_private *priv = usb_get_serial_port_data(port);

@@ -467,6 +467,8 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
 		dev_err(&port->dev, "set break failed: %d\n", status);
 
 	mutex_unlock(&priv->lock);
+
+	return status;
 }
 
 static int f81232_find_clk(speed_t baudrate)
drivers/usb/serial/f81534.c +3 -1
index 4083ae961be4..ef126cd3d94f 100644
@@ -656,7 +656,7 @@ static int f81534_set_port_config(struct usb_serial_port *port,
 	return status;
 }
 
-static void f81534_break_ctl(struct tty_struct *tty, int break_state)
+static int f81534_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);

@@ -675,6 +675,8 @@ static void f81534_break_ctl(struct tty_struct *tty, int break_state)
 		dev_err(&port->dev, "set break failed: %d\n", status);
 
 	mutex_unlock(&port_priv->lcr_mutex);
+
+	return status;
 }
 
 static int f81534_update_mctrl(struct usb_serial_port *port, unsigned int set,
drivers/usb/serial/ftdi_sio.c +7 -3
index 05e28a5ce42b..1bf23611be12 100644
@@ -2550,11 +2550,12 @@ static void ftdi_process_read_urb(struct urb *urb)
 		tty_flip_buffer_push(&port->port);
 }
 
-static void ftdi_break_ctl(struct tty_struct *tty, int break_state)
+static int ftdi_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 	u16 value;
+	int ret;
 
 	/* break_state = -1 to turn on break, and 0 to turn off break */
 	/* see drivers/char/tty_io.c to see it used */

@@ -2565,19 +2566,22 @@ static void ftdi_break_ctl(struct tty_struct *tty, int break_state)
 	else
 		value = priv->last_set_data_value;
 
-	if (usb_control_msg(port->serial->dev,
+	ret = usb_control_msg(port->serial->dev,
 			usb_sndctrlpipe(port->serial->dev, 0),
 			FTDI_SIO_SET_DATA_REQUEST,
 			FTDI_SIO_SET_DATA_REQUEST_TYPE,
 			value, priv->channel,
-			NULL, 0, WDR_TIMEOUT) < 0) {
+			NULL, 0, WDR_TIMEOUT);
+	if (ret < 0) {
 		dev_err(&port->dev, "%s FAILED to enable/disable break state (state was %d)\n",
 			__func__, break_state);
+		return ret;
 	}
 
 	dev_dbg(&port->dev, "%s break state is %d - urb is %d\n", __func__,
 		break_state, value);
 
+	return 0;
 }
 
 static bool ftdi_tx_empty(struct usb_serial_port *port)
drivers/usb/serial/io_edgeport.c +4 -2
index 3a4c0febf335..abe4bbb0ac65 100644
@@ -1560,12 +1560,12 @@ static int edge_ioctl(struct tty_struct *tty,
  * SerialBreak
  *	this function sends a break to the port
  *****************************************************************************/
-static void edge_break(struct tty_struct *tty, int break_state)
+static int edge_break(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct edgeport_port *edge_port = usb_get_serial_port_data(port);
 	struct edgeport_serial *edge_serial = usb_get_serial_data(port->serial);
-	int status;
+	int status = 0;
 
 	if (!edge_serial->is_epic ||
 	    edge_serial->epic_descriptor.Supports.IOSPChase) {

@@ -1597,6 +1597,8 @@ static void edge_break(struct tty_struct *tty, int break_state)
 			dev_dbg(&port->dev, "%s - error sending break set/clear command.\n",
 				__func__);
 	}
+
+	return status;
 }
drivers/usb/serial/io_ti.c +7 -2
index bc3c24ea42c1..7a3a6e539456 100644
@@ -2421,7 +2421,7 @@ static int edge_tiocmget(struct tty_struct *tty)
 	return result;
 }
 
-static void edge_break(struct tty_struct *tty, int break_state)
+static int edge_break(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct edgeport_port *edge_port = usb_get_serial_port_data(port);

@@ -2430,10 +2430,15 @@ static void edge_break(struct tty_struct *tty, int break_state)
 
 	if (break_state == -1)
 		bv = 1;	/* On */
+
 	status = ti_do_config(edge_port, UMPC_SET_CLR_BREAK, bv);
-	if (status)
+	if (status) {
 		dev_dbg(&port->dev, "%s - error %d sending break set/clear command.\n",
 			__func__, status);
+		return status;
+	}
+
+	return 0;
 }
 
 static void edge_heartbeat_schedule(struct edgeport_serial *edge_serial)
drivers/usb/serial/keyspan.c +4 -1
index 2966e0c4941e..93b17e0e05a3 100644
@@ -599,7 +599,7 @@ struct keyspan_port_private {
 #include "keyspan_usa67msg.h"
 
 
-static void keyspan_break_ctl(struct tty_struct *tty, int break_state)
+static int keyspan_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct keyspan_port_private 	*p_priv;

@@ -611,7 +611,10 @@ static void keyspan_break_ctl(struct tty_struct *tty, int break_state)
 	else
 		p_priv->break_on = 0;
 
+	/* FIXME: return errors */
 	keyspan_send_setup(port, 0);
+
+	return 0;
 }
drivers/usb/serial/keyspan_pda.c +6 -2
index 6fd15cd9e1eb..0eef358b314a 100644
@@ -299,7 +299,7 @@ static speed_t keyspan_pda_setbaud(struct usb_serial *serial, speed_t baud)
 	return baud;
 }
 
-static void keyspan_pda_break_ctl(struct tty_struct *tty, int break_state)
+static int keyspan_pda_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct usb_serial *serial = port->serial;

@@ -315,9 +315,13 @@ static void keyspan_pda_break_ctl(struct tty_struct *tty, int break_state)
 			4, /* set break */
 			USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT,
 			value, 0, NULL, 0, 2000);
-	if (result < 0)
+	if (result < 0) {
 		dev_dbg(&port->dev, "%s - error %d from usb_control_msg\n",
 			__func__, result);
+		return result;
+	}
+
+	return 0;
 }
 
 static void keyspan_pda_set_termios(struct tty_struct *tty,
drivers/usb/serial/mct_u232.c +3 -3
index d3852feb81a4..6570c8817a80 100644
@@ -47,7 +47,7 @@ static void mct_u232_read_int_callback(struct urb *urb);
 static void mct_u232_set_termios(struct tty_struct *tty,
 				 struct usb_serial_port *port,
 				 const struct ktermios *old_termios);
-static void mct_u232_break_ctl(struct tty_struct *tty, int break_state);
+static int  mct_u232_break_ctl(struct tty_struct *tty, int break_state);
 static int  mct_u232_tiocmget(struct tty_struct *tty);
 static int  mct_u232_tiocmset(struct tty_struct *tty,
 			unsigned int set, unsigned int clear);

@@ -677,7 +677,7 @@ static void mct_u232_set_termios(struct tty_struct *tty,
 	spin_unlock_irqrestore(&priv->lock, flags);
 } /* mct_u232_set_termios */
 
-static void mct_u232_break_ctl(struct tty_struct *tty, int break_state)
+static int mct_u232_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct mct_u232_private *priv = usb_get_serial_port_data(port);

@@ -691,7 +691,7 @@ static void mct_u232_break_ctl(struct tty_struct *tty, int break_state)
 		lcr |= MCT_U232_SET_BREAK;
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-	mct_u232_set_line_ctrl(port, lcr);
+	return mct_u232_set_line_ctrl(port, lcr);
 } /* mct_u232_break_ctl */
drivers/usb/serial/mos7720.c +5 -4
index 1d1f85fabc28..23544074eb1c 100644
@@ -996,7 +996,7 @@ static void mos7720_close(struct usb_serial_port *port)
 	mos7720_port->open = 0;
 }
 
-static void mos7720_break(struct tty_struct *tty, int break_state)
+static int mos7720_break(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	unsigned char data;

@@ -1007,7 +1007,7 @@ static void mos7720_break(struct tty_struct *tty, int break_state)
 
 	mos7720_port = usb_get_serial_port_data(port);
 	if (mos7720_port == NULL)
-		return;
+		return -ENODEV;
 
 	if (break_state == -1)
 		data = mos7720_port->shadowLCR | UART_LCR_SBC;

@@ -1015,8 +1015,9 @@ static void mos7720_break(struct tty_struct *tty, int break_state)
 		data = mos7720_port->shadowLCR & ~UART_LCR_SBC;
 
 	mos7720_port->shadowLCR  = data;
-	write_mos_reg(serial, port->port_number, MOS7720_LCR,
-		      mos7720_port->shadowLCR);
+
+	return write_mos_reg(serial, port->port_number, MOS7720_LCR,
+			     mos7720_port->shadowLCR);
 }
 
 /*
drivers/usb/serial/mos7840.c +4 -3
index 73370eaae0ab..8b0308d84270 100644
@@ -787,7 +787,7 @@ static void mos7840_close(struct usb_serial_port *port)
  * mos7840_break
  *	this function sends a break to the port
  *****************************************************************************/
-static void mos7840_break(struct tty_struct *tty, int break_state)
+static int mos7840_break(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct moschip_port *mos7840_port = usb_get_serial_port_data(port);

@@ -801,8 +801,9 @@ static void mos7840_break(struct tty_struct *tty, int break_state)
 	/* FIXME: no locking on shadowLCR anywhere in driver */
 	mos7840_port->shadowLCR = data;
 	dev_dbg(&port->dev, "%s mos7840_port->shadowLCR is %x\n", __func__, mos7840_port->shadowLCR);
-	mos7840_set_uart_reg(port, LINE_CONTROL_REGISTER,
-			     mos7840_port->shadowLCR);
+
+	return mos7840_set_uart_reg(port, LINE_CONTROL_REGISTER,
+				    mos7840_port->shadowLCR);
 }
 
 /*****************************************************************************
drivers/usb/serial/mxuport.c +3 -3
index faa0eedfe245..1f7bb3e4fcf2 100644
@@ -1230,7 +1230,7 @@ static void mxuport_close(struct usb_serial_port *port)
 }
 
 /* Send a break to the port. */
-static void mxuport_break_ctl(struct tty_struct *tty, int break_state)
+static int mxuport_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct usb_serial *serial = port->serial;

@@ -1244,8 +1244,8 @@ static void mxuport_break_ctl(struct tty_struct *tty, int break_state)
 		dev_dbg(&port->dev, "%s - clearing break\n", __func__);
 	}
 
-	mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_BREAK,
-			      enable, port->port_number);
+	return mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_BREAK,
+				     enable, port->port_number);
 }
 
 static int mxuport_resume(struct usb_serial *serial)
drivers/usb/serial/pl2303.c +9 -5
index 8949c1891164..d93f5d584557 100644
@@ -173,7 +173,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define PL2303_HXN_FLOWCTRL_RTS_CTS	0x18
 #define PL2303_HXN_FLOWCTRL_XON_XOFF	0x0c
 
-static void pl2303_set_break(struct usb_serial_port *port, bool enable);
+static int pl2303_set_break(struct usb_serial_port *port, bool enable);
 
 enum pl2303_type {
 	TYPE_H,

@@ -1060,7 +1060,7 @@ static int pl2303_carrier_raised(struct usb_serial_port *port)
 	return 0;
 }
 
-static void pl2303_set_break(struct usb_serial_port *port, bool enable)
+static int pl2303_set_break(struct usb_serial_port *port, bool enable)
 {
 	struct usb_serial *serial = port->serial;
 	u16 state;

@@ -1077,15 +1077,19 @@ static void pl2303_set_break(struct usb_serial_port *port, bool enable)
 	result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
 				 BREAK_REQUEST, BREAK_REQUEST_TYPE, state,
 				 0, NULL, 0, 100);
-	if (result)
+	if (result) {
 		dev_err(&port->dev, "error sending break = %d\n", result);
+		return result;
+	}
+
+	return 0;
 }
 
-static void pl2303_break_ctl(struct tty_struct *tty, int state)
+static int pl2303_break_ctl(struct tty_struct *tty, int state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 
-	pl2303_set_break(port, state);
+	return pl2303_set_break(port, state);
 }
 
 static void pl2303_update_line_status(struct usb_serial_port *port,
drivers/usb/serial/quatech2.c +6 -2
index fee581409bf6..821f25e52ec2 100644
@@ -741,7 +741,7 @@ static int qt2_tiocmset(struct tty_struct *tty,
 	return update_mctrl(port_priv, set, clear);
 }
 
-static void qt2_break_ctl(struct tty_struct *tty, int break_state)
+static int qt2_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct qt2_port_private *port_priv;

@@ -754,10 +754,14 @@ static void qt2_break_ctl(struct tty_struct *tty, int break_state)
 
 	status = qt2_control_msg(port->serial->dev, QT2_BREAK_CONTROL,
 				 val, port_priv->device_port);
-	if (status < 0)
+	if (status < 0) {
 		dev_warn(&port->dev,
 			 "%s - failed to send control message: %i\n", __func__,
 			 status);
+		return status;
+	}
+
+	return 0;
 }
drivers/usb/serial/ti_usb_3410_5052.c +7 -3
index b99f78224846..0fba25abf671 100644
@@ -319,7 +319,7 @@ static void ti_set_termios(struct tty_struct *tty,
 static int ti_tiocmget(struct tty_struct *tty);
 static int ti_tiocmset(struct tty_struct *tty,
 		unsigned int set, unsigned int clear);
-static void ti_break(struct tty_struct *tty, int break_state);
+static int ti_break(struct tty_struct *tty, int break_state);
 static void ti_interrupt_callback(struct urb *urb);
 static void ti_bulk_in_callback(struct urb *urb);
 static void ti_bulk_out_callback(struct urb *urb);

@@ -1071,7 +1071,7 @@ static int ti_tiocmset(struct tty_struct *tty,
 }
 
 
-static void ti_break(struct tty_struct *tty, int break_state)
+static int ti_break(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct ti_port *tport = usb_get_serial_port_data(port);

@@ -1083,8 +1083,12 @@ static void ti_break(struct tty_struct *tty, int break_state)
 		tport->tp_uart_base_addr + TI_UART_OFFSET_LCR,
 		TI_LCR_BREAK, break_state == -1 ? TI_LCR_BREAK : 0);
 
-	if (status)
+	if (status) {
 		dev_dbg(&port->dev, "%s - error setting break, %d\n", __func__, status);
+		return status;
+	}
+
+	return 0;
 }
 
 static int ti_get_port_from_code(unsigned char code)
drivers/usb/serial/upd78f0730.c +5 -2
index c47439bd90fa..46952182e04f 100644
@@ -238,12 +238,13 @@ static int upd78f0730_tiocmset(struct tty_struct *tty,
 	return res;
 }
 
-static void upd78f0730_break_ctl(struct tty_struct *tty, int break_state)
+static int upd78f0730_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct upd78f0730_port_private *private;
 	struct usb_serial_port *port = tty->driver_data;
 	struct upd78f0730_set_dtr_rts request;
 	struct device *dev = &port->dev;
+	int res;
 
 	private = usb_get_serial_port_data(port);
 

@@ -258,8 +259,10 @@ static void upd78f0730_break_ctl(struct tty_struct *tty, int break_state)
 	request.opcode = UPD78F0730_CMD_SET_DTR_RTS;
 	request.params = private->line_signals;
 
-	upd78f0730_send_ctl(port, &request, sizeof(request));
+	res = upd78f0730_send_ctl(port, &request, sizeof(request));
 	mutex_unlock(&private->lock);
+
+	return res;
 }
 
 static void upd78f0730_dtr_rts(struct usb_serial_port *port, int on)
drivers/usb/serial/usb-serial.c +1 -1
index f8404073558b..470634444af7 100644
@@ -539,7 +539,7 @@ static int serial_break(struct tty_struct *tty, int break_state)
 	dev_dbg(&port->dev, "%s\n", __func__);
 
 	if (port->serial->type->break_ctl)
-		port->serial->type->break_ctl(tty, break_state);
+		return port->serial->type->break_ctl(tty, break_state);
 
 	return 0;
 }
drivers/usb/serial/usb_debug.c +10 -3
index aaf4813e4971..6934970f180d 100644
@@ -47,12 +47,19 @@ MODULE_DEVICE_TABLE(usb, id_table_combined);
 /* This HW really does not support a serial break, so one will be
  * emulated when ever the break state is set to true.
  */
-static void usb_debug_break_ctl(struct tty_struct *tty, int break_state)
+static int usb_debug_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
+	int ret;
+
 	if (!break_state)
-		return;
-	usb_serial_generic_write(tty, port, USB_DEBUG_BRK, USB_DEBUG_BRK_SIZE);
+		return 0;
+
+	ret = usb_serial_generic_write(tty, port, USB_DEBUG_BRK, USB_DEBUG_BRK_SIZE);
+	if (ret < 0)
+		return ret;
+
+	return 0;
 }
 
 static void usb_debug_process_read_urb(struct urb *urb)
drivers/usb/serial/whiteheat.c +4 -3
index 7f82d40753ee..ca48e90a8e81 100644
@@ -87,7 +87,7 @@ static void whiteheat_set_termios(struct tty_struct *tty,
 static int  whiteheat_tiocmget(struct tty_struct *tty);
 static int  whiteheat_tiocmset(struct tty_struct *tty,
 			unsigned int set, unsigned int clear);
-static void whiteheat_break_ctl(struct tty_struct *tty, int break_state);
+static int whiteheat_break_ctl(struct tty_struct *tty, int break_state);
 
 static struct usb_serial_driver whiteheat_fake_device = {
 	.driver = {

@@ -449,10 +449,11 @@ static void whiteheat_set_termios(struct tty_struct *tty,
 	firm_setup_port(tty);
 }
 
-static void whiteheat_break_ctl(struct tty_struct *tty, int break_state)
+static int whiteheat_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	firm_set_break(port, break_state);
+
+	return firm_set_break(port, break_state);
 }
drivers/usb/serial/xr_serial.c +2 -2
index fdb0aae546c3..4ec7c5892b84 100644
@@ -503,7 +503,7 @@ static void xr_dtr_rts(struct usb_serial_port *port, int on)
 		xr_tiocmset_port(port, 0, TIOCM_DTR | TIOCM_RTS);
 }
 
-static void xr_break_ctl(struct tty_struct *tty, int break_state)
+static int xr_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct xr_data *data = usb_get_serial_port_data(port);

@@ -517,7 +517,7 @@ static void xr_break_ctl(struct tty_struct *tty, int break_state)
 
 	dev_dbg(&port->dev, "Turning break %s\n", state == 0 ? "off" : "on");
 
-	xr_set_reg_uart(port, type->tx_break, state);
+	return xr_set_reg_uart(port, type->tx_break, state);
 }
 
 /* Tx and Rx clock mask values obtained from section 3.3.4 of datasheet */
include/linux/usb/serial.h +1 -1
index 7eeb5f9c4f0d..1a0a4dc87980 100644
@@ -278,7 +278,7 @@ struct usb_serial_driver {
 	int  (*set_serial)(struct tty_struct *tty, struct serial_struct *ss);
 	void (*set_termios)(struct tty_struct *tty, struct usb_serial_port *port,
 			    const struct ktermios *old);
-	void (*break_ctl)(struct tty_struct *tty, int break_state);
+	int (*break_ctl)(struct tty_struct *tty, int break_state);
 	unsigned int (*chars_in_buffer)(struct tty_struct *tty);
 	void (*wait_until_sent)(struct tty_struct *tty, long timeout);
 	bool (*tx_empty)(struct usb_serial_port *port);
Signature · 1 line
2.39.3
maintainer
#2 lore raw

drivers/usb/serial/pl2303.c static void pl2303_set_break(struct usb_serial_port *port, bool enable) · line 1086

Quoted 12 lines · Johan Hovold
drivers/usb/serial/pl2303.c +5 -1
@@ -1077,15 +1077,19 @@ static void pl2303_set_break(struct usb_serial_port *port, bool enable)
  	result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
  				 BREAK_REQUEST, BREAK_REQUEST_TYPE, state,
  				 0, NULL, 0, 100);
-	if (result)
+	if (result) {
  		dev_err(&port->dev, "error sending break = %d\n", result);
+		return result;
+	}
+
+	return 0;
  }

Hi,

this code was always fishy, but I am afraid it worked by accident albeit spamming the logs.
If I may quote from the kerneldoc of usb_control_msg():

* usb_control_msg - Builds a control urb, sends it off and waits for completion

[..]

* Return: If successful, the number of bytes transferred. Otherwise, a negative
* error number.
*/

You need to test for < 0, not != 0

Regards
Oliver

author maintainer
#3 lore raw

Hi Oliver,

Quoted 13 lines · Oliver Neukum

drivers/usb/serial/pl2303.c static void pl2303_set_break(struct usb_serial_port *port, bool enable) · line 1086

Quoted 12 lines · Johan Hovold
drivers/usb/serial/pl2303.c +5 -1
@@ -1077,15 +1077,19 @@ static void pl2303_set_break(struct usb_serial_port *port, bool enable)
  	result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
  				 BREAK_REQUEST, BREAK_REQUEST_TYPE, state,
  				 0, NULL, 0, 100);
-	if (result)
+	if (result) {
  		dev_err(&port->dev, "error sending break = %d\n", result);
+		return result;
+	}
+
+	return 0;
  }

And thanks for taking a look.

Quoted 6 lines

this code was always fishy, but I am afraid it worked by accident albeit spamming the logs.
If I may quote from the kerneldoc of usb_control_msg():

* usb_control_msg - Builds a control urb, sends it off and waits for completion

Quoted 7 lines

[..]

* Return: If successful, the number of bytes transferred. Otherwise, a negative
* error number.
*/

You need to test for < 0, not != 0

No, the current code is just fine as there is no data stage so usb_control_msg() returns 0 or negative errno.

Johan

author maintainer
#4 lore raw

PATCH v2 2/3 USB: serial: cp210x: disable break signalling on CP2105 SCI

In reply to Johan Hovold

Only the first UART interface (ECI) on CP2105 supports break signalling.

Return an error on requests for break state changes for the second interface (SCI) to avoid transmitting a garbage character and waiting when break is not supported.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/cp210x.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/usb/serial/cp210x.c +6 -0
index 81e49ed9d147..1e61fe043171 100644
@@ -1437,8 +1437,14 @@ static int cp210x_tiocmget(struct tty_struct *tty)
 static int cp210x_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;
+	struct cp210x_serial_private *priv = usb_get_serial_data(port->serial);
 	u16 state;
 
+	if (priv->partnum == CP210X_PARTNUM_CP2105) {
+		if (cp210x_interface_num(port->serial) == 1)
+			return -ENOTTY;
+	}
+
 	if (break_state == 0)
 		state = BREAK_OFF;
 	else
Signature · 1 line
2.39.3
author maintainer
#5 lore raw

PATCH v2 3/3 USB: serial: report unsupported break signalling

In reply to Johan Hovold

Instead of returning success when a driver does not support break signalling, return an error to let user space know and to avoid waiting when break is not supported.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/usb-serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/serial/usb-serial.c +1 -1
index 470634444af7..7b4805c1004d 100644
@@ -541,7 +541,7 @@ static int serial_break(struct tty_struct *tty, int break_state)
 	if (port->serial->type->break_ctl)
 		return port->serial->type->break_ctl(tty, break_state);
 
-	return 0;
+	return -ENOTTY;
 }
 
 static int serial_proc_show(struct seq_file *m, void *v)
Signature · 1 line
2.39.3
Tested-by on the v2 series
#6 lore raw
In reply to Johan Hovold
Quoted 9 lines · Johan Hovold

This series starts returning errors from break handling and also uses that mechanism to report to user space when break signalling is not supported (e.g. when device or driver support is missing).

Note that the tty layer currently returns early but without reporting errors when a tty driver does not support break signalling. The intent expressed in commit 9e98966c7bb9 ("tty: rework break handling") from 2008 appears to be to allow missing support to be reported to user space however.

This worked as expected for me.

Tested-by Corey Minyard <[email protected]>
Quoted 47 lines

Johan

Changes in v2
- fix return of potentially uninitialised status variable in io_edgeport as reported by kernel test robot <[email protected]> and Dan Carpenter:

   https://lore.kernel.org/all/[email protected]/

Johan Hovold (3):
USB: serial: return errors from break handling
USB: serial: cp210x: disable break signalling on CP2105 SCI
USB: serial: report unsupported break signalling

 drivers/usb/serial/ark3116.c          |  7 +++--
 drivers/usb/serial/belkin_sa.c        | 12 ++++++---
 drivers/usb/serial/ch341.c            | 37 +++++++++++++++++----------
 drivers/usb/serial/cp210x.c           | 14 +++++++---
 drivers/usb/serial/digi_acceleport.c  |  7 ++---
 drivers/usb/serial/f81232.c           |  4 ++-
 drivers/usb/serial/f81534.c           |  4 ++-
 drivers/usb/serial/ftdi_sio.c         | 10 +++++---
 drivers/usb/serial/io_edgeport.c      |  6 +++--
 drivers/usb/serial/io_ti.c            |  9 +++++--
 drivers/usb/serial/keyspan.c          |  5 +++-
 drivers/usb/serial/keyspan_pda.c      |  8 ++++--
 drivers/usb/serial/mct_u232.c         |  6 ++---
 drivers/usb/serial/mos7720.c          |  9 ++++---
 drivers/usb/serial/mos7840.c          |  7 ++---
 drivers/usb/serial/mxuport.c          |  6 ++---
 drivers/usb/serial/pl2303.c           | 14 ++++++----
 drivers/usb/serial/quatech2.c         |  8 ++++--
 drivers/usb/serial/ti_usb_3410_5052.c | 10 +++++---
 drivers/usb/serial/upd78f0730.c       |  7 +++--
 drivers/usb/serial/usb-serial.c       |  4 +--
 drivers/usb/serial/usb_debug.c        | 13 +++++++---
 drivers/usb/serial/whiteheat.c        |  7 ++---
 drivers/usb/serial/xr_serial.c        |  4 +--
 include/linux/usb/serial.h            |  2 +-
 25 files changed, 147 insertions(+), 73 deletions(-)
Signature · 1 line
2.39.3
maintainer
#7 lore raw
In reply to Johan Hovold
Quoted 3 lines · Johan Hovold

This series starts returning errors from break handling and also uses that mechanism to report to user space when break signalling is not supported (e.g. when device or driver support is missing).

Hi,

do you eventually want this to be done for all serial devices? That is does cdc-acm need something like this patch?

Regards
Oliver

From 16430d9f109f904b2bfbac6e43a939209b6c4bc7 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <[email protected]>
Date: Tue, 6 Jun 2023 12:57:00 +0200
Subject: [PATCH] usb: cdc-acm: return correct error code on unsupported break

Return -ENOTTY if the device says that it doesn't support break so that the upper layers get error reporting right.

Signed-off-by Oliver Neukum <[email protected]>
---
 drivers/usb/class/cdc-acm.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/usb/class/cdc-acm.c +3 -0
index 11da5fb284d0..7751f5728716 100644
@@ -892,6 +892,9 @@ static int acm_tty_break_ctl(struct tty_struct *tty, int state)
 	struct acm *acm = tty->driver_data;
 	int retval;
 
+	if (!(acm->ctrl_caps & USB_CDC_CAP_BRK))
+		return -ENOTTY;
+
 	retval = acm_send_break(acm, state ? 0xffff : 0);
 	if (retval < 0)
 		dev_dbg(&acm->control->dev,
Signature · 1 line
2.40.1
author maintainer Acked-by
#8 lore raw
Quoted 7 lines · Oliver Neukum
Quoted 3 lines · Johan Hovold

This series starts returning errors from break handling and also uses that mechanism to report to user space when break signalling is not supported (e.g. when device or driver support is missing).

do you eventually want this to be done for all serial devices? That is does cdc-acm need something like this patch?

Looks good to me. If this turns out to confuse userspace we may have to turn that -ENOTTY into 0 in the tty layer, but we can still use it to avoid the unnecessary wait to "disable" the break state.

Quoted 9 lines

From 16430d9f109f904b2bfbac6e43a939209b6c4bc7 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <[email protected]>
Date: Tue, 6 Jun 2023 12:57:00 +0200
Subject: [PATCH] usb: cdc-acm: return correct error code on unsupported break

Return -ENOTTY if the device says that it doesn't support break so that the upper layers get error reporting right.

Signed-off-by Oliver Neukum <[email protected]>
Acked-by Johan Hovold <[email protected]>
Quoted 18 lines
---
 drivers/usb/class/cdc-acm.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/usb/class/cdc-acm.c +3 -0
index 11da5fb284d0..7751f5728716 100644
@@ -892,6 +892,9 @@ static int acm_tty_break_ctl(struct tty_struct *tty, int state)
 	struct acm *acm = tty->driver_data;
 	int retval;
 
+	if (!(acm->ctrl_caps & USB_CDC_CAP_BRK))
+		return -ENOTTY;
+
 	retval = acm_send_break(acm, state ? 0xffff : 0);
 	if (retval < 0)
 		dev_dbg(&acm->control->dev,
#9 lore raw
Quoted 6 lines · Johan Hovold
Quoted 4 lines · Oliver Neukum
Quoted 3 lines · Johan Hovold

This series starts returning errors from break handling and also uses that mechanism to report to user space when break signalling is not supported (e.g. when device or driver support is missing).

Quoted 14 lines
Quoted 2 lines

do you eventually want this to be done for all serial devices? That is does cdc-acm need something like this patch?

Looks good to me. If this turns out to confuse userspace we may have to turn that -ENOTTY into 0 in the tty layer, but we can still use it to avoid the unnecessary wait to "disable" the break state.

Quoted 7 lines

From 16430d9f109f904b2bfbac6e43a939209b6c4bc7 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <[email protected]>
Date: Tue, 6 Jun 2023 12:57:00 +0200
Subject: [PATCH] usb: cdc-acm: return correct error code on unsupported break

Return -ENOTTY if the device says that it doesn't support break so that the upper layers get error reporting right.

Yeah, when I asked for this, I didn't realize that no devices did this, I thought it was a one-off with this device. There is a distinct possibility that this will break userland. This sounds like a good plan.

-corey

Quoted 23 lines
Quoted 2 lines
Signed-off-by Oliver Neukum <[email protected]>
Acked-by Johan Hovold <[email protected]>
Quoted 18 lines
---
 drivers/usb/class/cdc-acm.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/usb/class/cdc-acm.c +3 -0
index 11da5fb284d0..7751f5728716 100644
@@ -892,6 +892,9 @@ static int acm_tty_break_ctl(struct tty_struct *tty, int state)
 	struct acm *acm = tty->driver_data;
 	int retval;
 
+	if (!(acm->ctrl_caps & USB_CDC_CAP_BRK))
+		return -ENOTTY;
+
 	retval = acm_send_break(acm, state ? 0xffff : 0);
 	if (retval < 0)
 		dev_dbg(&acm->control->dev,