Re: [PATCH v2 2/6] usb: xhci: tegra: Remove redundant mutex when setting phy mode

[Thierry Reding]

On Thu, Mar 26, 2026 at 02:17:33PM +0000, Diogo Ivo wrote:
> Hello,
> 
> On 3/24/26 11:48, Thierry Reding wrote:
> > On Tue, Jan 27, 2026 at 03:11:48PM +0000, Diogo Ivo wrote:
> > > As the PHY subsystem already synchronizes concurrent accesses to a PHY
> > > instance with a core-internal mutex remove the driver specific mutex
> > > synchronization.
> > > 
> > > Signed-off-by: Diogo Ivo <diogo.ivo@xxxxxxxxxxxxxxxxxx>
> > > ---
> > > v1->v2:
> > > - New patch
> > > ---
> > >   drivers/usb/host/xhci-tegra.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
> > > index 8b492871d21d..927861ca14f2 100644
> > > --- a/drivers/usb/host/xhci-tegra.c
> > > +++ b/drivers/usb/host/xhci-tegra.c
> > > @@ -1357,15 +1357,11 @@ static void tegra_xhci_id_work(struct work_struct *work)
> > >   	dev_dbg(tegra->dev, "host mode %s\n", str_on_off(tegra->host_mode));
> > > -	mutex_lock(&tegra->lock);
> > > -
> > >   	if (tegra->host_mode)
> > >   		phy_set_mode_ext(phy, PHY_MODE_USB_OTG, USB_ROLE_HOST);
> > >   	else
> > >   		phy_set_mode_ext(phy, PHY_MODE_USB_OTG, USB_ROLE_NONE);
> > > -	mutex_unlock(&tegra->lock);
> > > -
> > 
> > It looks to me like the mutex here is trying to protect against
> > tegra->host_mode changing while we're setting a different mode. That
> > doesn't seem to be taken care of by the PHY internal mutex.
> 
> After taking another look at it I think I understand your point for the
> mutex, but in that case wouldn't it also need to be held in the writer
> of host_mode, tegra_xhci_id_notify()?

Yes, I think it probably would need to. I don't know how likely it is,
but I think the purpose of this is to protect against the ID notifier
firing quickly in succession. Although, given that this runs on a work
queue and work queue instances are non-reentrant to my knowledge, I
don't think we need the mutex here after all.

> This patch has been picked up as-is into usb-next so it would be nice to
> figure this out before it gets merged in the next merge window.

Given the above, I think it's fine. Maybe the commit message doesn't
give a correct reason for why we don't need the mutex, but the resulting
code looks like it should be fine regardless.

Thierry

Attachment: signature.asc
Description: PGP signature