netdev - Re: [PATCH v3 19/20] cxl: add function for obtaining params from a region

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7ec2eb7a-62fa-f340-1ee1-2c9fec7df75f@amd.com>
Date: Mon, 16 Sep 2024 17:22:05 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Dave Jiang <dave.jiang@...el.com>, alejandro.lucero-palau@....com,
 linux-cxl@...r.kernel.org, netdev@...r.kernel.org, dan.j.williams@...el.com,
 martin.habets@...inx.com, edward.cree@....com, davem@...emloft.net,
 kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com
Subject: Re: [PATCH v3 19/20] cxl: add function for obtaining params from a
 region


On 9/13/24 18:48, Dave Jiang wrote:
>
> On 9/7/24 1:18 AM, alejandro.lucero-palau@....com wrote:
>> From: Alejandro Lucero <alucerop@....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@....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