Re: [PATCH v2] phy: renesas: phy-rzg3e-usb3: Use SYSTEM_SLEEP_PM_OPS instead of NOIRQ

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver manually manipulates the runtime PM usage count inside sleep callbacks, which defeats PM core synchronization and causes underflows on error paths.
- [High] Asynchronous pm_runtime_put() is used immediately before asserting a hardware reset, violating hardware ordering constraints and causing race conditions.
- [High] Flawed skip_reinit state machine leaves the PHY uninitialized if it runtime suspends after system resume.
--

commit cdc6f90932d427eedd494d80964982fd1f09abcb
Author: Ovidiu Panait <ovidiu.panait.rb@renesas.com>

phy: renesas: phy-rzg3e-usb3: Use SYSTEM_SLEEP_PM_OPS instead of NOIRQ

This patch moves the suspend/resume callbacks out of the noirq phase by
switching from NOIRQ_SYSTEM_SLEEP_PM_OPS to SYSTEM_SLEEP_PM_OPS. This aims
to fix a PM usage count underflow on suspend when the xhci driver is
unbound.

> diff --git a/drivers/phy/renesas/phy-rzg3e-usb3.c b/drivers/phy/renesas/phy-rzg3e-usb3.c
> index 6b3453ea0004c..7867ca3bc3ab2 100644
> --- a/drivers/phy/renesas/phy-rzg3e-usb3.c
> +++ b/drivers/phy/renesas/phy-rzg3e-usb3.c
> @@ -235,7 +235,7 @@ static int rzg3e_phy_usb3_resume(struct device *dev)
>  }
>  
>  static const struct dev_pm_ops rzg3e_phy_usb3_pm = {
> -	NOIRQ_SYSTEM_SLEEP_PM_OPS(rzg3e_phy_usb3_suspend, rzg3e_phy_usb3_resume)
> +	SYSTEM_SLEEP_PM_OPS(rzg3e_phy_usb3_suspend, rzg3e_phy_usb3_resume)
>  };

[Severity: High]
This isn't a bug introduced by this patch, but modifying these callbacks
brings up a preexisting issue. Does the manual manipulation of runtime PM
usage counts inside these sleep callbacks defeat the PM core's
synchronization?

The PM core takes a runtime PM reference on all devices during
device_prepare() to prevent concurrent transitions during system sleep, and
releases it in device_complete().

If rzg3e_phy_usb3_suspend() manually drops this reference, and
rzg3e_phy_usb3_resume() attempts to reacquire it but fails early on an error
path:

static int rzg3e_phy_usb3_resume(struct device *dev)
{
        ...
        ret = reset_control_deassert(r->rstc);
        if (ret)
                return ret;

        ret = pm_runtime_resume_and_get(dev);
        ...

Will this cause another negative usage count underflow when the PM core
unconditionally calls device_complete() and executes its own
pm_runtime_put()?

[Severity: High]
This is a pre-existing issue, but does the suspend path introduce a race
condition by using an asynchronous runtime put just before asserting the
hardware reset?

In rzg3e_phy_usb3_suspend(), the driver drops the runtime PM reference via
pm_runtime_put() and then immediately asserts the hardware reset line.
Because pm_runtime_put() is asynchronous, could the generic PM domain be
powered off concurrently with or immediately after the hardware reset is
synchronously asserted?

[Severity: High]
This is a pre-existing issue, but does the skip_reinit state machine leave
the PHY uninitialized if it runtime suspends after system resume?

During system resume, rzg3e_phy_usb3_resume() unconditionally sets
r->skip_reinit = true. This flag is only cleared during system suspend.

If the PHY consumer unbinds or runtime suspends, the PHY loses its hardware
state. When the consumer is later bound or runtime resumed, it calls
phy_init() to reinitialize. Since r->skip_reinit is still true, will
phy_init() skip the initialization sequence and leave the hardware broken
until the next full system suspend cycle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527082052.117289-1-ovidiu.panait.rb@renesas.com?part=1

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