| Message ID | 20190713210853.GA25753@lenoch (mailing list archive) |
|---|---|
| Headers | show |
| Series | usb: gadget: u_serial: Fix and cleanup | expand |
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
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
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
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
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(-)