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

["Rafael J. Wysocki" <rafael@kernel.org>]

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);