On 18-11-24 08:42 am, 胡连勤 wrote:
> From: Lianqin Hu <hulianqin@xxxxxxxx>
>
> Considering that in some extreme cases,
> when u_serial driver is accessed by multiple threads,
> Thread A is executing the open operation and calling the gs_open,
> Thread B is executing the disconnect operation and calling the
> gserial_disconnect function,The port->port_usb pointer will be set to NULL.
>
[...]
> ---
> drivers/usb/gadget/function/u_serial.c | 25 +++++++++++++++----------
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index 0a8c05b2746b..9ab2dbed60a8 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -124,6 +124,7 @@ struct gs_port {
> struct kfifo port_write_buf;
> wait_queue_head_t drain_wait; /* wait while writes drain */
> bool write_busy;
> + bool read_busy;
> wait_queue_head_t close_wait;
> bool suspended; /* port suspended */
> bool start_delayed; /* delay start when suspended */
> @@ -331,9 +332,11 @@ __acquires(&port->port_lock)
> /* drop lock while we call out; the controller driver
> * may need to call us back (e.g. for disconnect)
> */
> + port->read_busy = true;
> spin_unlock(&port->port_lock);
> status = usb_ep_queue(out, req, GFP_ATOMIC);
> spin_lock(&port->port_lock);
> + port->read_busy = false;
>
> if (status) {
> pr_debug("%s: %s %s err %d\n",
> @@ -1412,19 +1415,21 @@ void gserial_disconnect(struct gserial *gser)
> /* tell the TTY glue not to do I/O here any more */
> spin_lock(&port->port_lock);
>
> - gs_console_disconnect(port);
> + if (!port->read_busy) {
start_tx/rx rely on port->port_usb for queuing the requests, and if its
not null during disconnect, tx/rx would keep on queuing requests to UDC
even after disconnect (which is not ideal). Here in your case, after
read_busy is set, start_rx would queue something outside of spinlock,
meanwhile disconnect happens but port_usb is still valid (because
read_busy is set) and start_rx would break early. But start_tx could
continue queuing into disconnected UDC (if 'started' is non-zero, which
could happen due to timing). Can't you try something like this,
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -579,9 +579,12 @@ static int gs_start_io(struct gs_port *port)
* we didn't in gs_start_tx() */
tty_wakeup(port->port.tty);
} else {
- gs_free_requests(ep, head, &port->read_allocated);
- gs_free_requests(port->port_usb->in, &port->write_pool,
- &port->write_allocated);
+ /* Free reqs only if we are still connected */
+ if (port->port_usb) {
+ gs_free_requests(ep, head, &port->read_allocated);
+ gs_free_requests(port->port_usb->in,
&port->write_pool,
+ &port->write_allocated);
+ }
status = -EIO;
}
This will skip freeing reqs (and your crash) if port_usb is null and
freeing would be taken care by disconnect callback.
> + gs_console_disconnect(port);
>
> - /* REVISIT as above: how best to track this? */
> - port->port_line_coding = gser->port_line_coding;
> + /* REVISIT as above: how best to track this? */
> + port->port_line_coding = gser->port_line_coding;
>
> - port->port_usb = NULL;
> - gser->ioport = NULL;
> - if (port->port.count > 0) {
> - wake_up_interruptible(&port->drain_wait);
> - if (port->port.tty)
> - tty_hangup(port->port.tty);
> + port->port_usb = NULL;
> + gser->ioport = NULL;
> + if (port->port.count > 0) {
> + wake_up_interruptible(&port->drain_wait);
> + if (port->port.tty)
> + tty_hangup(port->port.tty);
> + }
> + port->suspended = false;
> }
> - port->suspended = false;
> spin_unlock(&port->port_lock);
> spin_unlock_irqrestore(&serial_port_lock, flags);
>
Regards,
Prashanth K
