[PATCH] usb: gadget: serial: fix Tx stall after buffer overflow

[PATCH] usb: gadget: serial: fix Tx stall after buffer overflow

[Sergey Organov]

Symptom: application opens /dev/ttyGS0 and starts sending (writing) to
it while either USB cable is not connected, or nobody listens on the
other side of the cable. If driver circular buffer overflows before
connection is established, no data will be written to the USB layer
until/unless /dev/ttyGS0 is closed and re-opened again by the
application (the latter besides having no means of being notified about
the event of establishing of the connection.)

Fix: on open and/or connect, kick Tx to flush circular buffer data to
USB layer.

NOTE: current version of the driver leaks data from one connection to
another through its internal circular buffer. It might be a good idea
to clear the buffer on open/close/connect/disconnect, in which case
the problem this patch solves would have been fixed in a different
manner. However, not only that's a more dramatic change, but to do it
right TTY-layer buffers are to be considered as well.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---

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

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index f986e5c..d333cda 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -563,6 +563,8 @@ static int gs_start_io(struct gs_port *port)
 
        /* unblock any pending writes into our circular buffer */
        if (started) {
+               pr_debug("gs_start_tx: ttyGS%d\n", port->port_num);
+               gs_start_tx(port);
                tty_wakeup(port->port.tty);
        } else {
                gs_free_requests(ep, head, &port->read_allocated);
-- 
2.10.0.1.g57b01a3

Re: [PATCH] usb: gadget: serial: fix Tx stall after buffer overflow

[Michał Mirosław]

On Fri, Jan 17, 2020 at 08:29:33AM +0300, Sergey Organov wrote:
[...]
> NOTE: current version of the driver leaks data from one connection to
> another through its internal circular buffer. It might be a good idea
> to clear the buffer on open/close/connect/disconnect, in which case
> the problem this patch solves would have been fixed in a different
> manner. However, not only that's a more dramatic change, but to do it
> right TTY-layer buffers are to be considered as well.

This is normal for serial devices, as they don't have any means to
signal connection and will usually transmit anyway when not connected.
In case of a console on the USB gadget-emulated serial port, it might
actually be convenient that the data is kept until connection.

> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -563,6 +563,8 @@ static int gs_start_io(struct gs_port *port)
>  
>         /* unblock any pending writes into our circular buffer */
>         if (started) {
> +               pr_debug("gs_start_tx: ttyGS%d\n", port->port_num);
> +               gs_start_tx(port);
>                 tty_wakeup(port->port.tty);

The tty_wakeup() will be called from gs_start_tx(), so should be removed
from here.

The pr_debug() in other callers of gs_start_tx() say:
"caller: start ttyGS%d".

Best Regards,
Michał Mirosław

Re: [PATCH] usb: gadget: serial: fix Tx stall after buffer overflow

[Sergey Organov]

Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:

> On Fri, Jan 17, 2020 at 08:29:33AM +0300, Sergey Organov wrote:
> [...]
>> NOTE: current version of the driver leaks data from one connection to
>> another through its internal circular buffer. It might be a good idea
>> to clear the buffer on open/close/connect/disconnect, in which case
>> the problem this patch solves would have been fixed in a different
>> manner. However, not only that's a more dramatic change, but to do it
>> right TTY-layer buffers are to be considered as well.
>
> This is normal for serial devices, as they don't have any means to
> signal connection and will usually transmit anyway when not connected.
> In case of a console on the USB gadget-emulated serial port, it might
> actually be convenient that the data is kept until connection.

Yeah, just wanted to make sure I did select the right way of fixing the
issue.

>
>> --- a/drivers/usb/gadget/function/u_serial.c
>> +++ b/drivers/usb/gadget/function/u_serial.c
>> @@ -563,6 +563,8 @@ static int gs_start_io(struct gs_port *port)
>>  
>>         /* unblock any pending writes into our circular buffer */
>>         if (started) {
>> +               pr_debug("gs_start_tx: ttyGS%d\n", port->port_num);
>> +               gs_start_tx(port);
>>                 tty_wakeup(port->port.tty);
>
> The tty_wakeup() will be called from gs_start_tx(), so should be removed
> from here.

Not exactly. tty_wakeup() will be called from gs_start_tx() only when
there has been something actually transferred from the buffer. I didn't
want to change behavior when the buffer is empty, so I kept the explicit
tty_wakeup() call in place, intentionally. Please let me know if you
still think it should be removed.

> The pr_debug() in other callers of gs_start_tx() say:
> "caller: start ttyGS%d".

???

$ git co gregkh/tty-next && grep -r 'caller: start tty' .
HEAD is now at 7788f54... serial_core: Remove unused member in uart_port
$ 

-- Sergey

Re: [PATCH] usb: gadget: serial: fix Tx stall after buffer overflow

[Michał Mirosław]

On Mon, Jan 20, 2020 at 09:06:18AM +0300, Sergey Organov wrote:
> Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:
> 
> > On Fri, Jan 17, 2020 at 08:29:33AM +0300, Sergey Organov wrote:
> >> --- a/drivers/usb/gadget/function/u_serial.c
> >> +++ b/drivers/usb/gadget/function/u_serial.c
> >> @@ -563,6 +563,8 @@ static int gs_start_io(struct gs_port *port)
> >>  
> >>         /* unblock any pending writes into our circular buffer */
> >>         if (started) {
> >> +               pr_debug("gs_start_tx: ttyGS%d\n", port->port_num);
> >> +               gs_start_tx(port);
> >>                 tty_wakeup(port->port.tty);
> >
> > The tty_wakeup() will be called from gs_start_tx(), so should be removed
> > from here.
> 
> Not exactly. tty_wakeup() will be called from gs_start_tx() only when
> there has been something actually transferred from the buffer. I didn't
> want to change behavior when the buffer is empty, so I kept the explicit
> tty_wakeup() call in place, intentionally. Please let me know if you
> still think it should be removed.

Indeed it is as you describe. You might add an argument that initializes
do_tty_wake, but I'm not sure saving one tty_wakeup() on open is worth
the trouble.

> > The pr_debug() in other callers of gs_start_tx() say:
> > "caller: start ttyGS%d".
> 
> ???
> 
> $ git co gregkh/tty-next && grep -r 'caller: start tty' .
> HEAD is now at 7788f54... serial_core: Remove unused member in uart_port
> $ 

Replace 'caller' with a function calling gs_start_io().

Best Regards,
Michał Mirosław

Re: [PATCH] usb: gadget: serial: fix Tx stall after buffer overflow

[Sergey Organov]

Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:

> On Mon, Jan 20, 2020 at 09:06:18AM +0300, Sergey Organov wrote:
>> Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:
>> 
>> > On Fri, Jan 17, 2020 at 08:29:33AM +0300, Sergey Organov wrote:
>> >> --- a/drivers/usb/gadget/function/u_serial.c
>> >> +++ b/drivers/usb/gadget/function/u_serial.c
>> >> @@ -563,6 +563,8 @@ static int gs_start_io(struct gs_port *port)
>> >>  
>> >>         /* unblock any pending writes into our circular buffer */
>> >>         if (started) {
>> >> +               pr_debug("gs_start_tx: ttyGS%d\n", port->port_num);
>> >> +               gs_start_tx(port);
>> >>                 tty_wakeup(port->port.tty);
>> >
>> > The tty_wakeup() will be called from gs_start_tx(), so should be removed
>> > from here.
>> 
>> Not exactly. tty_wakeup() will be called from gs_start_tx() only when
>> there has been something actually transferred from the buffer. I didn't
>> want to change behavior when the buffer is empty, so I kept the explicit
>> tty_wakeup() call in place, intentionally. Please let me know if you
>> still think it should be removed.
>
> Indeed it is as you describe. You might add an argument that initializes
> do_tty_wake, but I'm not sure saving one tty_wakeup() on open is worth
> the trouble.

OK, so let's leave it as is, at least for now.

>
>> > The pr_debug() in other callers of gs_start_tx() say:
>> > "caller: start ttyGS%d".
>> 
>> ???
>> 
>> $ git co gregkh/tty-next && grep -r 'caller: start tty' .
>> HEAD is now at 7788f54... serial_core: Remove unused member in uart_port
>> $ 
>
> Replace 'caller' with a function calling gs_start_io().

Thanks, now I see... Do you prefer:

   pr_debug("gs_start_io: start Tx on ttyGS%d\n", port->port_num);

then?

Alternatively, I'm OK with removing this new debug print.

What do you think?

Thanks,
-- Sergey Organov

Re: [PATCH] usb: gadget: serial: fix Tx stall after buffer overflow

[Michał Mirosław]

On Mon, Jan 20, 2020 at 04:38:16PM +0300, Sergey Organov wrote:
> Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:
> 
> > On Mon, Jan 20, 2020 at 09:06:18AM +0300, Sergey Organov wrote:
> >> Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:
> >> > The pr_debug() in other callers of gs_start_tx() say:
> >> > "caller: start ttyGS%d".
> >> 
> >> ???
> >> 
> >> $ git co gregkh/tty-next && grep -r 'caller: start tty' .
> >> HEAD is now at 7788f54... serial_core: Remove unused member in uart_port
> >> $ 
> >
> > Replace 'caller' with a function calling gs_start_io().
> 
> Thanks, now I see... Do you prefer:
> 
>    pr_debug("gs_start_io: start Tx on ttyGS%d\n", port->port_num);
> 
> then?
> 
> Alternatively, I'm OK with removing this new debug print.

Let's remove it. I was convinced that this is a caller of gs_start_io()
and not the function itself.  In this case callers already do the print.

BTW, the callers silently ignore (error) returns from this function. It
might be useful to add pr_err() catching the errors.

Best Regards,
Michał Mirosław

Re: [PATCH] usb: gadget: serial: fix Tx stall after buffer overflow

[Sergey Organov]

Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:

> On Mon, Jan 20, 2020 at 04:38:16PM +0300, Sergey Organov wrote:
>> Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:
>> 
>> > On Mon, Jan 20, 2020 at 09:06:18AM +0300, Sergey Organov wrote:
>> >> Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:
>> >> > The pr_debug() in other callers of gs_start_tx() say:
>> >> > "caller: start ttyGS%d".
>> >> 
>> >> ???
>> >> 
>> >> $ git co gregkh/tty-next && grep -r 'caller: start tty' .
>> >> HEAD is now at 7788f54... serial_core: Remove unused member in uart_port
>> >> $ 
>> >
>> > Replace 'caller' with a function calling gs_start_io().
>> 
>> Thanks, now I see... Do you prefer:
>> 
>>    pr_debug("gs_start_io: start Tx on ttyGS%d\n", port->port_num);
>> 
>> then?
>> 
>> Alternatively, I'm OK with removing this new debug print.
>
> Let's remove it. I was convinced that this is a caller of gs_start_io()
> and not the function itself.  In this case callers already do the
> print.

OK.

> BTW, the callers silently ignore (error) returns from this function. It
> might be useful to add pr_err() catching the errors.

... or actually handle the error returns? Anyway, that's beyond the
scope of the patch and my expertise.

I've sent re-roll of the patch according to our discussion.

Thanks,
-- Sergey

[PATCH v2] usb: gadget: serial: fix Tx stall after buffer overflow

[Sergey Organov]

Symptom: application opens /dev/ttyGS0 and starts sending (writing) to
it while either USB cable is not connected, or nobody listens on the
other side of the cable. If driver circular buffer overflows before
connection is established, no data will be written to the USB layer
until/unless /dev/ttyGS0 is closed and re-opened again by the
application (the latter besides having no means of being notified about
the event of establishing of the connection.)

Fix: on open and/or connect, kick Tx to flush circular buffer data to
USB layer.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---

Changes in v2:

- Add comment to document why tty_wakeup() is kept in place
- Don't add debug print
- Remove NOTE from description

 drivers/usb/gadget/function/u_serial.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index f986e5c..8167d37 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -561,8 +561,10 @@ static int gs_start_io(struct gs_port *port)
 	port->n_read = 0;
 	started = gs_start_rx(port);
 
-	/* unblock any pending writes into our circular buffer */
 	if (started) {
+		gs_start_tx(port);
+		/* Unblock any pending writes into our circular buffer, in case
+		 * we didn't in gs_start_tx() */
 		tty_wakeup(port->port.tty);
 	} else {
 		gs_free_requests(ep, head, &port->read_allocated);
-- 
2.10.0.1.g57b01a3

Re: [PATCH v2] usb: gadget: serial: fix Tx stall after buffer overflow

[Michał Mirosław]

On Tue, Jan 21, 2020 at 07:42:16AM +0300, Sergey Organov wrote:
> Symptom: application opens /dev/ttyGS0 and starts sending (writing) to
> it while either USB cable is not connected, or nobody listens on the
> other side of the cable. If driver circular buffer overflows before
> connection is established, no data will be written to the USB layer
> until/unless /dev/ttyGS0 is closed and re-opened again by the
> application (the latter besides having no means of being notified about
> the event of establishing of the connection.)
> 
> Fix: on open and/or connect, kick Tx to flush circular buffer data to
> USB layer.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> 
> Changes in v2:
> 
> - Add comment to document why tty_wakeup() is kept in place
> - Don't add debug print
> - Remove NOTE from description

Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Best Regards,
Michał Mirosław

[PATCH v3] usb: gadget: serial: fix Tx stall after buffer overflow

[Sergey Organov]

Symptom: application opens /dev/ttyGS0 and starts sending (writing) to
it while either USB cable is not connected, or nobody listens on the
other side of the cable. If driver circular buffer overflows before
connection is established, no data will be written to the USB layer
until/unless /dev/ttyGS0 is closed and re-opened again by the
application (the latter besides having no means of being notified about
the event of establishing of the connection.)

Fix: on open and/or connect, kick Tx to flush circular buffer data to
USB layer.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---

Changes in v3:

- Add Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Changes in v2:

- Add comment to document why tty_wakeup() is kept in place
- Don't add debug print
- Remove NOTE from description

 drivers/usb/gadget/function/u_serial.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index f986e5c..8167d37 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -561,8 +561,10 @@ static int gs_start_io(struct gs_port *port)
 	port->n_read = 0;
 	started = gs_start_rx(port);
 
-	/* unblock any pending writes into our circular buffer */
 	if (started) {
+		gs_start_tx(port);
+		/* Unblock any pending writes into our circular buffer, in case
+		 * we didn't in gs_start_tx() */
 		tty_wakeup(port->port.tty);
 	} else {
 		gs_free_requests(ep, head, &port->read_allocated);
-- 
2.10.0.1.g57b01a3