[PATCH] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context

[PATCH] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The OTG PHY initialization sequence needs to wait for 20 ms at a specific
step, as described in commit 72c0339c115b ("phy: renesas:
rcar-gen3-usb2: follow the hardware manual procedure").

Commit 55a387ebb921 ("phy: renesas: rcar-gen3-usb2: Lock around hardware
registers and driver data") tried to address various problems in the
rcar-gen3-usb2 driver and converted the mutex protecting HW register
accesses to a spin lock, leaving, however, a long delay in the critical
section protected by the spin lock. This may become a problem,
especially on RT kernels.

To address this, release the spin lock before sleeping for 20 ms as
required by the HW manual and reacquire it afterwards. To avoid other
threads entering the critical section and configuring the HW while the
software is waiting for the OTG initialization to complete, introduce the
otg_initializing variable alongside the otg_init_done completion. Any
other thread trying to configure the HW while the OTG PHY initialization
is in progress waits for the completion instead of immediately returning
errors to PHY users. The IRQs were also disabled while waiting for the OTG
PHY initialization to complete, as the interrupt handler may also apply HW
settings.

Fixes: 55a387ebb921 ("phy: renesas: rcar-gen3-usb2: Lock around hardware registers and driver data")
Cc: stable@vger.kernel.org
Reported-by: Pavel Machek <pavel@nabladev.com>
Closes: https://lore.kernel.org/all/afhkX2Ys2BG1gnqy@duo.ucw.cz
Reported-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
Closes: https://lore.kernel.org/all/afhkX2Ys2BG1gnqy@duo.ucw.cz
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/phy/renesas/phy-rcar-gen3-usb2.c | 163 ++++++++++++++++++-----
 1 file changed, 132 insertions(+), 31 deletions(-)

diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 9a45d840efeb..bdb64726f4cf 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -15,6 +15,7 @@
 #include <linux/extcon-provider.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/lockdep.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/mux/consumer.h>
@@ -138,12 +139,15 @@ struct rcar_gen3_chan {
 	struct rcar_gen3_phy rphys[NUM_OF_PHYS];
 	struct regulator *vbus;
 	struct work_struct work;
+	struct completion otg_init_done;
 	spinlock_t lock;	/* protects access to hardware and driver data structure. */
 	enum usb_dr_mode dr_mode;
+	int irq;
 	bool extcon_host;
 	bool is_otg_channel;
 	bool uses_otg_pins;
 	bool otg_internal_reg;
+	bool otg_initializing;
 };
 
 struct rcar_gen3_phy_drv_data {
@@ -386,32 +390,68 @@ static bool rcar_gen3_are_all_rphys_power_off(struct rcar_gen3_chan *ch)
 	return true;
 }
 
+static int rcar_gen3_phy_wait_otg_init(struct rcar_gen3_chan *channel,
+				       unsigned long *flags)
+{
+	unsigned long timeout = msecs_to_jiffies(25);
+	unsigned long ret = 1;
+
+	lockdep_assert_held(&channel->lock);
+
+	/*
+	 * The OTG can be initialized only once and needs to release the lock
+	 * and wait for 20 ms due to hardware constraints. Wait for the OTG PHY
+	 * initialization to complete if another PHY executes configuration
+	 * code while the OTG PHY is waiting. This avoids returning failures to
+	 * PHY users.
+	 */
+	if (READ_ONCE(channel->otg_initializing)) {
+		spin_unlock_irqrestore(&channel->lock, *flags);
+
+		ret = wait_for_completion_timeout(&channel->otg_init_done, timeout);
+
+		spin_lock_irqsave(&channel->lock, *flags);
+	}
+
+	return !ret ? -ETIMEDOUT : 0;
+}
+
 static ssize_t role_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t count)
 {
 	struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
 	bool is_b_device;
 	enum phy_mode cur_mode, new_mode;
+	unsigned long flags;
+	int ret = -EIO;
 
-	guard(spinlock_irqsave)(&ch->lock);
+	spin_lock_irqsave(&ch->lock, flags);
 
 	if (!ch->is_otg_channel || !rcar_gen3_is_any_otg_rphy_initialized(ch))
-		return -EIO;
+		goto unlock;
+
+	ret = rcar_gen3_phy_wait_otg_init(ch, &flags);
+	if (ret)
+		goto unlock;
 
-	if (sysfs_streq(buf, "host"))
+	if (sysfs_streq(buf, "host")) {
 		new_mode = PHY_MODE_USB_HOST;
-	else if (sysfs_streq(buf, "peripheral"))
+	} else if (sysfs_streq(buf, "peripheral")) {
 		new_mode = PHY_MODE_USB_DEVICE;
-	else
-		return -EINVAL;
+	} else {
+		ret = -EINVAL;
+		goto unlock;
+	}
 
 	/* is_b_device: true is B-Device. false is A-Device. */
 	is_b_device = rcar_gen3_check_id(ch);
 	cur_mode = rcar_gen3_get_phy_mode(ch);
 
 	/* If current and new mode is the same, this returns the error */
-	if (cur_mode == new_mode)
-		return -EINVAL;
+	if (cur_mode == new_mode) {
+		ret = -EINVAL;
+		goto unlock;
+	}
 
 	if (new_mode == PHY_MODE_USB_HOST) { /* And is_host must be false */
 		if (!is_b_device)	/* A-Peripheral */
@@ -425,7 +465,10 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr,
 			rcar_gen3_init_for_peri(ch);
 	}
 
-	return count;
+unlock:
+	spin_unlock_irqrestore(&ch->lock, flags);
+
+	return ret ?: count;
 }
 
 static ssize_t role_show(struct device *dev, struct device_attribute *attr,
@@ -441,14 +484,11 @@ static ssize_t role_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(role);
 
-static void rcar_gen3_init_otg(struct rcar_gen3_chan *ch)
+static void rcar_gen3_init_otg_phase0(struct rcar_gen3_chan *ch)
 {
 	void __iomem *usb2_base = ch->base;
 	u32 val;
 
-	if (!ch->is_otg_channel || rcar_gen3_is_any_otg_rphy_initialized(ch))
-		return;
-
 	/* Should not use functions of read-modify-write a register */
 	val = readl(usb2_base + USB2_LINECTRL1);
 	val = (val & ~USB2_LINECTRL1_DP_RPD) | USB2_LINECTRL1_DPRPD_EN |
@@ -471,7 +511,11 @@ static void rcar_gen3_init_otg(struct rcar_gen3_chan *ch)
 			writel(val | USB2_ADPCTRL_IDPULLUP, usb2_base + USB2_ADPCTRL);
 		}
 	}
-	mdelay(20);
+}
+
+static void rcar_gen3_init_otg_phase1(struct rcar_gen3_chan *ch)
+{
+	void __iomem *usb2_base = ch->base;
 
 	writel(0xffffffff, usb2_base + USB2_OBINTSTA);
 	writel(ch->phy_data->obint_enable_bits, usb2_base + USB2_OBINTEN);
@@ -510,6 +554,11 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
 		goto rpm_put;
 
 	scoped_guard(spinlock, &ch->lock) {
+		if (READ_ONCE(ch->otg_initializing)) {
+			dev_warn(dev, "%s: Got IRQ while waiting for OTG init!\n", __func__);
+			return IRQ_NONE;
+		}
+
 		status = readl(usb2_base + USB2_OBINTSTA);
 		if (status & ch->phy_data->obint_enable_bits) {
 			dev_vdbg(dev, "%s: %08x\n", __func__, status);
@@ -533,9 +582,15 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
 	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
 	struct rcar_gen3_chan *channel = rphy->ch;
 	void __iomem *usb2_base = channel->base;
+	unsigned long flags;
 	u32 val;
+	int ret;
 
-	guard(spinlock_irqsave)(&channel->lock);
+	spin_lock_irqsave(&channel->lock, flags);
+
+	ret = rcar_gen3_phy_wait_otg_init(channel, &flags);
+	if (ret)
+		goto unlock;
 
 	/* Initialize USB2 part */
 	val = readl(usb2_base + USB2_INT_ENABLE);
@@ -548,8 +603,22 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
 	}
 
 	/* Initialize otg part (only if we initialize a PHY with IRQs). */
-	if (rphy->int_enable_bits)
-		rcar_gen3_init_otg(channel);
+	if (rphy->int_enable_bits && channel->is_otg_channel &&
+	    !rcar_gen3_is_any_otg_rphy_initialized(channel)) {
+		rcar_gen3_init_otg_phase0(channel);
+		disable_irq_nosync(channel->irq);
+		reinit_completion(&channel->otg_init_done);
+		WRITE_ONCE(channel->otg_initializing, true);
+		spin_unlock_irqrestore(&channel->lock, flags);
+
+		fsleep(20000);
+
+		spin_lock_irqsave(&channel->lock, flags);
+		WRITE_ONCE(channel->otg_initializing, false);
+		complete_all(&channel->otg_init_done);
+		enable_irq(channel->irq);
+		rcar_gen3_init_otg_phase1(channel);
+	}
 
 	if (channel->phy_data->vblvl_ctrl) {
 		/* SIDDQ mode release */
@@ -568,7 +637,10 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
 
 	rphy->initialized = true;
 
-	return 0;
+unlock:
+	spin_unlock_irqrestore(&channel->lock, flags);
+
+	return ret;
 }
 
 static int rcar_gen3_phy_usb2_exit(struct phy *p)
@@ -576,9 +648,15 @@ static int rcar_gen3_phy_usb2_exit(struct phy *p)
 	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
 	struct rcar_gen3_chan *channel = rphy->ch;
 	void __iomem *usb2_base = channel->base;
+	unsigned long flags;
 	u32 val;
+	int ret;
 
-	guard(spinlock_irqsave)(&channel->lock);
+	spin_lock_irqsave(&channel->lock, flags);
+
+	ret = rcar_gen3_phy_wait_otg_init(channel, &flags);
+	if (ret)
+		goto unlock;
 
 	rphy->initialized = false;
 
@@ -588,7 +666,9 @@ static int rcar_gen3_phy_usb2_exit(struct phy *p)
 		val &= ~USB2_INT_ENABLE_UCOM_INTEN;
 	writel(val, usb2_base + USB2_INT_ENABLE);
 
-	return 0;
+unlock:
+	spin_unlock_irqrestore(&channel->lock, flags);
+	return ret;
 }
 
 static int rcar_gen3_phy_usb2_power_on(struct phy *p)
@@ -596,6 +676,7 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
 	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
 	struct rcar_gen3_chan *channel = rphy->ch;
 	void __iomem *usb2_base = channel->base;
+	unsigned long flags;
 	u32 val;
 	int ret = 0;
 
@@ -605,11 +686,15 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
 			return ret;
 	}
 
-	guard(spinlock_irqsave)(&channel->lock);
+	spin_lock_irqsave(&channel->lock, flags);
 
 	if (!rcar_gen3_are_all_rphys_power_off(channel))
 		goto out;
 
+	ret = rcar_gen3_phy_wait_otg_init(channel, &flags);
+	if (ret)
+		goto unlock;
+
 	val = readl(usb2_base + USB2_USBCTR);
 	val |= USB2_USBCTR_PLL_RST;
 	writel(val, usb2_base + USB2_USBCTR);
@@ -620,27 +705,41 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
 	/* The powered flag should be set for any other phys anyway */
 	rphy->powered = true;
 
-	return 0;
+unlock:
+	spin_unlock_irqrestore(&channel->lock, flags);
+
+	if (ret && channel->vbus && !channel->otg_internal_reg)
+		regulator_disable(channel->vbus);
+
+	return ret;
 }
 
 static int rcar_gen3_phy_usb2_power_off(struct phy *p)
 {
 	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
 	struct rcar_gen3_chan *channel = rphy->ch;
-	int ret = 0;
+	unsigned long flags;
+	int ret;
 
-	scoped_guard(spinlock_irqsave, &channel->lock) {
-		rphy->powered = false;
+	spin_lock_irqsave(&channel->lock, flags);
 
-		if (rcar_gen3_are_all_rphys_power_off(channel)) {
-			u32 val = readl(channel->base + USB2_USBCTR);
+	ret = rcar_gen3_phy_wait_otg_init(channel, &flags);
+	if (ret)
+		goto unlock;
 
-			val |= USB2_USBCTR_PLL_RST;
-			writel(val, channel->base + USB2_USBCTR);
-		}
+	rphy->powered = false;
+
+	if (rcar_gen3_are_all_rphys_power_off(channel)) {
+		u32 val = readl(channel->base + USB2_USBCTR);
+
+		val |= USB2_USBCTR_PLL_RST;
+		writel(val, channel->base + USB2_USBCTR);
 	}
 
-	if (channel->vbus && !channel->otg_internal_reg)
+unlock:
+	spin_unlock_irqrestore(&channel->lock, flags);
+
+	if (!ret && channel->vbus && !channel->otg_internal_reg)
 		ret = regulator_disable(channel->vbus);
 
 	return ret;
@@ -1007,6 +1106,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 		return ret;
 
 	spin_lock_init(&channel->lock);
+	init_completion(&channel->otg_init_done);
 	for (i = 0; i < NUM_OF_PHYS; i++) {
 		channel->rphys[i].phy = devm_phy_create(dev, NULL,
 							channel->phy_data->phy_usb2_ops);
@@ -1048,6 +1148,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 			return dev_err_probe(dev, ret,
 					     "Failed to request irq (%d)\n",
 					     irq);
+		channel->irq = irq;
 	}
 
 	provider = devm_of_phy_provider_register(dev, rcar_gen3_phy_usb2_xlate);
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] PM runtime reference leak in interrupt handler due to goto mixing.
- [High] Unconditional calls to `disable_irq_nosync()` with IRQ 0.
- [High] Disabling a shared IRQ line globally starves other devices.
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition in PHY power control.
- [Medium] TOCTOU race condition in `role_store` causes spurious `-EIO` errors.
- [Medium] Tight completion timeout causes spurious PHY initialization failures.
- [Medium] Spurious kernel warnings generated by valid shared interrupts.
--

commit 856d28e68a5bcfa08fa03aa685cb15772197dedc
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context

This commit releases the spin lock before sleeping for 20 ms during
OTG PHY initialization and introduces completion mechanisms to prevent
other threads from configuring the hardware concurrently. It also disables
IRQs while waiting to prevent interrupt handlers from applying settings.

Closes: https://lore.kernel.org/all/afhkX2Ys2BG1gnqy@duo.ucw.cz
Closes: https://lore.kernel.org/all/afhkX2Ys2BG1gnqy@duo.ucw.cz

> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index 79e820e2fe55d..5a18bbd87c3b9 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
[ ... ]
> @@ -388,32 +392,68 @@ static bool rcar_gen3_are_all_rphys_power_off(struct rcar_gen3_chan *ch)
>  	return true;
>  }
>  
> +static int rcar_gen3_phy_wait_otg_init(struct rcar_gen3_chan *channel,
> +				       unsigned long *flags)
> +{
> +	unsigned long timeout = msecs_to_jiffies(25);

Could this completion timeout be too tight?

The initializing thread uses fsleep(20000) which guarantees a minimum sleep
of 20ms. Due to scheduler jitter or timer wheel granularity (especially if
HZ=100, where 25ms rounds to just 2 or 3 jiffies), the 20ms sleep can easily
exceed 25ms of wall-clock time, causing this to spuriously return -ETIMEDOUT.

> +	unsigned long ret = 1;
> +
> +	lockdep_assert_held(&channel->lock);
[ ... ]
> @@ -427,7 +467,10 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr,
>  			  const char *buf, size_t count)
>  {
>  	struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
>  	bool is_b_device;
>  	enum phy_mode cur_mode, new_mode;
> +	unsigned long flags;
> +	int ret = -EIO;
>  
> -	guard(spinlock_irqsave)(&ch->lock);
> +	spin_lock_irqsave(&ch->lock, flags);
>  
>  	if (!ch->is_otg_channel || !rcar_gen3_is_any_otg_rphy_initialized(ch))
> -		return -EIO;
> +		goto unlock;
> +
> +	ret = rcar_gen3_phy_wait_otg_init(ch, &flags);

Does checking the initialized state before waiting for the lock to be
released introduce a race condition?

Because rcar_gen3_phy_usb2_init() only sets rphy->initialized = true after
it finishes its 20ms sleep and reacquires the lock, a concurrent sysfs write
to role_store() during this 20ms window will see initialized as false and
immediately return -EIO instead of waiting for initialization to complete.

> +	if (ret)
> +		goto unlock;
[ ... ]
> @@ -512,6 +556,11 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
>  		goto rpm_put;
>  
>  	scoped_guard(spinlock, &ch->lock) {
> +		if (READ_ONCE(ch->otg_initializing)) {
> +			dev_warn(dev, "%s: Got IRQ while waiting for OTG init!\n", __func__);
> +			return IRQ_NONE;
> +		}

Does returning early here leak the PM runtime reference?

The interrupt handler calls pm_runtime_get_noresume(dev) at the beginning of
the function, and releases it at the rpm_put label. By using return IRQ_NONE
inside the scoped_guard, the code bypasses pm_runtime_put_noidle() completely,
preventing the device from ever entering runtime suspend.

Additionally, since this interrupt is requested with IRQF_SHARED, could
another device sharing the line trigger a valid interrupt during the 20ms
OTG initialization window? If so, this would print a spurious warning to
the kernel log for a valid shared interrupt event.

> +
>  		status = readl(usb2_base + USB2_OBINTSTA);
>  		if (status & ch->phy_data->obint_enable_bits) {
>  			dev_vdbg(dev, "%s: %08x\n", __func__, status);
[ ... ]
> @@ -550,8 +605,22 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
>  	}
>  
>  	/* Initialize otg part (only if we initialize a PHY with IRQs). */
> -	if (rphy->int_enable_bits)
> -		rcar_gen3_init_otg(channel);
> +	if (rphy->int_enable_bits && channel->is_otg_channel &&
> +	    !rcar_gen3_is_any_otg_rphy_initialized(channel)) {
> +		rcar_gen3_init_otg_phase0(channel);
> +		disable_irq_nosync(channel->irq);

Could channel->irq be 0 here?

In rcar_gen3_phy_usb2_probe(), platform_get_irq_optional() is used. If it
returns -ENXIO, channel->irq remains 0. Since rphy->int_enable_bits is a
statically defined non-zero value for OTG PHYs, this would pass 0 to
disable_irq_nosync(), which manipulates critical system interrupts.

Also, does calling disable_irq_nosync() on a shared interrupt line starve
other devices?

Since the interrupt is requested with IRQF_SHARED, masking it at the
interrupt controller level means all other unrelated devices on the same
physical line will have their interrupts disabled for the full 20ms
duration. Should shared interrupts be masked locally at the device level
instead?

> +		reinit_completion(&channel->otg_init_done);
> +		WRITE_ONCE(channel->otg_initializing, true);
> +		spin_unlock_irqrestore(&channel->lock, flags);
[ ... ]
> @@ -598,6 +678,7 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
>  	struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
>  	struct rcar_gen3_chan *channel = rphy->ch;
>  	void __iomem *usb2_base = channel->base;
> +	unsigned long flags;
>  	u32 val;
>  	int ret = 0;
>  
> @@ -607,11 +688,15 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
>  			return ret;
>  	}
>  
> -	guard(spinlock_irqsave)(&channel->lock);
> +	spin_lock_irqsave(&channel->lock, flags);
>  
>  	if (!rcar_gen3_are_all_rphys_power_off(channel))
>  		goto out;
>  
> +	ret = rcar_gen3_phy_wait_otg_init(channel, &flags);
> +	if (ret)
> +		goto unlock;
> +
>  	val = readl(usb2_base + USB2_USBCTR);
>  	val |= USB2_USBCTR_PLL_RST;

Does dropping the lock in rcar_gen3_phy_wait_otg_init() introduce a race
condition with the !rcar_gen3_are_all_rphys_power_off() check?

If two threads concurrently power on different PHYs, both will evaluate the
condition as false because neither has reached rphy->powered = true at the
end of the function. Both threads might then sleep in
rcar_gen3_phy_wait_otg_init().

Upon waking and reacquiring the lock, both will unconditionally apply
USB2_USBCTR_PLL_RST, which could catastrophically disrupt the PHY that was
just initialized by the first thread.

Should the power off condition be re-evaluated after the lock is reacquired?

>  	writel(val, usb2_base + USB2_USBCTR);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514111300.2152386-1-claudiu.beznea@kernel.org?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 2597 bytes --]

Hi!

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The OTG PHY initialization sequence needs to wait for 20 ms at a specific
> step, as described in commit 72c0339c115b ("phy: renesas:
> rcar-gen3-usb2: follow the hardware manual procedure").
> 
> Commit 55a387ebb921 ("phy: renesas: rcar-gen3-usb2: Lock around hardware
> registers and driver data") tried to address various problems in the
> rcar-gen3-usb2 driver and converted the mutex protecting HW register
> accesses to a spin lock, leaving, however, a long delay in the critical
> section protected by the spin lock. This may become a problem,
> especially on RT kernels.
> 
> To address this, release the spin lock before sleeping for 20 ms as
> required by the HW manual and reacquire it afterwards. To avoid other
> threads entering the critical section and configuring the HW while the
> software is waiting for the OTG initialization to complete, introduce the
> otg_initializing variable alongside the otg_init_done completion. Any
> other thread trying to configure the HW while the OTG PHY initialization
> is in progress waits for the completion instead of immediately returning
> errors to PHY users. The IRQs were also disabled while waiting for the OTG
> PHY initialization to complete, as the interrupt handler may also apply HW
> settings.

Just... there has to be a better way.

> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +static int rcar_gen3_phy_wait_otg_init(struct rcar_gen3_chan *channel,
> +				       unsigned long *flags)
> +{
> +	unsigned long timeout = msecs_to_jiffies(25);
> +	unsigned long ret = 1;
> +
> +	lockdep_assert_held(&channel->lock);
> +
> +	/*
> +	 * The OTG can be initialized only once and needs to release the lock
> +	 * and wait for 20 ms due to hardware constraints. Wait for the OTG PHY
> +	 * initialization to complete if another PHY executes configuration
> +	 * code while the OTG PHY is waiting. This avoids returning failures to
> +	 * PHY users.
> +	 */
> +	if (READ_ONCE(channel->otg_initializing)) {
> +		spin_unlock_irqrestore(&channel->lock, *flags);

This is not nice, passing flags between functions like this is a red flag.

You are only accessing otg_initializing under the spinlock. That means
that READ_ONCE is reduntant.

But AFAICT spinlock is only held over this function to protect
channel->otg_initializing access. I suspect correct answer here is
getting rid of spinlock over this function, and using
test_bit(BIT_INITIALIZING, ...) or something similar.

Best regards,
								Pavel


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context

[David Laight]

On Thu, 14 May 2026 23:18:44 +0200
Pavel Machek <pavel@nabladev.com> wrote:

> Hi!
> 
> > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > 
> > The OTG PHY initialization sequence needs to wait for 20 ms at a specific
> > step, as described in commit 72c0339c115b ("phy: renesas:
> > rcar-gen3-usb2: follow the hardware manual procedure").
> > 
> > Commit 55a387ebb921 ("phy: renesas: rcar-gen3-usb2: Lock around hardware
> > registers and driver data") tried to address various problems in the
> > rcar-gen3-usb2 driver and converted the mutex protecting HW register
> > accesses to a spin lock, leaving, however, a long delay in the critical
> > section protected by the spin lock. This may become a problem,
> > especially on RT kernels.
> > 
> > To address this, release the spin lock before sleeping for 20 ms as
> > required by the HW manual and reacquire it afterwards. To avoid other
> > threads entering the critical section and configuring the HW while the
> > software is waiting for the OTG initialization to complete, introduce the
> > otg_initializing variable alongside the otg_init_done completion. Any
> > other thread trying to configure the HW while the OTG PHY initialization
> > is in progress waits for the completion instead of immediately returning
> > errors to PHY users. The IRQs were also disabled while waiting for the OTG
> > PHY initialization to complete, as the interrupt handler may also apply HW
> > settings.  
> 
> Just... there has to be a better way.
> 
> > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > +static int rcar_gen3_phy_wait_otg_init(struct rcar_gen3_chan *channel,
> > +				       unsigned long *flags)
> > +{
> > +	unsigned long timeout = msecs_to_jiffies(25);
> > +	unsigned long ret = 1;
> > +
> > +	lockdep_assert_held(&channel->lock);
> > +
> > +	/*
> > +	 * The OTG can be initialized only once and needs to release the lock
> > +	 * and wait for 20 ms due to hardware constraints. Wait for the OTG PHY
> > +	 * initialization to complete if another PHY executes configuration
> > +	 * code while the OTG PHY is waiting. This avoids returning failures to
> > +	 * PHY users.
> > +	 */
> > +	if (READ_ONCE(channel->otg_initializing)) {
> > +		spin_unlock_irqrestore(&channel->lock, *flags);  
> 
> This is not nice, passing flags between functions like this is a red flag.

It would be better to just inline the code.
And I'd guess you need to redo the initial tests after re-acquiring the lock?
Or even need to do a state change/reference count before releasing the
lock to stop other threads 'doing anything nasty'.

-- David

> 
> You are only accessing otg_initializing under the spinlock. That means
> that READ_ONCE is reduntant.
> 
> But AFAICT spinlock is only held over this function to protect
> channel->otg_initializing access. I suspect correct answer here is
> getting rid of spinlock over this function, and using
> test_bit(BIT_INITIALIZING, ...) or something similar.
> 
> Best regards,
> 								Pavel
> 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context

[Claudiu Beznea]

Hi, David,

On 5/15/26 12:47, David Laight wrote:
> On Thu, 14 May 2026 23:18:44 +0200
> Pavel Machek <pavel@nabladev.com> wrote:
> 
>> Hi!
>>
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> The OTG PHY initialization sequence needs to wait for 20 ms at a specific
>>> step, as described in commit 72c0339c115b ("phy: renesas:
>>> rcar-gen3-usb2: follow the hardware manual procedure").
>>>
>>> Commit 55a387ebb921 ("phy: renesas: rcar-gen3-usb2: Lock around hardware
>>> registers and driver data") tried to address various problems in the
>>> rcar-gen3-usb2 driver and converted the mutex protecting HW register
>>> accesses to a spin lock, leaving, however, a long delay in the critical
>>> section protected by the spin lock. This may become a problem,
>>> especially on RT kernels.
>>>
>>> To address this, release the spin lock before sleeping for 20 ms as
>>> required by the HW manual and reacquire it afterwards. To avoid other
>>> threads entering the critical section and configuring the HW while the
>>> software is waiting for the OTG initialization to complete, introduce the
>>> otg_initializing variable alongside the otg_init_done completion. Any
>>> other thread trying to configure the HW while the OTG PHY initialization
>>> is in progress waits for the completion instead of immediately returning
>>> errors to PHY users. The IRQs were also disabled while waiting for the OTG
>>> PHY initialization to complete, as the interrupt handler may also apply HW
>>> settings.
>>
>> Just... there has to be a better way.
>>
>>> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>>> +static int rcar_gen3_phy_wait_otg_init(struct rcar_gen3_chan *channel,
>>> +				       unsigned long *flags)
>>> +{
>>> +	unsigned long timeout = msecs_to_jiffies(25);
>>> +	unsigned long ret = 1;
>>> +
>>> +	lockdep_assert_held(&channel->lock);
>>> +
>>> +	/*
>>> +	 * The OTG can be initialized only once and needs to release the lock
>>> +	 * and wait for 20 ms due to hardware constraints. Wait for the OTG PHY
>>> +	 * initialization to complete if another PHY executes configuration
>>> +	 * code while the OTG PHY is waiting. This avoids returning failures to
>>> +	 * PHY users.
>>> +	 */
>>> +	if (READ_ONCE(channel->otg_initializing)) {
>>> +		spin_unlock_irqrestore(&channel->lock, *flags);
>>
>> This is not nice, passing flags between functions like this is a red flag.
> 
> It would be better to just inline the code.

I can do that, I tried to avoid it.

> And I'd guess you need to redo the initial tests after re-acquiring the lock?

Could you please let me know what do you mean by "initial tests"

> Or even need to do a state change/reference count before releasing the
> lock to stop other threads 'doing anything nasty'.

I'm not sure I understand this. I'll explain how the patch works:

The HW provides a single OTG PHY. As the HW has a single OTG PHY and the start 
of the OTG PHY initialization is done under spinlock, at any moment, a single 
thread could set the otg_initializing. That would be the thread configuring the 
OTG PHY. And once the OTG PHY was set there is no way it will run again (due to 
rphy->initialized). Unless the struct phy_ops::exit() is called for the OTG PHY.

With the solution in this patch, once a thread sets the otg_initializing, all 
the other threads that will want to set HW registers should wait due to the 
completion mechanism.

If there is any thread that executes wait_for_completion() while:
- otg_initializing is set and
- the OTG configuration thread released the spin lock to execute the fsleep()
if the wait_for_completion times out, the PHY code returns error to the caller 
so the thread calling into the PHY driver will not continue into the PHY driver.

If instead there is no timeout, the waiting thread will have to re-acquire the 
spin lock before executing any HW settings.

If there is no timeout, the code that setup the OTG PHY have already been 
running rphy->initialized = true for the OTG PHY, under spinlock, and no other 
thread will enter the above OTG initialization section:

	/* Initialize otg part (only if we initialize a PHY with IRQs). */
	if (rphy->int_enable_bits && channel->is_otg_channel &&
	    !rcar_gen3_is_any_otg_rphy_initialized(channel)) {
		rcar_gen3_init_otg_phase0(channel);
		disable_irq_nosync(channel->irq);
		reinit_completion(&channel->otg_init_done);
		WRITE_ONCE(channel->otg_initializing, true);
		spin_unlock_irqrestore(&channel->lock, flags);

		fsleep(20000);

		spin_lock_irqsave(&channel->lock, flags);
		WRITE_ONCE(channel->otg_initializing, false);
		complete_all(&channel->otg_init_done);
		enable_irq(channel->irq);
		rcar_gen3_init_otg_phase1(channel);
	}

Thank you,
Claudiu

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context

[Claudiu Beznea]

Hi, Pavel,

On 5/15/26 00:18, Pavel Machek wrote:
> Hi!
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The OTG PHY initialization sequence needs to wait for 20 ms at a specific
>> step, as described in commit 72c0339c115b ("phy: renesas:
>> rcar-gen3-usb2: follow the hardware manual procedure").
>>
>> Commit 55a387ebb921 ("phy: renesas: rcar-gen3-usb2: Lock around hardware
>> registers and driver data") tried to address various problems in the
>> rcar-gen3-usb2 driver and converted the mutex protecting HW register
>> accesses to a spin lock, leaving, however, a long delay in the critical
>> section protected by the spin lock. This may become a problem,
>> especially on RT kernels.
>>
>> To address this, release the spin lock before sleeping for 20 ms as
>> required by the HW manual and reacquire it afterwards. To avoid other
>> threads entering the critical section and configuring the HW while the
>> software is waiting for the OTG initialization to complete, introduce the
>> otg_initializing variable alongside the otg_init_done completion. Any
>> other thread trying to configure the HW while the OTG PHY initialization
>> is in progress waits for the completion instead of immediately returning
>> errors to PHY users. The IRQs were also disabled while waiting for the OTG
>> PHY initialization to complete, as the interrupt handler may also apply HW
>> settings.
> 
> Just... there has to be a better way.
> 
>> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>> +static int rcar_gen3_phy_wait_otg_init(struct rcar_gen3_chan *channel,
>> +				       unsigned long *flags)
>> +{
>> +	unsigned long timeout = msecs_to_jiffies(25);
>> +	unsigned long ret = 1;
>> +
>> +	lockdep_assert_held(&channel->lock);
>> +
>> +	/*
>> +	 * The OTG can be initialized only once and needs to release the lock
>> +	 * and wait for 20 ms due to hardware constraints. Wait for the OTG PHY
>> +	 * initialization to complete if another PHY executes configuration
>> +	 * code while the OTG PHY is waiting. This avoids returning failures to
>> +	 * PHY users.
>> +	 */
>> +	if (READ_ONCE(channel->otg_initializing)) {
>> +		spin_unlock_irqrestore(&channel->lock, *flags);
> 
> This is not nice, passing flags between functions like this is a red flag.
> 
> You are only accessing otg_initializing under the spinlock. That means
> that READ_ONCE is reduntant.
> 
> But AFAICT spinlock is only held over this function to protect
> channel->otg_initializing access. I suspect correct answer here is
> getting rid of spinlock over this function, and using
> test_bit(BIT_INITIALIZING, ...) or something similar.

If I understand correctly your point here, I don't think making the 
otg_initializing atomic (or using test_bit()) and moving it out of the spin lock 
works for all the cases. Suppose the following:

thread1:                               thread2:

0: sleep                               spin_lock_irqsave();

1: still sleep                         otg_initalizing = 1;

2: check otg_initializing              // ...

3: spin_lock_irqsave();                // ...

4:                                     spin_unlock_irqsave();

5:                                     fsleep(20000);


In this way, thread1 will get access to the HW registers once instruction at 
line 4 will be executed and be able to configure other HW registers when it 
should wait for the fsleep() to finish.

The point with the solution provided in this patch was to not allow any other 
thread to configure the HW while the fsleep() is executed.

Thank you,
Claudiu

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

sashiko review: Re: [PATCH] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context

[Claudiu Beznea]

 >commit 856d28e68a5bcfa08fa03aa685cb15772197dedc
 >Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
 >phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context
 >This commit releases the spin lock before sleeping for 20 ms during
 >OTG PHY initialization and introduces completion mechanisms to prevent
 >other threads from configuring the hardware concurrently. It also disables
 >IRQs while waiting to prevent interrupt handlers from applying settings.
 >Closes: https://lore.kernel.org/all/afhkX2Ys2BG1gnqy@duo.ucw.cz
 >Closes: https://lore.kernel.org/all/afhkX2Ys2BG1gnqy@duo.ucw.cz
 >> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c 
b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
 >> index 79e820e2fe55d..5a18bbd87c3b9 100644
 >> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
 >> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
 >[ ... ]
 >> @@ -388,32 +392,68 @@ static bool rcar_gen3_are_all_rphys_power_off(struct 
rcar_gen3_chan *ch)
 >>      return true;
 >>  }
 >>
 >> +static int rcar_gen3_phy_wait_otg_init(struct rcar_gen3_chan *channel,
 >> +                                   unsigned long *flags)
 >> +{
 >> +    unsigned long timeout = msecs_to_jiffies(25);
 >Could this completion timeout be too tight?

I tried to have a bit increased compared with the fsleep() value to avoid just 
being to tight but not to big.

 >The initializing thread uses fsleep(20000) which guarantees a minimum sleep
 >of 20ms. Due to scheduler jitter or timer wheel granularity (especially if
 >HZ=100, where 25ms rounds to just 2 or 3 jiffies), the 20ms sleep can easily
 >exceed 25ms of wall-clock time, causing this to spuriously return -ETIMEDOUT.
 >> +    unsigned long ret = 1;
 >> +
 >> +    lockdep_assert_held(&channel->lock);
 >[ ... ]
 >> @@ -427,7 +467,10 @@ static ssize_t role_store(struct device *dev, struct 
device_attribute *attr,
 >>                        const char *buf, size_t count)
 >>  {
 >>      struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
 >>      bool is_b_device;
 >>      enum phy_mode cur_mode, new_mode;
 >> +    unsigned long flags;
 >> +    int ret = -EIO;
 >>
 >> -    guard(spinlock_irqsave)(&ch->lock);
 >> +    spin_lock_irqsave(&ch->lock, flags);
 >>
 >>      if (!ch->is_otg_channel || !rcar_gen3_is_any_otg_rphy_initialized(ch))
 >> -            return -EIO;
 >> +            goto unlock;
 >> +
 >> +    ret = rcar_gen3_phy_wait_otg_init(ch, &flags);
 >Does checking the initialized state before waiting for the lock to be
 >released introduce a race condition?
 >Because rcar_gen3_phy_usb2_init() only sets rphy->initialized = true after
 >it finishes its 20ms sleep and reacquires the lock, a concurrent sysfs write
 >to role_store() during this 20ms window will see initialized as false and
 >immediately return -EIO instead of waiting for initialization to complete.

It is better to have it as suggested here, thus I'll adjust it as proposed.

 >> +    if (ret)
 >> +            goto unlock;
 >[ ... ]
 >> @@ -512,6 +556,11 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void 
*_ch)
 >>              goto rpm_put;
 >>
 >>      scoped_guard(spinlock, &ch->lock) {
 >> +            if (READ_ONCE(ch->otg_initializing)) {
 >> +                    dev_warn(dev, "%s: Got IRQ while waiting for OTG 
init!\n", __func__);
 >> +                    return IRQ_NONE;
 >> +            }
 >Does returning early here leak the PM runtime reference?

Indeed, I missed that, will adjust it in the next version.

 >The interrupt handler calls pm_runtime_get_noresume(dev) at the beginning of
 >the function, and releases it at the rpm_put label. By using return IRQ_NONE
 >inside the scoped_guard, the code bypasses pm_runtime_put_noidle() completely,
 >preventing the device from ever entering runtime suspend.
 >Additionally, since this interrupt is requested with IRQF_SHARED, could
 >another device sharing the line trigger a valid interrupt during the 20ms
 >OTG initialization window? If so, this would print a spurious warning to
 >the kernel log for a valid shared interrupt event.

That's a valid point. Better would be to just mask the interrupt locally.

 >> +
 >>              status = readl(usb2_base + USB2_OBINTSTA);
 >>              if (status & ch->phy_data->obint_enable_bits) {
 >>                      dev_vdbg(dev, "%s: %08x\n", __func__, status);
 >[ ... ]
 >> @@ -550,8 +605,22 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
 >>      }
 >>
 >>      /* Initialize otg part (only if we initialize a PHY with IRQs). */
 >> -    if (rphy->int_enable_bits)
 >> -            rcar_gen3_init_otg(channel);
 >> +    if (rphy->int_enable_bits && channel->is_otg_channel &&
 >> +        !rcar_gen3_is_any_otg_rphy_initialized(channel)) {
 >> +            rcar_gen3_init_otg_phase0(channel);
 >> +            disable_irq_nosync(channel->irq);
 >Could channel->irq be 0 here?

In theory, AFAIK, this code should not be executed when the interrupt is not 
present as the OTG PHY needs the interrupt.

 >In rcar_gen3_phy_usb2_probe(), platform_get_irq_optional() is used. If it
 >returns -ENXIO, channel->irq remains 0. Since rphy->int_enable_bits is a
 >statically defined non-zero value for OTG PHYs, this would pass 0 to
 >disable_irq_nosync(), which manipulates critical system interrupts.
 >Also, does calling disable_irq_nosync() on a shared interrupt line starve
 >other devices?
 >Since the interrupt is requested with IRQF_SHARED, masking it at the
 >interrupt controller level means all other unrelated devices on the same
 >physical line will have their interrupts disabled for the full 20ms
 >duration. Should shared interrupts be masked locally at the device level
 >instead?

That would be better, indeed.

 >> +            reinit_completion(&channel->otg_init_done);
 >> +            WRITE_ONCE(channel->otg_initializing, true);
 >> +            spin_unlock_irqrestore(&channel->lock, flags);
 >[ ... ]
 >> @@ -598,6 +678,7 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
 >>      struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
 >>      struct rcar_gen3_chan *channel = rphy->ch;
 >>      void __iomem *usb2_base = channel->base;
 >> +    unsigned long flags;
 >>      u32 val;
 >>      int ret = 0;
 >>
 >> @@ -607,11 +688,15 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
 >>                      return ret;
 >>      }
 >>
 >> -    guard(spinlock_irqsave)(&channel->lock);
 >> +    spin_lock_irqsave(&channel->lock, flags);
 >>
 >>      if (!rcar_gen3_are_all_rphys_power_off(channel))
 >>              goto out;
 >>
 >> +    ret = rcar_gen3_phy_wait_otg_init(channel, &flags);
 >> +    if (ret)
 >> +            goto unlock;
 >> +
 >>      val = readl(usb2_base + USB2_USBCTR);
 >>      val |= USB2_USBCTR_PLL_RST;
 >Does dropping the lock in rcar_gen3_phy_wait_otg_init() introduce a race
 >condition with the !rcar_gen3_are_all_rphys_power_off() check?
 >If two threads concurrently power on different PHYs, both will evaluate the
 >condition as false because neither has reached rphy->powered = true at the
 >end of the function. Both threads might then sleep in
 >rcar_gen3_phy_wait_otg_init().
 >Upon waking and reacquiring the lock, both will unconditionally apply
 >USB2_USBCTR_PLL_RST, which could catastrophically disrupt the PHY that was
 >just initialized by the first thread.
 >Should the power off condition be re-evaluated after the lock is reacquired?

Yes, rcar_gen3_phy_wait_otg_init() should be called first.


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy