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

From: Thierry Reding <thierry.reding@gmail.com>
To: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
Cc: Thierry Reding <thierry.reding@kernel.org>,
	 Mathias Nyman <mathias.nyman@intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	 Jonathan Hunter <jonathanh@nvidia.com>,
	JC Kuo <jckuo@nvidia.com>, Vinod Koul <vkoul@kernel.org>,
	 Kishon Vijay Abraham I <kishon@kernel.org>,
	Rob Herring <robh@kernel.org>,
	 Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	 Neil Armstrong <neil.armstrong@linaro.org>,
	linux-usb@vger.kernel.org, linux-tegra@vger.kernel.org,
	 linux-kernel@vger.kernel.org, linux-phy@lists.infradead.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v2 2/6] usb: xhci: tegra: Remove redundant mutex when setting phy mode
Date: Fri, 27 Mar 2026 15:06:23 +0100	[thread overview]
Message-ID: <acaNBQkGgQ_N6Krh@orome> (raw)
In-Reply-To: <00aeda7a-e5e5-4779-b212-6e56c2c5ec31@tecnico.ulisboa.pt>

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

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@tecnico.ulisboa.pt>
> > > ---
> > > 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]