Re: [EXT] Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Thu, Nov 18, 2021 at 03:00:16PM +0000, Xu Yang wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Thursday, November 18, 2021 9:18 PM
> > To: Xu Yang <xu.yang_2@nxp.com>
> > Cc: linux@roeck-us.net; gregkh@linuxfoundation.org; linux-
> > usb@vger.kernel.org; Jun Li <jun.li@nxp.com>; dl-linux-imx <linux-
> > imx@nxp.com>
> > Subject: [EXT] Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but
> > leave a pending timer
> > 
> > Caution: EXT Email
> > 
> > Hi,
> > 
> > On Thu, Nov 18, 2021 at 05:23:52PM +0800, Xu Yang wrote:
> > > @@ -6428,6 +6432,9 @@ void tcpm_unregister_port(struct tcpm_port
> > > *port)  {
> > >       int i;
> > 
> > You need to take the port lock here, no?
> > 
> >         mutex_lock(&port->lock);
> > 
> > > +     kthread_destroy_worker(port->wq);
> > > +     port->wq = NULL;
> > 
> >         mutex_unlock(&port->lock);
> 
> I think we should not take the port lock before kthread_destroy_worker() since
> a deadlock might occur. Considering a work is pending and tcpm_unregister_port
> is called at this time, the worker needs to flush all the works after taking
> the port lock in tcpm_unregister_port(). However, the work can't take the port
> lock anymore.

The point is that you create a race with that code. If the port lock
is not useful in this case, there needs to be something else.

thanks,

-- 
heikki