Re: [PATCH v6 26/28] cxl: add function for obtaining region range

[Zhi Wang <zhiw@nvidia.com>]

On Mon, 9 Dec 2024 09:48:01 +0000
Alejandro Lucero Palau <alucerop@amd.com> wrote:

> 
> On 12/3/24 18:53, Zhi Wang wrote:
> > On Mon, 2 Dec 2024 17:12:20 +0000
> > <alejandro.lucero-palau@amd.com> wrote:
> >
> >> From: Alejandro Lucero <alucerop@amd.com>
> >>
> >> A CXL region struct contains the physical address to work with.
> >>
> >> Add a function for getting the cxl region range to be used for mapping
> >> such memory range.
> >>
> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> >> ---
> >>   drivers/cxl/core/region.c | 15 +++++++++++++++
> >>   drivers/cxl/cxl.h         |  1 +
> >>   include/cxl/cxl.h         |  1 +
> >>   3 files changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> >> index 5cb7991268ce..021e9b373cdd 100644
> >> --- a/drivers/cxl/core/region.c
> >> +++ b/drivers/cxl/core/region.c
> >> @@ -2667,6 +2667,21 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
> >>   	return ERR_PTR(rc);
> >>   }
> >>   
> >> +int cxl_get_region_range(struct cxl_region *region, struct range *range)
> >> +{
> >> +	if (!region)
> >> +		return -ENODEV;
> >> +
> > I am leaning towards having a WARN_ON_ONCE() above.
> >
> 
> Not sure. The call is quite simple and to check the error should be 
> enough for understanding what is going on.
> 

A sane caller would never calls this function with region == NULL. If
that happens, it mostly means the caller itself has been problematic
already, e.g. stack overflow. someone wrongly overwrites the pointer and
the caller is not even aware of it. Thus it calls this function with
region == NULL.

In this case, we should not let it silently slip away. We should have
WARN_ON or WARN_ON_ONCE to notify the admin that the system might be
unstable now and some weird stuff happened and memory was
randomly over-written.

It is different from the second check, in which the caller is sane and get
a error code.
 
> In this case any error implies a problem with a previous call when 
> creating the region which was not likely checked for errors.
> 
> And if a log is necessary, I think a WARN_ON should be used instead.
> 
> 
> >> +	if (!region->params.res)
> >> +		return -ENOSPC;
> >> +
> >> +	range->start = region->params.res->start;
> >> +	range->end = region->params.res->end;
> >> +
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL_NS_GPL(cxl_get_region_range, CXL);
> >> +
> >>   static ssize_t __create_region_show(struct cxl_root_decoder *cxlrd, char *buf)
> >>   {
> >>   	return sysfs_emit(buf, "region%u\n", atomic_read(&cxlrd->region_id));
> >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> >> index cc9e3d859fa6..32d2bd0520d4 100644
> >> --- a/drivers/cxl/cxl.h
> >> +++ b/drivers/cxl/cxl.h
> >> @@ -920,6 +920,7 @@ void cxl_coordinates_combine(struct access_coordinate *out,
> >>   
> >>   bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
> >>   
> >> +int cxl_get_region_range(struct cxl_region *region, struct range *range);
> >>   /*
> >>    * Unit test builds overrides this to __weak, find the 'strong' version
> >>    * of these symbols in tools/testing/cxl/.
> >> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> >> index 14be26358f9c..0ed9e32f25dd 100644
> >> --- a/include/cxl/cxl.h
> >> +++ b/include/cxl/cxl.h
> >> @@ -65,4 +65,5 @@ struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
> >>   				     bool no_dax);
> >>   
> >>   int cxl_accel_region_detach(struct cxl_endpoint_decoder *cxled);
> >> +int cxl_get_region_range(struct cxl_region *region, struct range *range);
> >>   #endif
>