Re: [PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management

[Dan Williams <dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> On Tue, 30 Apr 2024 14:59:00 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > A recent bugfix to cxl_pmem_region_alloc() to fix an
> > error-unwind-memleak [1], highlighted a use case for scope-based resource
> > management.
> > 
> > Delete the goto for releasing @cxl_region_rwsem, and return error codes
> > directly from error condition paths.
> > 
> > The caller, devm_cxl_add_pmem_region(), is no longer given @cxlr_pmem
> > directly it must retrieve it from @cxlr->cxlr_pmem. This retrieval from
> > @cxlr was already in place for @cxlr->cxl_nvb, and converting
> > cxl_pmem_region_alloc() to return an int makes it less awkward to handle
> > no_free_ptr().
> > 
> > Cc: Li Zhijian <lizhijian@fujitsu.com>
> > Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Closes: http://lore.kernel.org/r/20240430174540.000039ce@Huawei.com
> > Link: http://lore.kernel.org/r/20240428030748.318985-1-lizhijian@fujitsu.com
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Trivial comments inline  Up to Dave or you on whether you want to act on them.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
[..]
> > -	cxlr_pmem = kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets),
> > -			    GFP_KERNEL);
> > -	if (!cxlr_pmem) {
> > -		cxlr_pmem = ERR_PTR(-ENOMEM);
> > -		goto out;
> > -	}
> > +	struct cxl_pmem_region *cxlr_pmem __free(kfree) = kzalloc(
> > +		struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL);
> 
> That is rather ugly.  Maybe bring the kzalloc down onto the second line and
> wrap the GFP_KERNEL (or just go over 80 chars)?

I have made a conscious decision to stop caring about manually making 80
column collisions look nicer or selectively breaking the 80 column limit
rule. For this drivers/cxl/ corner of the kernel, whatever the
.clang-format template allows, works for me.

So if you can propose a .clang-format change that makes things prettier
in this case I would entertain it, but otherwise, life's too short.

Along those lines it would be nice if usages of __free caused the
automatic line break to happen after the "=" sign rather than later at
the arguments to kzalloc() like the above. However, I don't know the
template incantations to achieve that.

[..]
> > @@ -2861,9 +2851,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
> >  	struct device *dev;
> >  	int rc;
> >  
> > -	cxlr_pmem = cxl_pmem_region_alloc(cxlr);
> > -	if (IS_ERR(cxlr_pmem))
> > -		return PTR_ERR(cxlr_pmem);
> > +	rc = cxl_pmem_region_alloc(cxlr);
> > +	if (rc)
> > +		return rc;
> > +	cxlr_pmem = cxlr->cxlr_pmem;
> 
> Is the local variable worth retaining? It's only used twice.
> 
> The fact it's a clxr->cxlr* also seems rather like it could be named
> in a more compact fashion inside the cxl_region.  Would pmemr
> work for instance?

I am ambivalent about a cxlr_pmem rename. It would need to be a separate
patch to handle the 46 instances of cxlr_pmem.