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

[Hans de Goede <hdegoede@redhat.com>]

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

Regards,

Hans