| Message ID | 1386901910-1016-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
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? > + 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. Ian.
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.
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 ) + 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. - */ + */ if ( na > 2 && memcmp(range, addr, na * 4) != 0 ) return DT_BAD_ADDR; @@ -856,7 +868,7 @@ static const struct dt_bus dt_busses[] = { .name = "default", .addresses = "reg", - .match = NULL, + .match = dt_bus_default_match, .count_cells = dt_bus_default_count_cells, .map = dt_bus_default_map, .translate = dt_bus_default_translate, @@ -871,7 +883,6 @@ static const struct dt_bus *dt_match_bus(const struct dt_device_node *np) for ( i = 0; i < ARRAY_SIZE(dt_busses); i++ ) if ( !dt_busses[i].match || dt_busses[i].match(np) ) return &dt_busses[i]; - BUG(); return NULL; } @@ -890,7 +901,10 @@ static const __be32 *dt_get_address(const struct dt_device_node *dev, parent = dt_get_parent(dev); if ( parent == NULL ) return NULL; + bus = dt_match_bus(parent); + if ( !bus ) + return NULL; bus->count_cells(dev, &na, &ns); if ( !DT_CHECK_ADDR_COUNT(na) ) @@ -994,6 +1008,8 @@ static u64 __dt_translate_address(const struct dt_device_node *dev, if ( parent == NULL ) goto bail; bus = dt_match_bus(parent); + if ( !bus ) + goto bail; /* Count address cells & copy address locally */ bus->count_cells(dev, &na, &ns); @@ -1026,6 +1042,11 @@ static u64 __dt_translate_address(const struct dt_device_node *dev, /* Get new parent bus and counts */ pbus = dt_match_bus(parent); + if ( pbus == NULL ) + { + dt_printk("DT: %s is not a valid bus\n", parent->full_name); + break; + } pbus->count_cells(dev, &pna, &pns); if ( !DT_CHECK_COUNTS(pna, pns) ) { @@ -1164,6 +1185,8 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev) return 0; bus = dt_match_bus(parent); + if ( !bus ) + return 0; bus->count_cells(dev, &na, &ns); if ( !DT_CHECK_COUNTS(na, ns) )
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(-)