diff mbox series

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

Message ID 20241202171222.62595-27-alejandro.lucero-palau@amd.com (mailing list archive)
State Superseded
Headers show
Series cxl: add type2 device basic support | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Lucero Palau, Alejandro Dec. 2, 2024, 5:12 p.m. UTC 
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(+)

Comments

Zhi Wang Dec. 3, 2024, 6:53 p.m. UTC | #1 
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.

> +	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
Alejandro Lucero Palau Dec. 9, 2024, 9:48 a.m. UTC | #2 
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.

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
Zhi Wang Dec. 9, 2024, 4:29 p.m. UTC | #3 
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
>
Alejandro Lucero Palau Dec. 9, 2024, 5:47 p.m. UTC | #4 
On 12/9/24 16:29, Zhi Wang wrote:
> 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.
>   


It makes sense.

I'll add it. I was close to send v7 ... :-)

A bit more of work but it does worth it.

Thanks!


>> 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
diff mbox series

Patch

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;
+
+	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