[U-Boot] [PATCH 0/2] lib: fdtdec: Couple of fies
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(-)
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");
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
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!
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)
+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
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.
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!
On Thursday 03 January 2019 11:44 PM, sjg@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!
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@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
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@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
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@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
participants (6)
-
J, KEERTHY -
Keerthy
-
Simon Glass -
Simon Glass -
sjg＠google.com
-
Stephen Warren