Re: [PATCH 1/4] powerpc/pseries/iommu: Update call to ibm,query-pe-dma-windows

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 23/06/2020 12:14, Leonardo Bras wrote:
> On Tue, 2020-06-23 at 11:12 +1000, Alexey Kardashevskiy wrote:
> [snip]
>>>>> +static int query_ddw_out_sz(struct device_node *par_dn)
>>>>
>>>> Can easily be folded into query_ddw().
>>>
>>> Sure, but it will get inlined by the compiler, and I think it reads
>>> better this way. 
>>> I mean, I understand you have a reason to think it's better to fold it
>>> in query_ddw(), and I would like to better understand that to improve
>>> my code in the future.
>>
>> You have numbers 5 and 6 (the number of parameters) twice in the file,
>> this is why I brought it up. query_ddw_out_sz() can potentially return
>> something else than 5 or 6 and you will have to change the callsite(s)
>> then, since these are not macros, this allows to think there may be more
>> places with 5 and 6. Dunno. A single function will simplify things imho.
> 
> That's a good point. Thanks!
> 
>>
>>
>>>>> +{
>>>>> +	int ret;
>>>>> +	u32 ddw_ext[3];
>>>>> +
>>>>> +	ret = of_property_read_u32_array(par_dn, "ibm,ddw-extensions",
>>>>> +					 &ddw_ext[0], 3);
>>>>> +	if (ret || ddw_ext[0] < 2 || ddw_ext[2] != 1)
>>>>
>>>> Oh that PAPR thing again :-/
>>>>
>>>> ===
>>>> The “ibm,ddw-extensions” property value is a list of integers the first
>>>> integer indicates the number of extensions implemented and subsequent
>>>> integers, one per extension, provide a value associated with that
>>>> extension.
>>>> ===
>>>>
>>>> So ddw_ext[0] is length.
>>>> Listindex==2 is for "reset" says PAPR and
>>>> Listindex==3 is for this new 64bit "largest_available_block".
>>>>
>>>> So I'd expect ddw_ext[2] to have the "reset" token and ddw_ext[3] to
>>>> have "1" for this new feature but indexes are smaller. I am confused.
>>>> Either way these "2" and "3" needs to be defined in macros, "0" probably
>>>> too.
>>>
>>> Remember these indexes are not C-like 0-starting indexes, where the
>>> size would be Listindex==1.
>>
>> Yeah I can see that is the assumption but out of curiosity - is it
>> written anywhere? Across PAPR, they index bytes from 1 but bits from 0 :-/
> 
> From LoPAR: 
> The “ibm,ddw-extensions” property value is a list of integers the first
> integer indicates the number of extensions implemented and subsequent
> integers, one per extension, provide a value associated with that
> extension.
> 
> And the list/table then shows extensions from 2 on onwards:
> List index 2 : Token of the ibm,reset-pe-dma-windows RTAS Call
> (...)


I means a place saying "we number things starting from 1 and not from
zero", this kind of thing. The code implementing the spec uses the C
language so it would make sense to count from zero, otoh the writer
probably did not write any code for ages :)



-- 
Alexey