Re: [PATCH v2] USB: serial: generic: recover from a stalled bulk-in endpoint

[Oliver Neukum]

On 01.09.26 05:49, Julian Oes wrote:
> A USB serial port can go permanently silent when its bulk-in endpoint is
> halted: the read URBs complete with -EPIPE, which the generic read
> callback has always treated as fatal, and no further data arrives until
> user space closes and reopens the tty.

But why do you get a port stalling?
It seems your hardware is quite broken.

[..]
> Use a dedicated work item rather than the existing per-port work. The
> latter is scheduled from every write completion and must not be
> cancelled on close, as the line discipline depends on it. Stall
> recovery resubmits the read URBs and must therefore be cancelled
> wherever the reads are stopped, that is, on close, suspend and
> disconnect.

Well, I am sorry, but no.

Your conceptual mistake is seeing the recovery from stall
as an indivisible process. It is not, as it has two parts.
Once your port is in a stall, you should send the feature
request to unblock the halt. There is no reason to cancel the
feature request if you close a port. You just need to refrain
from resubmitting the read URB.
In fact, if you were to be really comprehensive you need
to wait for the result of a feature request on the way
when you reopen a port.

> @@ -128,8 +144,16 @@ void usb_serial_generic_close(struct usb_serial_port *port)
>   		spin_unlock_irqrestore(&port->lock, flags);
>   	}
>   	if (port->bulk_in_size) {
> -		for (i = 0; i < ARRAY_SIZE(port->read_urbs); ++i)
> -			usb_kill_urb(port->read_urbs[i]);
> +		usb_serial_generic_kill_read_urbs(port);
> +		/*
> +		 * The read URBs are dead now so no further stall can be
> +		 * reported, but stall recovery may already be running and may
> +		 * have resubmitted them. Wait for it to finish before killing
> +		 * the URBs for good.
> +		 */
> +		cancel_delayed_work_sync(&port->stall_work);
> +		usb_serial_generic_kill_read_urbs(port);

And that is a race condition. Rekilling does not help reliably.
If your timing is unlucky enough any subsequent operation can be a nop.
A correct sequence would be something like

poison URBs -> cancel the works -> unpoison the URBs

	Regards
		Oliver