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

Hi,
On 8/10/2026 3:55 PM, [email protected] wrote:
> 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?

Thanks for catching this
Will address this in the next version.

Thanks,
Nagendra

>