On 23-08-24 12:28 pm, Michael Nazzareno Trimarchi wrote: > Hi > > On Fri, Aug 23, 2024 at 8:40 AM 胡连勤 <hulianqin@xxxxxxxx> wrote: >> >> Hello linux community expert: >> >>>> Fixes: c1dca562be8a ("usb gadget: split out serial core") >>>> Cc: stable@xxxxxxxxxxxxxxx >>>> Signed-off-by: Lianqin Hu <hulianqin@xxxxxxxx> >>>> --- >>>> v6: >>>> - Update the commit text >>>> - Add the Fixes tag >>>> - CC stable kernel >>>> - Add serial_port_lock protection when checking port pointer >>>> - Optimize code comments >>>> - Delete log printing >> >>> You need to list ALL of the versions here, I seem to have missed v4 and >>> v5 somewhere so I don't know what changed there. >> [...] >>> nested spinlocks, why? Did you run this with lockdep enabled to verify you aren't hitting a different bug now? >> Because there is a competition relationship between this function and the gserial_disconnect function, >> the gserial_disconnect function first obtains serial_port_lock and then obtains port->port_lock. >> The purpose of nesting is to ensure that when gs_read_complete is executed, it can be successfully executed after obtaining serial_port_lock. >> gserial_disconnect(..) >> { >> struct gs_port *port = gser->ioport; >> ... >> spin_lock_irqsave(&serial_port_lock, flags); >> spin_lock(&port->port_lock); >> ... >> gser->ioport = NULL; ---> port = NULL; >> ... >> spin_unlock(&port->port_lock); >> spin_unlock_irqrestore(&serial_port_lock, flags); >> } >> >> After enabling the lockdep function (CONFIG_DEBUG_LOCK_ALLOC=y), there is no lockdep-related warning information. >> >>> And why is one irqsave and one not? That feels odd, it might be right, but you need to document here why the difference. >> After the gs_read_complete function is executed, spin_unlock_irqrestore is used to restore the previous state, > > 胡连勤 this is not a common locking pattern that is the reason that > should be properly described. This pattern was already used on gser_suspend/resume callbacks, this was done because the lock was stored under port (and port itself was becoming null), hence we added a static spinlock to mitigate it. > >> - /* Queue all received data until the tty layer is ready for it. */ >> spin_lock(&port->port_lock); >> + spin_unlock(&serial_port_lock); >> + >> + /* Queue all received data until the tty layer is ready for it. */ >> list_add_tail(&req->list, &port->read_queue); >> schedule_delayed_work(&port->push, 0); >> - spin_unlock(&port->port_lock); >> + spin_unlock_irqrestore(&port->port_lock, flags); ---> Here we use spin_unlock_irqrestore to restore the state >> } >> >> Thanks > > Thank you