[0/4] usb: typec: ucsi: Fix various locking issues

Message

Hans de Goede Aug. 9, 2020, 2:19 p.m. UTC

Hi Heikki, et al.,

As discussed before here is my fix for the AB BA lock inversion issue
which lockdep found in the ucsi code.

While working on the AB BA fix I added a
WARN_ON(!mutex_is_locked(&ucsi->ppm_lock)) to ucsi_run_command() and
that found me some more unrelated issues of ucsi_run_command() getting
called without the ppm_lock held. This is fixed in the second patch.
This made me realize that the ppm_lock handling should probably just
be pushed down to inside ucsi_run_command() instead of letting the
callers worry about this.

In essence the first 3 patches are all related and all boil down to
push the ppm_lock handling down into ucsi_run_command(), removing the
difference between ucsi_run_command() and ucsi_send_command(). I have
been thinking that we may want to just squash these 3 together. I've
left them as separate patches now as thet document how I go to the
end result after the 3th patch and having them separate might be
easier for reviewing purposes. Let me know if you want a v2 of this
patchset with them squashed into a single "usb: typec: ucsi: Rework
ppm_lock handling" commit (with the commit messages merged).

The 4th patch makes the also already discussed change of holding
con->lock for the entire duration of ucsi_register_port().

Regards,

Hans

Comments

Heikki Krogerus Aug. 13, 2020, 11:07 a.m. UTC | #1

On Sun, Aug 09, 2020 at 04:19:00PM +0200, Hans de Goede wrote:
> Hi Heikki, et al.,
> 
> As discussed before here is my fix for the AB BA lock inversion issue
> which lockdep found in the ucsi code.
> 
> While working on the AB BA fix I added a
> WARN_ON(!mutex_is_locked(&ucsi->ppm_lock)) to ucsi_run_command() and
> that found me some more unrelated issues of ucsi_run_command() getting
> called without the ppm_lock held. This is fixed in the second patch.
> This made me realize that the ppm_lock handling should probably just
> be pushed down to inside ucsi_run_command() instead of letting the
> callers worry about this.
> 
> In essence the first 3 patches are all related and all boil down to
> push the ppm_lock handling down into ucsi_run_command(), removing the
> difference between ucsi_run_command() and ucsi_send_command(). I have
> been thinking that we may want to just squash these 3 together. I've
> left them as separate patches now as thet document how I go to the
> end result after the 3th patch and having them separate might be
> easier for reviewing purposes. Let me know if you want a v2 of this
> patchset with them squashed into a single "usb: typec: ucsi: Rework
> ppm_lock handling" commit (with the commit messages merged).
> 
> The 4th patch makes the also already discussed change of holding
> con->lock for the entire duration of ucsi_register_port().

I'm fine with having them in separate pathes like that. This all looks
good to me. Thanks for fixing this. For the whole series:

Acked-by: Heikki Krogerus <[e-mail]>

Guenter Roeck Aug. 13, 2020, 1:48 p.m. UTC | #2

On Thu, Aug 13, 2020 at 02:07:13PM +0300, Heikki Krogerus wrote:
> On Sun, Aug 09, 2020 at 04:19:00PM +0200, Hans de Goede wrote:
> > Hi Heikki, et al.,
> > 
> > As discussed before here is my fix for the AB BA lock inversion issue
> > which lockdep found in the ucsi code.
> > 
> > While working on the AB BA fix I added a
> > WARN_ON(!mutex_is_locked(&ucsi->ppm_lock)) to ucsi_run_command() and
> > that found me some more unrelated issues of ucsi_run_command() getting
> > called without the ppm_lock held. This is fixed in the second patch.
> > This made me realize that the ppm_lock handling should probably just
> > be pushed down to inside ucsi_run_command() instead of letting the
> > callers worry about this.
> > 
> > In essence the first 3 patches are all related and all boil down to
> > push the ppm_lock handling down into ucsi_run_command(), removing the
> > difference between ucsi_run_command() and ucsi_send_command(). I have
> > been thinking that we may want to just squash these 3 together. I've
> > left them as separate patches now as thet document how I go to the
> > end result after the 3th patch and having them separate might be
> > easier for reviewing purposes. Let me know if you want a v2 of this
> > patchset with them squashed into a single "usb: typec: ucsi: Rework
> > ppm_lock handling" commit (with the commit messages merged).
> > 
> > The 4th patch makes the also already discussed change of holding
> > con->lock for the entire duration of ucsi_register_port().
> 
> I'm fine with having them in separate pathes like that. This all looks
> good to me. Thanks for fixing this. For the whole series:
> 
> Acked-by: Heikki Krogerus <[e-mail]>
> 

I for my part wasn't able to review patches 1-3 separately, so I
squashed them and reviewed the result, which looks good.

For the series:

Reviewed-by: Guenter Roeck <[e-mail]>

Guenter