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

On Tue, Aug 25, 2026 at 3:37 PM Ilpo Järvinen
<[email protected]> wrote:
>
> On Tue, 25 Aug 2026, Rafael J. Wysocki (Intel) 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.
>
> So you'd be fine with something like this:
>
>       union acpi_object *out_obj __free(some_other_name_than_ACPI_FREE) = ...
>
> ?

Basically, yes.

> > 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".
>
> I personally don't hang myself into "free" or "constructor" terminology
> but look it more pragmatically, if ACPI_FREE() should be called before
> the object goes out of scope, __free() is a tool for that.

It is also good to consider the example given in cleanup.h though, in
which there is the

            struct object *obj __free(remove_free) = NULL;

pattern leading to a real bug due to a cleanup ordering issue.

I would prefer to avoid encouraging people to create code patterns
that are potentially problematic.  The "free" or "constructor"
terminology mentioned above boils down to avoiding those code patterns
which also matters in practice.

> Have Rust people tried to wrap this interface already (there seems to
> acpi.rs but it seems very limited in scope)? I'd expect them to be just as
> interested in knowing what to do when something is at the end of its
> lifetime.

No, they haven't, AFAICS.  At least I'm not aware of any attempts to do so.