Re: [PATCH V3 2/6] PM: runtime: Add pm_runtime_if_active guard and conditional variant

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `pm_runtime_if_active_try` guard evaluates to an error when runtime PM is disabled, violating the API contract and breaking IRQ handlers.
- [Medium] The conditionally defined base guard `guard(pm_runtime_if_active)` silently corrupts the runtime PM usage counter if used directly.
--

--- Patch [2]: [PATCH V3 2/6] PM: runtime: Add pm_runtime_if_active guard and conditional variant ---
commit 57173006d8cc8596d1863023936dfd41e4325af1
Author: Golla Nagendra <[email protected]>

PM: runtime: Add pm_runtime_if_active guard and conditional variant

Add a pm_runtime_if_active base guard and its _try conditional variant
to pm_runtime.h for drivers that need to conditionally acquire a runtime
PM reference only when the device is already active.

> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -609,6 +609,13 @@ DEFINE_GUARD(pm_runtime_active, struct device *,
>  	     pm_runtime_get_sync(_T), pm_runtime_put(_T));
>  DEFINE_GUARD(pm_runtime_active_auto, struct device *,
>  	     pm_runtime_get_sync(_T), pm_runtime_put_autosuspend(_T));
> +/*
> + * Do not use directly -- the destructor calls pm_runtime_put()
> + * unconditionally, which underflows if no reference was acquired.
> + * Use only via the _try variant below.
> + */
> +DEFINE_GUARD(pm_runtime_if_active, struct device *,
> +	     pm_runtime_get_if_active(_T), pm_runtime_put(_T));

[Severity: Medium]
While the comment warns against direct usage, does exposing this base
guard via DEFINE_GUARD in include/linux/pm_runtime.h create a risky API
footgun?

If a developer uses guard(pm_runtime_if_active)(&dev) directly and the
device is not active, pm_runtime_get_if_active() returns 0 without
incrementing the usage counter. At scope exit, the destructor
unconditionally calls pm_runtime_put(), which would underflow the usage
counter and could prevent the device from suspending.

Could a lock-tracking guard like DEFINE_LOCK_GUARD_1 be used instead to
avoid defining an unsafe base guard?

>  /*
>   * Use the following guards with ACQUIRE()/ACQUIRE_ERR().
>   *
> @@ -624,6 +631,8 @@ DEFINE_GUARD_COND(pm_runtime_active_auto, _try,
>  		  pm_runtime_get_active(_T, RPM_TRANSPARENT), _RET == 0)
>  DEFINE_GUARD_COND(pm_runtime_active_auto, _try_enabled,
>  		  pm_runtime_resume_and_get(_T), _RET == 0)
> +DEFINE_GUARD_COND(pm_runtime_if_active, _try,
> +		  pm_runtime_get_if_active(_T) ?: -EAGAIN, _RET == 1)

[Severity: High]
Does this strict condition break interrupt handlers when runtime PM is
disabled?

If CONFIG_PM is disabled (or runtime PM is not enabled for the device),
pm_runtime_get_if_active() returns -EINVAL. With the _RET == 1 check
in DEFINE_GUARD_COND, the guard condition fails and evaluates to
ERR_PTR(-EINVAL).

If a driver uses PM_RUNTIME_ACQUIRE_IF_ACTIVE() in an interrupt handler
on such a system, the handler will bail out instead of servicing the valid
interrupt. This can lead to an unhandled IRQ storm that permanently
disables the device's interrupt line.

The documented API contract for _try guards states they do not produce
an error when runtime PM is disabled. Should this handle the -EINVAL
case to safely bypass the guard when PM is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2