linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup
@ 2019-07-13 21:08 Ladislav Michl
  2019-07-13 21:10 ` [PATCH v2 1/5] usb: gadget: u_serial: Fix console_req complete event race Ladislav Michl
                   ` (5 more replies)
  0 siblings, 6 replies; 10+ messages in thread
From: Ladislav Michl @ 2019-07-13 21:08 UTC (permalink / raw)
  To: linux-usb, linux-serial
  Cc: Felipe Balbi, Greg Kroah-Hartman, Michał Mirosław

Following patchset makes console work (patch 1 and 4) for an AT91SAM9G20
board connected to xhci_hcd and does some cleanup.
Tested with "console=ttyS0,115200n8 console=ttyGS0" on kernel command line
and following inittab:
console::respawn:/sbin/getty -L 115200 ttyS0 vt100
console::respawn:/sbin/getty -L 115200 ttyGS0 vt100

However there is an issue remaining:
Disconnect triggers WARN_ON in gs_close:
------------[ cut here ]------------
WARNING: CPU: 0 PID: 501 at drivers/usb/gadget/function/u_serial.c:706 gs_close+0x3c/0x1e4
Modules linked in:
CPU: 0 PID: 501 Comm: getty Not tainted 5.2.0-rc7 #44
Hardware name: Atmel AT91SAM9
[<c0107514>] (unwind_backtrace) from [<c01051c0>] (show_stack+0x10/0x18)
[<c01051c0>] (show_stack) from [<c05465a8>] (dump_stack+0x18/0x24)
[<c05465a8>] (dump_stack) from [<c010fa80>] (__warn+0xcc/0xe4)
[<c010fa80>] (__warn) from [<c010fad0>] (warn_slowpath_null+0x38/0x48)
[<c010fad0>] (warn_slowpath_null) from [<c03b6648>] (gs_close+0x3c/0x1e4)
[<c03b6648>] (gs_close) from [<c03036b0>] (tty_release+0x1d4/0x460)
[<c03036b0>] (tty_release) from [<c01ce464>] (__fput+0xe4/0x1b0)
[<c01ce464>] (__fput) from [<c0124e1c>] (task_work_run+0x8c/0xa8)
[<c0124e1c>] (task_work_run) from [<c011155c>] (do_exit+0x354/0x814)
[<c011155c>] (do_exit) from [<c0111a9c>] (do_group_exit+0x54/0xb8)
[<c0111a9c>] (do_group_exit) from [<c011a190>] (get_signal+0x18c/0x658)
[<c011a190>] (get_signal) from [<c0104bbc>] (do_work_pending+0xe0/0x44c)
[<c0104bbc>] (do_work_pending) from [<c0101068>] (slow_work_pending+0xc/0x20)
Exception stack(0xc3797fb0 to 0xc3797ff8)
7fa0:                                     00000000 beb87d0c 00000001 00000000
7fc0: 0009a150 00000000 00099c04 00000003 0009a198 0007e049 00099bd4 0009a1e4
7fe0: b6e3f000 beb87cd8 00018210 b6dbcc40 60000010 00000000
---[ end trace 70af570fde0de49b ]---

Explanation lives in drivers/tty/serial/ip22zilog.c at line 751 (see also
__tty_hangup), but naive approach adding
	if (test_bit(TTY_HUPPING, &tty->flags))
		return 0;
at the beginning of gs_close does not work as this way port->port.count
never drops back to zero. Ideas welcome.

Btw, it would seem nicer to have single output kfifo for both kernel
console and tty, but that involves greater changes and I'd like to have
driver working in stable as well. 

Changes:
- v2: Fixed locking, patches splitted, starving write fixed.

Ladislav Michl (5):
  usb: gadget: u_serial: Fix console_req complete event race
  usb: gadget: u_serial: Remove console specific alloc/free req
    functions
  usb: gadget: u_serial: Fix console_req access race
  usb: gadget: u_serial: Fix starving write
  usb: gadget: u_serial: Use bool for req_busy

 drivers/usb/gadget/function/u_serial.c | 120 +++++++++++--------------
 1 file changed, 51 insertions(+), 69 deletions(-)

-- 
2.22.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/5] usb: gadget: u_serial: Fix console_req complete event race
  2019-07-13 21:08 [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
@ 2019-07-13 21:10 ` Ladislav Michl
  2019-07-13 21:10 ` [PATCH v2 2/5] usb: gadget: u_serial: Remove console specific alloc/free req functions Ladislav Michl
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Ladislav Michl @ 2019-07-13 21:10 UTC (permalink / raw)
  To: linux-usb, linux-serial
  Cc: Felipe Balbi, Greg Kroah-Hartman, Michał Mirosław

gs_complete_out might be called before con_lock following usb_ep_queue
is locked, which prevents any future output on the console. Fix that by
resetting req_busy only if usb_ep_queue fails. While there also put
variable access we are racing with connection/disconnection events
under con_lock as well.

Fixes: a5beaaf39455 ("usb: gadget: Add the console support for usb-to-serial port")
Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 Changes:
 - v2: None

 drivers/usb/gadget/function/u_serial.c | 41 +++++++++++---------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 65f634ec7fc2..876af4085cea 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -984,47 +984,40 @@ static int gs_console_thread(void *data)
 	struct gs_port *port;
 	struct usb_request *req;
 	struct usb_ep *ep;
-	int xfer, ret, count, size;
+	int len, size, status;
 
+	spin_lock_irq(&info->con_lock);
 	do {
 		port = info->port;
-		set_current_state(TASK_INTERRUPTIBLE);
-		if (!port || !port->port_usb
-		    || !port->port_usb->in || !info->console_req)
+		if (!port || !port->port_usb || !info->console_req)
 			goto sched;
 
 		req = info->console_req;
 		ep = port->port_usb->in;
+		len = kfifo_len(&info->con_buf);
+		if (len > 0 && !info->req_busy) {
+			size = ep->maxpacket;
+			if (len < size)
+				size = len;
 
-		spin_lock_irq(&info->con_lock);
-		count = kfifo_len(&info->con_buf);
-		size = ep->maxpacket;
-
-		if (count > 0 && !info->req_busy) {
-			set_current_state(TASK_RUNNING);
-			if (count < size)
-				size = count;
-
-			xfer = kfifo_out(&info->con_buf, req->buf, size);
-			req->length = xfer;
-
-			spin_unlock(&info->con_lock);
-			ret = usb_ep_queue(ep, req, GFP_ATOMIC);
-			spin_lock(&info->con_lock);
-			if (ret < 0)
-				info->req_busy = 0;
-			else
-				info->req_busy = 1;
+			req->length = kfifo_out(&info->con_buf, req->buf, size);
+			info->req_busy = 1;
 
 			spin_unlock_irq(&info->con_lock);
+			status = usb_ep_queue(ep, req, GFP_ATOMIC);
+			spin_lock_irq(&info->con_lock);
+			if (status < 0)
+				info->req_busy = 0;
 		} else {
-			spin_unlock_irq(&info->con_lock);
 sched:
+			spin_unlock_irq(&info->con_lock);
 			if (kthread_should_stop()) {
 				set_current_state(TASK_RUNNING);
 				break;
 			}
 			schedule();
+			set_current_state(TASK_INTERRUPTIBLE);
+			spin_lock_irq(&info->con_lock);
 		}
 	} while (1);
 
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/5] usb: gadget: u_serial: Remove console specific alloc/free req functions
  2019-07-13 21:08 [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
  2019-07-13 21:10 ` [PATCH v2 1/5] usb: gadget: u_serial: Fix console_req complete event race Ladislav Michl
@ 2019-07-13 21:10 ` Ladislav Michl
  2019-07-13 21:11 ` [PATCH v2 3/5] usb: gadget: u_serial: Fix console_req access race Ladislav Michl
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Ladislav Michl @ 2019-07-13 21:10 UTC (permalink / raw)
  To: linux-usb, linux-serial
  Cc: Felipe Balbi, Greg Kroah-Hartman, Michał Mirosław

Driver already contains request allocation and deallocation
functions, so use them also for console_req.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 Changes:
 - v2: Move locking changes into separate patch

 drivers/usb/gadget/function/u_serial.c | 28 ++------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 876af4085cea..cb88a640179e 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -892,30 +892,6 @@ static struct tty_driver *gs_tty_driver;
 static struct gscons_info gscons_info;
 static struct console gserial_cons;
 
-static struct usb_request *gs_request_new(struct usb_ep *ep)
-{
-	struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
-	if (!req)
-		return NULL;
-
-	req->buf = kmalloc(ep->maxpacket, GFP_ATOMIC);
-	if (!req->buf) {
-		usb_ep_free_request(ep, req);
-		return NULL;
-	}
-
-	return req;
-}
-
-static void gs_request_free(struct usb_request *req, struct usb_ep *ep)
-{
-	if (!req)
-		return;
-
-	kfree(req->buf);
-	usb_ep_free_request(ep, req);
-}
-
 static void gs_complete_out(struct usb_ep *ep, struct usb_request *req)
 {
 	struct gscons_info *info = &gscons_info;
@@ -955,7 +931,7 @@ static int gs_console_connect(int port_num)
 	port = ports[port_num].port;
 	ep = port->port_usb->in;
 	if (!info->console_req) {
-		info->console_req = gs_request_new(ep);
+		info->console_req = gs_alloc_req(ep, ep->maxpacket, GFP_ATOMIC);
 		if (!info->console_req)
 			return -ENOMEM;
 		info->console_req->complete = gs_complete_out;
@@ -974,7 +950,7 @@ static void gs_console_disconnect(struct usb_ep *ep)
 	struct gscons_info *info = &gscons_info;
 	struct usb_request *req = info->console_req;
 
-	gs_request_free(req, ep);
+	gs_free_req(ep, req);
 	info->console_req = NULL;
 }
 
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/5] usb: gadget: u_serial: Fix console_req access race
  2019-07-13 21:08 [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
  2019-07-13 21:10 ` [PATCH v2 1/5] usb: gadget: u_serial: Fix console_req complete event race Ladislav Michl
  2019-07-13 21:10 ` [PATCH v2 2/5] usb: gadget: u_serial: Remove console specific alloc/free req functions Ladislav Michl
@ 2019-07-13 21:11 ` Ladislav Michl
  2019-07-13 21:11 ` [PATCH v2 4/5] usb: gadget: u_serial: Fix starving write Ladislav Michl
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Ladislav Michl @ 2019-07-13 21:11 UTC (permalink / raw)
  To: linux-usb, linux-serial
  Cc: Felipe Balbi, Greg Kroah-Hartman, Michał Mirosław

console_req is always null when calling gs_console_connect, so
remove check and put access under con_lock as we are racing with
gs_console_thread.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 Changes:
 - v2: New patch

 drivers/usb/gadget/function/u_serial.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index cb88a640179e..558a6929ce68 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -930,15 +930,12 @@ static int gs_console_connect(int port_num)
 
 	port = ports[port_num].port;
 	ep = port->port_usb->in;
-	if (!info->console_req) {
-		info->console_req = gs_alloc_req(ep, ep->maxpacket, GFP_ATOMIC);
-		if (!info->console_req)
-			return -ENOMEM;
-		info->console_req->complete = gs_complete_out;
-	}
-
-	info->port = port;
 	spin_lock(&info->con_lock);
+	info->console_req = gs_alloc_req(ep, ep->maxpacket, GFP_ATOMIC);
+	if (!info->console_req)
+		return -ENOMEM;
+	info->console_req->complete = gs_complete_out;
+	info->port = port;
 	info->req_busy = 0;
 	spin_unlock(&info->con_lock);
 	pr_vdebug("port[%d] console connect!\n", port_num);
@@ -948,10 +945,11 @@ static int gs_console_connect(int port_num)
 static void gs_console_disconnect(struct usb_ep *ep)
 {
 	struct gscons_info *info = &gscons_info;
-	struct usb_request *req = info->console_req;
 
-	gs_free_req(ep, req);
+	spin_lock(&info->con_lock);
+	gs_free_req(ep, info->console_req);
 	info->console_req = NULL;
+	spin_unlock(&info->con_lock);
 }
 
 static int gs_console_thread(void *data)
@@ -1008,7 +1006,6 @@ static int gs_console_setup(struct console *co, char *options)
 	info->port = NULL;
 	info->console_req = NULL;
 	info->req_busy = 0;
-	spin_lock_init(&info->con_lock);
 
 	status = kfifo_alloc(&info->con_buf, GS_CONSOLE_BUF_SIZE, GFP_KERNEL);
 	if (status) {
@@ -1064,6 +1061,9 @@ static struct console gserial_cons = {
 
 static void gserial_console_init(void)
 {
+	struct gscons_info *info = &gscons_info;
+
+	spin_lock_init(&info->con_lock);
 	register_console(&gserial_cons);
 }
 
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 4/5] usb: gadget: u_serial: Fix starving write
  2019-07-13 21:08 [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
                   ` (2 preceding siblings ...)
  2019-07-13 21:11 ` [PATCH v2 3/5] usb: gadget: u_serial: Fix console_req access race Ladislav Michl
@ 2019-07-13 21:11 ` Ladislav Michl
  2019-07-13 21:12 ` [PATCH v2 5/5] usb: gadget: u_serial: Use bool for req_busy Ladislav Michl
  2019-07-14 10:04 ` [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup Michał Mirosław
  5 siblings, 0 replies; 10+ messages in thread
From: Ladislav Michl @ 2019-07-13 21:11 UTC (permalink / raw)
  To: linux-usb, linux-serial
  Cc: Felipe Balbi, Greg Kroah-Hartman, Michał Mirosław

Writing to ttyGS unconnected to host currently fills port_write_buf
which then causes gs_write_room to return 0 and boot hangs waiting
for connection.
Fix that by dropping previsous data if free space in port_write_buf
reaches treshold and host is unconnected.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 Changes:
 - v2: New patch

 drivers/usb/gadget/function/u_serial.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 558a6929ce68..2dd6e1211d4a 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -79,6 +79,7 @@
  */
 #define QUEUE_SIZE		16
 #define WRITE_BUF_SIZE		8192		/* TX only */
+#define WRITE_BUF_TRESHOLD	1024
 #define GS_CONSOLE_BUF_SIZE	8192
 
 /* console info */
@@ -562,7 +563,7 @@ static int gs_start_io(struct gs_port *port)
 
 	/* unblock any pending writes into our circular buffer */
 	if (started) {
-		tty_wakeup(port->port.tty);
+		gs_start_tx(port);
 	} else {
 		gs_free_requests(ep, head, &port->read_allocated);
 		gs_free_requests(port->port_usb->in, &port->write_pool,
@@ -758,6 +759,7 @@ static int gs_write(struct tty_struct *tty, const unsigned char *buf, int count)
 {
 	struct gs_port	*port = tty->driver_data;
 	unsigned long	flags;
+	int		avail, cnt;
 
 	pr_vdebug("gs_write: ttyGS%d (%p) writing %d bytes\n",
 			port->port_num, tty, count);
@@ -766,8 +768,16 @@ static int gs_write(struct tty_struct *tty, const unsigned char *buf, int count)
 	if (count)
 		count = kfifo_in(&port->port_write_buf, buf, count);
 	/* treat count == 0 as flush_chars() */
-	if (port->port_usb)
+	if (port->port_usb) {
 		gs_start_tx(port);
+	} else {
+		avail = kfifo_avail(&port->port_write_buf);
+		if (avail < WRITE_BUF_TRESHOLD) {
+			cnt = WRITE_BUF_TRESHOLD - avail;
+			while (cnt--)
+				kfifo_skip(&port->port_write_buf);
+		}
+	}
 	spin_unlock_irqrestore(&port->port_lock, flags);
 
 	return count;
@@ -784,6 +794,9 @@ static int gs_put_char(struct tty_struct *tty, unsigned char ch)
 
 	spin_lock_irqsave(&port->port_lock, flags);
 	status = kfifo_put(&port->port_write_buf, ch);
+	if (!port->port_usb &&
+	     kfifo_avail(&port->port_write_buf) < WRITE_BUF_TRESHOLD)
+		kfifo_skip(&port->port_write_buf);
 	spin_unlock_irqrestore(&port->port_lock, flags);
 
 	return status;
@@ -799,6 +812,8 @@ static void gs_flush_chars(struct tty_struct *tty)
 	spin_lock_irqsave(&port->port_lock, flags);
 	if (port->port_usb)
 		gs_start_tx(port);
+	else
+		kfifo_reset_out(&port->port_write_buf);
 	spin_unlock_irqrestore(&port->port_lock, flags);
 }
 
@@ -806,11 +821,10 @@ static int gs_write_room(struct tty_struct *tty)
 {
 	struct gs_port	*port = tty->driver_data;
 	unsigned long	flags;
-	int		room = 0;
+	int		room;
 
 	spin_lock_irqsave(&port->port_lock, flags);
-	if (port->port_usb)
-		room = kfifo_avail(&port->port_write_buf);
+	room = kfifo_avail(&port->port_write_buf);
 	spin_unlock_irqrestore(&port->port_lock, flags);
 
 	pr_vdebug("gs_write_room: (%d,%p) room=%d\n",
@@ -823,7 +837,7 @@ static int gs_chars_in_buffer(struct tty_struct *tty)
 {
 	struct gs_port	*port = tty->driver_data;
 	unsigned long	flags;
-	int		chars = 0;
+	int		chars;
 
 	spin_lock_irqsave(&port->port_lock, flags);
 	chars = kfifo_len(&port->port_write_buf);
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 5/5] usb: gadget: u_serial: Use bool for req_busy
  2019-07-13 21:08 [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
                   ` (3 preceding siblings ...)
  2019-07-13 21:11 ` [PATCH v2 4/5] usb: gadget: u_serial: Fix starving write Ladislav Michl
@ 2019-07-13 21:12 ` Ladislav Michl
  2019-07-14 10:04 ` [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup Michał Mirosław
  5 siblings, 0 replies; 10+ messages in thread
From: Ladislav Michl @ 2019-07-13 21:12 UTC (permalink / raw)
  To: linux-usb, linux-serial
  Cc: Felipe Balbi, Greg Kroah-Hartman, Michał Mirosław

Let's make code more consistent by using bool for req_busy
as it is done for similar variables in struct gs_port.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 Changes:
 - v2: None

 drivers/usb/gadget/function/u_serial.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 2dd6e1211d4a..1b1359a168e7 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -89,7 +89,7 @@ struct gscons_info {
 	struct kfifo		con_buf;
 	/* protect the buf and busy flag */
 	spinlock_t		con_lock;
-	int			req_busy;
+	bool			req_busy;
 	struct usb_request	*console_req;
 };
 
@@ -918,7 +918,7 @@ static void gs_complete_out(struct usb_ep *ep, struct usb_request *req)
 	case 0:
 		/* normal completion */
 		spin_lock(&info->con_lock);
-		info->req_busy = 0;
+		info->req_busy = false;
 		spin_unlock(&info->con_lock);
 
 		wake_up_process(info->console_thread);
@@ -950,7 +950,6 @@ static int gs_console_connect(int port_num)
 		return -ENOMEM;
 	info->console_req->complete = gs_complete_out;
 	info->port = port;
-	info->req_busy = 0;
 	spin_unlock(&info->con_lock);
 	pr_vdebug("port[%d] console connect!\n", port_num);
 	return 0;
@@ -989,13 +988,13 @@ static int gs_console_thread(void *data)
 				size = len;
 
 			req->length = kfifo_out(&info->con_buf, req->buf, size);
-			info->req_busy = 1;
+			info->req_busy = true;
 
 			spin_unlock_irq(&info->con_lock);
 			status = usb_ep_queue(ep, req, GFP_ATOMIC);
 			spin_lock_irq(&info->con_lock);
 			if (status < 0)
-				info->req_busy = 0;
+				info->req_busy = false;
 		} else {
 sched:
 			spin_unlock_irq(&info->con_lock);
@@ -1019,7 +1018,7 @@ static int gs_console_setup(struct console *co, char *options)
 
 	info->port = NULL;
 	info->console_req = NULL;
-	info->req_busy = 0;
+	info->req_busy = false;
 
 	status = kfifo_alloc(&info->con_buf, GS_CONSOLE_BUF_SIZE, GFP_KERNEL);
 	if (status) {
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup
  2019-07-13 21:08 [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
                   ` (4 preceding siblings ...)
  2019-07-13 21:12 ` [PATCH v2 5/5] usb: gadget: u_serial: Use bool for req_busy Ladislav Michl
@ 2019-07-14 10:04 ` Michał Mirosław
  2019-07-14 11:37   ` Michał Mirosław
  2019-07-15 10:40   ` Ladislav Michl
  5 siblings, 2 replies; 10+ messages in thread
From: Michał Mirosław @ 2019-07-14 10:04 UTC (permalink / raw)
  To: Ladislav Michl; +Cc: linux-usb, linux-serial, Felipe Balbi, Greg Kroah-Hartman

On Sat, Jul 13, 2019 at 11:08:53PM +0200, Ladislav Michl wrote:
> Following patchset makes console work (patch 1 and 4) for an AT91SAM9G20
> board connected to xhci_hcd and does some cleanup.
> Tested with "console=ttyS0,115200n8 console=ttyGS0" on kernel command line
> and following inittab:
> console::respawn:/sbin/getty -L 115200 ttyS0 vt100
> console::respawn:/sbin/getty -L 115200 ttyGS0 vt100
> 
> However there is an issue remaining:
> Disconnect triggers WARN_ON in gs_close:
[...]

Hi,

Can you try a patchset I sent some time ago [1] if it fixes your case?

Best Regards,
Michał Mirosław

[1] https://www.spinics.net/lists/linux-usb/msg177537.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup
  2019-07-14 10:04 ` [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup Michał Mirosław
@ 2019-07-14 11:37   ` Michał Mirosław
  2019-07-14 13:22     ` Ladislav Michl
  2019-07-15 10:40   ` Ladislav Michl
  1 sibling, 1 reply; 10+ messages in thread
From: Michał Mirosław @ 2019-07-14 11:37 UTC (permalink / raw)
  To: Ladislav Michl; +Cc: linux-usb, linux-serial, Felipe Balbi, Greg Kroah-Hartman

On Sun, Jul 14, 2019 at 12:04:26PM +0200, Michał Mirosław wrote:
> On Sat, Jul 13, 2019 at 11:08:53PM +0200, Ladislav Michl wrote:
> > Following patchset makes console work (patch 1 and 4) for an AT91SAM9G20
> > board connected to xhci_hcd and does some cleanup.
> > Tested with "console=ttyS0,115200n8 console=ttyGS0" on kernel command line
> > and following inittab:
> > console::respawn:/sbin/getty -L 115200 ttyS0 vt100
> > console::respawn:/sbin/getty -L 115200 ttyGS0 vt100
> > 
> > However there is an issue remaining:
> > Disconnect triggers WARN_ON in gs_close:
> [...]
> 
> Hi,
> 
> Can you try a patchset I sent some time ago [1] if it fixes your case?

You can pull it from branch usb-console on https://rere.qmqm.pl/git/linux
(rebased on current usb-next tree).

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup
  2019-07-14 11:37   ` Michał Mirosław
@ 2019-07-14 13:22     ` Ladislav Michl
  0 siblings, 0 replies; 10+ messages in thread
From: Ladislav Michl @ 2019-07-14 13:22 UTC (permalink / raw)
  To: Michał Mirosław
  Cc: linux-usb, linux-serial, Felipe Balbi, Greg Kroah-Hartman

On Sun, Jul 14, 2019 at 01:37:17PM +0200, Michał Mirosław wrote:
> On Sun, Jul 14, 2019 at 12:04:26PM +0200, Michał Mirosław wrote:
> > On Sat, Jul 13, 2019 at 11:08:53PM +0200, Ladislav Michl wrote:
> > > Following patchset makes console work (patch 1 and 4) for an AT91SAM9G20
> > > board connected to xhci_hcd and does some cleanup.
> > > Tested with "console=ttyS0,115200n8 console=ttyGS0" on kernel command line
> > > and following inittab:
> > > console::respawn:/sbin/getty -L 115200 ttyS0 vt100
> > > console::respawn:/sbin/getty -L 115200 ttyGS0 vt100
> > > 
> > > However there is an issue remaining:
> > > Disconnect triggers WARN_ON in gs_close:
> > [...]
> > 
> > Hi,
> > 
> > Can you try a patchset I sent some time ago [1] if it fixes your case?
> 
> You can pull it from branch usb-console on https://rere.qmqm.pl/git/linux
> (rebased on current usb-next tree).

Thank you! I has just in the middle of pulling patches from mail archive.

Your first two patches are definitely worth applying and I'll add my
Tested-by after more testing. 3/5 is unfortunately "I do not care" as
not having use case, 4/5 is something I do not see use case either, but
I'm fine with that, except for #ifdefs, but 5/5 should be solved the
other way around - just wait until everything is written out, of course
only if USB is connected. That's what serial consoles does and that's
what I expect from kernel console.

Your patch set does not attempt to solve tty write to unconnected USB
and WARN_ON on gs_close, so I'll wait for comments to those two issues
and redo fix on top of "reimplement console support" as getting rid of
the kthread is definitely superior to fixing it :)

Best regards,
	ladis

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup
  2019-07-14 10:04 ` [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup Michał Mirosław
  2019-07-14 11:37   ` Michał Mirosław
@ 2019-07-15 10:40   ` Ladislav Michl
  1 sibling, 0 replies; 10+ messages in thread
From: Ladislav Michl @ 2019-07-15 10:40 UTC (permalink / raw)
  To: Michał Mirosław
  Cc: linux-usb, linux-serial, Felipe Balbi, Greg Kroah-Hartman

Hi Michał,

On Sun, Jul 14, 2019 at 12:04:26PM +0200, Michał Mirosław wrote:
> On Sat, Jul 13, 2019 at 11:08:53PM +0200, Ladislav Michl wrote:
> > Following patchset makes console work (patch 1 and 4) for an AT91SAM9G20
> > board connected to xhci_hcd and does some cleanup.
> > Tested with "console=ttyS0,115200n8 console=ttyGS0" on kernel command line
> > and following inittab:
> > console::respawn:/sbin/getty -L 115200 ttyS0 vt100
> > console::respawn:/sbin/getty -L 115200 ttyGS0 vt100
> > 
> > However there is an issue remaining:
> > Disconnect triggers WARN_ON in gs_close:
> [...]
> 
> Hi,
> 
> Can you try a patchset I sent some time ago [1] if it fixes your case?

your "usb: gadget: u_serial: add missing port entry locking" and
"usb: gadget: u_serial: reimplement console support" fixes every single
problem I had with the driver (including that WARN_ON on gs_close),
except starving console write.
Fortunately my patch "usb: gadget: u_serial: Fix starving write"
applies on top of your first two patches.

So here's my
Tested-by: Ladislav Michl <ladis@linux-mips.org>
and also one nit for your second patch (there are people running cocci scripts,
so save them some time ;-)):
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -931,8 +931,8 @@ static void gs_console_complete_out(struct usb_ep *ep, struct usb_request *req)
 static void __gs_console_push(struct gs_console *cons)
 {
 	struct usb_request *req = cons->req;
-	struct usb_ep *ep = cons->console.data;
-	size_t size = 0;
+	struct usb_ep *ep;
+	size_t size;
 
 	if (!req)
 		return;	/* disconnected */
@@ -940,6 +940,7 @@ static void __gs_console_push(struct gs_console *cons)
 	if (req->length)
 		return;	/* busy */
 
+	ep = cons->console.data;
 	size = kfifo_out(&cons->buf, req->buf, ep->maxpacket);
 	if (!size)
 		return;

I'm dropping all my patches except the fourth one in favour of your solution
as it is clearly superior. Care to resend it possibly with above fix and
said fourth patch as a single serie?

Thank you,
	ladis

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-07-15 10:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-13 21:08 [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
2019-07-13 21:10 ` [PATCH v2 1/5] usb: gadget: u_serial: Fix console_req complete event race Ladislav Michl
2019-07-13 21:10 ` [PATCH v2 2/5] usb: gadget: u_serial: Remove console specific alloc/free req functions Ladislav Michl
2019-07-13 21:11 ` [PATCH v2 3/5] usb: gadget: u_serial: Fix console_req access race Ladislav Michl
2019-07-13 21:11 ` [PATCH v2 4/5] usb: gadget: u_serial: Fix starving write Ladislav Michl
2019-07-13 21:12 ` [PATCH v2 5/5] usb: gadget: u_serial: Use bool for req_busy Ladislav Michl
2019-07-14 10:04 ` [PATCH v2 0/5] usb: gadget: u_serial: Fix and cleanup Michał Mirosław
2019-07-14 11:37   ` Michał Mirosław
2019-07-14 13:22     ` Ladislav Michl
2019-07-15 10:40   ` Ladislav Michl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).