| Message ID | 1460042230-15205-3-git-send-email-mugunthanvnm@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Joe Hershberger |
| Headers | show |
+Stephen Hi Mugunthan, On 7 April 2016 at 09:17, Mugunthan V N <mugunthanvnm@ti.com> wrote: > Size cell and address cell should be read from the parent node > and should not assume with data structures as an example > TI DRA7xx SoC is enabled as 64bit as there is LPAE support > but the addresses specified in DT are all 32 bit sizes. So > changing the code to read from parent node instead of > calculations. I don't understand this. Can you please reword it and shorten the sentences? > > Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com> > --- > lib/fdtdec.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > index 70acc29..8a5fb8c 100644 > --- a/lib/fdtdec.c > +++ b/lib/fdtdec.c > @@ -88,15 +88,20 @@ fdt_addr_t fdtdec_get_addr_size_fixed(const void *blob, int node, > const fdt32_t *prop_addr, *prop_size, *prop_after_size; > int len; > fdt_addr_t addr; > + int parent; > > debug("%s: %s: ", __func__, prop_name); > > - if (na > (sizeof(fdt_addr_t) / sizeof(fdt32_t))) { > + parent = fdt_parent_offset(blob, node); This is a very slow function. I hope this changes is not needed. > + > + na = fdt_address_cells(blob, parent); > + if (na < 1) { > debug("(na too large for fdt_addr_t type)\n"); > return FDT_ADDR_T_NONE; > } > > - if (ns > (sizeof(fdt_size_t) / sizeof(fdt32_t))) { > + ns = fdt_size_cells(blob, parent); > + if (ns < 0) { > debug("(ns too large for fdt_size_t type)\n"); > return FDT_ADDR_T_NONE; > } > -- > 2.8.1.101.g72d917a > Regards, Simon
On 04/09/2016 12:35 PM, Simon Glass wrote: > +Stephen > > Hi Mugunthan, > > On 7 April 2016 at 09:17, Mugunthan V N <mugunthanvnm@ti.com> wrote: >> Size cell and address cell should be read from the parent node >> and should not assume with data structures as an example >> TI DRA7xx SoC is enabled as 64bit as there is LPAE support >> but the addresses specified in DT are all 32 bit sizes. So >> changing the code to read from parent node instead of >> calculations. > > I don't understand this. Can you please reword it and shorten the sentences? >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c >> index 70acc29..8a5fb8c 100644 >> --- a/lib/fdtdec.c >> +++ b/lib/fdtdec.c >> @@ -88,15 +88,20 @@ fdt_addr_t fdtdec_get_addr_size_fixed(const void *blob, int node, >> const fdt32_t *prop_addr, *prop_size, *prop_after_size; >> int len; >> fdt_addr_t addr; >> + int parent; >> >> debug("%s: %s: ", __func__, prop_name); >> >> - if (na > (sizeof(fdt_addr_t) / sizeof(fdt32_t))) { >> + parent = fdt_parent_offset(blob, node); > > This is a very slow function. I hope this changes is not needed. > >> + >> + na = fdt_address_cells(blob, parent); >> + if (na < 1) { >> debug("(na too large for fdt_addr_t type)\n"); >> return FDT_ADDR_T_NONE; >> } >> >> - if (ns > (sizeof(fdt_size_t) / sizeof(fdt32_t))) { >> + ns = fdt_size_cells(blob, parent); >> + if (ns < 0) { >> debug("(ns too large for fdt_size_t type)\n"); >> return FDT_ADDR_T_NONE; >> } The entire point of fdtdec_get_addr_size_fixed() is for use-cases where na and ns are known and fixed ahead of time, or have already been retrieved from the parent node by the caller. This patch is incorrect. I expect the correct fix is to call e.g. fdtdec_get_addr_size_auto_parent() or fdtdec_get_addr_size_auto_noparent() from somewhere, rather than calling fdtdec_get_addr_size_fixed().
Stephen On Sunday 10 April 2016 09:11 AM, Stephen Warren wrote: > On 04/09/2016 12:35 PM, Simon Glass wrote: >> +Stephen >> >> Hi Mugunthan, >> >> On 7 April 2016 at 09:17, Mugunthan V N <mugunthanvnm@ti.com> wrote: >>> Size cell and address cell should be read from the parent node >>> and should not assume with data structures as an example >>> TI DRA7xx SoC is enabled as 64bit as there is LPAE support >>> but the addresses specified in DT are all 32 bit sizes. So >>> changing the code to read from parent node instead of >>> calculations. >> >> I don't understand this. Can you please reword it and shorten the >> sentences? > >>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c >>> index 70acc29..8a5fb8c 100644 >>> --- a/lib/fdtdec.c >>> +++ b/lib/fdtdec.c >>> @@ -88,15 +88,20 @@ fdt_addr_t fdtdec_get_addr_size_fixed(const void >>> *blob, int node, >>> const fdt32_t *prop_addr, *prop_size, *prop_after_size; >>> int len; >>> fdt_addr_t addr; >>> + int parent; >>> >>> debug("%s: %s: ", __func__, prop_name); >>> >>> - if (na > (sizeof(fdt_addr_t) / sizeof(fdt32_t))) { >>> + parent = fdt_parent_offset(blob, node); >> >> This is a very slow function. I hope this changes is not needed. >> >>> + >>> + na = fdt_address_cells(blob, parent); >>> + if (na < 1) { >>> debug("(na too large for fdt_addr_t type)\n"); >>> return FDT_ADDR_T_NONE; >>> } >>> >>> - if (ns > (sizeof(fdt_size_t) / sizeof(fdt32_t))) { >>> + ns = fdt_size_cells(blob, parent); >>> + if (ns < 0) { >>> debug("(ns too large for fdt_size_t type)\n"); >>> return FDT_ADDR_T_NONE; >>> } > > The entire point of fdtdec_get_addr_size_fixed() is for use-cases where > na and ns are known and fixed ahead of time, or have already been > retrieved from the parent node by the caller. This patch is incorrect. I > expect the correct fix is to call e.g. > fdtdec_get_addr_size_auto_parent() or > fdtdec_get_addr_size_auto_noparent() from somewhere, rather than calling > fdtdec_get_addr_size_fixed(). Will fix thos in v2 by using fdtdec_get_addr_size_auto_parent() Regards Mugunthan V N
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 70acc29..8a5fb8c 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -88,15 +88,20 @@ fdt_addr_t fdtdec_get_addr_size_fixed(const void *blob, int node, const fdt32_t *prop_addr, *prop_size, *prop_after_size; int len; fdt_addr_t addr; + int parent; debug("%s: %s: ", __func__, prop_name); - if (na > (sizeof(fdt_addr_t) / sizeof(fdt32_t))) { + parent = fdt_parent_offset(blob, node); + + na = fdt_address_cells(blob, parent); + if (na < 1) { debug("(na too large for fdt_addr_t type)\n"); return FDT_ADDR_T_NONE; } - if (ns > (sizeof(fdt_size_t) / sizeof(fdt32_t))) { + ns = fdt_size_cells(blob, parent); + if (ns < 0) { debug("(ns too large for fdt_size_t type)\n"); return FDT_ADDR_T_NONE; }
Size cell and address cell should be read from the parent node and should not assume with data structures as an example TI DRA7xx SoC is enabled as 64bit as there is LPAE support but the addresses specified in DT are all 32 bit sizes. So changing the code to read from parent node instead of calculations. Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com> --- lib/fdtdec.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)