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

On Wed, Aug 26, 2026 at 11:04 AM Ilpo Järvinen
<[email protected]> wrote:
>
> On Tue, 25 Aug 2026, Sakari Ailus wrote:
>
> > Hi Rafael, Ilpo,
> >
> > On Tue, Aug 25, 2026 at 03:08:07PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > 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's indeed a valid concern, still quite unlikely in practice but
> > probably hard to find when it happens, so avoiding that is definitely
> > preferred. That being said, the biggest trap in cleanup.h is probably in
> > scoped_guard(), and doing __free() = NULL somewhere doesn't matter much in
> > the end.
>
> __free() = NULL is not that hard to catch during review (or even in
> code already in-tree), have done that dozens of times myself by now.
> Checkpatch, too, should be able to catch that easily, if it doesn't
> already.
>
> None of those cases I've commented on had a bug, so it was just for
> teaching submitters & readers of that code the correct __free() pattern.
> I think the concern is largely overblown given how rare actual bugs are
> even if the wrong pattern is used. Put that to contrast to memleaks found
> on our rollback paths, __free() looks a clear win despite very rare to
> occur caveats.
>
> Given what I've seen, I'd say on dangerous level __free() = NULL is
> somewhere around using MAGIC_SIZE_DEFINE instead sizeof(*obj) when doing
> mem allocs. It usually isn't buggy even if we don't want to teach people
> to use it.
>
> Besides, it was actually Rafael himself who brought the unsafe pattern
> into this discussion (I immediately noticed the problem but since it was
> not an actual patch, I didn't raise a concern). Sakari's patch did use the
> correct pattern (and if it wouldn't have done so, there would have been a
> review comment from me ;-)). In my reply to Rafael, I intentionally left
> the right side open with "= ..." to not place that NULL there.
>
> > In this case I'll just call ACPI_FREE() sooner.
>
> That works too in this case, yes.

So overall, something like this can be defined

static inline void free_acpi_object(union acpi_object *obj)
{
        ACPI_FREE(obj);
}

DEFINE_FREE(acpi_object_free, union acpi_object *, free_acpi_object(_T))

and used along with acpi_evaluate_dsm*().

Moreover, a wrapper around acpi_evaluate_object() returning a pointer
to union acpi_object on success and NULL on failure can be defined and
used along with the above.

IMV that would be way cleaner than wrapping a raw cleanup macro around
ACPI_FREE().

I generally think that there needs to be a wrapper layer around the
ACPICA API, so drivers don't need to call functions returning
acpi_status directly, for instance.

Someday we'll get there.