| 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: <87b2a1a0-a3f3-491f-bc54-73631a5c2f0a@nvidia.com>
Date: Tue, 1 Apr 2025 07:30:43 +0000
From: Wayne Chang <waynec@...dia.com>
To: Thierry Reding <thierry.reding@...il.com>
CC: Jon Hunter <jonathanh@...dia.com>, Jui Chang Kuo <jckuo@...dia.com>,
"vkoul@...nel.org" <vkoul@...nel.org>, "kishon@...nel.org"
<kishon@...nel.org>, "linux-phy@...ts.infradead.org"
<linux-phy@...ts.infradead.org>, "linux-tegra@...r.kernel.org"
<linux-tegra@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "stable@...r.kernel.org"
<stable@...r.kernel.org>
Subject: Re: [PATCH 1/1] phy: tegra: xusb: Use a bitmask for UTMI pad power
state tracking
Hi Thierry,
Thanks for your review! I'll update in the next patchset.
thanks,
Wayne.
On 3/21/25 20:22, Thierry Reding wrote:
> On Fri, Mar 14, 2025 at 03:33:48PM +0800, Wayne Chang wrote:
>> The current implementation uses bias_pad_enable as a reference count to
>> manage the shared bias pad for all UTMI PHYs. However, during system
>> suspension with connected USB devices, multiple power-down requests for
>> the UTMI pad result in a mismatch in the reference count, which in turn
>> produces warnings such as:
>>
>> [ 237.762967] WARNING: CPU: 10 PID: 1618 at tegra186_utmi_pad_power_down+0x160/0x170
>> [ 237.763103] Call trace:
>> [ 237.763104] tegra186_utmi_pad_power_down+0x160/0x170
>> [ 237.763107] tegra186_utmi_phy_power_off+0x10/0x30
>> [ 237.763110] phy_power_off+0x48/0x100
>> [ 237.763113] tegra_xusb_enter_elpg+0x204/0x500
>> [ 237.763119] tegra_xusb_suspend+0x48/0x140
>> [ 237.763122] platform_pm_suspend+0x2c/0xb0
>> [ 237.763125] dpm_run_callback.isra.0+0x20/0xa0
>> [ 237.763127] __device_suspend+0x118/0x330
>> [ 237.763129] dpm_suspend+0x10c/0x1f0
>> [ 237.763130] dpm_suspend_start+0x88/0xb0
>> [ 237.763132] suspend_devices_and_enter+0x120/0x500
>> [ 237.763135] pm_suspend+0x1ec/0x270
>>
>> The root cause was traced back to the dynamic power-down changes
>> introduced in commit a30951d31b25 ("xhci: tegra: USB2 pad power controls"),
>> where the UTMI pad was being powered down without verifying its current
>> state. This unbalanced behavior led to discrepancies in the reference
>> count.
>>
>> To rectify this issue, this patch replaces the single reference counter
>> with a bitmask, renamed to utmi_pad_enabled. Each bit in the mask
>> corresponds to one of the four USB2 PHYs, allowing us to track each pad's
>> enablement status individually.
>>
>> With this change:
>> - The bias pad is powered on only when the mask is clear.
>> - Each UTMI pad is powered on or down based on its corresponding bit
>> in the mask, preventing redundant operations.
>> - The overall power state of the shared bias pad is maintained
>> correctly during suspend/resume cycles.
>>
>> Cc: stable@...r.kernel.org
>> Fixes: a30951d31b25 ("xhci: tegra: USB2 pad power controls")
>> Signed-off-by: Wayne Chang <waynec@...dia.com>
>> ---
>> drivers/phy/tegra/xusb-tegra186.c | 25 +++++++++++++++++--------
>> 1 file changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
>> index fae6242aa730..77bb27a34738 100644
>> --- a/drivers/phy/tegra/xusb-tegra186.c
>> +++ b/drivers/phy/tegra/xusb-tegra186.c
>> @@ -237,6 +237,8 @@
>> #define DATA0_VAL_PD BIT(1)
>> #define USE_XUSB_AO BIT(4)
>>
>> +#define TEGRA_UTMI_PAD_MAX 4
>> +
>> #define TEGRA186_LANE(_name, _offset, _shift, _mask, _type) \
>> { \
>> .name = _name, \
>> @@ -269,7 +271,7 @@ struct tegra186_xusb_padctl {
>>
>> /* UTMI bias and tracking */
>> struct clk *usb2_trk_clk;
>> - unsigned int bias_pad_enable;
>> + DECLARE_BITMAP(utmi_pad_enabled, TEGRA_UTMI_PAD_MAX);
>>
>> /* padctl context */
>> struct tegra186_xusb_padctl_context context;
>> @@ -605,7 +607,7 @@ static void tegra186_utmi_bias_pad_power_on(struct tegra_xusb_padctl *padctl)
>>
>> mutex_lock(&padctl->lock);
>>
>> - if (priv->bias_pad_enable++ > 0) {
>> + if (!bitmap_empty(priv->utmi_pad_enabled, TEGRA_UTMI_PAD_MAX)) {
>> mutex_unlock(&padctl->lock);
>> return;
>> }
>> @@ -669,12 +671,7 @@ static void tegra186_utmi_bias_pad_power_off(struct tegra_xusb_padctl *padctl)
>>
>> mutex_lock(&padctl->lock);
>>
>> - if (WARN_ON(priv->bias_pad_enable == 0)) {
>> - mutex_unlock(&padctl->lock);
>> - return;
>> - }
>> -
>> - if (--priv->bias_pad_enable > 0) {
>> + if (!bitmap_empty(priv->utmi_pad_enabled, TEGRA_UTMI_PAD_MAX)) {
>> mutex_unlock(&padctl->lock);
>> return;
>> }
>> @@ -697,6 +694,7 @@ static void tegra186_utmi_pad_power_on(struct phy *phy)
>> {
>> struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>> struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> + struct tegra186_xusb_padctl *priv = to_tegra186_xusb_padctl(padctl);
>> struct tegra_xusb_usb2_port *port;
>> struct device *dev = padctl->dev;
>> unsigned int index = lane->index;
>> @@ -705,6 +703,9 @@ static void tegra186_utmi_pad_power_on(struct phy *phy)
>> if (!phy)
>> return;
>>
>> + if (test_bit(index, priv->utmi_pad_enabled))
>> + return;
> Don't we need to take the padctl->lock mutex before this...
>
>> +
>> port = tegra_xusb_find_usb2_port(padctl, index);
>> if (!port) {
>> dev_err(dev, "no port found for USB2 lane %u\n", index);
>> @@ -724,18 +725,24 @@ static void tegra186_utmi_pad_power_on(struct phy *phy)
>> value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
>> value &= ~USB2_OTG_PD_DR;
>> padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
>> +
>> + set_bit(index, priv->utmi_pad_enabled);
> ... and release it here? Otherwise we might end up testing, setting and/
> or clearing from two pads concurrently and loose consistency.
>
>> }
>>
>> static void tegra186_utmi_pad_power_down(struct phy *phy)
>> {
>> struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
>> struct tegra_xusb_padctl *padctl = lane->pad->padctl;
>> + struct tegra186_xusb_padctl *priv = to_tegra186_xusb_padctl(padctl);
>> unsigned int index = lane->index;
>> u32 value;
>>
>> if (!phy)
>> return;
>>
>> + if (!test_bit(index, priv->utmi_pad_enabled))
>> + return;
>> +
> Same here...
>
>> dev_dbg(padctl->dev, "power down UTMI pad %u\n", index);
>>
>> value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
>> @@ -748,6 +755,8 @@ static void tegra186_utmi_pad_power_down(struct phy *phy)
>>
>> udelay(2);
>>
>> + clear_bit(index, priv->utmi_pad_enabled);
> and here.
>
> Thierry
Powered by blists - more mailing lists