Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Mar 14, 2025 at 11:49:01AM +0000, Jonathan Cameron wrote:
> On Tue, 04 Mar 2025 18:22:53 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:

...

> > +	struct region_devres *dr __free(kfree) = NULL;
> > +	struct resource *res;
> > +
> 	Avoid split of the constructor / destructor with a ternary though
> 	error check is worse... Hmm.
> 
> 	struct region_devres *dr __free(kfree) =
> 		dev ? devres_alloc(devm_region_release, sizeof(*dr), GFP_KERNEL) : NULL;
> 	if (dev && !dr)
> 		return ERR_PTR(-ENOMEM);
> 
> So on balance up to you,

I'm against ternary, it looks unreadable in this case.

...

> but at very least put that __free(kfree) direclty
> above the set.

This I am neutral about. Any thing works for me.

The rest you suggested is +1 from me.

> > +	if (dev) {
> > +		dr = devres_alloc(devm_region_release,
> > +				  sizeof(struct region_devres), GFP_KERNEL);
> > +		if (!dr)
> > +			return ERR_PTR(-ENOMEM);
> > +	}
> > +
> > +	res = __get_free_mem_region(base, size, align, name, desc, flags);
> > +
> Probably no blank line here would be nicer.
> 
> > +	if (IS_ERR(res) || !dr)
> 
> This conflates error and don't care if error cases and briefly
> made me scratch my head.
> 
> Maybe though it increase indent don't do early
> return on the !dr case.
> 
> 	if (IS_ERR(res))
> 		return res;
> 
> 	if (dr) {
> 		dr->parent = base;
> 
> 		...
> 
> 	}
> 
> 	return res;
> 
> > +		return res;
> > +
> > +	dr->parent = base;
> > +	dr->start = res->start;
> > +	dr->n = resource_size(res);
> > +
> > +	/* See 'struct region_devres' definition for details */
> > +	if ((flags & GFR_REQUEST_REGION) == 0)
> > +		dr->res = res;
> > +
> > +	devres_add(dev, no_free_ptr(dr));

-- 
With Best Regards,
Andy Shevchenko