| 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: <c3f54ffb-cf4b-47ae-871a-6dd60b17c9cb@tecnico.ulisboa.pt>
Date: Tue, 13 Jan 2026 15:10:54 +0000
From: Diogo Ivo <diogo.ivo@...nico.ulisboa.pt>
To: Jon Hunter <jonathanh@...dia.com>, 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 1/13/26 14:48, Jon Hunter wrote:
>
> 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?
Currently the worst case in wait_role_none() is around 300us but again
this is simply because I chose the values with no criteria except that
in my testing they have worked thus far. Do you have access to any
internal documentation where the transition length is documented?
In any case I think that the underlying principle of minimizing the time
we hold the mutex is solid, no?
Diogo
> Jon
>
Powered by blists - more mailing lists