* [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