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

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

[alison.schofield]

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;
+
 	rc = cxl_extended_linear_cache_resize(cxlr, res);
-	if (rc) {
+	if (rc && rc != -EOPNOTSUPP) {
 		/*
 		 * Failing to support extended linear cache region resize does not
 		 * prevent the region from functioning. Only causes cxl list showing
 		 * incorrect region size.
 		 */
 		dev_warn(cxlmd->dev.parent,
-			 "Extended linear cache calculation failed.\n");
+			 "Extended linear cache calculation failed rc:%d\n", rc);
 	}
 
 	rc = insert_resource(cxlrd->res, res);

base-commit: 26600bf10173beda5358d194ec425a1cfafa2fe2
-- 
2.37.3

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

[Li Ming]

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.

Other looks good to me.

Reviewed-by: Li Ming <ming.li@zohomail.com>


Ming

[snip]

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

[Alison Schofield]

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]
> 
> 

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

[Li Ming]

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]
>>
>>

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

[Alison Schofield]

On Wed, Mar 05, 2025 at 12:41:26PM +0800, Li Ming wrote:
> 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.

Thanks for reviewing Ming. I'm waffling on this. Leaning towards letting
it be because this patch is about quieting the noise in the absence of
ACPI_HMAT and that check is not creating any noise.

Watch for a v2.

--Alison

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