usb serial: pl2303 driver TxD "break" stays after close() bug

usb serial: pl2303 driver TxD "break" stays after close() bug

[Jari Ruusu]

Tested on 3.10.67 and 3.18.5 kernels with ATEN UC-232A usb-serial adapter.
No patch, sorry. To actually see the stuck "break" signal on TxD line, you
need either some sort of LED or voltmeter connected to the data transmit
line. Other RS-232 serial ports that I have access to (normal PC hardware
serial ports and FTDI usb-serial adapters) do not have this bug.


/* pl2303 TxD "break" stays after close() demo */

#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <termios.h>
#include <fcntl.h>
#include <stdlib.h>

int main(int argc, char **argv)
{
    struct termios tt;
    int fd;

    if(argc < 2) {
        fprintf(stderr, "usage: %s /dev/ttyUSB0\n", argv[0]);
        exit(1);
    }
    if((fd = open(argv[1], O_RDWR | O_NOCTTY, 0)) == -1) {
        perror("serial port open failed");
        exit(1);
    }
    tcgetattr(fd, &tt);
    cfmakeraw(&tt);
    tcsetattr(fd, TCSANOW, &tt);
    if(ioctl(fd, TIOCSBRK, 0) == -1) {
        perror("set BRK failed");
        exit(1);
    }
    close(fd);
    exit(0);
}

-- 
Jari Ruusu  4096R/8132F189 12D6 4C3A DCDA 0AA4 27BD  ACDF F073 3C80 8132 F189

Re: usb serial: pl2303 driver TxD "break" stays after close() bug

[Johan Hovold]

On Thu, Feb 05, 2015 at 12:56:20PM +0200, Jari Ruusu wrote:
> Tested on 3.10.67 and 3.18.5 kernels with ATEN UC-232A usb-serial adapter.
> No patch, sorry. To actually see the stuck "break" signal on TxD line, you
> need either some sort of LED or voltmeter connected to the data transmit
> line. Other RS-232 serial ports that I have access to (normal PC hardware
> serial ports and FTDI usb-serial adapters) do not have this bug.

Thanks for the report. I believe you may be right here, but I'm not able
to verify this until earliest next week.

What happens when you reopen the port? Is the break state cleared then?

Thanks,
Johan

Re: usb serial: pl2303 driver TxD "break" stays after close() bug

[Jari Ruusu]

On 2/19/15, Johan Hovold <johan@kernel.org> wrote:
> What happens when you reopen the port? Is the break state cleared then?

Stuck "break" signal is not cleared on re-open.

To clear it, you need to poke it with ioctl(fd, TIOCCBRK, 0)
or disconnect the device.

-- 
Jari Ruusu  4096R/8132F189 12D6 4C3A DCDA 0AA4 27BD  ACDF F073 3C80 8132 F189

Re: usb serial: pl2303 driver TxD "break" stays after close() bug

[Johan Hovold]

On Thu, Feb 19, 2015 at 03:38:39PM +0200, Jari Ruusu wrote:
> On 2/19/15, Johan Hovold <johan@kernel.org> wrote:
> > What happens when you reopen the port? Is the break state cleared then?
> 
> Stuck "break" signal is not cleared on re-open.
> 
> To clear it, you need to poke it with ioctl(fd, TIOCCBRK, 0)
> or disconnect the device.

That's definitely a bug.

Care to test the patch below?

Thanks,
Johan


>From ee3f2d35adc59822d72f3908078da7f361c26577 Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Fri, 20 Feb 2015 10:05:51 +0700
Subject: [PATCH] USB: pl2303: disable break on shutdown

Currently an enabled break state is not disabled on final close nor on
re-open and has to be disabled manually.

Fix this by disabling break on port shutdown.

Reported-by: Jari Ruusu <jariruusu@users.sourceforge.net>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/pl2303.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index 0f872e6b2c87..f6fe4737185d 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -132,6 +132,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_OVERRUN_ERROR		0x40
 #define UART_CTS			0x80
 
+static void pl2303_set_break(struct usb_serial_port *port, bool enable);
 
 enum pl2303_type {
 	TYPE_01,	/* Type 0 and 1 (difference unknown) */
@@ -613,6 +614,7 @@ static void pl2303_dtr_rts(struct usb_serial_port *port, int on)
 
 static void pl2303_close(struct usb_serial_port *port)
 {
+	pl2303_set_break(port, false);
 	usb_serial_generic_close(port);
 	usb_kill_urb(port->interrupt_in_urb);
 }
@@ -741,17 +743,16 @@ static int pl2303_ioctl(struct tty_struct *tty,
 	return -ENOIOCTLCMD;
 }
 
-static void pl2303_break_ctl(struct tty_struct *tty, int break_state)
+static void pl2303_set_break(struct usb_serial_port *port, bool enable)
 {
-	struct usb_serial_port *port = tty->driver_data;
 	struct usb_serial *serial = port->serial;
 	u16 state;
 	int result;
 
-	if (break_state == 0)
-		state = BREAK_OFF;
-	else
+	if (enable)
 		state = BREAK_ON;
+	else
+		state = BREAK_OFF;
 
 	dev_dbg(&port->dev, "%s - turning break %s\n", __func__,
 			state == BREAK_OFF ? "off" : "on");
@@ -763,6 +764,13 @@ static void pl2303_break_ctl(struct tty_struct *tty, int break_state)
 		dev_err(&port->dev, "error sending break = %d\n", result);
 }
 
+static void pl2303_break_ctl(struct tty_struct *tty, int state)
+{
+	struct usb_serial_port *port = tty->driver_data;
+
+	pl2303_set_break(port, state);
+}
+
 static void pl2303_update_line_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      unsigned int actual_length)
-- 
2.0.5

Re: usb serial: pl2303 driver TxD "break" stays after close() bug

[Jari Ruusu]

On 2/20/15, Johan Hovold <johan@kernel.org> wrote:
> On Thu, Feb 19, 2015 at 03:38:39PM +0200, Jari Ruusu wrote:
>> To clear it, you need to poke it with ioctl(fd, TIOCCBRK, 0)
>> or disconnect the device.
>
> That's definitely a bug.
>
> Care to test the patch below?

Your patch fixes the bug. Thanks.

-- 
Jari Ruusu  4096R/8132F189 12D6 4C3A DCDA 0AA4 27BD  ACDF F073 3C80 8132 F189

Re: usb serial: pl2303 driver TxD "break" stays after close() bug

[Johan Hovold]

On Fri, Feb 20, 2015 at 03:11:12PM +0200, Jari Ruusu wrote:
> On 2/20/15, Johan Hovold <johan@kernel.org> wrote:
> > On Thu, Feb 19, 2015 at 03:38:39PM +0200, Jari Ruusu wrote:
> >> To clear it, you need to poke it with ioctl(fd, TIOCCBRK, 0)
> >> or disconnect the device.
> >
> > That's definitely a bug.
> >
> > Care to test the patch below?
> 
> Your patch fixes the bug. Thanks.

Thanks for testing. I'll queue this up for 3.20 next week.

Johan

[PATCH] USB: pl2303: disable break on shutdown

[Johan Hovold]

Currently an enabled break state is not disabled on final close nor on
re-open and has to be disabled manually.

Fix this by disabling break on port shutdown.

Reported-by: Jari Ruusu <jariruusu@users.sourceforge.net>
Tested-by: Jari Ruusu <jariruusu@users.sourceforge.net>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/pl2303.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index 0f872e6b2c87..829604d11f3f 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -132,6 +132,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_OVERRUN_ERROR		0x40
 #define UART_CTS			0x80
 
+static void pl2303_set_break(struct usb_serial_port *port, bool enable);
 
 enum pl2303_type {
 	TYPE_01,	/* Type 0 and 1 (difference unknown) */
@@ -615,6 +616,7 @@ static void pl2303_close(struct usb_serial_port *port)
 {
 	usb_serial_generic_close(port);
 	usb_kill_urb(port->interrupt_in_urb);
+	pl2303_set_break(port, false);
 }
 
 static int pl2303_open(struct tty_struct *tty, struct usb_serial_port *port)
@@ -741,17 +743,16 @@ static int pl2303_ioctl(struct tty_struct *tty,
 	return -ENOIOCTLCMD;
 }
 
-static void pl2303_break_ctl(struct tty_struct *tty, int break_state)
+static void pl2303_set_break(struct usb_serial_port *port, bool enable)
 {
-	struct usb_serial_port *port = tty->driver_data;
 	struct usb_serial *serial = port->serial;
 	u16 state;
 	int result;
 
-	if (break_state == 0)
-		state = BREAK_OFF;
-	else
+	if (enable)
 		state = BREAK_ON;
+	else
+		state = BREAK_OFF;
 
 	dev_dbg(&port->dev, "%s - turning break %s\n", __func__,
 			state == BREAK_OFF ? "off" : "on");
@@ -763,6 +764,13 @@ static void pl2303_break_ctl(struct tty_struct *tty, int break_state)
 		dev_err(&port->dev, "error sending break = %d\n", result);
 }
 
+static void pl2303_break_ctl(struct tty_struct *tty, int state)
+{
+	struct usb_serial_port *port = tty->driver_data;
+
+	pl2303_set_break(port, state);
+}
+
 static void pl2303_update_line_status(struct usb_serial_port *port,
 				      unsigned char *data,
 				      unsigned int actual_length)
-- 
2.0.5