Re: [PATCH] cxl/region: Quiet some dev_warn()s in extended linear cache setup

[Alison Schofield <alison.schofield@intel.com>]

On Tue, Mar 04, 2025 at 01:14:47PM +0800, Li Ming wrote:
> On 3/4/2025 12:33 PM, alison.schofield@intel.com wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > Extended Linear Cache (ELC) setup code emits a dev_warn(), "Extended
> > linear cache calculation failed." for issues found while setting up
> > the ELC.
> >
> > For platforms without CONFIG_ACPI_HMAT, every auto region setup will
> > emit the warning because the default !ACPI_HMAT return value is
> > EOPNOTSUPP. Suppress it by skipping the warn for EOPNOTSUPP. Change
> > the EOPNOTSUPP in the actual ELC failure path to ENXIO.
> >
> > In a much less likely path, the dev_warn() is emitted if the size of
> > the region resource is NULL. Move that size check out of the ELC setup
> > code since NULL resource size is unrelated to the ELC setup and should
> > cause an immediate failure to construct the auto region.
> >
> > For good measure, add the rc value to the dev_warn(). It will either
> > be the -ENOENT returned by HMAT if the mem target is not found, or
> > the -ENXIO from the region driver calculation.
> >
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/region.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 8537b6a9ca18..2d2d2f221902 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -3235,13 +3235,10 @@ static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
> >  	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> >  	struct cxl_region_params *p = &cxlr->params;
> >  	int nid = phys_to_target_node(res->start);
> > -	resource_size_t size, cache_size, start;
> > +	resource_size_t size = resource_size(res);
> > +	resource_size_t cache_size, start;
> >  	int rc;
> >  
> > -	size = resource_size(res);
> > -	if (!size)
> > -		return -EINVAL;
> > -
> >  	rc = cxl_acpi_get_extended_linear_cache_size(res, nid, &cache_size);
> >  	if (rc)
> >  		return rc;
> > @@ -3253,7 +3250,7 @@ static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
> >  		dev_warn(&cxlr->dev,
> >  			 "Extended Linear Cache size %pa != CXL size %pa. No Support!",
> >  			 &cache_size, &size);
> > -		return -EOPNOTSUPP;
> > +		return -ENXIO;
> >  	}
> >  
> >  	/*
> > @@ -3304,15 +3301,18 @@ static int __construct_region(struct cxl_region *cxlr,
> >  	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
> >  				    dev_name(&cxlr->dev));
> >  
> > +	if (!resource_size(res))
> > +		return -EINVAL;
> > +
> 
> Hi Alison,
> 
> 
> There is a 'res = kmalloc(sizeof(*res), GFP_KERNEL);' in __construct_region() before this return, I think that you should release 'res' before returning -EINVAL.

Thanks for the review. I zoomed out a bit and can't find how
we can get here without a valid hpa_range which makes me
want to remove the check altogher. Then again it's harmless,
and not in any performance path.

Checking with DaveJ ?


> 
> Other looks good to me.
> 
> Reviewed-by: Li Ming <ming.li@zohomail.com>
> 
> 
> Ming
> 
> [snip]
> 
>