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

[Takashi Iwai <tiwai@suse.de>]

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