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: <875xce7m11.wl-tiwai@suse.de>
Date: Thu, 16 Oct 2025 20:13:14 +0200
From: Takashi Iwai <tiwai@...e.de>
To: David Lechner <dlechner@...libre.com>
Cc: Takashi Iwai <tiwai@...e.de>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Jonathan Cameron <jonathan.cameron@...wei.com>,
	Linux PM <linux-pm@...r.kernel.org>,
	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>,
	"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 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@...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
> 
> 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