Hello Prashanth:
> >
> >>> 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.
> >>
> >>
> > First of all, the patch you gave can solve the problem we are currently
> facing.
> >
> > When we first encountered this problem, we also thought about adding a
> > null check operation to deal with it, but we saw that the entry of
> > this function (gs_start_io) had a null check operation for
> > port->port_usb, so I gave up the idea of null check during free_req (maybe
> I made a simple problem complicated), and thought about optimizing it from
> the software logic, so that the port->port_usb pointer is always valid before
> gs_start_io is executed.
> >
>
> If it solves the problem, i guess you can use the null pointer check as I
> suggested and send a new patchset, the current one will introduce new
> problems. Keep the issue analysis as it is in commit text since its descriptive
> enough to understand the problem.
OK, I'll do a stress test and send out a new patch later.
Thanks
