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>]

Hi Hans,

Sorry about the late reply. I just returned from 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?

thanks,

-- 
heikki