Re: AB BA lock inversion in ucsi driver caused by "usb: typec: ucsi: displayport: Fix a potential race during registration"

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

On Mon, Jul 27, 2020 at 02:54:00PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/27/20 2:37 PM, Heikki Krogerus wrote:
> > Hi Hans,
> > 
> > Sorry about the late reply. I just returned from vacation.
> 
> NP. I hope you've enjoyed your vacation.
> 
> > On Fri, Jul 17, 2020 at 12:04:58PM +0200, Hans de Goede wrote:
> > > Hi Heikki,
> > > 
> > > I've been running my personal kernel builds with lockdep enabled
> > > (more people should do that) and it found an AB BA lock inversion in the
> > > ucsi driver. This has been introduced by commit 081da1325d35 ("usb: typec:
> > > ucsi: displayport: Fix a potential race during registration").
> > > 
> > > The problem is as follows:
> > > 
> > > AB order:
> > > 
> > > 1. ucsi_init takes ucsi->ppm_lock (it runs with that locked for the duration of the function)
> > > 2. usci_init eventually end up calling ucsi_register_displayport, which takes
> > >     ucsi_connector->lock
> > > 
> > > BA order:
> > > 
> > > 1. ucsi_handle_connector_change work is started, takes ucsi_connector->lock
> > > 2. ucsi_handle_connector_change calls ucsi_send_command which takes ucsi->ppm_lock
> > > 
> > > I think this can be fixed by doing the following:
> > > 
> > > a. Make ucsi_init drop the ucsi->ppm_lock before it starts registering ports; and
> > >     replacing any ucsi_run_command calls after this point with ucsi_send_command
> > >     (which is a wrapper around run_command taking the lock while handling the command)
> > > 
> > > b. Move the taking of the ucsi_connector->lock from ucsi_register_displayport into
> > >     ucsi_register_port() to make sure that nothing can touch the connector/port until
> > >     ucsi_register_port() has completed.
> > > 
> > > 
> > > b. is not stricly necessary but it brings the locking during init more inline
> > > with locking done during runtime so this seems like a good idea.
> > 
> > Makes sense. So b. it is. Can you prepare the patch for that?
> 
> b. is just a cleanup / extra step on top of a. we need a. to fix the inversion.
> 
> If you are ok with that I can try to make some time for this...

That would be great!

thanks,

-- 
heikki