Re: [RFC PATCH 05/13] cxl: fix check about pmem resource

[Alejandro Lucero Palau <alucerop@amd.com>]

On 5/17/24 15:40, Jonathan Cameron wrote:
> On Thu, 16 May 2024 09:11:54 +0100
> <alucerop@amd.com> wrote:
>
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Current check is using resource_size which counts on a range bigger than
>> 0. For a resource with start and end being 0, resource_size returns 1
>> and implying a false positive. Use the end not being zero as the new check.
>>
>> Note: If I´m not missing anything here, this should be extended to the
>> whole linux kernel where resource_size is being used in conditionals,
>> and where the likely right fix is to modify resource_size itself
>> checking for the range not being 0.
> The start and end being 0 is a valid resource of length 1 so that
> should not need fixing.


Uhmmm ... I have problems understanding this but I guess there's a good 
reason for it.


> These should have been initialized with DEFINE_RES_MEM()
> / DEFINE_RES_NAMED()
> That will happily set the .end to -1 resulting in a wrap around
> so that you get all bits set.


That makes sense, but I wonder if we should have some function for doing 
default initialization of those fields like this which some users, like 
an accelerator with no pmem, will likely leave untouched.

Maybe inside cxl_accel_state_create in patch2 something like this:

     cxlds->dpa_res = DEFINE_RES(0, -1);

     cxlds->ram_res = DEFINE_RES(0, -1);

     cxlds->pmem_res = DEFINE_RES(0, -1);


>
> Sometimes this gives slightly confusing range prints but otherwise
> I think it is fine.
>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/hdm.c    | 4 ++--
>>   drivers/cxl/core/memdev.c | 8 ++++----
>>   drivers/cxl/mem.c         | 2 +-
>>   3 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 47d9faf5897f..c5f70741d70a 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -432,12 +432,12 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
>>   	 * Only allow modes that are supported by the current partition
>>   	 * configuration
>>   	 */
>> -	if (mode == CXL_DECODER_PMEM && !resource_size(&cxlds->pmem_res)) {
>> +	if (mode == CXL_DECODER_PMEM && !cxlds->pmem_res.end) {
>>   		dev_dbg(dev, "no available pmem capacity\n");
>>   		rc = -ENXIO;
>>   		goto out;
>>   	}
>> -	if (mode == CXL_DECODER_RAM && !resource_size(&cxlds->ram_res)) {
>> +	if (mode == CXL_DECODER_RAM && !cxlds->ram_res.end) {
>>   		dev_dbg(dev, "no available ram capacity\n");
>>   		rc = -ENXIO;
>>   		goto out;
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 0336b3f14f4a..b61d57d0d4f4 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -197,14 +197,14 @@ static int cxl_get_poison_by_memdev(struct cxl_memdev *cxlmd)
>>   	int rc = 0;
>>   
>>   	/* CXL 3.0 Spec 8.2.9.8.4.1 Separate pmem and ram poison requests */
>> -	if (resource_size(&cxlds->pmem_res)) {
>> +	if (cxlds->pmem_res.end) {
>>   		offset = cxlds->pmem_res.start;
>>   		length = resource_size(&cxlds->pmem_res);
>>   		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
>>   		if (rc)
>>   			return rc;
>>   	}
>> -	if (resource_size(&cxlds->ram_res)) {
>> +	if (cxlds->ram_res.end) {
>>   		offset = cxlds->ram_res.start;
>>   		length = resource_size(&cxlds->ram_res);
>>   		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
>> @@ -266,7 +266,7 @@ static int __cxl_dpa_to_region(struct device *dev, void *arg)
>>   		return 0;
>>   
>>   	cxled = to_cxl_endpoint_decoder(dev);
>> -	if (!cxled->dpa_res || !resource_size(cxled->dpa_res))
>> +	if (!cxled->dpa_res || !cxled->dpa_res->end)
>>   		return 0;
>>   
>>   	if (dpa > cxled->dpa_res->end || dpa < cxled->dpa_res->start)
>> @@ -302,7 +302,7 @@ static int cxl_validate_poison_dpa(struct cxl_memdev *cxlmd, u64 dpa)
>>   	if (!IS_ENABLED(CONFIG_DEBUG_FS))
>>   		return 0;
>>   
>> -	if (!resource_size(&cxlds->dpa_res)) {
>> +	if (!cxlds->dpa_res.end) {
>>   		dev_dbg(cxlds->dev, "device has no dpa resource\n");
>>   		return -EINVAL;
>>   	}
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 6dc2bf1e2b1a..a168343d2d4d 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -174,7 +174,7 @@ static int cxl_mem_probe(struct device *dev)
>>   	if (rc)
>>   		return rc;
>>   
>> -	if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) {
>> +	if (cxlds->pmem_res.end && IS_ENABLED(CONFIG_CXL_PMEM)) {
>>   		rc = devm_cxl_add_nvdimm(cxlmd);
>>   		if (rc == -ENODEV)
>>   			dev_info(dev, "PMEM disabled by platform\n");