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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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@xxxxxxx>
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> >
> > ---
> >
> > 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel