| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <703a470d-87fc-4580-a743-952e422984d4@nvidia.com>
Date: Tue, 13 Jan 2026 14:48:36 +0000
From: Jon Hunter <jonathanh@...dia.com>
To: Diogo Ivo <diogo.ivo@...nico.ulisboa.pt>,
Mathias Nyman <mathias.nyman@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Thierry Reding <thierry.reding@...il.com>, JC Kuo <jckuo@...dia.com>,
Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>
Cc: linux-usb@...r.kernel.org, linux-tegra@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH 3/5] phy: tegra: xusb: Fix ordering issue when switching
roles on USB2 ports
On 13/01/2026 14:05, Diogo Ivo wrote:
>
>
> On 1/13/26 11:56, Jon Hunter wrote:
>>
>> On 04/12/2025 21:27, Diogo Ivo wrote:
>>> The current implementation of USB2 role switching on Tegra relies on
>>> whichever the previous USB controller driver was using the PHY to first
>>> "yield" it back to USB_ROLE_NONE before the next controller configures
>>> it for the new role. However, no mechanism to guarantee this ordering
>>> was implemented, and currently, in the general case, the configuration
>>> functions tegra_xhci_id_work() and tegra_xudc_usb_role_sw_work() end up
>>> running in the same order regardless of the transition being HOST-
>>> >DEVICE
>>> or DEVICE->HOST, leading to one of these transitions ending up in a
>>> non-working state due to the new configuration being clobbered by the
>>> previous controller driver setting USB_ROLE_NONE after the fact.
>>>
>>> Fix this by introducing a helper that waits for the USB2 port’s current
>>> role to become USB_ROLE_NONE and add it in the configuration functions
>>> above before setting the role to either USB_ROLE_HOST or
>>> USB_ROLE_DEVICE. The specific parameters of the helper function are
>>> choices that seem reasonable in my testing and have no other basis.
>>
>> This is no information here about why 6 * 50/60us is deemed to be
>> sufficient? May be it is, but a comment would be nice.
>>
>>> This was tested on a Tegra210 platform (Smaug). However, due to the
>>> similar
>>> approach in Tegra186 it is likely that not only this problem exists
>>> there
>>> but that this patch also fixes it.
>>>
>>> Signed-off-by: Diogo Ivo <diogo.ivo@...nico.ulisboa.pt>
>>> ---
>>> drivers/phy/tegra/xusb.c | 23 +++++++++++++++++++++++
>>> drivers/usb/gadget/udc/tegra-xudc.c | 4 ++++
>>> drivers/usb/host/xhci-tegra.c | 15 ++++++++++-----
>>> include/linux/phy/tegra/xusb.h | 1 +
>>> 4 files changed, 38 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
>>> index c89df95aa6ca..e05c3f2d1421 100644
>>> --- a/drivers/phy/tegra/xusb.c
>>> +++ b/drivers/phy/tegra/xusb.c
>>> @@ -740,6 +740,29 @@ static void
>>> tegra_xusb_parse_usb_role_default_mode(struct tegra_xusb_port *port)
>>> }
>>> }
>>> +bool tegra_xusb_usb2_port_wait_role_none(struct tegra_xusb_padctl
>>> *padctl, int index)
>>> +{
>>> + struct tegra_xusb_usb2_port *usb2 =
>>> tegra_xusb_find_usb2_port(padctl,
>>> + index);
>>> + int retries = 5;
>>> +
>>> + if (!usb2) {
>>> + dev_err(&usb2->base.dev, "no port found for USB2 lane %u\n",
>>> index);
>>
>> This appears to be a bug. If !usb2 then dereference usb2->base anyway.
>
> It is a bug, will fix in v2.
>
>>> + return false;
>>> + }
>>> +
>>> + do {
>>> + if (usb2->role == USB_ROLE_NONE)
>>> + return true;
>>> +
>>> + usleep_range(50, 60);
>>> + } while (retries--);
>>> +
>>> + dev_err(&usb2->base.dev, "timed out waiting for USB_ROLE_NONE");
>>> +
>>> + return false;
>>> +}
>>> +
>>> static int tegra_xusb_usb2_port_parse_dt(struct
>>> tegra_xusb_usb2_port *usb2)
>>> {
>>> struct tegra_xusb_port *port = &usb2->base;
>>> diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/
>>> gadget/ udc/tegra-xudc.c
>>> index 0c38fc37b6e6..72d725659e5f 100644
>>> --- a/drivers/usb/gadget/udc/tegra-xudc.c
>>> +++ b/drivers/usb/gadget/udc/tegra-xudc.c
>>> @@ -698,8 +698,12 @@ static void tegra_xudc_restore_port_speed(struct
>>> tegra_xudc *xudc)
>>> static void tegra_xudc_device_mode_on(struct tegra_xudc *xudc)
>>> {
>>> + int port = tegra_xusb_padctl_get_port_number(xudc->curr_utmi_phy);
>>> int err;
>>> + if (!tegra_xusb_usb2_port_wait_role_none(xudc->padctl, port))
>>> + return;
>>> +
>>> pm_runtime_get_sync(xudc->dev);
>>> tegra_phy_xusb_utmi_pad_power_on(xudc->curr_utmi_phy);
>>> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-
>>> tegra.c
>>> index 9c69fccdc6e8..9944593166a3 100644
>>> --- a/drivers/usb/host/xhci-tegra.c
>>> +++ b/drivers/usb/host/xhci-tegra.c
>>> @@ -1352,18 +1352,23 @@ static void tegra_xhci_id_work(struct
>>> work_struct *work)
>>> struct tegra_xusb_mbox_msg msg;
>>> struct phy *phy = tegra_xusb_get_phy(tegra, "usb2",
>>> tegra->otg_usb2_port);
>>> + enum usb_role role = USB_ROLE_NONE;
>>> u32 status;
>>> int ret;
>>> dev_dbg(tegra->dev, "host mode %s\n", str_on_off(tegra-
>>> >host_mode));
>>> - mutex_lock(&tegra->lock);
>>
>> Extra blank line here.
>
> Will fix in v2.
>
>>> - 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);
>>> + if (tegra->host_mode) {
>>> + if (!tegra_xusb_usb2_port_wait_role_none(tegra->padctl,
>>> + tegra->otg_usb2_port))
>>> + return;
>>> + role = USB_ROLE_HOST;
>>> + }
>>> +
>>> + mutex_lock(&tegra->lock);
>>> + phy_set_mode_ext(phy, PHY_MODE_USB_OTG, role);
>>> mutex_unlock(&tegra->lock);
>>
>> I am trying to understand why you opted to implement it this way
>> around and not add the wait loop after setting to the mode to
>> USB_ROLE_NONE in the original code all within the context of the mutex?
>
> I did that to minimize the amount of time we wait while holding the
> mutex, as we can now possibly wait a significant amount of time for the
> role switch. Is this an unneccessary optimization?
Do you mean it will be longer than a few 100us?
Jon
--
nvpublic
Powered by blists - more mailing lists