USB: serial: xr: fix up remaining issues in new driver
Subsystems (2)
Johan Hovold
maintainercover USB: serial: xr: fix up remaining issues in new driver
This series fixes the remaining issues in the new MaxLinear driver that were pointed out here:
https://lore.kernel.org/r/[email protected]
Johan
Johan Hovold (10):
USB: serial: xr: fix NULL-deref at probe
USB: serial: xr: fix interface leak at disconnect
USB: serial: xr: use subsystem usb_device at probe
USB: serial: xr: use termios flag helpers
USB: serial: xr: document vendor-request recipient
USB: serial: xr: clean up line-settings handling
USB: serial: xr: simplify line-speed logic
USB: serial: xr: fix gpio-mode handling
USB: serial: xr: fix pin configuration
USB: serial: xr: fix B0 handling
drivers/usb/serial/xr_serial.c | 102 +++++++++++++++++++-------------- 1 file changed, 59 insertions(+), 43 deletions(-)
Signature · 1 line
2.26.2
Johan Hovold
maintainerPATCH 1/10 USB: serial: xr: fix NULL-deref at probe
Make sure that the probed device has an interface 0 to avoid dereferencing a NULL pointer in case of a malicious device or during USB-descriptor fuzzing.
--- drivers/usb/serial/xr_serial.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/usb/serial/xr_serial.c +3 -0
@@ -552,6 +552,9 @@ static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)/* But claim the control interface during data interface probe */ control_interface =usb_ifnum_to_if (usb_dev,0 ); +if (!control_interface) +return -ENODEV ; + ret =usb_driver_claim_interface (driver, control_interface,NULL );if (ret) {dev_err (&serial->interface->dev,"Failed to claim control interface\n" );
Signature · 1 line
2.26.2
Johan Hovold
maintainerPATCH 1/1 USB: serial: cp210x: suppress modem-control error on open and close
In reply to Johan HovoldThe CP210X_SET_MHS request cannot be used to control DTR/RTS when hardware flow control is enabled and instead returns an error which is currently logged as:
cp210x ttyUSB0: failed set request 0x7 status: -32
Add a crtscts flag to keep track of the hardware flow-control setting and use it to suppress the request in dtr_rts().
Note that both lines are still deasserted when disabling the UART as part of close().
Also note that TIOCMSET is left unchanged and will continue to return an error to user-space when flow control is enabled (i.e. instead of disabling and re-enabling auto-RTS when RTS is deasserted and re-asserted).
--- drivers/usb/serial/cp210x.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/usb/serial/cp210x.c +15 -1
@@ -264,7 +264,8 @@ struct cp210x_port_private {u8 bInterfaceNumber;bool event_mode;enum cp210x_event_state event_state; -u8 lsr; +u8 lsr; +bool crtscts; };static struct usb_serial_driver cp210x_device = {
@@ -1117,6 +1118,7 @@ static bool cp210x_termios_change(const struct ktermios *a, const struct ktermiostatic void cp210x_set_flow_control (struct tty_struct *tty,struct usb_serial_port *port,struct ktermios *old_termios) { +struct cp210x_port_private *port_priv =usb_get_serial_port_data (port);struct cp210x_special_chars chars;struct cp210x_flow_ctl flow_ctl;u32 flow_repl;
@@ -1161,10 +1163,12 @@ static void cp210x_set_flow_control(struct tty_struct *tty, ctl_hs |=CP210X_SERIAL_CTS_HANDSHAKE ; flow_repl &= ~CP210X_SERIAL_RTS_MASK ; flow_repl |=CP210X_SERIAL_RTS_SHIFT (CP210X_SERIAL_RTS_FLOW_CTL ); + port_priv->crtscts = true; }else { ctl_hs &= ~CP210X_SERIAL_CTS_HANDSHAKE ; flow_repl &= ~CP210X_SERIAL_RTS_MASK ; flow_repl |=CP210X_SERIAL_RTS_SHIFT (CP210X_SERIAL_RTS_ACTIVE ); + port_priv->crtscts = false; }if (I_IXOFF (tty))
@@ -1298,6 +1302,16 @@ static int cp210x_tiocmset_port(struct usb_serial_port *port,static void cp210x_dtr_rts (struct usb_serial_port *port,int on) { +struct cp210x_port_private *port_priv =usb_get_serial_port_data (port); + +/* +* CP210X_SET_MHS cannot be used to control DTR/RTS when hardware flow +* control is enabled. Note that both lines are still deasserted when +* disabling the UART. +*/ +if (port_priv->crtscts) +return ; +if (on)cp210x_tiocmset_port (port,TIOCM_DTR |TIOCM_RTS ,0 );else
Signature · 1 line
2.26.2
Johan Hovold
maintainerRe: [PATCH] USB: serial: cp210x: suppress modem-control error on open and close
Quoted 12 lines · Johan Hovold
The
CP210X_SET_MHSrequest cannot be used to control DTR/RTS when hardware flow control is enabled and instead returns an error which is currently logged as:cp210x ttyUSB0: failed set request 0x7 status: -32Add a crtscts flag to keep track of the hardware flow-control setting and use it to suppress the request in
dtr_rts().Note that both lines are still deasserted when disabling the UART as part of
close().
Quoted 8 lines
Also note that TIOCMSET is left unchanged and will continue to return an error to user-space when flow control is enabled (i.e. instead of disabling and re-enabling auto-RTS when RTS is deasserted and re-asserted).
Reported-by Pho Tran <[email protected]>Signed-off-by Johan Hovold <[email protected]>---
Oops, this one wasn't supposed to be submitted again. Please ignore.
Johan
Johan Hovold
maintainerPATCH 2/10 USB: serial: xr: fix interface leak at disconnect
In reply to Johan HovoldMake sure to release the control interface at disconnect so that the driver can be unbound without leaking resources (and later rebound).
--- drivers/usb/serial/xr_serial.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/usb/serial/xr_serial.c +10 -0
@@ -564,6 +564,15 @@ static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)return 0 ; } +static void xr_disconnect (struct usb_serial *serial) +{ +struct usb_driver *driver = serial->type->usb_driver; +struct usb_interface *control_interface; + + control_interface =usb_ifnum_to_if (serial->dev,0 ); +usb_driver_release_interface (driver, control_interface); +} +static const struct usb_device_id id_table[] = { {USB_DEVICE (0x04e2 ,0x1410 ) },/* XR21V141X */ { }
@@ -578,6 +587,7 @@ static struct usb_serial_driver xr_device = { .id_table = id_table, .num_ports = 1, .probe = xr_probe, + .disconnect = xr_disconnect, .open = xr_open, .close = xr_close, .break_ctl = xr_break_ctl,
Signature · 1 line
2.26.2
Johan Hovold
maintainerPATCH 3/10 USB: serial: xr: use subsystem usb_device at probe
In reply to Johan HovoldUse the subsystem struct usb_device pointer at probe instead of deriving it from the interface pointer.
--- drivers/usb/serial/xr_serial.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/serial/xr_serial.c +1 -2
@@ -541,7 +541,6 @@ static void xr_close(struct usb_serial_port *port)static int xr_probe (struct usb_serial *serial,const struct usb_device_id *id) { -struct usb_device *usb_dev =interface_to_usbdev (serial->interface);struct usb_driver *driver = serial->type->usb_driver;struct usb_interface *control_interface;int ret;
@@ -551,7 +550,7 @@ static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)return -ENODEV ;/* But claim the control interface during data interface probe */ - control_interface =usb_ifnum_to_if (usb_dev,0 ); + control_interface =usb_ifnum_to_if (serial->dev,0 );if (!control_interface)return -ENODEV ;
Signature · 1 line
2.26.2
Johan Hovold
maintainerPATCH 4/10 USB: serial: xr: use termios flag helpers
In reply to Johan HovoldUse the termios flag helpers consistently, including for CRTSCTS.
--- drivers/usb/serial/xr_serial.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/serial/xr_serial.c +1 -2
@@ -401,7 +401,6 @@ static int xr_set_baudrate(struct tty_struct *tty,static void xr_set_flow_mode (struct tty_struct *tty,struct usb_serial_port *port) { -unsigned int cflag = tty->termios.c_cflag;u8 flow, gpio_mode;int ret;
@@ -409,7 +408,7 @@ static void xr_set_flow_mode(struct tty_struct *tty,if (ret)return ; -if (cflag &CRTSCTS ) { +if (C_CRTSCTS (tty)) {dev_dbg (&port->dev,"Enabling hardware flow ctrl\n" );/*
Signature · 1 line
2.26.2
Johan Hovold
maintainerPATCH 5/10 USB: serial: xr: document vendor-request recipient
In reply to Johan HovoldAdd the missing device-recipient define to the vendor control requests for completeness.
--- drivers/usb/serial/xr_serial.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/serial/xr_serial.c +4 -4
@@ -116,8 +116,8 @@ static int xr_set_reg(struct usb_serial_port *port, u8 block, u8 reg, u8 val) ret =usb_control_msg (serial->dev,usb_sndctrlpipe (serial->dev,0 ),XR21V141X_SET_REQ , -USB_DIR_OUT |USB_TYPE_VENDOR , val, - reg | (block <<8 ),NULL ,0 , +USB_DIR_OUT |USB_TYPE_VENDOR |USB_RECIP_DEVICE , + val, reg | (block <<8 ),NULL ,0 ,USB_CTRL_SET_TIMEOUT );if (ret <0 ) {dev_err (&port->dev,"Failed to set reg 0x%02x: %d\n" , reg, ret);
@@ -140,8 +140,8 @@ static int xr_get_reg(struct usb_serial_port *port, u8 block, u8 reg, u8 *val) ret =usb_control_msg (serial->dev,usb_rcvctrlpipe (serial->dev,0 ),XR21V141X_GET_REQ , -USB_DIR_IN |USB_TYPE_VENDOR ,0 , - reg | (block <<8 ), dmabuf,1 , +USB_DIR_IN |USB_TYPE_VENDOR |USB_RECIP_DEVICE , +0 , reg | (block <<8 ), dmabuf,1 ,USB_CTRL_GET_TIMEOUT );if (ret ==1 ) { *val = *dmabuf;
Signature · 1 line
2.26.2
Johan Hovold
maintainerPATCH 6/10 USB: serial: xr: clean up line-settings handling
In reply to Johan HovoldShift the line-setting values when defining them rather than in set_termios() for consistency and improved readability.
--- drivers/usb/serial/xr_serial.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-)
drivers/usb/serial/xr_serial.c +15 -19
@@ -71,17 +71,17 @@ struct xr_txrx_clk_mask {#define XR21V141X_UART_DATA_8 0x8#define XR21V141X_UART_PARITY_MASK GENMASK(6, 4) -#define XR21V141X_UART_PARITY_SHIFT 0x4 -#define XR21V141X_UART_PARITY_NONE 0x0 -#define XR21V141X_UART_PARITY_ODD 0x1 -#define XR21V141X_UART_PARITY_EVEN 0x2 -#define XR21V141X_UART_PARITY_MARK 0x3 -#define XR21V141X_UART_PARITY_SPACE 0x4 +#define XR21V141X_UART_PARITY_SHIFT 4 +#define XR21V141X_UART_PARITY_NONE (0x0 << XR21V141X_UART_PARITY_SHIFT) +#define XR21V141X_UART_PARITY_ODD (0x1 << XR21V141X_UART_PARITY_SHIFT) +#define XR21V141X_UART_PARITY_EVEN (0x2 << XR21V141X_UART_PARITY_SHIFT) +#define XR21V141X_UART_PARITY_MARK (0x3 << XR21V141X_UART_PARITY_SHIFT) +#define XR21V141X_UART_PARITY_SPACE (0x4 << XR21V141X_UART_PARITY_SHIFT)#define XR21V141X_UART_STOP_MASK BIT(7) -#define XR21V141X_UART_STOP_SHIFT 0x7 -#define XR21V141X_UART_STOP_1 0x0 -#define XR21V141X_UART_STOP_2 0x1 +#define XR21V141X_UART_STOP_SHIFT 7 +#define XR21V141X_UART_STOP_1 (0x0 << XR21V141X_UART_STOP_SHIFT) +#define XR21V141X_UART_STOP_2 (0x1 << XR21V141X_UART_STOP_SHIFT)#define XR21V141X_UART_FLOW_MODE_NONE 0x0#define XR21V141X_UART_FLOW_MODE_HW 0x1
@@ -477,25 +477,21 @@ static void xr_set_termios(struct tty_struct *tty,if (C_PARENB (tty)) {if (C_CMSPAR (tty)) {if (C_PARODD (tty)) - bits |=XR21V141X_UART_PARITY_MARK << -XR21V141X_UART_PARITY_SHIFT ; + bits |=XR21V141X_UART_PARITY_MARK ;else - bits |=XR21V141X_UART_PARITY_SPACE << -XR21V141X_UART_PARITY_SHIFT ; + bits |=XR21V141X_UART_PARITY_SPACE ; }else {if (C_PARODD (tty)) - bits |=XR21V141X_UART_PARITY_ODD << -XR21V141X_UART_PARITY_SHIFT ; + bits |=XR21V141X_UART_PARITY_ODD ;else - bits |=XR21V141X_UART_PARITY_EVEN << -XR21V141X_UART_PARITY_SHIFT ; + bits |=XR21V141X_UART_PARITY_EVEN ; } }if (C_CSTOPB (tty)) - bits |=XR21V141X_UART_STOP_2 <<XR21V141X_UART_STOP_SHIFT ; + bits |=XR21V141X_UART_STOP_2 ;else - bits |=XR21V141X_UART_STOP_1 <<XR21V141X_UART_STOP_SHIFT ; + bits |=XR21V141X_UART_STOP_1 ; ret =xr_set_reg_uart (port,XR21V141X_REG_FORMAT , bits);if (ret)
Signature · 1 line
2.26.2
Johan Hovold
maintainerPATCH 7/10 USB: serial: xr: simplify line-speed logic
In reply to Johan HovoldSimplify the changed-line-speed conditional expression.
--- drivers/usb/serial/xr_serial.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/serial/xr_serial.c +1 -2
@@ -451,8 +451,7 @@ static void xr_set_termios(struct tty_struct *tty,u8 bits =0 ;int ret; -if ((old_termios && tty->termios.c_ospeed != old_termios->c_ospeed) || - !old_termios) +if (!old_termios || (tty->termios.c_ospeed != old_termios->c_ospeed))xr_set_baudrate (tty, port);switch (C_CSIZE (tty)) {
Signature · 1 line
2.26.2
Johan Hovold
maintainerPATCH 8/10 USB: serial: xr: fix gpio-mode handling
In reply to Johan HovoldFix the gpio-mode handling so that all the pins are under driver control (i.e. in gpio mode) when hardware flow control is disabled.
This is specifically needed to be able to control RTS.
--- drivers/usb/serial/xr_serial.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/usb/serial/xr_serial.c +3 -6
@@ -408,14 +408,11 @@ static void xr_set_flow_mode(struct tty_struct *tty,if (ret)return ; +/* Set GPIO mode for controlling the pins manually by default. */ + gpio_mode &= ~XR21V141X_UART_MODE_GPIO_MASK ; +if (C_CRTSCTS (tty)) {dev_dbg (&port->dev,"Enabling hardware flow ctrl\n" ); - -/* -* RTS/CTS is the default flow control mode, so set GPIO mode -* for controlling the pins manually by default. -*/ - gpio_mode &= ~XR21V141X_UART_MODE_GPIO_MASK ; gpio_mode |=XR21V141X_UART_MODE_RTS_CTS ; flow =XR21V141X_UART_FLOW_MODE_HW ; }else if (I_IXON (tty)) {
Signature · 1 line
2.26.2
Johan Hovold
maintainerPATCH 9/10 USB: serial: xr: fix pin configuration
In reply to Johan HovoldMake sure that the modem pins are set up correctly when opening the port to avoid leaving, for example, DTR and RTS configured as inputs, which is the device default.
This is specifically needed to be able to control DTR and RTS when hardware flow control is disabled.
--- drivers/usb/serial/xr_serial.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/usb/serial/xr_serial.c +8 -0
@@ -502,6 +502,7 @@ static void xr_set_termios(struct tty_struct *tty,static int xr_open (struct tty_struct *tty,struct usb_serial_port *port) { +u8 gpio_dir;int ret; ret =xr_uart_enable (port);
@@ -510,6 +511,13 @@ static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)return ret; } +/* +* Configure DTR and RTS as outputs and RI, CD, DSR and CTS as +* inputs. +*/ + gpio_dir =XR21V141X_UART_MODE_DTR |XR21V141X_UART_MODE_RTS ; +xr_set_reg_uart (port,XR21V141X_REG_GPIO_DIR , gpio_dir); +/* Setup termios */ if (tty)xr_set_termios (tty, port,NULL );
Signature · 1 line
2.26.2
Johan Hovold
maintainerPATCH 10/10 USB: serial: xr: fix B0 handling
In reply to Johan HovoldFix up B0 handling which should leave the baud rate unchanged and specifically not report back a non-B0 rate when B0 is requested; must temporarily disable hardware flow control so that RTS can be deasserted; and should reassert DTR/RTS when moving from B0.
--- drivers/usb/serial/xr_serial.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
drivers/usb/serial/xr_serial.c +14 -9
@@ -341,8 +341,11 @@ static int xr_set_baudrate(struct tty_struct *tty,u16 tx_mask, rx_mask;int ret; - baud =clamp (tty->termios.c_ospeed,XR21V141X_MIN_SPEED , -XR21V141X_MAX_SPEED ); + baud = tty->termios.c_ospeed; +if (!baud) +return 0 ; + + baud =clamp (baud,XR21V141X_MIN_SPEED ,XR21V141X_MAX_SPEED ); divisor =XR_INT_OSC_HZ / baud; idx = ((32 *XR_INT_OSC_HZ ) / baud) &0x1f ; tx_mask = xr21v141x_txrx_clk_masks[idx].tx;
@@ -399,7 +402,8 @@ static int xr_set_baudrate(struct tty_struct *tty, }static void xr_set_flow_mode (struct tty_struct *tty, -struct usb_serial_port *port) +struct usb_serial_port *port, +struct ktermios *old_termios) {u8 flow, gpio_mode;int ret;
@@ -411,7 +415,7 @@ static void xr_set_flow_mode(struct tty_struct *tty,/* Set GPIO mode for controlling the pins manually by default. */ gpio_mode &= ~XR21V141X_UART_MODE_GPIO_MASK ; -if (C_CRTSCTS (tty)) { +if (C_CRTSCTS (tty) &&C_BAUD (tty) !=B0 ) {dev_dbg (&port->dev,"Enabling hardware flow ctrl\n" ); gpio_mode |=XR21V141X_UART_MODE_RTS_CTS ; flow =XR21V141X_UART_FLOW_MODE_HW ;
@@ -438,6 +442,11 @@ static void xr_set_flow_mode(struct tty_struct *tty,xr_uart_enable (port);xr_set_reg_uart (port,XR21V141X_REG_GPIO_MODE , gpio_mode); + +if (C_BAUD (tty) ==B0 ) +xr_dtr_rts (port,0 ); +else if (old_termios && (old_termios->c_cflag &CBAUD ) ==B0 ) +xr_dtr_rts (port,1 ); } static void xr_set_termios(struct tty_struct *tty,
@@ -493,11 +502,7 @@ static void xr_set_termios(struct tty_struct *tty,if (ret)return ; -/* If baud rate is B0, clear DTR and RTS */ -if (C_BAUD (tty) ==B0 ) -xr_dtr_rts (port,0 ); - -xr_set_flow_mode (tty, port); +xr_set_flow_mode (tty, port, old_termios); }static int xr_open (struct tty_struct *tty,struct usb_serial_port *port)
Signature · 1 line
2.26.2
Johan Hovold
maintainerQuoted 17 lines · Johan Hovold
This series fixes the remaining issues in the new MaxLinear driver that were pointed out here:
https://lore.kernel.org/r/[email protected]
Johan Hovold (10):
USB: serial: xr: fix NULL-deref at probe
USB: serial: xr: fix interface leak at disconnect
USB: serial: xr: use subsystemusb_deviceat probe
USB: serial: xr: use termios flag helpers
USB: serial: xr: document vendor-request recipient
USB: serial: xr: clean up line-settings handling
USB: serial: xr: simplify line-speed logic
USB: serial: xr: fix gpio-mode handling
USB: serial: xr: fix pin configuration
USB: serial: xr: fix B0 handling
Quoted 2 lines
drivers/usb/serial/xr_serial.c | 102 +++++++++++++++++++-------------- 1 file changed, 59 insertions(+), 43 deletions(-)
I have applied these now.
Johan