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

[Li Ming <ming.li@zohomail.com>]

On 3/5/2025 8:40 AM, Alison Schofield wrote:
> 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 ?
>
You are right, __construct_region() is only invoked during region auto-assembly. the size of cxled's hpa_range is already checked in init_hdm_decoder() if the cxl endpoint decoder is committed.


Ming

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