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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unsafe Runtime PM checking in the shared interrupt handler allows potential hardware access to a suspended device.
- [High] Missing cancel_work_sync() in the device removal path leads to a Use-After-Free on the work item.
--

--- Patch [1]: [PATCH v5] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context ---
commit f2213f45df6955a79ff35919d521e08aa0971e01
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. This patch releases the spin lock before sleeping and reacquires it
afterwards, introducing an otg_initializing variable to avoid other threads
entering the critical section.

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 9a45d840efeb2..ceaac691ec891 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c

[ ... ]

> @@ -502,6 +554,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 +562,102 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
>  	if (pm_runtime_suspended(dev))
>  		goto rpm_put;

[Severity: High]
This isn't a bug introduced by this patch, but does using
pm_runtime_get_noresume() here allow the IRQ handler to access powered-down
hardware?

If this device shares an IRQ line with another active device, and the other
device triggers an interrupt while this USB PHY device is in the process of
runtime suspending (RPM_SUSPENDING), pm_runtime_suspended() will return false.

This allows the handler to proceed and read USB2_OBINTSTA while the hardware
is actively powering down. Invalid register reads from suspended hardware
often return ~0u, which in a shared IRQ handler leads to spurious interrupt
handling and potentially causes an IRQ storm.

Could this be updated to use pm_runtime_get_if_active() to safely check the
active status before proceeding?

[ ... ]

While reviewing the file, I also noticed the following pre-existing issue in
the removal path:

drivers/phy/renesas/phy-rcar-gen3-usb2.c:rcar_gen3_phy_usb2_remove() {
    struct rcar_gen3_chan *channel = platform_get_drvdata(pdev);

    if (channel->is_otg_channel)
        device_remove_file(&pdev->dev, &dev_attr_role);
}

[Severity: High]
This is a pre-existing issue, but does the device removal path fail to cancel
the pending work item?

If a role switch event occurs and schedule_work(&ch->work) is called just
before the driver is unbound, rcar_gen3_phy_usb2_remove() will execute and
return without cancelling the work.

Devres cleanup will then free the channel memory structure. When the
workqueue dispatches rcar_gen3_phy_usb2_work(), it will access ch->extcon
on the freed channel object, causing a use-after-free.

Should a call to cancel_work_sync(&channel->work) be added to the remove
function?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716183246.3183877-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