[U-Boot] [PATCH 0/2] lib: fdtdec: Couple of fies

[U-Boot] [PATCH 0/2] lib: fdtdec: Couple of fies

[Keerthy]

Couple of fixes for fdtdec_get_addr_size and fdtdec_get_addr_size_fixed
functions.

Keerthy (2):
  lib: fdtdec: fdtdec_get_addr_size_fixed remove checks
  lib: fdtdec: fixup fdtdec_get_addr_size

 lib/fdtdec.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH 1/2] lib: fdtdec: fdtdec_get_addr_size_fixed remove checks

[Keerthy]

With 8 bytes addressing even on 32 bit machines these checks
are no longer valid. Remove them.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 lib/fdtdec.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 6f8ec0d..18663ce 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -95,16 +95,6 @@ fdt_addr_t fdtdec_get_addr_size_fixed(const void *blob, int node,
 
 	debug("%s: %s: ", __func__, prop_name);
 
-	if (na > (sizeof(fdt_addr_t) / sizeof(fdt32_t))) {
-		debug("(na too large for fdt_addr_t type)\n");
-		return FDT_ADDR_T_NONE;
-	}
-
-	if (ns > (sizeof(fdt_size_t) / sizeof(fdt32_t))) {
-		debug("(ns too large for fdt_size_t type)\n");
-		return FDT_ADDR_T_NONE;
-	}
-
 	prop = fdt_getprop(blob, node, prop_name, &len);
 	if (!prop) {
 		debug("(not found)\n");
-- 
1.9.1

[U-Boot] [PATCH 2/2] lib: fdtdec: fixup fdtdec_get_addr_size

[Keerthy]

fix up fdtdec_get_addr_size to use fdtdec_get_addr_size_auto_noparent
so that the address cells and size cells are obtained from the
parent instead of going by the fixed length.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 lib/fdtdec.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 18663ce..11a30e1 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -178,11 +178,8 @@ fdt_addr_t fdtdec_get_addr_size_auto_noparent(const void *blob, int node,
 fdt_addr_t fdtdec_get_addr_size(const void *blob, int node,
 				const char *prop_name, fdt_size_t *sizep)
 {
-	int ns = sizep ? (sizeof(fdt_size_t) / sizeof(fdt32_t)) : 0;
-
-	return fdtdec_get_addr_size_fixed(blob, node, prop_name, 0,
-					  sizeof(fdt_addr_t) / sizeof(fdt32_t),
-					  ns, sizep, false);
+	return fdtdec_get_addr_size_auto_noparent(blob, node, prop_name, 0,
+						  sizep, false);
 }
 
 fdt_addr_t fdtdec_get_addr(const void *blob, int node, const char *prop_name)
-- 
1.9.1

[U-Boot] [PATCH 1/2] lib: fdtdec: fdtdec_get_addr_size_fixed remove checks

[Simon Glass]

On Fri, 21 Dec 2018 at 09:24, Keerthy <j-keerthy@ti.com> wrote:
>
> With 8 bytes addressing even on 32 bit machines these checks
> are no longer valid. Remove them.
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  lib/fdtdec.c | 10 ----------
>  1 file changed, 10 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/2] lib: fdtdec: fixup fdtdec_get_addr_size

[Simon Glass]

+Stephen

Hi Keethy,

On Fri, 21 Dec 2018 at 09:24, Keerthy <j-keerthy@ti.com> wrote:
>
> fix up fdtdec_get_addr_size to use fdtdec_get_addr_size_auto_noparent
> so that the address cells and size cells are obtained from the
> parent instead of going by the fixed length.

Why? Can you please expand this?

cc Stephen too.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

[U-Boot] [PATCH 2/2] lib: fdtdec: fixup fdtdec_get_addr_size

[Stephen Warren]

On 12/21/18 9:24 AM, Keerthy wrote:
> fix up fdtdec_get_addr_size to use fdtdec_get_addr_size_auto_noparent
> so that the address cells and size cells are obtained from the
> parent instead of going by the fixed length.

This patch makes perfect sense to me. However, I am worried about the 
potential existence of code that assumes the current fixed-size logic; 
in the past when fixing similar issues like this we've often run into 
code that was use "get addr" functions when it should have been using 
"get u32" functions and similar, which then broke when we fixed the 
implementation to do the right thing. I guess we should still apply the 
patch, and fix up any fallout as it appears.

[U-Boot] [PATCH 2/2] lib: fdtdec: fixup fdtdec_get_addr_size

[sjg at google.com]

On 12/21/18 9:24 AM, Keerthy wrote:
> fix up fdtdec_get_addr_size to use fdtdec_get_addr_size_auto_noparent
> so that the address cells and size cells are obtained from the
> parent instead of going by the fixed length.

This patch makes perfect sense to me. However, I am worried about the
potential existence of code that assumes the current fixed-size logic;
in the past when fixing similar issues like this we've often run into
code that was use "get addr" functions when it should have been using
"get u32" functions and similar, which then broke when we fixed the
implementation to do the right thing. I guess we should still apply the
patch, and fix up any fallout as it appears.

Applied to u-boot-dm/next, thanks!

[U-Boot] [PATCH 1/2] lib: fdtdec: fdtdec_get_addr_size_fixed remove checks

[sjg at google.com]

On Fri, 21 Dec 2018 at 09:24, Keerthy <j-keerthy@ti.com> wrote:
>
> With 8 bytes addressing even on 32 bit machines these checks
> are no longer valid. Remove them.
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  lib/fdtdec.c | 10 ----------
>  1 file changed, 10 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm/next, thanks!

[U-Boot] [PATCH 2/2] lib: fdtdec: fixup fdtdec_get_addr_size

[Keerthy]

On Thursday 03 January 2019 11:44 PM, sjg at google.com wrote:
> On 12/21/18 9:24 AM, Keerthy wrote:
>> fix up fdtdec_get_addr_size to use fdtdec_get_addr_size_auto_noparent
>> so that the address cells and size cells are obtained from the
>> parent instead of going by the fixed length.
> 
> This patch makes perfect sense to me. However, I am worried about the
> potential existence of code that assumes the current fixed-size logic;
> in the past when fixing similar issues like this we've often run into
> code that was use "get addr" functions when it should have been using
> "get u32" functions and similar, which then broke when we fixed the
> implementation to do the right thing. I guess we should still apply the
> patch, and fix up any fallout as it appears.

Thanks Simon!

> 
> Applied to u-boot-dm/next, thanks!
> 

[U-Boot] [PATCH 2/2] lib: fdtdec: fixup fdtdec_get_addr_size

[Simon Glass]

Hi Keerthy,

On Thu, 3 Jan 2019 at 21:39, Keerthy <j-keerthy@ti.com> wrote:
>
>
>
> On Thursday 03 January 2019 11:44 PM, sjg at google.com wrote:
> > On 12/21/18 9:24 AM, Keerthy wrote:
> >> fix up fdtdec_get_addr_size to use fdtdec_get_addr_size_auto_noparent
> >> so that the address cells and size cells are obtained from the
> >> parent instead of going by the fixed length.
> >
> > This patch makes perfect sense to me. However, I am worried about the
> > potential existence of code that assumes the current fixed-size logic;
> > in the past when fixing similar issues like this we've often run into
> > code that was use "get addr" functions when it should have been using
> > "get u32" functions and similar, which then broke when we fixed the
> > implementation to do the right thing. I guess we should still apply the
> > patch, and fix up any fallout as it appears.
>
> Thanks Simon!

Unfortunately this breaks the tests (make qcheck). Can you please take a look?

Regards,
Simon

[U-Boot] [PATCH 2/2] lib: fdtdec: fixup fdtdec_get_addr_size

[J, KEERTHY]

On 1/15/2019 6:23 AM, Simon Glass wrote:
> Hi Keerthy,
> 
> On Thu, 3 Jan 2019 at 21:39, Keerthy <j-keerthy@ti.com> wrote:
>>
>>
>>
>> On Thursday 03 January 2019 11:44 PM, sjg at google.com wrote:
>>> On 12/21/18 9:24 AM, Keerthy wrote:
>>>> fix up fdtdec_get_addr_size to use fdtdec_get_addr_size_auto_noparent
>>>> so that the address cells and size cells are obtained from the
>>>> parent instead of going by the fixed length.
>>>
>>> This patch makes perfect sense to me. However, I am worried about the
>>> potential existence of code that assumes the current fixed-size logic;
>>> in the past when fixing similar issues like this we've often run into
>>> code that was use "get addr" functions when it should have been using
>>> "get u32" functions and similar, which then broke when we fixed the
>>> implementation to do the right thing. I guess we should still apply the
>>> patch, and fix up any fallout as it appears.
>>
>> Thanks Simon!
> 
> Unfortunately this breaks the tests (make qcheck). Can you please take a look?


Simon,

Can you paste the logs? I am at u-boot master tip and my qcheck seems to 
err out.
https://pastebin.ubuntu.com/p/7Q53TMqxMQ/

Regards,
Keerthy
> 
> Regards,
> Simon
> 

[U-Boot] [PATCH 2/2] lib: fdtdec: fixup fdtdec_get_addr_size

[Simon Glass]

Hi Keerthy,

On Mon, 14 Jan 2019 at 18:52, J, KEERTHY <j-keerthy@ti.com> wrote:
>
>
>
> On 1/15/2019 6:23 AM, Simon Glass wrote:
> > Hi Keerthy,
> >
> > On Thu, 3 Jan 2019 at 21:39, Keerthy <j-keerthy@ti.com> wrote:
> >>
> >>
> >>
> >> On Thursday 03 January 2019 11:44 PM, sjg at google.com wrote:
> >>> On 12/21/18 9:24 AM, Keerthy wrote:
> >>>> fix up fdtdec_get_addr_size to use fdtdec_get_addr_size_auto_noparent
> >>>> so that the address cells and size cells are obtained from the
> >>>> parent instead of going by the fixed length.
> >>>
> >>> This patch makes perfect sense to me. However, I am worried about the
> >>> potential existence of code that assumes the current fixed-size logic;
> >>> in the past when fixing similar issues like this we've often run into
> >>> code that was use "get addr" functions when it should have been using
> >>> "get u32" functions and similar, which then broke when we fixed the
> >>> implementation to do the right thing. I guess we should still apply the
> >>> patch, and fix up any fallout as it appears.
> >>
> >> Thanks Simon!
> >
> > Unfortunately this breaks the tests (make qcheck). Can you please take a look?
>
>
> Simon,
>
> Can you paste the logs? I am at u-boot master tip and my qcheck seems to
> err out.
> https://pastebin.ubuntu.com/p/7Q53TMqxMQ/

You should get the tests running. In this case see README.sandbox.
Otherwise any patch you send may break U-Boot.

Regards,
Simon