Re: [PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function

[Michal Orzel <michal.orzel@xxxxxxx>]

On 07/05/2024 16:15, Luca Fancellu wrote:
> 
> 
> Hi Michal,
> 
> 
>>>>>
>>>>> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>>>>                       const struct dt_device_node *node)
>>>>> {
>>>>> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct 
>>>>> kernel_info *kinfo,
>>>>>        if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
>>>>>            owner_dom_io = false;
>>>> Looking at owner_dom_io, why don't you move parsing role and setting 
>>>> owner_dom_io accordingly to handle_shared_mem_bank()?
>>>
>>> I think I wanted to keep all dt_* functions on the same level inside 
>>> process_shm, otherwise yes, I could
>>> pass down shm_node and do the reading of role_str in 
>>> handle_shared_mem_bank, or I could derive
>>> owner_dom_io from role_str being passed or not, something like:
>>>
>>> role_str = NULL;
>>> dt_property_read_string(shm_node, "role", &role_str)
>>>
>>> [inside handle_shared_mem_bank]:
>>> If ( role_str )
>>>    owner_dom_io = false;
>>>
>>> And pass only role_str to handle_shared_mem_bank.
>>>
>>> Is this comment to reduce the number of parameters passed? I guess it’s not 
>>> for where we call
>> In this series as well as the previous one you limit the number of arguments 
>> passed to quite a few functions.
>> So naturally I would expect it to be done here as well. owner_dom_io is used 
>> only by handle_shared_mem_bank, so it makes more sense to move parsing to 
>> this
>> function so that it is self-contained.
> 
> Ok I will, just to be on the same page here, you mean having 
> dt_property_read_string inside handle_shared_mem_bank?
> Or the above example would work for you as well? That one would have role_str 
> passed instead of shm_node.
I'm ok with the solution above to pass role_str.

~Michal