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

On Tue, Aug 25, 2026 at 3:51 PM Ilpo Järvinen
<[email protected]> wrote:
>
> On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:
>
> > On Tue, Aug 25, 2026 at 3:02 PM Rafael J. Wysocki (Intel)
> > <[email protected]> wrote:
> > >
> > > On Tue, Aug 25, 2026 at 2:32 PM Ilpo Järvinen
> > > <[email protected]> wrote:
> > > >
> > > > On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) wrote:
> > > >
> > > > > On Tue, Aug 25, 2026 at 2:07 PM Sakari Ailus
> > > > > <[email protected]> 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
> > > > > > > <[email protected]> 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 agree with all of the above.

Though I think that cleanup ordering issues are easier to overlook
when the "__free(...) = NULL" pattern is used, which is why I wouldn't
like to encourage using it.