| Message ID | 1402919103-29642-9-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 16 Jun 2014, Ian Campbell wrote: > Instead use fdt_node_check_compatible from libfdt. Unfortunately the two functions are not equivalent: fdt_node_check_compatible uses memcmp while device_tree_node_compatible uses strcasecmp that ignores cases. At the very least we should make a note of this in the commit message. > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > xen/arch/arm/bootfdt.c | 28 ++-------------------------- > 1 file changed, 2 insertions(+), 26 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index a80055c..8ab45c9 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -31,30 +31,6 @@ static bool_t __init device_tree_node_matches(const void *fdt, int node, > && (name[match_len] == '@' || name[match_len] == '\0'); > } > > -static bool_t __init device_tree_node_compatible(const void *fdt, int node, > - const char *match) > -{ > - int len, l; > - int mlen; > - const void *prop; > - > - mlen = strlen(match); > - > - prop = fdt_getprop(fdt, node, "compatible", &len); > - if ( prop == NULL ) > - return 0; > - > - while ( len > 0 ) { > - if ( !dt_compat_cmp(prop, match) ) > - return 1; > - l = strlen(prop) + 1; > - prop += l; > - len -= l; > - } > - > - return 0; > -} > - > static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells, > u32 size_cells, u64 *start, u64 *size) > { > @@ -261,8 +237,8 @@ static int __init early_scan_node(const void *fdt, > { > if ( device_tree_node_matches(fdt, node, "memory") ) > process_memory_node(fdt, node, name, address_cells, size_cells); > - else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) || > - device_tree_node_compatible(fdt, node, "multiboot,module" )) > + else if ( fdt_node_check_compatible(fdt, node, "xen,multiboot-module" ) || > + fdt_node_check_compatible(fdt, node, "multiboot,module" )) > process_multiboot_node(fdt, node, name, address_cells, size_cells); > else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") ) > process_chosen_node(fdt, node, name, address_cells, size_cells); > -- > 1.7.10.4 >
On Wed, 2014-06-18 at 15:43 +0100, Stefano Stabellini wrote: > On Mon, 16 Jun 2014, Ian Campbell wrote: > > Instead use fdt_node_check_compatible from libfdt. > > Unfortunately the two functions are not equivalent: > fdt_node_check_compatible uses memcmp while device_tree_node_compatible > uses strcasecmp that ignores cases. I hadn't spotted this. They can't both be spec complaint I think. I expect fdt_node_check_compatible (from libfdt) is more likely to be the one which is correct. > At the very least we should make a note of this in the commit message. Will do.
On 06/18/2014 03:47 PM, Ian Campbell wrote: > On Wed, 2014-06-18 at 15:43 +0100, Stefano Stabellini wrote: >> On Mon, 16 Jun 2014, Ian Campbell wrote: >>> Instead use fdt_node_check_compatible from libfdt. >> >> Unfortunately the two functions are not equivalent: >> fdt_node_check_compatible uses memcmp while device_tree_node_compatible >> uses strcasecmp that ignores cases. > > I hadn't spotted this. > > They can't both be spec complaint I think. I expect > fdt_node_check_compatible (from libfdt) is more likely to be the one > which is correct. In dt_device_is_compatible we use strcasecmp as Linux does. For Linux, it looks like the use a less restrictive way in generic code (i.e strcasecmp) and more restrictive on SPARC (i.e strcmp) I can't find anything in the spec to say which one is better. But I think we want to stay compatible with Linux we have to keep device_tree_node_compatible and use it in place of fdt_node_check_compatible. We could also modify the latter function for our convenience. Regards.
On Wed, 18 Jun 2014, Julien Grall wrote: > On 06/18/2014 03:47 PM, Ian Campbell wrote: > > On Wed, 2014-06-18 at 15:43 +0100, Stefano Stabellini wrote: > >> On Mon, 16 Jun 2014, Ian Campbell wrote: > >>> Instead use fdt_node_check_compatible from libfdt. > >> > >> Unfortunately the two functions are not equivalent: > >> fdt_node_check_compatible uses memcmp while device_tree_node_compatible > >> uses strcasecmp that ignores cases. > > > > I hadn't spotted this. > > > > They can't both be spec complaint I think. I expect > > fdt_node_check_compatible (from libfdt) is more likely to be the one > > which is correct. > > In dt_device_is_compatible we use strcasecmp as Linux does. > > For Linux, it looks like the use a less restrictive way in generic code > (i.e strcasecmp) and more restrictive on SPARC (i.e strcmp) > > I can't find anything in the spec to say which one is better. But I > think we want to stay compatible with Linux we have to keep > device_tree_node_compatible and use it in place of > fdt_node_check_compatible. > > We could also modify the latter function for our convenience. Who wants to open Pandora's box and send an email to the LKML?
On Wed, 2014-06-18 at 15:57 +0100, Julien Grall wrote: > On 06/18/2014 03:47 PM, Ian Campbell wrote: > > On Wed, 2014-06-18 at 15:43 +0100, Stefano Stabellini wrote: > >> On Mon, 16 Jun 2014, Ian Campbell wrote: > >>> Instead use fdt_node_check_compatible from libfdt. > >> > >> Unfortunately the two functions are not equivalent: > >> fdt_node_check_compatible uses memcmp while device_tree_node_compatible > >> uses strcasecmp that ignores cases. > > > > I hadn't spotted this. > > > > They can't both be spec complaint I think. I expect > > fdt_node_check_compatible (from libfdt) is more likely to be the one > > which is correct. > > In dt_device_is_compatible we use strcasecmp as Linux does. > > For Linux, it looks like the use a less restrictive way in generic code > (i.e strcasecmp) and more restrictive on SPARC (i.e strcmp) > > I can't find anything in the spec to say which one is better. But I > think we want to stay compatible with Linux we have to keep > device_tree_node_compatible and use it in place of > fdt_node_check_compatible. Urk, what a mess. This patch was pretty tangential to the series, I'll just drop it. Ian. > We could also modify the latter function for our convenience. > > Regards. >
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index a80055c..8ab45c9 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -31,30 +31,6 @@ static bool_t __init device_tree_node_matches(const void *fdt, int node, && (name[match_len] == '@' || name[match_len] == '\0'); } -static bool_t __init device_tree_node_compatible(const void *fdt, int node, - const char *match) -{ - int len, l; - int mlen; - const void *prop; - - mlen = strlen(match); - - prop = fdt_getprop(fdt, node, "compatible", &len); - if ( prop == NULL ) - return 0; - - while ( len > 0 ) { - if ( !dt_compat_cmp(prop, match) ) - return 1; - l = strlen(prop) + 1; - prop += l; - len -= l; - } - - return 0; -} - static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells, u32 size_cells, u64 *start, u64 *size) { @@ -261,8 +237,8 @@ static int __init early_scan_node(const void *fdt, { if ( device_tree_node_matches(fdt, node, "memory") ) process_memory_node(fdt, node, name, address_cells, size_cells); - else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) || - device_tree_node_compatible(fdt, node, "multiboot,module" )) + else if ( fdt_node_check_compatible(fdt, node, "xen,multiboot-module" ) || + fdt_node_check_compatible(fdt, node, "multiboot,module" )) process_multiboot_node(fdt, node, name, address_cells, size_cells); else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") ) process_chosen_node(fdt, node, name, address_cells, size_cells);
Instead use fdt_node_check_compatible from libfdt. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- xen/arch/arm/bootfdt.c | 28 ++-------------------------- 1 file changed, 2 insertions(+), 26 deletions(-)