Re: [PATCH] xen/dts: Don't translate invalid address

[Julien Grall <julien.grall@linaro.org>]

On 01/06/2014 03:24 PM, Ian Campbell wrote:
> On Fri, 2013-12-13 at 02:31 +0000, Julien Grall wrote:
>> ePAR specifies that if the property "ranges" doesn't exist in a bus node:
>>
>> "it is assumed that no mapping exists between children of node and the parent
>> address space".
>>
>> Modify dt_number_of_address to check if the list of ranges are valid. Return
>> 0 (ie there is zero range) if the list is not valid.
>>
>> This patch has been tested on the Arndale where the bug can occur with the
>> '/hdmi' node.
>>
>> Reported-by: <tsahee@gmx.com>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>
>> This patch is a bug fix for Xen 4.4. Without it, Xen can't boot on the Arndale
>> because it's unable to translate a wrong address.
>> ---
>>   xen/common/device_tree.c |   31 +++++++++++++++++++++++++++----
>>   1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 84e709d..9b9a348 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -93,7 +93,7 @@ struct dt_bus
>>   {
>>       const char *name;
>>       const char *addresses;
>> -    int (*match)(const struct dt_device_node *parent);
>> +    bool_t (*match)(const struct dt_device_node *parent);
>>       void (*count_cells)(const struct dt_device_node *child,
>>                           int *addrc, int *sizec);
>>       u64 (*map)(__be32 *addr, const __be32 *range, int na, int ns, int pna);
>> @@ -793,6 +793,18 @@ int dt_n_size_cells(const struct dt_device_node *np)
>>   /*
>>    * Default translator (generic bus)
>>    */
>> +static bool_t dt_bus_default_match(const struct dt_device_node *parent)
>> +{
>> +    /* Root node doesn't have "ranges" property */
>> +    if ( parent->parent == NULL )
>
> Calling the parameter "parent" led to me confusedly wondering why it was
> the grandparent which mattered. I suppose "parent" in this sense means
> the "parent bus" as opposed to parent node? Or does it just fall out of
> the fact that in the caller it is the parent which is passed through?
>
> Can we call it something else, like "bus" or "node" or something else
> appropriate?


Right, the name is confusing. It took me few minutes, even if I wrote 
the code, to understand it :). I blindly copied the code from Linux 
of_bus structure.

The best name would be "node", because the function is trying to find if 
the parameters is a bus or not.

>> +        return 1;
>> +
>> +    /* The default bus is only used when the "ranges" property exists.
>> +     * Otherwise we can't translate the address
>> +     */
>> +    return (dt_get_property(parent, "ranges", NULL) != NULL);
>> +}
>> +
>>   static void dt_bus_default_count_cells(const struct dt_device_node *dev,
>>                                   int *addrc, int *sizec)
>>   {
>> @@ -819,7 +831,7 @@ static u64 dt_bus_default_map(__be32 *addr, const __be32 *range,
>>        * If the number of address cells is larger than 2 we assume the
>>        * mapping doesn't specify a physical address. Rather, the address
>>        * specifies an identifier that must match exactly.
>> -     */
>> +      */
>
> Spurious w/s change.
>
> Other that those two changes the patch looks good.

I will fix it.



-- 
Julien Grall