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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 07/05/2024 15:57, Luca Fancellu wrote:
> 
> 
> Hi Michal,
> 
>>>
>>> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
>>> +                                         bool owner_dom_io,
>>> +                                         const char *role_str,
>>> +                                         const struct membank *shm_bank)
>>> +{
>>> +    paddr_t pbase, psize;
>>> +    int ret;
>>> +
>>> +    BUG_ON(!shm_bank);
>> not needed
>>
>>> +
>>> +    pbase = shm_bank->start;
>>> +    psize = shm_bank->size;
>> please add empty line here
> 
> Will do
>>>
>>> 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.

~Michal