diff mbox series

[v3,19/20] cxl: add function for obtaining params from a region

Message ID 20240907081836.5801-20-alejandro.lucero-palau@amd.com (mailing list archive)
State Changes Requested
Headers show
Series cxl: add Type2 device support | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 25 this patch: 25
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 6 maintainers not CCed: alison.schofield@intel.com vishal.l.verma@intel.com ira.weiny@intel.com dave.jiang@intel.com jonathan.cameron@huawei.com dave@stgolabs.net
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 36 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 3 this patch: 3
netdev/source_inline success Was 0 now: 0

Commit Message

Lucero Palau, Alejandro Sept. 7, 2024, 8:18 a.m. UTC 
From: Alejandro Lucero <alucerop@amd.com>

A CXL region struct contains the physical address to work with.

Add a function for given a opaque cxl region struct returns the params
to be used for mapping such memory range.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/core/region.c | 16 ++++++++++++++++
 drivers/cxl/cxl.h         |  2 ++
 include/linux/cxl/cxl.h   |  2 ++
 3 files changed, 20 insertions(+)

Comments

Dave Jiang Sept. 13, 2024, 5:48 p.m. UTC | #1 
On 9/7/24 1:18 AM, 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 given a opaque cxl region struct returns the params
> to be used for mapping such memory range.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/region.c | 16 ++++++++++++++++
>  drivers/cxl/cxl.h         |  2 ++
>  include/linux/cxl/cxl.h   |  2 ++
>  3 files changed, 20 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 45b4891035a6..e0e2342bb1ed 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2662,6 +2662,22 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
>  	return ERR_PTR(rc);
>  }
>  
> +int cxl_get_region_params(struct cxl_region *region, resource_size_t *start,
> +			  resource_size_t *end)

Maybe just pass in a 'struct range' to be filled out instead of start/end?

> +{
> +	if (!region)
> +		return -ENODEV;
> +
> +	if (!region->params.res)
> +		return -ENOSPC;
> +
> +	*start = region->params.res->start;
> +	*end = region->params.res->end;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_region_params, CXL);

I think at least for me, it's better to have the introduction of helper functions to be with the code where it gets called to provide the full picture and thus make a better review. Especially if the function is fairly small. So maybe squash this patch and the next one. There may be a few other situations like this in this series worth the same consideration.
  
> +
>  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 120e961f2e31..b26833ff52c0 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -904,6 +904,8 @@ void cxl_coordinates_combine(struct access_coordinate *out,
>  bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
>  
>  int cxl_region_detach(struct cxl_endpoint_decoder *cxled);
> +int cxl_get_region_params(struct cxl_region *region, resource_size_t *start,
> +			  resource_size_t *end);
>  /*
>   * Unit test builds overrides this to __weak, find the 'strong' version
>   * of these symbols in tools/testing/cxl/.
> diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h
> index 169683d75030..ef3bd8329bd8 100644
> --- a/include/linux/cxl/cxl.h
> +++ b/include/linux/cxl/cxl.h
> @@ -76,4 +76,6 @@ struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
>  				     struct cxl_endpoint_decoder *cxled);
>  
>  int cxl_region_detach(struct cxl_endpoint_decoder *cxled);
> +int cxl_get_region_params(struct cxl_region *region, resource_size_t *start,
> +			  resource_size_t *end);
>  #endif
Alejandro Lucero Palau Sept. 16, 2024, 4:22 p.m. UTC | #2 
On 9/13/24 18:48, Dave Jiang wrote:
>
> On 9/7/24 1:18 AM, 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 given a opaque cxl region struct returns the params
>> to be used for mapping such memory range.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/region.c | 16 ++++++++++++++++
>>   drivers/cxl/cxl.h         |  2 ++
>>   include/linux/cxl/cxl.h   |  2 ++
>>   3 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 45b4891035a6..e0e2342bb1ed 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -2662,6 +2662,22 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
>>   	return ERR_PTR(rc);
>>   }
>>   
>> +int cxl_get_region_params(struct cxl_region *region, resource_size_t *start,
>> +			  resource_size_t *end)
> Maybe just pass in a 'struct range' to be filled out instead of start/end?
>

It makes sense.

I'll do it.


>> +{
>> +	if (!region)
>> +		return -ENODEV;
>> +
>> +	if (!region->params.res)
>> +		return -ENOSPC;
>> +
>> +	*start = region->params.res->start;
>> +	*end = region->params.res->end;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_region_params, CXL);
> I think at least for me, it's better to have the introduction of helper functions to be with the code where it gets called to provide the full picture and thus make a better review. Especially if the function is fairly small. So maybe squash this patch and the next one. There may be a few other situations like this in this series worth the same consideration.
>    


The next patch is not too big either, but I wanted to make things easier 
in that one.

Anyway, I'll do so.

Thanks


>> +
>>   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 120e961f2e31..b26833ff52c0 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -904,6 +904,8 @@ void cxl_coordinates_combine(struct access_coordinate *out,
>>   bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
>>   
>>   int cxl_region_detach(struct cxl_endpoint_decoder *cxled);
>> +int cxl_get_region_params(struct cxl_region *region, resource_size_t *start,
>> +			  resource_size_t *end);
>>   /*
>>    * Unit test builds overrides this to __weak, find the 'strong' version
>>    * of these symbols in tools/testing/cxl/.
>> diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h
>> index 169683d75030..ef3bd8329bd8 100644
>> --- a/include/linux/cxl/cxl.h
>> +++ b/include/linux/cxl/cxl.h
>> @@ -76,4 +76,6 @@ struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
>>   				     struct cxl_endpoint_decoder *cxled);
>>   
>>   int cxl_region_detach(struct cxl_endpoint_decoder *cxled);
>> +int cxl_get_region_params(struct cxl_region *region, resource_size_t *start,
>> +			  resource_size_t *end);
>>   #endif
diff mbox series

Patch

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 45b4891035a6..e0e2342bb1ed 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2662,6 +2662,22 @@  static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
 	return ERR_PTR(rc);
 }
 
+int cxl_get_region_params(struct cxl_region *region, resource_size_t *start,
+			  resource_size_t *end)
+{
+	if (!region)
+		return -ENODEV;
+
+	if (!region->params.res)
+		return -ENOSPC;
+
+	*start = region->params.res->start;
+	*end = region->params.res->end;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_get_region_params, 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 120e961f2e31..b26833ff52c0 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -904,6 +904,8 @@  void cxl_coordinates_combine(struct access_coordinate *out,
 bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
 
 int cxl_region_detach(struct cxl_endpoint_decoder *cxled);
+int cxl_get_region_params(struct cxl_region *region, resource_size_t *start,
+			  resource_size_t *end);
 /*
  * Unit test builds overrides this to __weak, find the 'strong' version
  * of these symbols in tools/testing/cxl/.
diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h
index 169683d75030..ef3bd8329bd8 100644
--- a/include/linux/cxl/cxl.h
+++ b/include/linux/cxl/cxl.h
@@ -76,4 +76,6 @@  struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
 				     struct cxl_endpoint_decoder *cxled);
 
 int cxl_region_detach(struct cxl_endpoint_decoder *cxled);
+int cxl_get_region_params(struct cxl_region *region, resource_size_t *start,
+			  resource_size_t *end);
 #endif