linux-kernel - Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ikge7v01.wl-tiwai@suse.de>
Date: Thu, 16 Oct 2025 16:59:26 +0200
From: Takashi Iwai <tiwai@...e.de>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Jonathan Cameron <jonathan.cameron@...wei.com>,
	Linux PM <linux-pm@...r.kernel.org>,
	Takashi Iwai <tiwai@...e.de>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux PCI <linux-pci@...r.kernel.org>,
	Alex Williamson <alex.williamson@...hat.com>,
	Bjorn Helgaas <helgaas@...nel.org>,
	Zhang Qilong <zhangqilong3@...wei.com>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	Frank Li <Frank.Li@....com>,
	Dhruva Gole <d-gole@...com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Linux ACPI <linux-acpi@...r.kernel.org>,
	Dan Williams <dan.j.williams@...el.com>,
	David Lechner <dlechner@...libre.com>,
	"Fabio M. De Francesco" <fabio.maria.de.francesco@...ux.intel.com>
Subject: Re: [PATCH v1 1/3] PM: runtime: Introduce PM_RUNTIME_ACQUIRE_OR_FAIL() macro

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@...wei.com> wrote:
> >
> > On Wed, 15 Oct 2025 16:02:02 +0200
> > "Rafael J. Wysocki" <rafael@...nel.org> wrote:
> >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...el.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