* [PATCH 0/3] usb: gadget: u_serial: Fix and cleanup
@ 2019-07-03 16:33 Ladislav Michl
2019-07-03 16:34 ` [PATCH 1/3] usb: gadget: u_serial: Fix console_req complete event race Ladislav Michl
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Ladislav Michl @ 2019-07-03 16:33 UTC (permalink / raw)
To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman
Following patchset makes console work (patch 1) for at AT91SAM9G20 board
connected to xhci_hcd and does some cleanup.
Tested with "console=ttyS0,115200n8 console=ttyGS0,115200n8" on kernel
command line and following inittab:
console::respawn:/sbin/getty -L 115200 ttyS0 vt100
console::respawn:/sbin/getty -L 115200 ttyGS0 vt100
There are issues remaining:
- first usb disconnect works while each next 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 ]---
- init (both busybox' and systemd) waits for usb host to be plugged in,
otherwise boot is stuck and continues after host is connected.
Will investigate those two later, however comments and suggestions
to the following patches are appreciated.
Ladislav Michl (3):
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: Use bool for req_busy
drivers/usb/gadget/function/u_serial.c | 88 +++++++++-----------------
1 file changed, 30 insertions(+), 58 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] usb: gadget: u_serial: Fix console_req complete event race
2019-07-03 16:33 [PATCH 0/3] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
@ 2019-07-03 16:34 ` Ladislav Michl
2019-07-03 16:34 ` [PATCH 2/3] usb: gadget: u_serial: Remove console specific alloc/free req functions Ladislav Michl
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Ladislav Michl @ 2019-07-03 16:34 UTC (permalink / raw)
To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman
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>
---
drivers/usb/gadget/function/u_serial.c | 38 +++++++++++---------------
1 file changed, 16 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 65f634ec7fc2..f8abb9c68e62 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -984,47 +984,41 @@ 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;
-
- spin_lock_irq(&info->con_lock);
- count = kfifo_len(&info->con_buf);
- size = ep->maxpacket;
-
- if (count > 0 && !info->req_busy) {
+ len = kfifo_len(&info->con_buf);
+ if (len > 0 && !info->req_busy) {
set_current_state(TASK_RUNNING);
- if (count < size)
- size = count;
+ size = ep->maxpacket;
+ if (len < size)
+ size = len;
- 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();
+ spin_lock_irq(&info->con_lock);
}
} while (1);
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] usb: gadget: u_serial: Remove console specific alloc/free req functions
2019-07-03 16:33 [PATCH 0/3] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
2019-07-03 16:34 ` [PATCH 1/3] usb: gadget: u_serial: Fix console_req complete event race Ladislav Michl
@ 2019-07-03 16:34 ` Ladislav Michl
2019-07-03 16:35 ` [PATCH 3/3] usb: gadget: u_serial: Use bool for req_busy Ladislav Michl
2019-07-04 16:32 ` [PATCH 0/3] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
3 siblings, 0 replies; 6+ messages in thread
From: Ladislav Michl @ 2019-07-03 16:34 UTC (permalink / raw)
To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman
Driver already contains request allocation and deallocation
functions, so use them also for console_req.
While console_req is always null when calling gs_console_connect
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>
---
drivers/usb/gadget/function/u_serial.c | 43 +++++++-------------------
1 file changed, 11 insertions(+), 32 deletions(-)
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index f8abb9c68e62..04b4338b4ae1 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;
@@ -954,15 +930,15 @@ static int gs_console_connect(int port_num)
port = ports[port_num].port;
ep = port->port_usb->in;
+ spin_lock(&info->con_lock);
+ info->console_req = gs_alloc_req(ep, ep->maxpacket, GFP_ATOMIC);
if (!info->console_req) {
- info->console_req = gs_request_new(ep);
- if (!info->console_req)
- return -ENOMEM;
- info->console_req->complete = gs_complete_out;
+ spin_unlock(&info->con_lock);
+ return -ENOMEM;
}
+ info->console_req->complete = gs_complete_out;
info->port = port;
- spin_lock(&info->con_lock);
info->req_busy = 0;
spin_unlock(&info->con_lock);
pr_vdebug("port[%d] console connect!\n", port_num);
@@ -972,10 +948,13 @@ 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_request_free(req, ep);
- info->console_req = NULL;
+ spin_lock(&info->con_lock);
+ if (info->console_req) {
+ gs_free_req(ep, info->console_req);
+ info->console_req = NULL;
+ }
+ spin_unlock(&info->con_lock);
}
static int gs_console_thread(void *data)
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] usb: gadget: u_serial: Use bool for req_busy
2019-07-03 16:33 [PATCH 0/3] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
2019-07-03 16:34 ` [PATCH 1/3] usb: gadget: u_serial: Fix console_req complete event race Ladislav Michl
2019-07-03 16:34 ` [PATCH 2/3] usb: gadget: u_serial: Remove console specific alloc/free req functions Ladislav Michl
@ 2019-07-03 16:35 ` Ladislav Michl
2019-07-04 16:32 ` [PATCH 0/3] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
3 siblings, 0 replies; 6+ messages in thread
From: Ladislav Michl @ 2019-07-03 16:35 UTC (permalink / raw)
To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman
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>
---
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 04b4338b4ae1..130613751723 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -88,7 +88,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;
};
@@ -904,7 +904,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);
@@ -939,7 +939,6 @@ static int gs_console_connect(int port_num)
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;
@@ -982,13 +981,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);
@@ -1011,7 +1010,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;
spin_lock_init(&info->con_lock);
status = kfifo_alloc(&info->con_buf, GS_CONSOLE_BUF_SIZE, GFP_KERNEL);
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] usb: gadget: u_serial: Fix and cleanup
2019-07-03 16:33 [PATCH 0/3] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
` (2 preceding siblings ...)
2019-07-03 16:35 ` [PATCH 3/3] usb: gadget: u_serial: Use bool for req_busy Ladislav Michl
@ 2019-07-04 16:32 ` Ladislav Michl
2019-07-04 22:10 ` Ladislav Michl
3 siblings, 1 reply; 6+ messages in thread
From: Ladislav Michl @ 2019-07-04 16:32 UTC (permalink / raw)
To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman
On Wed, Jul 03, 2019 at 06:33:55PM +0200, Ladislav Michl wrote:
> Following patchset makes console work (patch 1) for at AT91SAM9G20 board
> connected to xhci_hcd and does some cleanup.
> Tested with "console=ttyS0,115200n8 console=ttyGS0,115200n8" on kernel
> command line and following inittab:
> console::respawn:/sbin/getty -L 115200 ttyS0 vt100
> console::respawn:/sbin/getty -L 115200 ttyGS0 vt100
>
> There are issues remaining:
>
> - first usb disconnect works while each next 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 ]---
This one is explained in drivers/tty/serial/ip22zilog.c line 751 (see also
__tty_hangup) And it also explains why patch 2 in this serie is actually wrong.
Will send v2.
> - init (both busybox' and systemd) waits for usb host to be plugged in,
> otherwise boot is stuck and continues after host is connected.
>
> Will investigate those two later, however comments and suggestions
> to the following patches are appreciated.
>
> Ladislav Michl (3):
> 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: Use bool for req_busy
>
> drivers/usb/gadget/function/u_serial.c | 88 +++++++++-----------------
> 1 file changed, 30 insertions(+), 58 deletions(-)
>
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] usb: gadget: u_serial: Fix and cleanup
2019-07-04 16:32 ` [PATCH 0/3] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
@ 2019-07-04 22:10 ` Ladislav Michl
0 siblings, 0 replies; 6+ messages in thread
From: Ladislav Michl @ 2019-07-04 22:10 UTC (permalink / raw)
To: linux-usb; +Cc: Felipe Balbi, Greg Kroah-Hartman
On Thu, Jul 04, 2019 at 06:32:56PM +0200, Ladislav Michl wrote:
> On Wed, Jul 03, 2019 at 06:33:55PM +0200, Ladislav Michl wrote:
> > Following patchset makes console work (patch 1) for at AT91SAM9G20 board
> > connected to xhci_hcd and does some cleanup.
> > Tested with "console=ttyS0,115200n8 console=ttyGS0,115200n8" on kernel
> > command line and following inittab:
> > console::respawn:/sbin/getty -L 115200 ttyS0 vt100
> > console::respawn:/sbin/getty -L 115200 ttyGS0 vt100
> >
> > There are issues remaining:
> >
> > - first usb disconnect works while each next 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 ]---
>
> This one is explained in drivers/tty/serial/ip22zilog.c line 751 (see also
> __tty_hangup) And it also explains why patch 2 in this serie is actually wrong.
> Will send v2.
As an naive approach I tried following:
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 130613751723..4551005a9d23 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -701,6 +704,9 @@ static void gs_close(struct tty_struct *tty, struct file *file)
spin_lock_irq(&port->port_lock);
+ if (test_bit(TTY_HUPPING, &tty->flags))
+ goto exit;
+
if (port->port.count != 1) {
if (port->port.count == 0)
WARN_ON(1);
However this way port->port.count never drops back to zero. Any ideas?
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-04 22:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-03 16:33 [PATCH 0/3] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
2019-07-03 16:34 ` [PATCH 1/3] usb: gadget: u_serial: Fix console_req complete event race Ladislav Michl
2019-07-03 16:34 ` [PATCH 2/3] usb: gadget: u_serial: Remove console specific alloc/free req functions Ladislav Michl
2019-07-03 16:35 ` [PATCH 3/3] usb: gadget: u_serial: Use bool for req_busy Ladislav Michl
2019-07-04 16:32 ` [PATCH 0/3] usb: gadget: u_serial: Fix and cleanup Ladislav Michl
2019-07-04 22:10 ` 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).