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

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

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.

-- 
 i.