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

[PATCH v4] 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.

The OTG can only be initialized once. It is initialized by the first PHY
that calls struct phy_ops::rcar_gen3_phy_usb2_init().

To avoid failures when multiple PHYs call struct
phy_ops::rcar_gen3_phy_usb2_init() simultaneously, and the PHY responsible
for initializing the OTG either fails or deinit quiqly and another PHY
takes over the PHY init role), the code waiting for the
channel->otg_init_done completion retries up to NUM_OF_PHYS times.

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>
---

Changes in v4:
- increase the timeout to 120 ms and added a macro for it USB2_OTG_INIT_TIMEOUT
- add retry mechanism when waiting for otg_init_done completion
- read the status registers in rcar_gen3_phy_usb2_irq() before returning,
  in case an unexpected IRQ is received while waiting for OTG init
- mask channel->phy_data->obint_enable_bits in rcar_gen3_phy_usb2_irqs_mask_all()
- updated the patch description

Changes in v3:
- initialize ret value in role_store()
- jump to exit label in role_store() to have the same exit path
- dropped scoped_guard() in rcar_gen3_phy_usb2_irq() to avoid mixing
  goto statements with scoped_guard()
- take into account USB2_INT_ENABLE_UCOM_INTEN when masking/unmasking IRQs
- increase the completion timeout to 40ms
- in rcar_gen3_phy_usb2_power_on() disable regulator on failure path

Changes in v2:
- dropped the rcar_gen3_phy_wait_otg_init() and used inline the code; with it
  the comment that was previously in rcar_gen3_phy_wait_otg_init() was added
  in struct rcar_gen3_chan for above the org_initializing variable
- dropped READ_ONCE()/WRITE_ONCE() arround struct rcar_gen3_chan::otg_initializing
- checked for struct rcar_gen3_chan::otg_initializing before checking for
  rcar_gen3_is_any_otg_rphy_initialized() in all code places
- added rcar_gen3_phy_usb2_irqs_mask_all() and rcar_gen3_phy_usb2_irqs_unmask()
  to mask/unmask only the interrupts at the controller level since the line is
  shared
- along with it dropped disable_irq_nosync()/enable_irq() from v1 along with
  struct rcar_gen3_chan::irq
- increased the completion timeout to 30 ms

 drivers/phy/renesas/phy-rcar-gen3-usb2.c | 258 +++++++++++++++++++----
 1 file changed, 217 insertions(+), 41 deletions(-)

diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 9a45d840efeb..e1873928cc84 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -106,6 +106,13 @@
 /*  RZ/G2L specific */
 #define USB2_LINECTRL1_USB2_IDMON	BIT(0)
 
+/*
+ * The OTG initialization is expected to finish in 20ms. Choose a large enough
+ * timeout to avoid waiters exit prematurely the waiting section under heavy
+ * CPU load.
+ */
+#define USB2_OTG_INIT_TIMEOUT		msecs_to_jiffies(120)
+
 #define NUM_OF_PHYS			4
 enum rcar_gen3_phy_index {
 	PHY_INDEX_BOTH_HC,
@@ -138,12 +145,20 @@ 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;
 	bool extcon_host;
 	bool is_otg_channel;
 	bool uses_otg_pins;
 	bool otg_internal_reg;
+	/*
+	 * The OTG can be initialized only once and needs to release the spinlock
+	 * and wait for 20 ms due to hardware constraints. If a thread executes
+	 * PHY configuration code while the OTG PHY is waiting for the 20 ms, the
+	 * thread will have to wait for the OTG PHY initialization to complete.
+	 */
+	bool otg_initializing;
 };
 
 struct rcar_gen3_phy_drv_data {
@@ -392,26 +407,52 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr,
 	struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
 	bool is_b_device;
 	enum phy_mode cur_mode, new_mode;
+	int retries = NUM_OF_PHYS;
+	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;
+	if (!ch->is_otg_channel)
+		goto unlock;
+
+	while (retries-- && ch->otg_initializing) {
+		spin_unlock_irqrestore(&ch->lock, flags);
+
+		ret = wait_for_completion_timeout(&ch->otg_init_done,
+						  USB2_OTG_INIT_TIMEOUT);
+		ret = ret ? 0 : -ETIMEDOUT;
+		if (ret && !retries)
+			goto exit;
+
+		spin_lock_irqsave(&ch->lock, flags);
+	}
+
+	if (!rcar_gen3_is_any_otg_rphy_initialized(ch)) {
+		ret = -EIO;
+		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;
+	}
+
+	ret = 0;
 
 	if (new_mode == PHY_MODE_USB_HOST) { /* And is_host must be false */
 		if (!is_b_device)	/* A-Peripheral */
@@ -425,7 +466,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);
+exit:
+	return ret ?: count;
 }
 
 static ssize_t role_show(struct device *dev, struct device_attribute *attr,
@@ -441,14 +485,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 +512,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);
@@ -502,6 +547,7 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
 	void __iomem *usb2_base = ch->base;
 	struct device *dev = ch->dev;
 	irqreturn_t ret = IRQ_NONE;
+	unsigned long flags;
 	u32 status;
 
 	pm_runtime_get_noresume(dev);
@@ -509,33 +555,91 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
 	if (pm_runtime_suspended(dev))
 		goto rpm_put;
 
-	scoped_guard(spinlock, &ch->lock) {
-		status = readl(usb2_base + USB2_OBINTSTA);
-		if (status & ch->phy_data->obint_enable_bits) {
-			dev_vdbg(dev, "%s: %08x\n", __func__, status);
-			if (ch->phy_data->vblvl_ctrl)
-				writel(USB2_OBINTSTA_CLEAR, usb2_base + USB2_OBINTSTA);
-			else
-				writel(ch->phy_data->obint_enable_bits, usb2_base + USB2_OBINTSTA);
-			rcar_gen3_device_recognition(ch);
-			rcar_gen3_configure_vblvl_ctrl(ch);
-			ret = IRQ_HANDLED;
-		}
+	spin_lock_irqsave(&ch->lock, flags);
+
+	status = readl(usb2_base + USB2_OBINTSTA);
+	if (status & ch->phy_data->obint_enable_bits) {
+		dev_vdbg(dev, "%s: %08x\n", __func__, status);
+		if (ch->phy_data->vblvl_ctrl)
+			writel(USB2_OBINTSTA_CLEAR, usb2_base + USB2_OBINTSTA);
+		else
+			writel(ch->phy_data->obint_enable_bits, usb2_base + USB2_OBINTSTA);
+
+		ret = IRQ_HANDLED;
+
+		/* This should not happen! */
+		if (ch->otg_initializing)
+			goto unlock;
+
+		rcar_gen3_device_recognition(ch);
+		rcar_gen3_configure_vblvl_ctrl(ch);
 	}
 
+unlock:
+	spin_unlock_irqrestore(&ch->lock, flags);
 rpm_put:
 	pm_runtime_put_noidle(dev);
 	return ret;
 }
 
+static void rcar_gen3_phy_usb2_irqs_mask_all(struct rcar_gen3_chan *channel,
+					     u32 *masked_irqs_bits)
+{
+	u32 val, bitmask = USB2_INT_ENABLE_UCOM_INTEN;
+	void __iomem *usb2_base = channel->base;
+
+	for (unsigned int i = 0; i < NUM_OF_PHYS; i++)
+		bitmask |= channel->rphys[i].int_enable_bits;
+
+	val = readl(usb2_base + USB2_INT_ENABLE);
+	*masked_irqs_bits = val & bitmask;
+	/*
+	 * Don't report channel->phy_data->obint_enable_bits IRQs. These are
+	 * unmasked anyway in rcar_gen3_init_otg_phase1().
+	 */
+	val &= ~(bitmask | channel->phy_data->obint_enable_bits);
+	writel(val, usb2_base + USB2_INT_ENABLE);
+}
+
+static void rcar_gen3_phy_usb2_irqs_unmask(struct rcar_gen3_chan *channel,
+					   u32 irqs_bits)
+{
+	u32 val, bitmask = USB2_INT_ENABLE_UCOM_INTEN;
+	void __iomem *usb2_base = channel->base;
+
+	for (unsigned int i = 0; i < NUM_OF_PHYS; i++)
+		bitmask |= channel->rphys[i].int_enable_bits;
+
+	val = readl(usb2_base + USB2_INT_ENABLE);
+	val &= ~bitmask;
+	val |= irqs_bits;
+	writel(val, usb2_base + USB2_INT_ENABLE);
+}
+
 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;
+	int retries = NUM_OF_PHYS;
+	unsigned long flags;
 	u32 val;
 
-	guard(spinlock_irqsave)(&channel->lock);
+	spin_lock_irqsave(&channel->lock, flags);
+
+	while (retries-- && channel->otg_initializing) {
+		int ret;
+
+		spin_unlock_irqrestore(&channel->lock, flags);
+
+		ret = wait_for_completion_timeout(&channel->otg_init_done,
+						  USB2_OTG_INIT_TIMEOUT);
+		ret = ret ? 0 : -ETIMEDOUT;
+		if (ret && !retries)
+			return ret;
+
+		spin_lock_irqsave(&channel->lock, flags);
+	}
 
 	/* Initialize USB2 part */
 	val = readl(usb2_base + USB2_INT_ENABLE);
@@ -548,8 +652,24 @@ 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)) {
+		u32 masked_irq_bits = 0;
+
+		rcar_gen3_init_otg_phase0(channel);
+		rcar_gen3_phy_usb2_irqs_mask_all(channel, &masked_irq_bits);
+		reinit_completion(&channel->otg_init_done);
+		channel->otg_initializing = true;
+		spin_unlock_irqrestore(&channel->lock, flags);
+
+		fsleep(20000);
+
+		spin_lock_irqsave(&channel->lock, flags);
+		channel->otg_initializing = false;
+		complete_all(&channel->otg_init_done);
+		rcar_gen3_phy_usb2_irqs_unmask(channel, masked_irq_bits);
+		rcar_gen3_init_otg_phase1(channel);
+	}
 
 	if (channel->phy_data->vblvl_ctrl) {
 		/* SIDDQ mode release */
@@ -568,6 +688,8 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
 
 	rphy->initialized = true;
 
+	spin_unlock_irqrestore(&channel->lock, flags);
+
 	return 0;
 }
 
@@ -576,9 +698,25 @@ 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;
+	int retries = NUM_OF_PHYS;
+	unsigned long flags;
 	u32 val;
 
-	guard(spinlock_irqsave)(&channel->lock);
+	spin_lock_irqsave(&channel->lock, flags);
+
+	while (retries-- && channel->otg_initializing) {
+		int ret;
+
+		spin_unlock_irqrestore(&channel->lock, flags);
+
+		ret = wait_for_completion_timeout(&channel->otg_init_done,
+						  USB2_OTG_INIT_TIMEOUT);
+		ret = ret ? 0 : -ETIMEDOUT;
+		if (ret && !retries)
+			return ret;
+
+		spin_lock_irqsave(&channel->lock, flags);
+	}
 
 	rphy->initialized = false;
 
@@ -588,6 +726,7 @@ static int rcar_gen3_phy_usb2_exit(struct phy *p)
 		val &= ~USB2_INT_ENABLE_UCOM_INTEN;
 	writel(val, usb2_base + USB2_INT_ENABLE);
 
+	spin_unlock_irqrestore(&channel->lock, flags);
 	return 0;
 }
 
@@ -596,6 +735,8 @@ 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;
+	int retries = NUM_OF_PHYS;
+	unsigned long flags;
 	u32 val;
 	int ret = 0;
 
@@ -605,7 +746,19 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
 			return ret;
 	}
 
-	guard(spinlock_irqsave)(&channel->lock);
+	spin_lock_irqsave(&channel->lock, flags);
+
+	while (retries-- && channel->otg_initializing) {
+		spin_unlock_irqrestore(&channel->lock, flags);
+
+		ret = wait_for_completion_timeout(&channel->otg_init_done,
+						  USB2_OTG_INIT_TIMEOUT);
+		ret = ret ? 0 : -ETIMEDOUT;
+		if (ret && !retries)
+			goto disable_regulator;
+
+		spin_lock_irqsave(&channel->lock, flags);
+	}
 
 	if (!rcar_gen3_are_all_rphys_power_off(channel))
 		goto out;
@@ -620,27 +773,49 @@ 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;
+	spin_unlock_irqrestore(&channel->lock, flags);
+
+disable_regulator:
+	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 retries = NUM_OF_PHYS;
+	unsigned long flags;
 	int ret = 0;
 
-	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);
+	while (retries-- && channel->otg_initializing) {
+		spin_unlock_irqrestore(&channel->lock, flags);
 
-			val |= USB2_USBCTR_PLL_RST;
-			writel(val, channel->base + USB2_USBCTR);
-		}
+		ret = wait_for_completion_timeout(&channel->otg_init_done,
+						  USB2_OTG_INIT_TIMEOUT);
+		ret = ret ? 0 : -ETIMEDOUT;
+		if (ret && !retries)
+			return ret;
+
+		spin_lock_irqsave(&channel->lock, flags);
 	}
 
-	if (channel->vbus && !channel->otg_internal_reg)
+	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);
+	}
+
+	spin_unlock_irqrestore(&channel->lock, flags);
+
+	if (!ret && channel->vbus && !channel->otg_internal_reg)
 		ret = regulator_disable(channel->vbus);
 
 	return ret;
@@ -1007,6 +1182,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);
-- 
2.43.0


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

Re: [PATCH v4] 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:

New issues:
- [High] A lingering `-ETIMEDOUT` from a previous loop iteration causes spurious failures and resource leaks in `rcar_gen3_phy_usb2_power_on()` and `rcar_gen3_phy_usb2_power_off()`.
- [High] `rcar_gen3_phy_usb2_irqs_mask_all()` erroneously clears `obint_enable_bits` from `USB2_INT_ENABLE` instead of `USB2_OBINTEN`, failing to mask OTG interrupts and corrupting general interrupt enables.
- [Medium] The OTG initialization retry loop short-circuits on the final retry, allowing concurrent hardware modification if a new initialization phase has started.
- [Medium] Calling `reinit_completion()` while there are active waiters causes a data race on `x->done`, violating the completion API.

Pre-existing issues:
- [High] The interrupt handler uses `pm_runtime_get_noresume()` and `pm_runtime_suspended()` to guard hardware access, which is racy and can lead to invalid register reads.
- [High] The sysfs attributes `role_show` and `role_store` access hardware registers without ensuring the device is actively powered.
- [High] Several paths call `pm_runtime_resume_and_get()` while holding the `ch->lock` spinlock, which can block and trigger a scheduling-while-atomic BUG.
--

--- Patch [1]: [PATCH v4] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context ---
commit 9fdad1bb87b936de49c69272d521e46e26b7e16a
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

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").

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>

> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index 9a45d840efeb2..e1873928cc847 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c

[ ... ]

> @@ -392,26 +407,52 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr,
>  	struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
>  	bool is_b_device;
>  	enum phy_mode cur_mode, new_mode;
> +	int retries = NUM_OF_PHYS;
> +	unsigned long flags;
> +	int ret = -EIO;
>  
> -	guard(spinlock_irqsave)(&ch->lock);
> +	spin_lock_irqsave(&ch->lock, flags);

[Severity: High]
This isn't a bug introduced by this patch, but does this call chain sleep while
holding the spinlock?

role_store() acquires ch->lock via spin_lock_irqsave(), then calls
rcar_gen3_init_from_a_peri_to_a_host(), which can cascade into
rcar_gen3_enable_vbus_ctrl().

For configurations using the internal regulator (where no_adp_ctrl is true),
this calls regulator_hardware_enable(), invoking the driver's .enable callback
(rcar_gen3_phy_usb2_regulator_enable).

That callback unconditionally calls pm_runtime_resume_and_get(), which can sleep
synchronously while waiting for the device to resume. Could this trigger a
scheduling-while-atomic panic?

>  
> -	if (!ch->is_otg_channel || !rcar_gen3_is_any_otg_rphy_initialized(ch))
> -		return -EIO;
> +	if (!ch->is_otg_channel)
> +		goto unlock;
> +
> +	while (retries-- && ch->otg_initializing) {
> +		spin_unlock_irqrestore(&ch->lock, flags);
> +
> +		ret = wait_for_completion_timeout(&ch->otg_init_done,
> +						  USB2_OTG_INIT_TIMEOUT);
> +		ret = ret ? 0 : -ETIMEDOUT;
> +		if (ret && !retries)
> +			goto exit;
> +
> +		spin_lock_irqsave(&ch->lock, flags);
> +	}
> +
> +	if (!rcar_gen3_is_any_otg_rphy_initialized(ch)) {
> +		ret = -EIO;
> +		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);

[Severity: High]
This is a pre-existing issue, but are these hardware register accesses safe
without ensuring the device is actively powered?

role_store() calls rcar_gen3_check_id() and rcar_gen3_get_phy_mode(), which read
hardware registers like USB2_ADPCTRL and USB2_LINECTRL1.

Since these sysfs callbacks don't use pm_runtime_get_sync() before hardware
accesses, could reading these registers while the PHY device is runtime
suspended lead to invalid data reads or trigger asynchronous external aborts?

[ ... ]

> @@ -502,6 +547,7 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
>  	void __iomem *usb2_base = ch->base;
>  	struct device *dev = ch->dev;
>  	irqreturn_t ret = IRQ_NONE;
> +	unsigned long flags;
>  	u32 status;
>  
>  	pm_runtime_get_noresume(dev);
> @@ -509,33 +555,91 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
>  	if (pm_runtime_suspended(dev))
>  		goto rpm_put;

[Severity: High]
This is a pre-existing issue, but does this runtime PM check protect against
concurrent suspend transitions?

The handler uses pm_runtime_get_noresume() which increments the usage counter
but does not check the status atomically. If the device uses a shared IRQ line
and another device triggers an interrupt while this device is in the
RPM_SUSPENDING state, pm_runtime_suspended() will return false.

Could the handler then proceed to read hardware registers that are actively
being powered down, leading to invalid reads (e.g., 0xffffffff) and spurious
interrupt processing? Should this use pm_runtime_get_if_active() instead?

[ ... ]

> +static void rcar_gen3_phy_usb2_irqs_mask_all(struct rcar_gen3_chan *channel,
> +					     u32 *masked_irqs_bits)
> +{
> +	u32 val, bitmask = USB2_INT_ENABLE_UCOM_INTEN;
> +	void __iomem *usb2_base = channel->base;
> +
> +	for (unsigned int i = 0; i < NUM_OF_PHYS; i++)
> +		bitmask |= channel->rphys[i].int_enable_bits;
> +
> +	val = readl(usb2_base + USB2_INT_ENABLE);
> +	*masked_irqs_bits = val & bitmask;
> +	/*
> +	 * Don't report channel->phy_data->obint_enable_bits IRQs. These are
> +	 * unmasked anyway in rcar_gen3_init_otg_phase1().
> +	 */
> +	val &= ~(bitmask | channel->phy_data->obint_enable_bits);
> +	writel(val, usb2_base + USB2_INT_ENABLE);
> +}

[Severity: High]
Does this mistakenly apply the obint_enable_bits to the wrong register?

This function reads the USB2_INT_ENABLE register, but obint_enable_bits belong
to the USB2_OBINTEN register.

When rcar_gen3_phy_usb2_irqs_unmask() is called later, it only restores bits
defined in bitmask, which doesn't include the obint bits. Will this leave the
corrupted bits in USB2_INT_ENABLE cleared permanently and fail to mask OTG
interrupts during the initialization window?

[ ... ]

>  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;
> +	int retries = NUM_OF_PHYS;
> +	unsigned long flags;
>  	u32 val;
>  
> -	guard(spinlock_irqsave)(&channel->lock);
> +	spin_lock_irqsave(&channel->lock, flags);
> +
> +	while (retries-- && channel->otg_initializing) {

[Severity: Medium]
Is there a race condition when the retries counter hits zero?

On the final loop iteration (when retries evaluates to 0 inside the loop), if
wait_for_completion_timeout() succeeds without timing out, the loop condition
will be evaluated again.

Because retries is 0, the post-decrement operator returns 0, causing the &&
to short-circuit. The channel->otg_initializing variable is not checked, and
the loop exits.

If another thread had just started a new initialization phase and set
otg_initializing = true, could the exiting thread proceed to modify hardware
concurrently?

> +		int ret;
> +
> +		spin_unlock_irqrestore(&channel->lock, flags);
> +
> +		ret = wait_for_completion_timeout(&channel->otg_init_done,
> +						  USB2_OTG_INIT_TIMEOUT);
> +		ret = ret ? 0 : -ETIMEDOUT;
> +		if (ret && !retries)
> +			return ret;
> +
> +		spin_lock_irqsave(&channel->lock, flags);
> +	}

[ ... ]

>  	/* 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)) {
> +		u32 masked_irq_bits = 0;
> +
> +		rcar_gen3_init_otg_phase0(channel);
> +		rcar_gen3_phy_usb2_irqs_mask_all(channel, &masked_irq_bits);
> +		reinit_completion(&channel->otg_init_done);
> +		channel->otg_initializing = true;

[Severity: Medium]
Is it safe to reinitialize the completion here?

Waiters from a previous wait_for_completion_timeout() might still be inside
wait_for_common() processing their wakeup.

Since the completion API mandates that reinit_completion() must only be called
when there are no active waiters, could this cause a data race on x->done and
lead to unpredictable wake-up behavior for prior waiters?

[ ... ]

>  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;
> +	int retries = NUM_OF_PHYS;
> +	unsigned long flags;
>  	u32 val;
>  	int ret = 0;
>  
> @@ -605,7 +746,19 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
>  			return ret;
>  	}
>  
> -	guard(spinlock_irqsave)(&channel->lock);
> +	spin_lock_irqsave(&channel->lock, flags);
> +
> +	while (retries-- && channel->otg_initializing) {
> +		spin_unlock_irqrestore(&channel->lock, flags);
> +
> +		ret = wait_for_completion_timeout(&channel->otg_init_done,
> +						  USB2_OTG_INIT_TIMEOUT);
> +		ret = ret ? 0 : -ETIMEDOUT;
> +		if (ret && !retries)
> +			goto disable_regulator;
> +
> +		spin_lock_irqsave(&channel->lock, flags);
> +	}
>  
>  	if (!rcar_gen3_are_all_rphys_power_off(channel))
>  		goto out;
> @@ -620,27 +773,49 @@ 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;
> +	spin_unlock_irqrestore(&channel->lock, flags);
> +
> +disable_regulator:
> +	if (ret && channel->vbus && !channel->otg_internal_reg)
> +		regulator_disable(channel->vbus);
> +
> +	return ret;
>  }

[Severity: High]
Could a lingering -ETIMEDOUT from the previous loop iteration cause a spurious
failure here?

If a thread enters the wait loop while another thread is executing the OTG
initialization, wait_for_completion_timeout() might time out at least once,
setting ret to -ETIMEDOUT.

If the OTG initialization completes before the next loop condition evaluation,
channel->otg_initializing becomes false and the loop exits normally.

However, ret retains the stale -ETIMEDOUT value. In power_on(), this causes the
disable_regulator cleanup path to execute despite success.

Should ret be reset to 0 after exiting the wait loop?

[ ... ]

>  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 retries = NUM_OF_PHYS;
> +	unsigned long flags;
>  	int ret = 0;
>  
> -	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);
> +	while (retries-- && channel->otg_initializing) {
> +		spin_unlock_irqrestore(&channel->lock, flags);
>  
> -			val |= USB2_USBCTR_PLL_RST;
> -			writel(val, channel->base + USB2_USBCTR);
> -		}
> +		ret = wait_for_completion_timeout(&channel->otg_init_done,
> +						  USB2_OTG_INIT_TIMEOUT);
> +		ret = ret ? 0 : -ETIMEDOUT;
> +		if (ret && !retries)
> +			return ret;
> +
> +		spin_lock_irqsave(&channel->lock, flags);
>  	}
>  
> -	if (channel->vbus && !channel->otg_internal_reg)
> +	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);
> +	}
> +
> +	spin_unlock_irqrestore(&channel->lock, flags);
> +
> +	if (!ret && channel->vbus && !channel->otg_internal_reg)
>  		ret = regulator_disable(channel->vbus);
>  
>  	return ret;
>  }

[Severity: High]
Similar to power_on(), could a stale -ETIMEDOUT in ret from the wait loop
prevent the regulator from being disabled here, leaking the resource?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709140257.843685-1-claudiu.beznea+renesas@tuxon.dev?part=1

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