Re: [PATCH] media: atomisp: fix CAS scaler descriptor leaks

[Andy Shevchenko]

On Mon, Jun 29, 2026 at 03:01:10PM +0300, Dan Carpenter wrote:
> On Mon, Jun 29, 2026 at 02:16:21PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 29, 2026 at 11:30:40AM +0300, Dan Carpenter wrote:
> > > On Sat, Jun 27, 2026 at 02:01:51PM +0800, Dawei Feng wrote:

...

> > > free_output_stage:
> > > 	if (need_scalar) {
> > > 		kfree(mycs->is_output_stage);
> > > 		mycs->is_output_stage = NULL;
> > > 	}
> > > free_scalar_binary:
> > > 	if (need_scalar) {
> > > 		kfree(mycs->yuv_scaler_binary);
> > > 		mycs->yuv_scaler_binary = NULL;
> > > 	}
> > 
> > If we go this way, double check that the checks are needed as we have kfree()
> > to be NULL-aware.
> 
> Dawei, Andy is the one reviewing atomisp so his opinion matters more
> than mine here.  So do what he says.
> 
> But I don't really agree...
> 
> In this case, sure, hopefully the caller zeroes the ->yuv_scaler_binary
> pointer, but if we just follow the simple rule of only undoing things
> which we have done then we don't need to check.  The function is
> self contained and self explanatory.
> 
> And more generally, I've always hated patches which delete NULL
> checks before a ionmap() or whatever.  Hiding the NULL check inside the
> free function makes the code less self contained.  The real fix is to
> stop mixing allocated and unallocated pointers.  Then you don't need a
> NULL check because you already know.  (Also I think those iounmap()
> patches were wrong because some arches have a warning when you unmap
> a NULL).

I fully agree with the statement against iounmap(), but for regular memory
freeing it's almost an idiomatic to just call it with valid pointer (note that
NULL *is* valid pointer, just may not be dereferenced). You can also read recent
discussion with Linus with Kees on some other topic where it was explained the
malloc(0) to give a valid pointer (as an "empty" something). Ah, now I remember
what was that, it was about ARRAY_END() macro.

So, having unconditional kfree() for (optional) memory allocations is perfectly
fine.

-- 
With Best Regards,
Andy Shevchenko