Re: [PATCH v2 3/5] ACPI: Support __free() from cleanup.h for ACPI objects

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4394 bytes --]

On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:

> On Tue, Aug 25, 2026 at 3:02 PM Rafael J. Wysocki (Intel)
> <rafael@kernel.org> wrote:
> >
> > On Tue, Aug 25, 2026 at 2:32 PM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:
> > >
> > > > On Tue, Aug 25, 2026 at 2:07 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Rafael,
> > > > >
> > > > > On Tue, Aug 25, 2026 at 01:51:18PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > > > On Mon, Aug 24, 2026 at 11:13 PM Sakari Ailus
> > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > >
> > > > > > > Use DEFINE_FREE() to allow ACPI objects to be released automatically.
> > > > > >
> > > > > > But at least some of them are allocated by ACPICA functions like
> > > > > > acpi_evaluate_object() and so they have no proper constructors.
> > > > >
> > > > > You could still assign the return buffer to a local variable. It's not
> > > > > ideal API-wise though.
> > > >
> > > > Exactly.
> > > >
> > > > > I'm not quite sure what was the point you wanted to make but I reckon this
> > > > > wasn't an ack. :-)
> > > >
> > > > Using the _FREE with variables that are not initialized through a
> > > > constructor is questionable, so this is generally not particularly
> > > > clean.
> > >
> > > The driver does call ACPI_FREE() for that pointer so clearly it's already
> > > using something ending with "_FREE" already. So unless Rafael is
> > > suggesting ACPI_FREE() should be renamed, I'm a bit lost what that
> > > even means on concrete terms.
> >
> > Sorry for the confusion.
> >
> > I just don't want people to do things like this:
> >
> >     struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER };
> >     union acpi_object *out_obj __free(ACPI_FREE);
> >     acpi_status status;
> >
> >     status = acpi_evaluate_object(handle, METHOD_NAME, NULL, &output);
> >     if (ACPI_FAILURE(status))
> >         return AN_ERROR;
> >
> >    out_obj = output.pointer;
> >
> > > > There is no cleanup.h in ACPICA that is a more traditional C code
> > > > base, so mixing up ACPICA code, which ACPI_FREE() is strictly
> > > > speaking, with cleanup.h stuff is not particularly straightforward
> > > > IMV.  I'd rather not do it.
> > >
> > > Perhaps add the DEFINE_FREE() into int3472 driver then, it seems to be
> > > what intel/pmc is already doing (I don't seem to anymore recall why it was
> > > added there). Using cleanup.h for that variable it clearly simplifies the
> > > code flow.
> >
> > Well, fair enough, but as I said elsewhere, the code flow
> > simplification can also be achieved in a different way.
> >
> > > It feels a bit stupid to duplicate it there but I guess we'll
> > > just have to live with that if there's no place in any acpi related
> > > headers for cleanup.h.
> >
> > If there is a cleanup.h "free" that can only be used with objects
> > returned by acpi_evaluate_dsm_typed(), I'll be fine with that.
> >
> > Or if everyone agrees that doing
> >
> >     union acpi_object *out_obj __free(ACPI_FREE) = NULL;
> >
> > is not confusing and fine, I may just say "Hey, I don't care that much".
> 
> And particularly there is this paragraph in a comment in cleanup.h:
> 
>  * Given that the "__free(...) = NULL" pattern for variables defined at
>  * the top of the function poses this potential interdependency problem
>  * the recommendation is to always define and assign variables in one
>  * statement and not group variable definitions at the top of the
>  * function when __free() is used.
> 
> regarding a broken code example, so I would think that this is not a
> made-up concern.

That comment relates to how defining the variables at the start of 
functions may result in wrong/unexpected cleanup order. More imporantly, 
the comment is not an argument for not using __free() but an instruction 
on what is the correct pattern to use it so those ordering issues do not 
occur.

The cleanups will execute in reverse order the variables where defined so 
a variable defined at start may be cleaned up only after releasing a lock 
that was taken mid-function (which often is safe but one can easily 
envision cases where the lock should be still held when the cleanup runs).

-- 
 i.