[PATCH v1 0/3] PM: runtimePCI/ACPI: TAD: Auto-cleanup macros for runtime PM

[PATCH v1 0/3] PM: runtimePCI/ACPI: TAD: Auto-cleanup macros for runtime PM

[Rafael J. Wysocki]

Hi All,

This series introduces a helper macro on top of the recently added runtime PM
usage counter guard definitions, uses it in the PCI sysfs code (patch [1/3])
and modifies the runtime PM usage counter handling in the ACPI TAD driver
with the help of it (patch [3/3]).  Patch [3/2] is a preparation for the
latter.

Thanks!

[PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There appears to be an emerging pattern in which guard
pm_runtime_active_try is used for resuming the given device and
incrementing its runtime PM usage counter if the resume has been
successful, that is followed by an ACQUIRE_ERR() check on the guard
variable and if that triggers, a specific error code is returned, for
example:

	ACQUIRE(pm_runtime_active_try, pm)(dev);
	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
		return -ENXIO

Introduce a macro called PM_RUNTIME_ACQUIRE_OR_FAIL() representing the
above sequence of statements that can be used to avoid code duplication
wherever that sequence would be used.

Use this macro right away in the PCI sysfs code where the above pattern
is already present.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Admittedly, the new macro is slightly on the edge, but it really helps
reduce code duplication, so here it goes.

Any feedback (good, bad, ugly) will be appreciated.

Thanks!

---
 drivers/pci/pci-sysfs.c    |    4 +---
 include/linux/pm_runtime.h |    5 +++++
 2 files changed, 6 insertions(+), 3 deletions(-)

--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1517,9 +1517,7 @@ static ssize_t reset_method_store(struct
 		return count;
 	}
 
-	ACQUIRE(pm_runtime_active_try, pm)(dev);
-	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
-		return -ENXIO;
+	PM_RUNTIME_ACQUIRE_OR_FAIL(dev, -ENXIO);
 
 	if (sysfs_streq(buf, "default")) {
 		pci_init_reset_methods(pdev);
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -637,6 +637,11 @@ DEFINE_GUARD_COND(pm_runtime_active_auto
 DEFINE_GUARD_COND(pm_runtime_active_auto, _try_enabled,
 		  pm_runtime_resume_and_get(_T))
 
+#define PM_RUNTIME_ACQUIRE_OR_FAIL(dev, error)		\
+	ACQUIRE(pm_runtime_active_try, __pm)(dev);	\
+	if (ACQUIRE_ERR(pm_runtime_active_try, &__pm))	\
+		return error
+
 /**
  * pm_runtime_put_sync - Drop device usage counter and run "idle check" if 0.
  * @dev: Target device.

[PATCH v1 2/3] ACPI: TAD: Rearrange runtime PM operations in acpi_tad_remove()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It is not necessary to resume the device upfront in acpi_tad_remove()
because both acpi_tad_disable_timer() and acpi_tad_clear_status()
attempt to resume it, but it is better to prevent it from suspending
between these calls by incrementing its runtime PM usage counter.

Accordingly, replace the pm_runtime_get_sync() call in acpi_tad_remove()
with a pm_runtime_get_noresume() one and put the latter right before the
first invocation of acpi_tad_disable_timer().

In addition, use pm_runtime_put_noidle() to drop the device's runtime
PM usage counter after using pm_runtime_get_noresume() to bump it up
to follow a common pattern and use pm_runtime_suspend() for suspending
the device afterward.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/acpi_tad.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

--- a/drivers/acpi/acpi_tad.c
+++ b/drivers/acpi/acpi_tad.c
@@ -563,8 +563,6 @@ static void acpi_tad_remove(struct platf
 
 	device_init_wakeup(dev, false);
 
-	pm_runtime_get_sync(dev);
-
 	if (dd->capabilities & ACPI_TAD_RT)
 		sysfs_remove_group(&dev->kobj, &acpi_tad_time_attr_group);
 
@@ -573,6 +571,8 @@ static void acpi_tad_remove(struct platf
 
 	sysfs_remove_group(&dev->kobj, &acpi_tad_attr_group);
 
+	pm_runtime_get_noresume(dev);
+
 	acpi_tad_disable_timer(dev, ACPI_TAD_AC_TIMER);
 	acpi_tad_clear_status(dev, ACPI_TAD_AC_TIMER);
 	if (dd->capabilities & ACPI_TAD_DC_WAKE) {
@@ -580,7 +580,8 @@ static void acpi_tad_remove(struct platf
 		acpi_tad_clear_status(dev, ACPI_TAD_DC_TIMER);
 	}
 
-	pm_runtime_put_sync(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_suspend(dev);
 	pm_runtime_disable(dev);
 	acpi_remove_cmos_rtc_space_handler(handle);
 }

[PATCH v1 3/3] ACPI: TAD: Improve runtime PM using guard macros

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Use guard pm_runtime_active_try to simplify runtime PM cleanup and
implement runtime resume error handling in multiple places.

Also use guard pm_runtime_noresume to simplify acpi_tad_remove().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/acpi_tad.c |   45 +++++++++++++--------------------------------
 1 file changed, 13 insertions(+), 32 deletions(-)

--- a/drivers/acpi/acpi_tad.c
+++ b/drivers/acpi/acpi_tad.c
@@ -90,12 +90,9 @@ static int acpi_tad_set_real_time(struct
 	args[0].buffer.pointer = (u8 *)rt;
 	args[0].buffer.length = sizeof(*rt);
 
-	pm_runtime_get_sync(dev);
+	PM_RUNTIME_ACQUIRE_OR_FAIL(dev, -ENXIO);
 
 	status = acpi_evaluate_integer(handle, "_SRT", &arg_list, &retval);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status) || retval)
 		return -EIO;
 
@@ -111,12 +108,9 @@ static int acpi_tad_get_real_time(struct
 	acpi_status status;
 	int ret = -EIO;
 
-	pm_runtime_get_sync(dev);
+	PM_RUNTIME_ACQUIRE_OR_FAIL(dev, -ENXIO);
 
 	status = acpi_evaluate_object(handle, "_GRT", NULL, &output);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status))
 		goto out_free;
 
@@ -266,12 +260,9 @@ static int acpi_tad_wake_set(struct devi
 	args[0].integer.value = timer_id;
 	args[1].integer.value = value;
 
-	pm_runtime_get_sync(dev);
+	PM_RUNTIME_ACQUIRE_OR_FAIL(dev, -ENXIO);
 
 	status = acpi_evaluate_integer(handle, method, &arg_list, &retval);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status) || retval)
 		return -EIO;
 
@@ -314,12 +305,9 @@ static ssize_t acpi_tad_wake_read(struct
 
 	args[0].integer.value = timer_id;
 
-	pm_runtime_get_sync(dev);
+	PM_RUNTIME_ACQUIRE_OR_FAIL(dev, -ENXIO);
 
 	status = acpi_evaluate_integer(handle, method, &arg_list, &retval);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status))
 		return -EIO;
 
@@ -370,12 +358,9 @@ static int acpi_tad_clear_status(struct
 
 	args[0].integer.value = timer_id;
 
-	pm_runtime_get_sync(dev);
+	PM_RUNTIME_ACQUIRE_OR_FAIL(dev, -ENXIO);
 
 	status = acpi_evaluate_integer(handle, "_CWS", &arg_list, &retval);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status) || retval)
 		return -EIO;
 
@@ -411,12 +396,9 @@ static ssize_t acpi_tad_status_read(stru
 
 	args[0].integer.value = timer_id;
 
-	pm_runtime_get_sync(dev);
+	PM_RUNTIME_ACQUIRE_OR_FAIL(dev, -ENXIO);
 
 	status = acpi_evaluate_integer(handle, "_GWS", &arg_list, &retval);
-
-	pm_runtime_put_sync(dev);
-
 	if (ACPI_FAILURE(status))
 		return -EIO;
 
@@ -571,16 +553,15 @@ static void acpi_tad_remove(struct platf
 
 	sysfs_remove_group(&dev->kobj, &acpi_tad_attr_group);
 
-	pm_runtime_get_noresume(dev);
-
-	acpi_tad_disable_timer(dev, ACPI_TAD_AC_TIMER);
-	acpi_tad_clear_status(dev, ACPI_TAD_AC_TIMER);
-	if (dd->capabilities & ACPI_TAD_DC_WAKE) {
-		acpi_tad_disable_timer(dev, ACPI_TAD_DC_TIMER);
-		acpi_tad_clear_status(dev, ACPI_TAD_DC_TIMER);
+	scoped_guard(pm_runtime_noresume, dev) {
+		acpi_tad_disable_timer(dev, ACPI_TAD_AC_TIMER);
+		acpi_tad_clear_status(dev, ACPI_TAD_AC_TIMER);
+		if (dd->capabilities & ACPI_TAD_DC_WAKE) {
+			acpi_tad_disable_timer(dev, ACPI_TAD_DC_TIMER);
+			acpi_tad_clear_status(dev, ACPI_TAD_DC_TIMER);
+		}
 	}
 
-	pm_runtime_put_noidle(dev);
 	pm_runtime_suspend(dev);
 	pm_runtime_disable(dev);
 	acpi_remove_cmos_rtc_space_handler(handle);

Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

[Jonathan Cameron]

On Wed, 15 Oct 2025 16:02:02 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> There appears to be an emerging pattern in which guard
> pm_runtime_active_try is used for resuming the given device and
> incrementing its runtime PM usage counter if the resume has been
> successful, that is followed by an ACQUIRE_ERR() check on the guard
> variable and if that triggers, a specific error code is returned, for
> example:
> 
> 	ACQUIRE(pm_runtime_active_try, pm)(dev);
> 	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> 		return -ENXIO
> 
> Introduce a macro called PM_RUNTIME_ACQUIRE_OR_FAIL() representing the
> above sequence of statements that can be used to avoid code duplication
> wherever that sequence would be used.
> 
> Use this macro right away in the PCI sysfs code where the above pattern
> is already present.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Admittedly, the new macro is slightly on the edge, but it really helps
> reduce code duplication, so here it goes.

Fully agree with the 'on the edge'. 

This looks somewhat like the some of the earlier attempts to come up with
a general solution before ACQUIRE().  Linus was fairly clear on his opinion of
a proposal that looked a bit similar to this
cond_guard(mutex_intr, return -EINTR, &mutex);

https://lore.kernel.org/all/CAHk-=win7bwWhPJ=iuW4h-sDTqbX6v9_LJnMaO3KxVfPSs81bQ@mail.gmail.com/

+CC a few people who might have better memories of where things went than I do.

The solution you have here has the benefit of clarity that all it can do is
return the error code.

Thanks,

Jonathan


> 
> Any feedback (good, bad, ugly) will be appreciated.
> 
> Thanks!
> 
> ---
>  drivers/pci/pci-sysfs.c    |    4 +---
>  include/linux/pm_runtime.h |    5 +++++
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1517,9 +1517,7 @@ static ssize_t reset_method_store(struct
>  		return count;
>  	}
>  
> -	ACQUIRE(pm_runtime_active_try, pm)(dev);
> -	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> -		return -ENXIO;
> +	PM_RUNTIME_ACQUIRE_OR_FAIL(dev, -ENXIO);
>  
>  	if (sysfs_streq(buf, "default")) {
>  		pci_init_reset_methods(pdev);
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -637,6 +637,11 @@ DEFINE_GUARD_COND(pm_runtime_active_auto
>  DEFINE_GUARD_COND(pm_runtime_active_auto, _try_enabled,
>  		  pm_runtime_resume_and_get(_T))
>  
> +#define PM_RUNTIME_ACQUIRE_OR_FAIL(dev, error)		\
> +	ACQUIRE(pm_runtime_active_try, __pm)(dev);	\
> +	if (ACQUIRE_ERR(pm_runtime_active_try, &__pm))	\
> +		return error
> +
>  /**
>   * pm_runtime_put_sync - Drop device usage counter and run "idle check" if 0.
>   * @dev: Target device.
> 
> 
> 

Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

[Rafael J. Wysocki]

On Thu, Oct 16, 2025 at 2:39 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Wed, 15 Oct 2025 16:02:02 +0200
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > There appears to be an emerging pattern in which guard
> > pm_runtime_active_try is used for resuming the given device and
> > incrementing its runtime PM usage counter if the resume has been
> > successful, that is followed by an ACQUIRE_ERR() check on the guard
> > variable and if that triggers, a specific error code is returned, for
> > example:
> >
> >       ACQUIRE(pm_runtime_active_try, pm)(dev);
> >       if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> >               return -ENXIO
> >
> > Introduce a macro called PM_RUNTIME_ACQUIRE_OR_FAIL() representing the
> > above sequence of statements that can be used to avoid code duplication
> > wherever that sequence would be used.
> >
> > Use this macro right away in the PCI sysfs code where the above pattern
> > is already present.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > Admittedly, the new macro is slightly on the edge, but it really helps
> > reduce code duplication, so here it goes.
>
> Fully agree with the 'on the edge'.
>
> This looks somewhat like the some of the earlier attempts to come up with
> a general solution before ACQUIRE().  Linus was fairly clear on his opinion of
> a proposal that looked a bit similar to this
> cond_guard(mutex_intr, return -EINTR, &mutex);
>
> https://lore.kernel.org/all/CAHk-=win7bwWhPJ=iuW4h-sDTqbX6v9_LJnMaO3KxVfPSs81bQ@mail.gmail.com/
>
> +CC a few people who might have better memories of where things went than I do.
>
> The solution you have here has the benefit of clarity that all it can do is
> return the error code.

Well, I could call the macro PM_RUNTIME_ACQUIRE_OR_RETURN_ERROR(), but
FAIL is just shorter. :-)

Seriously though, the odd syntax bothers me, but it has come from
looking at the multiple pieces of code that otherwise would have
repeated exactly the same code pattern including the guard name in two
places and the pm variable that has no role beyond guarding.

> > ---
> >  drivers/pci/pci-sysfs.c    |    4 +---
> >  include/linux/pm_runtime.h |    5 +++++
> >  2 files changed, 6 insertions(+), 3 deletions(-)
> >
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -1517,9 +1517,7 @@ static ssize_t reset_method_store(struct
> >               return count;
> >       }
> >
> > -     ACQUIRE(pm_runtime_active_try, pm)(dev);
> > -     if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> > -             return -ENXIO;
> > +     PM_RUNTIME_ACQUIRE_OR_FAIL(dev, -ENXIO);
> >
> >       if (sysfs_streq(buf, "default")) {
> >               pci_init_reset_methods(pdev);
> > --- a/include/linux/pm_runtime.h
> > +++ b/include/linux/pm_runtime.h
> > @@ -637,6 +637,11 @@ DEFINE_GUARD_COND(pm_runtime_active_auto
> >  DEFINE_GUARD_COND(pm_runtime_active_auto, _try_enabled,
> >                 pm_runtime_resume_and_get(_T))
> >
> > +#define PM_RUNTIME_ACQUIRE_OR_FAIL(dev, error)               \
> > +     ACQUIRE(pm_runtime_active_try, __pm)(dev);      \
> > +     if (ACQUIRE_ERR(pm_runtime_active_try, &__pm))  \
> > +             return error
> > +
> >  /**
> >   * pm_runtime_put_sync - Drop device usage counter and run "idle check" if 0.
> >   * @dev: Target device.
> >
> >
> >
>

Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

[Takashi Iwai]

On Thu, 16 Oct 2025 15:46:08 +0200,
Rafael J. Wysocki wrote:
> 
> On Thu, Oct 16, 2025 at 2:39 PM Jonathan Cameron
> <jonathan.cameron@huawei.com> wrote:
> >
> > On Wed, 15 Oct 2025 16:02:02 +0200
> > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > There appears to be an emerging pattern in which guard
> > > pm_runtime_active_try is used for resuming the given device and
> > > incrementing its runtime PM usage counter if the resume has been
> > > successful, that is followed by an ACQUIRE_ERR() check on the guard
> > > variable and if that triggers, a specific error code is returned, for
> > > example:
> > >
> > >       ACQUIRE(pm_runtime_active_try, pm)(dev);
> > >       if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> > >               return -ENXIO
> > >
> > > Introduce a macro called PM_RUNTIME_ACQUIRE_OR_FAIL() representing the
> > > above sequence of statements that can be used to avoid code duplication
> > > wherever that sequence would be used.
> > >
> > > Use this macro right away in the PCI sysfs code where the above pattern
> > > is already present.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >
> > > Admittedly, the new macro is slightly on the edge, but it really helps
> > > reduce code duplication, so here it goes.
> >
> > Fully agree with the 'on the edge'.
> >
> > This looks somewhat like the some of the earlier attempts to come up with
> > a general solution before ACQUIRE().  Linus was fairly clear on his opinion of
> > a proposal that looked a bit similar to this
> > cond_guard(mutex_intr, return -EINTR, &mutex);
> >
> > https://lore.kernel.org/all/CAHk-=win7bwWhPJ=iuW4h-sDTqbX6v9_LJnMaO3KxVfPSs81bQ@mail.gmail.com/
> >
> > +CC a few people who might have better memories of where things went than I do.
> >
> > The solution you have here has the benefit of clarity that all it can do is
> > return the error code.
> 
> Well, I could call the macro PM_RUNTIME_ACQUIRE_OR_RETURN_ERROR(), but
> FAIL is just shorter. :-)
> 
> Seriously though, the odd syntax bothers me, but it has come from
> looking at the multiple pieces of code that otherwise would have
> repeated exactly the same code pattern including the guard name in two
> places and the pm variable that has no role beyond guarding.

While I see the benefit of simplification, IMO, embedding a code
flow control inside the macro argument makes it really harder to
follow.

Is the problem about the messy ACQUIRE_ERR() invocation?  If so, it
could be replaced with something shorter (and without extra type),
e.g. replace 
	ret = ACQUIRE_ERR(pm_runtime_active_try, &pm);
with
	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);

Since all runtime PM guard usage is to the same object, we can have a
common macro.

Also, in the past, I thought of a macro like below that stores the
error code in the given variable ret:

#define __guard_cond_ret(_name, _var, _ret, _args)	\
	CLASS(_name, _var)(_args);			\
	(_ret) = __guard_err(_name)(&_var)
#define guard_cond_ret(_name, _ret, _args) \
	__guard_cond_ret(_name, __UNIQUE_ID(guard), _ret, _args)

... so that it'd work for runtime PM like:

	int ret;

	guard_cond_ret(pm_runtime_active, ret)(dev);
	if (ret)
		return ret;
	
Of course, a clear drawback is that the assignment of ret isn't
obvious, but the code flow isn't skewed much in this way.


thanks,

Takashi

Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

[Rafael J. Wysocki]

On Thu, Oct 16, 2025 at 4:59 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Thu, 16 Oct 2025 15:46:08 +0200,
> Rafael J. Wysocki wrote:
> >
> > On Thu, Oct 16, 2025 at 2:39 PM Jonathan Cameron
> > <jonathan.cameron@huawei.com> wrote:
> > >
> > > On Wed, 15 Oct 2025 16:02:02 +0200
> > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > >
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > There appears to be an emerging pattern in which guard
> > > > pm_runtime_active_try is used for resuming the given device and
> > > > incrementing its runtime PM usage counter if the resume has been
> > > > successful, that is followed by an ACQUIRE_ERR() check on the guard
> > > > variable and if that triggers, a specific error code is returned, for
> > > > example:
> > > >
> > > >       ACQUIRE(pm_runtime_active_try, pm)(dev);
> > > >       if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> > > >               return -ENXIO
> > > >
> > > > Introduce a macro called PM_RUNTIME_ACQUIRE_OR_FAIL() representing the
> > > > above sequence of statements that can be used to avoid code duplication
> > > > wherever that sequence would be used.
> > > >
> > > > Use this macro right away in the PCI sysfs code where the above pattern
> > > > is already present.
> > > >
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > >
> > > > Admittedly, the new macro is slightly on the edge, but it really helps
> > > > reduce code duplication, so here it goes.
> > >
> > > Fully agree with the 'on the edge'.
> > >
> > > This looks somewhat like the some of the earlier attempts to come up with
> > > a general solution before ACQUIRE().  Linus was fairly clear on his opinion of
> > > a proposal that looked a bit similar to this
> > > cond_guard(mutex_intr, return -EINTR, &mutex);
> > >
> > > https://lore.kernel.org/all/CAHk-=win7bwWhPJ=iuW4h-sDTqbX6v9_LJnMaO3KxVfPSs81bQ@mail.gmail.com/
> > >
> > > +CC a few people who might have better memories of where things went than I do.
> > >
> > > The solution you have here has the benefit of clarity that all it can do is
> > > return the error code.
> >
> > Well, I could call the macro PM_RUNTIME_ACQUIRE_OR_RETURN_ERROR(), but
> > FAIL is just shorter. :-)
> >
> > Seriously though, the odd syntax bothers me, but it has come from
> > looking at the multiple pieces of code that otherwise would have
> > repeated exactly the same code pattern including the guard name in two
> > places and the pm variable that has no role beyond guarding.
>
> While I see the benefit of simplification, IMO, embedding a code
> flow control inside the macro argument makes it really harder to
> follow.
>
> Is the problem about the messy ACQUIRE_ERR() invocation?  If so, it
> could be replaced with something shorter (and without extra type),
> e.g. replace
>         ret = ACQUIRE_ERR(pm_runtime_active_try, &pm);
> with
>         ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
>
> Since all runtime PM guard usage is to the same object, we can have a
> common macro.

Well, it's not a problem, but an observation that the code pattern is
exactly the same in all of the places that use this macro, so it
represents this exact same code pattern that otherwise will be
repeated in multiple places.

I have no problem with copy-pasting this code pattern between
different use sites, but that's something we generally avoid as a
rule, don't we?

Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

[David Lechner]

On 10/16/25 9:59 AM, Takashi Iwai wrote:
> On Thu, 16 Oct 2025 15:46:08 +0200,
> Rafael J. Wysocki wrote:
>>
>> On Thu, Oct 16, 2025 at 2:39 PM Jonathan Cameron
>> <jonathan.cameron@huawei.com> wrote:
>>>
>>> On Wed, 15 Oct 2025 16:02:02 +0200
>>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>>>
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> There appears to be an emerging pattern in which guard
>>>> pm_runtime_active_try is used for resuming the given device and
>>>> incrementing its runtime PM usage counter if the resume has been
>>>> successful, that is followed by an ACQUIRE_ERR() check on the guard
>>>> variable and if that triggers, a specific error code is returned, for
>>>> example:
>>>>
>>>>       ACQUIRE(pm_runtime_active_try, pm)(dev);
>>>>       if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
>>>>               return -ENXIO
>>>>
>>>> Introduce a macro called PM_RUNTIME_ACQUIRE_OR_FAIL() representing the
>>>> above sequence of statements that can be used to avoid code duplication
>>>> wherever that sequence would be used.
>>>>
>>>> Use this macro right away in the PCI sysfs code where the above pattern
>>>> is already present.
>>>>
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> ---
>>>>
>>>> Admittedly, the new macro is slightly on the edge, but it really helps
>>>> reduce code duplication, so here it goes.
>>>
>>> Fully agree with the 'on the edge'.
>>>
>>> This looks somewhat like the some of the earlier attempts to come up with
>>> a general solution before ACQUIRE().  Linus was fairly clear on his opinion of
>>> a proposal that looked a bit similar to this
>>> cond_guard(mutex_intr, return -EINTR, &mutex);
>>>
>>> https://lore.kernel.org/all/CAHk-=win7bwWhPJ=iuW4h-sDTqbX6v9_LJnMaO3KxVfPSs81bQ@mail.gmail.com/
>>>
>>> +CC a few people who might have better memories of where things went than I do.
>>>
>>> The solution you have here has the benefit of clarity that all it can do is
>>> return the error code.
>>
>> Well, I could call the macro PM_RUNTIME_ACQUIRE_OR_RETURN_ERROR(), but
>> FAIL is just shorter. :-)
>>
>> Seriously though, the odd syntax bothers me, but it has come from
>> looking at the multiple pieces of code that otherwise would have
>> repeated exactly the same code pattern including the guard name in two
>> places and the pm variable that has no role beyond guarding.
> 
> While I see the benefit of simplification, IMO, embedding a code
> flow control inside the macro argument makes it really harder to
> follow.
> 
> Is the problem about the messy ACQUIRE_ERR() invocation?  If so, it
> could be replaced with something shorter (and without extra type),
> e.g. replace 
> 	ret = ACQUIRE_ERR(pm_runtime_active_try, &pm);
> with
> 	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> 
> Since all runtime PM guard usage is to the same object, we can have a
> common macro.
> 
> Also, in the past, I thought of a macro like below that stores the
> error code in the given variable ret:
> 
> #define __guard_cond_ret(_name, _var, _ret, _args)	\
> 	CLASS(_name, _var)(_args);			\
> 	(_ret) = __guard_err(_name)(&_var)
> #define guard_cond_ret(_name, _ret, _args) \
> 	__guard_cond_ret(_name, __UNIQUE_ID(guard), _ret, _args)
> 
> ... so that it'd work for runtime PM like:
> 
> 	int ret;
> 
> 	guard_cond_ret(pm_runtime_active, ret)(dev);
> 	if (ret)
> 		return ret;
> 	
> Of course, a clear drawback is that the assignment of ret isn't
> obvious, but the code flow isn't skewed much in this way.
> 
> 
> thanks,
> 
> Takashi

FWIW, a while back, I suggested something like this where ret was
a parameter rather than a return value [1]. Linus did not seem to
be a fan (said it was "disgusting syntax").

[1]: https://lore.kernel.org/all/CAHk-=whn07tnDosPfn+UcAtWHBcLg=KqA16SHVv0GV4t8P1fHw@mail.gmail.com/

Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

[Takashi Iwai]

On Thu, 16 Oct 2025 18:46:56 +0200,
David Lechner wrote:
> 
> On 10/16/25 9:59 AM, Takashi Iwai wrote:
> > On Thu, 16 Oct 2025 15:46:08 +0200,
> > Rafael J. Wysocki wrote:
> >>
> >> On Thu, Oct 16, 2025 at 2:39 PM Jonathan Cameron
> >> <jonathan.cameron@huawei.com> wrote:
> >>>
> >>> On Wed, 15 Oct 2025 16:02:02 +0200
> >>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >>>
> >>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>>
> >>>> There appears to be an emerging pattern in which guard
> >>>> pm_runtime_active_try is used for resuming the given device and
> >>>> incrementing its runtime PM usage counter if the resume has been
> >>>> successful, that is followed by an ACQUIRE_ERR() check on the guard
> >>>> variable and if that triggers, a specific error code is returned, for
> >>>> example:
> >>>>
> >>>>       ACQUIRE(pm_runtime_active_try, pm)(dev);
> >>>>       if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> >>>>               return -ENXIO
> >>>>
> >>>> Introduce a macro called PM_RUNTIME_ACQUIRE_OR_FAIL() representing the
> >>>> above sequence of statements that can be used to avoid code duplication
> >>>> wherever that sequence would be used.
> >>>>
> >>>> Use this macro right away in the PCI sysfs code where the above pattern
> >>>> is already present.
> >>>>
> >>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>> ---
> >>>>
> >>>> Admittedly, the new macro is slightly on the edge, but it really helps
> >>>> reduce code duplication, so here it goes.
> >>>
> >>> Fully agree with the 'on the edge'.
> >>>
> >>> This looks somewhat like the some of the earlier attempts to come up with
> >>> a general solution before ACQUIRE().  Linus was fairly clear on his opinion of
> >>> a proposal that looked a bit similar to this
> >>> cond_guard(mutex_intr, return -EINTR, &mutex);
> >>>
> >>> https://lore.kernel.org/all/CAHk-=win7bwWhPJ=iuW4h-sDTqbX6v9_LJnMaO3KxVfPSs81bQ@mail.gmail.com/
> >>>
> >>> +CC a few people who might have better memories of where things went than I do.
> >>>
> >>> The solution you have here has the benefit of clarity that all it can do is
> >>> return the error code.
> >>
> >> Well, I could call the macro PM_RUNTIME_ACQUIRE_OR_RETURN_ERROR(), but
> >> FAIL is just shorter. :-)
> >>
> >> Seriously though, the odd syntax bothers me, but it has come from
> >> looking at the multiple pieces of code that otherwise would have
> >> repeated exactly the same code pattern including the guard name in two
> >> places and the pm variable that has no role beyond guarding.
> > 
> > While I see the benefit of simplification, IMO, embedding a code
> > flow control inside the macro argument makes it really harder to
> > follow.
> > 
> > Is the problem about the messy ACQUIRE_ERR() invocation?  If so, it
> > could be replaced with something shorter (and without extra type),
> > e.g. replace 
> > 	ret = ACQUIRE_ERR(pm_runtime_active_try, &pm);
> > with
> > 	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> > 
> > Since all runtime PM guard usage is to the same object, we can have a
> > common macro.
> > 
> > Also, in the past, I thought of a macro like below that stores the
> > error code in the given variable ret:
> > 
> > #define __guard_cond_ret(_name, _var, _ret, _args)	\
> > 	CLASS(_name, _var)(_args);			\
> > 	(_ret) = __guard_err(_name)(&_var)
> > #define guard_cond_ret(_name, _ret, _args) \
> > 	__guard_cond_ret(_name, __UNIQUE_ID(guard), _ret, _args)
> > 
> > ... so that it'd work for runtime PM like:
> > 
> > 	int ret;
> > 
> > 	guard_cond_ret(pm_runtime_active, ret)(dev);
> > 	if (ret)
> > 		return ret;
> > 	
> > Of course, a clear drawback is that the assignment of ret isn't
> > obvious, but the code flow isn't skewed much in this way.
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> FWIW, a while back, I suggested something like this where ret was
> a parameter rather than a return value [1]. Linus did not seem to
> be a fan (said it was "disgusting syntax").
> 
> [1]: https://lore.kernel.org/all/CAHk-=whn07tnDosPfn+UcAtWHBcLg=KqA16SHVv0GV4t8P1fHw@mail.gmail.com/

Yeah, I myself also find it suboptimal, hence it wasn't really
proposed...  It's a limit of macro, unfortunately.


thanks,

Takashi

Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

[Rafael J. Wysocki]

On Thursday, October 16, 2025 8:13:14 PM CEST Takashi Iwai wrote:
> On Thu, 16 Oct 2025 18:46:56 +0200,
> David Lechner wrote:
> > 
> > On 10/16/25 9:59 AM, Takashi Iwai wrote:
> > > On Thu, 16 Oct 2025 15:46:08 +0200,
> > > Rafael J. Wysocki wrote:
> > >>
> > >> On Thu, Oct 16, 2025 at 2:39 PM Jonathan Cameron
> > >> <jonathan.cameron@huawei.com> wrote:
> > >>>
> > >>> On Wed, 15 Oct 2025 16:02:02 +0200
> > >>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > >>>
> > >>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >>>>
> > >>>> There appears to be an emerging pattern in which guard
> > >>>> pm_runtime_active_try is used for resuming the given device and
> > >>>> incrementing its runtime PM usage counter if the resume has been
> > >>>> successful, that is followed by an ACQUIRE_ERR() check on the guard
> > >>>> variable and if that triggers, a specific error code is returned, for
> > >>>> example:
> > >>>>
> > >>>>       ACQUIRE(pm_runtime_active_try, pm)(dev);
> > >>>>       if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> > >>>>               return -ENXIO
> > >>>>
> > >>>> Introduce a macro called PM_RUNTIME_ACQUIRE_OR_FAIL() representing the
> > >>>> above sequence of statements that can be used to avoid code duplication
> > >>>> wherever that sequence would be used.
> > >>>>
> > >>>> Use this macro right away in the PCI sysfs code where the above pattern
> > >>>> is already present.
> > >>>>
> > >>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >>>> ---
> > >>>>
> > >>>> Admittedly, the new macro is slightly on the edge, but it really helps
> > >>>> reduce code duplication, so here it goes.
> > >>>
> > >>> Fully agree with the 'on the edge'.
> > >>>
> > >>> This looks somewhat like the some of the earlier attempts to come up with
> > >>> a general solution before ACQUIRE().  Linus was fairly clear on his opinion of
> > >>> a proposal that looked a bit similar to this
> > >>> cond_guard(mutex_intr, return -EINTR, &mutex);
> > >>>
> > >>> https://lore.kernel.org/all/CAHk-=win7bwWhPJ=iuW4h-sDTqbX6v9_LJnMaO3KxVfPSs81bQ@mail.gmail.com/
> > >>>
> > >>> +CC a few people who might have better memories of where things went than I do.
> > >>>
> > >>> The solution you have here has the benefit of clarity that all it can do is
> > >>> return the error code.
> > >>
> > >> Well, I could call the macro PM_RUNTIME_ACQUIRE_OR_RETURN_ERROR(), but
> > >> FAIL is just shorter. :-)
> > >>
> > >> Seriously though, the odd syntax bothers me, but it has come from
> > >> looking at the multiple pieces of code that otherwise would have
> > >> repeated exactly the same code pattern including the guard name in two
> > >> places and the pm variable that has no role beyond guarding.
> > > 
> > > While I see the benefit of simplification, IMO, embedding a code
> > > flow control inside the macro argument makes it really harder to
> > > follow.
> > > 
> > > Is the problem about the messy ACQUIRE_ERR() invocation?  If so, it
> > > could be replaced with something shorter (and without extra type),
> > > e.g. replace 
> > > 	ret = ACQUIRE_ERR(pm_runtime_active_try, &pm);
> > > with
> > > 	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> > > 
> > > Since all runtime PM guard usage is to the same object, we can have a
> > > common macro.
> > > 
> > > Also, in the past, I thought of a macro like below that stores the
> > > error code in the given variable ret:
> > > 
> > > #define __guard_cond_ret(_name, _var, _ret, _args)	\
> > > 	CLASS(_name, _var)(_args);			\
> > > 	(_ret) = __guard_err(_name)(&_var)
> > > #define guard_cond_ret(_name, _ret, _args) \
> > > 	__guard_cond_ret(_name, __UNIQUE_ID(guard), _ret, _args)
> > > 
> > > ... so that it'd work for runtime PM like:
> > > 
> > > 	int ret;
> > > 
> > > 	guard_cond_ret(pm_runtime_active, ret)(dev);
> > > 	if (ret)
> > > 		return ret;
> > > 	
> > > Of course, a clear drawback is that the assignment of ret isn't
> > > obvious, but the code flow isn't skewed much in this way.
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > 
> > FWIW, a while back, I suggested something like this where ret was
> > a parameter rather than a return value [1]. Linus did not seem to
> > be a fan (said it was "disgusting syntax").
> > 
> > [1]: https://lore.kernel.org/all/CAHk-=whn07tnDosPfn+UcAtWHBcLg=KqA16SHVv0GV4t8P1fHw@mail.gmail.com/
> 
> Yeah, I myself also find it suboptimal, hence it wasn't really
> proposed...  It's a limit of macro, unfortunately.

The macro from the $subject patch can be split along the lines of the appended
patch to avoid the "disgusting syntax" issue, although it then becomes less
attractive as far as I'm concerned.  It still allows the details unrelated to
the rest of the code to be hidden though.

---
 drivers/acpi/acpi_tad.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

--- a/drivers/acpi/acpi_tad.c
+++ b/drivers/acpi/acpi_tad.c
@@ -31,6 +31,12 @@ MODULE_DESCRIPTION("ACPI Time and Alarm
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Rafael J. Wysocki");
 
+#define PM_RUNTIME_ACQUIRE_ACTIVE(dev)	\
+	ACQUIRE(pm_runtime_active_try, pm_runtime_active_guard_var)(dev)
+
+#define PM_RUNTIME_ACQUIRE_ACTIVE_ERR	\
+	ACQUIRE_ERR(pm_runtime_active_try, &pm_runtime_active_guard_var)
+
 /* ACPI TAD capability flags (ACPI 6.2, Section 9.18.2) */
 #define ACPI_TAD_AC_WAKE	BIT(0)
 #define ACPI_TAD_DC_WAKE	BIT(1)
@@ -264,8 +270,8 @@ static int acpi_tad_wake_set(struct devi
 	args[0].integer.value = timer_id;
 	args[1].integer.value = value;
 
-	ACQUIRE(pm_runtime_active_try, pm)(dev);
-	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
+	PM_RUNTIME_ACQUIRE_ACTIVE(dev);
+	if (PM_RUNTIME_ACQUIRE_ACTIVE_ERR)
 		return -ENXIO;
 
 	status = acpi_evaluate_integer(handle, method, &arg_list, &retval);

Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

[dan.j.williams]

Rafael J. Wysocki wrote:
[..]
> > > [1]: https://lore.kernel.org/all/CAHk-=whn07tnDosPfn+UcAtWHBcLg=KqA16SHVv0GV4t8P1fHw@mail.gmail.com/
> > 
> > Yeah, I myself also find it suboptimal, hence it wasn't really
> > proposed...  It's a limit of macro, unfortunately.
> 
> The macro from the $subject patch can be split along the lines of the appended
> patch to avoid the "disgusting syntax" issue, although it then becomes less
> attractive as far as I'm concerned.  It still allows the details unrelated to
> the rest of the code to be hidden though.
> 
> ---
>  drivers/acpi/acpi_tad.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> --- a/drivers/acpi/acpi_tad.c
> +++ b/drivers/acpi/acpi_tad.c
> @@ -31,6 +31,12 @@ MODULE_DESCRIPTION("ACPI Time and Alarm
>  MODULE_LICENSE("GPL v2");
>  MODULE_AUTHOR("Rafael J. Wysocki");
>  
> +#define PM_RUNTIME_ACQUIRE_ACTIVE(dev)	\
> +	ACQUIRE(pm_runtime_active_try, pm_runtime_active_guard_var)(dev)
> +
> +#define PM_RUNTIME_ACQUIRE_ACTIVE_ERR	\
> +	ACQUIRE_ERR(pm_runtime_active_try, &pm_runtime_active_guard_var)
> +
>  /* ACPI TAD capability flags (ACPI 6.2, Section 9.18.2) */
>  #define ACPI_TAD_AC_WAKE	BIT(0)
>  #define ACPI_TAD_DC_WAKE	BIT(1)
> @@ -264,8 +270,8 @@ static int acpi_tad_wake_set(struct devi
>  	args[0].integer.value = timer_id;
>  	args[1].integer.value = value;
>  
> -	ACQUIRE(pm_runtime_active_try, pm)(dev);
> -	if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> +	PM_RUNTIME_ACQUIRE_ACTIVE(dev);
> +	if (PM_RUNTIME_ACQUIRE_ACTIVE_ERR)
>  		return -ENXIO;

This defeats one of the other motivations for ACQUIRE() vs
scoped_cond_guard() in that it drops the error code from
pm_runtime_active_try. Maybe it is the case that failure is always
-ENXIO, but from a future code evolution standpoint do you want to
commit to always translating _try errors to a local error code?

Btw, was acpi_tad_wake_set() buggy previously for ignoring
pm_runtime_get_sync() errors, or is it a regression risk now for
honoring errors?

Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

[Rafael J. Wysocki]

On Thu, Oct 16, 2025 at 9:45 PM <dan.j.williams@intel.com> wrote:
>
> Rafael J. Wysocki wrote:
> [..]
> > > > [1]: https://lore.kernel.org/all/CAHk-=whn07tnDosPfn+UcAtWHBcLg=KqA16SHVv0GV4t8P1fHw@mail.gmail.com/
> > >
> > > Yeah, I myself also find it suboptimal, hence it wasn't really
> > > proposed...  It's a limit of macro, unfortunately.
> >
> > The macro from the $subject patch can be split along the lines of the appended
> > patch to avoid the "disgusting syntax" issue, although it then becomes less
> > attractive as far as I'm concerned.  It still allows the details unrelated to
> > the rest of the code to be hidden though.
> >
> > ---
> >  drivers/acpi/acpi_tad.c |   10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > --- a/drivers/acpi/acpi_tad.c
> > +++ b/drivers/acpi/acpi_tad.c
> > @@ -31,6 +31,12 @@ MODULE_DESCRIPTION("ACPI Time and Alarm
> >  MODULE_LICENSE("GPL v2");
> >  MODULE_AUTHOR("Rafael J. Wysocki");
> >
> > +#define PM_RUNTIME_ACQUIRE_ACTIVE(dev)       \
> > +     ACQUIRE(pm_runtime_active_try, pm_runtime_active_guard_var)(dev)
> > +
> > +#define PM_RUNTIME_ACQUIRE_ACTIVE_ERR        \
> > +     ACQUIRE_ERR(pm_runtime_active_try, &pm_runtime_active_guard_var)
> > +
> >  /* ACPI TAD capability flags (ACPI 6.2, Section 9.18.2) */
> >  #define ACPI_TAD_AC_WAKE     BIT(0)
> >  #define ACPI_TAD_DC_WAKE     BIT(1)
> > @@ -264,8 +270,8 @@ static int acpi_tad_wake_set(struct devi
> >       args[0].integer.value = timer_id;
> >       args[1].integer.value = value;
> >
> > -     ACQUIRE(pm_runtime_active_try, pm)(dev);
> > -     if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> > +     PM_RUNTIME_ACQUIRE_ACTIVE(dev);
> > +     if (PM_RUNTIME_ACQUIRE_ACTIVE_ERR)
> >               return -ENXIO;
>
> This defeats one of the other motivations for ACQUIRE() vs
> scoped_cond_guard() in that it drops the error code from
> pm_runtime_active_try.

No, it doesn't.  PM_RUNTIME_ACQUIRE_ACTIVE_ERR is that error code.  Or
did I misunderstand what you said?

> Maybe it is the case that failure is always
> -ENXIO, but from a future code evolution standpoint do you want to
> commit to always translating _try errors to a local error code?

No, I don't.

> Btw, was acpi_tad_wake_set() buggy previously for ignoring
> pm_runtime_get_sync() errors, or is it a regression risk now for
> honoring errors?

You may call it buggy strictly speaking, but it just assumed that if
the runtime resume failed, the subsequent operation would just fail
either, so -EIO would be returned to the caller.

This change allows distinguishing resume errors from I/O errors.

Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

[dan.j.williams]

Rafael J. Wysocki wrote:
> On Thu, Oct 16, 2025 at 9:45 PM <dan.j.williams@intel.com> wrote:
> >
> > Rafael J. Wysocki wrote:
> > [..]
> > > > > [1]: https://lore.kernel.org/all/CAHk-=whn07tnDosPfn+UcAtWHBcLg=KqA16SHVv0GV4t8P1fHw@mail.gmail.com/
> > > >
> > > > Yeah, I myself also find it suboptimal, hence it wasn't really
> > > > proposed...  It's a limit of macro, unfortunately.
> > >
> > > The macro from the $subject patch can be split along the lines of the appended
> > > patch to avoid the "disgusting syntax" issue, although it then becomes less
> > > attractive as far as I'm concerned.  It still allows the details unrelated to
> > > the rest of the code to be hidden though.
> > >
> > > ---
> > >  drivers/acpi/acpi_tad.c |   10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > --- a/drivers/acpi/acpi_tad.c
> > > +++ b/drivers/acpi/acpi_tad.c
> > > @@ -31,6 +31,12 @@ MODULE_DESCRIPTION("ACPI Time and Alarm
> > >  MODULE_LICENSE("GPL v2");
> > >  MODULE_AUTHOR("Rafael J. Wysocki");
> > >
> > > +#define PM_RUNTIME_ACQUIRE_ACTIVE(dev)       \
> > > +     ACQUIRE(pm_runtime_active_try, pm_runtime_active_guard_var)(dev)
> > > +
> > > +#define PM_RUNTIME_ACQUIRE_ACTIVE_ERR        \
> > > +     ACQUIRE_ERR(pm_runtime_active_try, &pm_runtime_active_guard_var)
> > > +
> > >  /* ACPI TAD capability flags (ACPI 6.2, Section 9.18.2) */
> > >  #define ACPI_TAD_AC_WAKE     BIT(0)
> > >  #define ACPI_TAD_DC_WAKE     BIT(1)
> > > @@ -264,8 +270,8 @@ static int acpi_tad_wake_set(struct devi
> > >       args[0].integer.value = timer_id;
> > >       args[1].integer.value = value;
> > >
> > > -     ACQUIRE(pm_runtime_active_try, pm)(dev);
> > > -     if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> > > +     PM_RUNTIME_ACQUIRE_ACTIVE(dev);
> > > +     if (PM_RUNTIME_ACQUIRE_ACTIVE_ERR)
> > >               return -ENXIO;
> >
> > This defeats one of the other motivations for ACQUIRE() vs
> > scoped_cond_guard() in that it drops the error code from
> > pm_runtime_active_try.
> 
> No, it doesn't.  PM_RUNTIME_ACQUIRE_ACTIVE_ERR is that error code.  Or
> did I misunderstand what you said?

Oh, what I am saying is that pm_runtime_get_active() returns a distinct
error code like -EACCES or -EINPROGRESS etc. The
PM_RUNTIME_ACQUIRE_ACTIVE_ERR proposal ignores that value and open codes
returning -ENXIO.

> > Maybe it is the case that failure is always -ENXIO, but from a
> > future code evolution standpoint do you want to commit to always
> > translating _try errors to a local error code?
> 
> No, I don't.
> 
> > Btw, was acpi_tad_wake_set() buggy previously for ignoring
> > pm_runtime_get_sync() errors, or is it a regression risk now for
> > honoring errors?
> 
> You may call it buggy strictly speaking, but it just assumed that if
> the runtime resume failed, the subsequent operation would just fail
> either, so -EIO would be returned to the caller.
> 
> This change allows distinguishing resume errors from I/O errors.

Ah, ok, makes sense.

Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

[Rafael J. Wysocki]

On Thu, Oct 16, 2025 at 10:59 PM <dan.j.williams@intel.com> wrote:
>
> Rafael J. Wysocki wrote:
> > On Thu, Oct 16, 2025 at 9:45 PM <dan.j.williams@intel.com> wrote:
> > >
> > > Rafael J. Wysocki wrote:
> > > [..]
> > > > > > [1]: https://lore.kernel.org/all/CAHk-=whn07tnDosPfn+UcAtWHBcLg=KqA16SHVv0GV4t8P1fHw@mail.gmail.com/
> > > > >
> > > > > Yeah, I myself also find it suboptimal, hence it wasn't really
> > > > > proposed...  It's a limit of macro, unfortunately.
> > > >
> > > > The macro from the $subject patch can be split along the lines of the appended
> > > > patch to avoid the "disgusting syntax" issue, although it then becomes less
> > > > attractive as far as I'm concerned.  It still allows the details unrelated to
> > > > the rest of the code to be hidden though.
> > > >
> > > > ---
> > > >  drivers/acpi/acpi_tad.c |   10 ++++++++--
> > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > >
> > > > --- a/drivers/acpi/acpi_tad.c
> > > > +++ b/drivers/acpi/acpi_tad.c
> > > > @@ -31,6 +31,12 @@ MODULE_DESCRIPTION("ACPI Time and Alarm
> > > >  MODULE_LICENSE("GPL v2");
> > > >  MODULE_AUTHOR("Rafael J. Wysocki");
> > > >
> > > > +#define PM_RUNTIME_ACQUIRE_ACTIVE(dev)       \
> > > > +     ACQUIRE(pm_runtime_active_try, pm_runtime_active_guard_var)(dev)
> > > > +
> > > > +#define PM_RUNTIME_ACQUIRE_ACTIVE_ERR        \
> > > > +     ACQUIRE_ERR(pm_runtime_active_try, &pm_runtime_active_guard_var)
> > > > +
> > > >  /* ACPI TAD capability flags (ACPI 6.2, Section 9.18.2) */
> > > >  #define ACPI_TAD_AC_WAKE     BIT(0)
> > > >  #define ACPI_TAD_DC_WAKE     BIT(1)
> > > > @@ -264,8 +270,8 @@ static int acpi_tad_wake_set(struct devi
> > > >       args[0].integer.value = timer_id;
> > > >       args[1].integer.value = value;
> > > >
> > > > -     ACQUIRE(pm_runtime_active_try, pm)(dev);
> > > > -     if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> > > > +     PM_RUNTIME_ACQUIRE_ACTIVE(dev);
> > > > +     if (PM_RUNTIME_ACQUIRE_ACTIVE_ERR)
> > > >               return -ENXIO;
> > >
> > > This defeats one of the other motivations for ACQUIRE() vs
> > > scoped_cond_guard() in that it drops the error code from
> > > pm_runtime_active_try.
> >
> > No, it doesn't.  PM_RUNTIME_ACQUIRE_ACTIVE_ERR is that error code.  Or
> > did I misunderstand what you said?
>
> Oh, what I am saying is that pm_runtime_get_active() returns a distinct
> error code like -EACCES or -EINPROGRESS etc. The
> PM_RUNTIME_ACQUIRE_ACTIVE_ERR proposal ignores that value and open codes
> returning -ENXIO.

No, it doesn't.

You can still do

ret = PM_RUNTIME_ACQUIRE_ACTIVE_ERR;
if (ret)
        return ret;

if the caller needs to know the original resume error code.

The code being updated in the example patch returns -ENXIO, but it
does so before the change either.