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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `role_store` function returns incorrect values (-EIO on success, or `count` on failure) due to improperly repurposing the `ret` variable.
- [High] Early returns on timeout in `power_on`, `power_off`, and `exit` bypass critical teardown, leaving resources leaked.
- [High] The top-level common interrupt enable (`UCOM_INTEN`) is left active during the 20ms sleep, which can lead to an unhandled IRQ storm.
- [Medium] The 30ms timeout for `wait_for_completion_timeout()` is far too tight compared to the 20ms `fsleep()`, leading to highly probable spurious timeouts.
- [Medium] Mixing `goto`-based error handling and `scoped_guard()` cleanup within the same function.
--

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

This commit addresses a long delay in atomic context by releasing the spin
lock, sleeping for 20 ms, and using a completion and state flag to handle
concurrent configuration requests.

Link: https://lore.kernel.org/all/afhkX2Ys2BG1gnqy@duo.ucw.cz
Link: 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..4379ea044135a 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
[ ... ]
> @@ -394,26 +402,48 @@ 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;
> +	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;
> +
> +	if (ch->otg_initializing) {
> +		unsigned long timeout = msecs_to_jiffies(30);
> +
> +		spin_unlock_irqrestore(&ch->lock, flags);
> +
> +		ret = wait_for_completion_timeout(&ch->otg_init_done, timeout);
> +		ret = ret ? 0 : -ETIMEDOUT;
> +		if (ret)
> +			return ret;
> +
> +		spin_lock_irqsave(&ch->lock, flags);
> +	}
[ ... ]
> -	return count;
> +unlock:
> +	spin_unlock_irqrestore(&ch->lock, flags);
> +
> +	return ret ?: count;
>  }

[Severity: High]
Is the logic for the return value here inverted? The ret variable starts as
-EIO and is never reassigned on the success path, meaning role_store will
return -EIO on success. Conversely, if wait_for_completion_timeout fails,
ret becomes 0, so the function will incorrectly return count on failure.

[ ... ]
> @@ -512,6 +546,11 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
>  		goto rpm_put;
>  
>  	scoped_guard(spinlock, &ch->lock) {
> +		if (ch->otg_initializing) {
> +			ret = IRQ_NONE;
> +			goto rpm_put;
> +		}
> +

[Severity: Medium]
Is it intentional to mix goto statements with scoped_guard() within this
function? The subsystem guidelines typically prefer either scope-based cleanup
or goto-based cleanup, but mixing them in the same routine is discouraged.

[ ... ]
> +static void rcar_gen3_phy_usb2_irqs_mask_all(struct rcar_gen3_chan *channel,
> +					     u32 *masked_irqs_bits)
> +{
> +	void __iomem *usb2_base = channel->base;
> +	u32 val, bitmask = 0;
> +
> +	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;
> +	val &= ~bitmask;
> +	writel(val, usb2_base + USB2_INT_ENABLE);
> +}

[Severity: High]
Does this adequately mask the top-level common interrupt enable (UCOM_INTEN)?
If an interrupt fires during the 20ms sleep phase, and the handler exits early
with IRQ_NONE when otg_initializing is true, could this cause an unhandled
IRQ storm without clearing the hardware status?

[ ... ]
> @@ -550,8 +634,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);

[Severity: Medium]
Is a 30ms timeout for wait_for_completion_timeout() sufficient across
accessing functions? Considering the background initialization uses
fsleep(20000), which can natively span up to 30ms depending on jiffy
granularity and scheduling jitter, could this result in spurious timeouts?

[ ... ]
> @@ -607,7 +726,20 @@ 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 (channel->otg_initializing) {
> +		unsigned long timeout = msecs_to_jiffies(30);
> +
> +		spin_unlock_irqrestore(&channel->lock, flags);
> +
> +		ret = wait_for_completion_timeout(&channel->otg_init_done, timeout);
> +		ret = ret ? 0 : -ETIMEDOUT;
> +		if (ret)
> +			return ret;
> +
> +		spin_lock_irqsave(&channel->lock, flags);
> +	}

[Severity: High]
Does returning early on timeout bypass the regulator_disable() teardown
at the end of the function, potentially causing a regulator leak? The same
pattern appears to apply to power_off and exit if they rely on final teardown
blocks that this early return avoids.

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

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