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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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
dt_property_read_string isn’t it?

Cheers,
Luca