Re: [PATCH v2 2/5] cxl: Introduce to_{ram,pmem}_{res,perf}() helpers

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

On 1/23/25 21:04, Dan Williams wrote:

<snip>

> xa_for_each(dsmas_xa, index, dent) {
>>> -		if (resource_size(&cxlds->ram_res) &&
>>> -		    range_contains(&ram_range, &dent->dpa_range))
>>> -			update_perf_entry(dev, dent, &mds->ram_perf);
>>> -		else if (resource_size(&cxlds->pmem_res) &&
>>> -			 range_contains(&pmem_range, &dent->dpa_range))
>>> -			update_perf_entry(dev, dent, &mds->pmem_perf);
>>> -		else
>>> -			dev_dbg(dev, "no partition for dsmas dpa: %pra\n",
>>> -				&dent->dpa_range);
>>> +		for (int i = 0; i < ARRAY_SIZE(partition); i++) {
>>> +			const struct resource *res = partition[i];
>>> +			struct range range = {
>>> +				.start = res->start,
>>> +				.end = res->end,
>>> +			};
>>> +
>>> +			if (range_contains(&range, &dent->dpa_range))
>>> +				update_perf_entry(dev, dent, perf[i]);
>> This is checking if range contains dent->dpa_range.
>>
>> I think it is just the opposite.
> This looks like an equivalent conversion to me, what am I missing?


I guess I was confused with dpa_range in dent and assumed that was the 
full device DPA. But now after studying the code in more detail, I see 
it is right ... but confusing :-)


If I'm not wrong again, that range_contains should really be ranges 
being equal, what range_contains does also cover ...


It would help some comment explaining what the function is doing with 
the information coming from two different sources, one being the device 
CDAT, with potentially performance numbers per partition, and the other 
being the mailbox GET_PARTITION_INFO command.


Anyway, apologies for the noise.


<snip>

>>> @@ -567,6 +578,9 @@ static bool dpa_perf_contains(struct cxl_dpa_perf *perf,
>>>    		.end = dpa_res->end,
>>>    	};
>>>    
>>> +	if (!perf)
>>> +		return false;
>>> +
>> This change seems to be an improvement or hardening. Not against doing
>> it, but seizing the change, the function can be simplified using the
>> parameter without any local variable.
> No, it's not pure hardening, it is actively avoiding NULL pointer
> de-references introduced by the new scheme to not track empty
> partitions.
>
> I.e the new to_{ram,pmem}_perf() helpers return NULL when the partition
> is zero-sized. Previously this code path would do range checks on empty
> partitions.


That's true.


But I do not see the reason for using that local variable that I know is 
not related with your change.


<snip>

>>>    
>>>    	if (!cxlds->media_ready) {
>>>    		cxlds->dpa_res = DEFINE_RES_MEM(0, 0);
>>> -		cxlds->ram_res = DEFINE_RES_MEM(0, 0);
>>> -		cxlds->pmem_res = DEFINE_RES_MEM(0, 0);
>>> +		*ram_res = DEFINE_RES_MEM(0, 0);
>>> +		*pmem_res = DEFINE_RES_MEM(0, 0);
>>
>> This is a good example for the discussion about the  patch hardening
>> resource_contains. The initialization seems fine but IORESOURCE_UNSET
>> not used.
> To the contrary, I think these changes are an example of "updating
> resource_contains() to check for zero is a band-aid that does not fix
> the root problem".
>

I agree, but it does not preclude some resource initialization as you do 
here not containing the IORESOURCE_UNSET flag and some other code 
triggering the same problem I faced, so the hardening of 
resource_contains still useful, IMO. But I'll tell Bjorn Helgaas that 
patch is not required anymore for CXL leaving to him the decision to 
apply it if he considers so, following the concerns you expressed there.

>> it could be argued the resource is set, but it is a zero-size resource
>> leading to problems in current CXL code.
> I challenge you to find problems in current CXL code after these
> partition reworks.


That's an unfair challenge ... you removed resource_contains ... :-)


Let me think I got some credit for that after my heads-up ;-)