Re: [PATCH v3] cxl/acpi: Cleanup __cxl_parse_cfmws()

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 30 Apr 2024 15:46:08 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Fri, 05 Apr 2024 15:05:50 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >   
> > > As a follow on to the recent rework of __cxl_parse_cfmws() to always
> > > return errors [1], use cleanup.h helpers to remove goto and other cleanups
> > > now that logging is moved to the cxl_parse_cfmws() wrapper.
> > > 
> > > This ends up adding more code than it deletes, but __cxl_parse_cfmws()
> > > itself does get smaller. The takeaway from the cond_no_free_ptr()
> > > discussion [2] was to not add new macros to handle the cases where
> > > no_free_ptr() is awkward, instead rework the code to have helpers and
> > > clearer delineation of responsibility.
> > > 
> > > Now one might say that  __free(del_cxl_resource) is excessive given it
> > > is immediately registered with add_or_reset_cxl_resource(). The
> > > rationale for keeping it is that it forces use of "no_free_ptr()" on the
> > > argument passed to add_or_reset_cxl_resource(). That in turn makes it
> > > clear that @res is NULL for the rest of the function which is part of
> > > the point of the cleanup helpers, to turn subtle use after free errors
> > > [3] into loud NULL pointer de-references.
> > > 
> > > Link: http://lore.kernel.org/r/170820177238.631006.1012639681618409284.stgit@dwillia2-xfh.jf.intel.com [1]
> > > Link: http://lore.kernel.org/r/CAHk-=whBVhnh=KSeBBRet=E7qJAwnPR_aj5em187Q3FiD+LXnA@mail.gmail.com [2]
> > > Link: http://lore.kernel.org/r/20230714093146.2253438-1-leitao@debian.org [3]
> > > Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > > Closes: http://lore.kernel.org/r/20240219124041.00002bda@Huawei.com
> > > Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> > > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> > 
> > I guess I may have missed the boat on reviewing this one, but if not
> > a few late comments inline.  
> > 
> > Jonathan
> > 
> >   
> > > ---
> > > Changes since v2:
> > > - Drop kvasprintf() usage in alloc_cxl_resource (Alison)
> > > 
> > >  drivers/cxl/acpi.c |   93 +++++++++++++++++++++++++++++-----------------------
> > >  drivers/cxl/cxl.h  |    5 +++
> > >  2 files changed, 56 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > index af5cb818f84d..32091379a97b 100644
> > > --- a/drivers/cxl/acpi.c
> > > +++ b/drivers/cxl/acpi.c  
> >   
> > > +
> > > +static struct resource *alloc_cxl_resource(resource_size_t base,
> > > +					   resource_size_t n, int id)
> > > +{
> > > +	struct resource *res __free(kfree) = kzalloc(sizeof(*res), GFP_KERNEL);
> > > +
> > > +	if (!res)
> > > +		return NULL;
> > > +
> > > +	res->start = base;
> > > +	res->end = base + n - 1;
> > > +	res->flags = IORESOURCE_MEM;
> > > +	res->name = kasprintf(GFP_KERNEL, "CXL Window %d", id);  
> > 
> > If you are going to call a function something as generic
> > as 'alloc_cxl_resource' I wouldn't expect to see such a specific string.
> > 
> > alloc_cxl_fmw_resource() would be fine with this.  
> 
> This name was chosen for symmetry with the existing "del_cxl_resource()",
> and it really does feed a generic "CXL resource" concept since the rest of
> the kernel does not care about the ACPI'ism of "CFMWS". The kernel
> considers these resources with IORES_DESC_CXL specified.

hmm. Ok. Smells funny, but fair enough.

> 
> >   
> > > +	if (!res->name)
> > > +		return NULL;
> > > +
> > > +	return no_free_ptr(res);
> > > +}  
> >   
> > > +
> > > +DEFINE_FREE(put_cxlrd, struct cxl_root_decoder *,
> > > +	    if (!IS_ERR_OR_NULL(_T)) put_device(&_T->cxlsd.cxld.dev))
> > > +DEFINE_FREE(del_cxl_resource, struct resource *, if (_T) del_cxl_resource(_T))
> > >  static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
> > >  			     struct cxl_cfmws_context *ctx)
> > >  {
> > >  	int target_map[CXL_DECODER_MAX_INTERLEAVE];
> > >  	struct cxl_port *root_port = ctx->root_port;
> > > -	struct resource *cxl_res = ctx->cxl_res;
> > >  	struct cxl_cxims_context cxims_ctx;
> > > -	struct cxl_root_decoder *cxlrd;
> > >  	struct device *dev = ctx->dev;
> > >  	cxl_calc_hb_fn cxl_calc_hb;
> > >  	struct cxl_decoder *cxld;
> > >  	unsigned int ways, i, ig;
> > > -	struct resource *res;
> > >  	int rc;
> > >  
> > >  	rc = cxl_acpi_cfmws_verify(dev, cfmws);
> > > -	if (rc) {
> > > -		dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
> > > -			cfmws->base_hpa,
> > > -			cfmws->base_hpa + cfmws->window_size - 1);  
> > 
> > Not sure how this is related to rest of the change.  
> 
> Recall that in 5c6224bfabbf ("cxl/acpi: Fix load failures due to single
> window creation failure") was a fix that also unified some of the print
> messages in cxl_parse_cfmws(). This follow-on cleanup finalizes that with
> conversion to scoped-based resource management and removal of redundant
> error messages.
> 
> Otherwise:
> 
> __cxl_parse_cfmws()
>     dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
>             cfmws->base_hpa,
>             cfmws->base_hpa + cfmws->window_size - 1);
> 
> ...duplicates:
> 
> cxl_parse_cfmws()
>     dev_err(dev,
>             "Failed to add decode range: [%#llx - %#llx] (%d)\n",
>             cfmws->base_hpa,
>             cfmws->base_hpa + cfmws->window_size - 1, rc);
Ah. I guess this is one of those 'other cleanups' in the commit
message.

Fair enough,

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Jonathan