| Message ID | 1443108590-16871-2-git-send-email-p.marczak@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
On 09/24/2015 09:29 AM, Przemyslaw Marczak wrote: > After rework of lib/fdtdec.c by commit: > > commit 02464e386bb5f0a022c121f95ae75cf583759d95 > Author: Stephen Warren <swarren@nvidia.com> > Date: Thu Aug 6 15:31:02 2015 -0600 That'd usually be abbreviated as: Commit 02464e386bb5 "fdt: add new fdt address parsing functions". Of course, if you want to shame me that's justified too:-) Tracking down regressions sucks:-( > the function fdtdec_get_addr() doesn't work as previous, > because the implementation assumes that properties '#address-cells' > and '#size-cells' are equal to 1, which can be not true sometimes. "are equal to" should be "is at least"; the purpose of that rework was to support values greater than one. > The new API introduced fdtdec_get_addr_size_auto_parent() for the 'reg' > property parsing, but the implementation assumes, that #size-cells > can't be less than 1. > > This causes that the following children's 'reg' property can't be reached: > > parent@0x0 { > #address-cells = <1>; > #size-cells = <0>; > children@0x100 { > reg = < 0x100 >; > }; > }; > > Change the condition value from '1' to '0', which allows parsing property > with at least zero #size-cells, fixes the issue. > > Now, fdtdec_get_addr_size_auto_parent() works properly. Sorry about that. This patch, Acked-by: Stephen Warren <swarren@nvidia.com> (but not tested, but since this allows a previously failing case, it's hard to see how this patch could cause any problems.)
Hello Stephen, On 09/24/2015 07:14 PM, Stephen Warren wrote: > On 09/24/2015 09:29 AM, Przemyslaw Marczak wrote: >> After rework of lib/fdtdec.c by commit: >> >> commit 02464e386bb5f0a022c121f95ae75cf583759d95 >> Author: Stephen Warren <swarren@nvidia.com> >> Date: Thu Aug 6 15:31:02 2015 -0600 > > That'd usually be abbreviated as: > > Commit 02464e386bb5 "fdt: add new fdt address parsing functions". Ok, I will update the commit message. > > Of course, if you want to shame me that's justified too:-) Tracking down > regressions sucks:-( > Oh no no... maybe a little :) >> the function fdtdec_get_addr() doesn't work as previous, >> because the implementation assumes that properties '#address-cells' >> and '#size-cells' are equal to 1, which can be not true sometimes. > > "are equal to" should be "is at least"; the purpose of that rework was > to support values greater than one. > But it describe the fdtdec_get_addr(), which calls fdtdec_get_addr_size_fixed(...) and for this call we have: na = sizeof(fdt_addr_t) / sizeof(fdt32_t) == 1 ns = sizeof(fdt_size_t) / sizeof(fdt32_t) == 1 This is consistent with the description for this function in include/fdtdec.h. >> The new API introduced fdtdec_get_addr_size_auto_parent() for the 'reg' >> property parsing, but the implementation assumes, that #size-cells >> can't be less than 1. >> >> This causes that the following children's 'reg' property can't be >> reached: >> >> parent@0x0 { >> #address-cells = <1>; >> #size-cells = <0>; >> children@0x100 { >> reg = < 0x100 >; >> }; >> }; >> >> Change the condition value from '1' to '0', which allows parsing property >> with at least zero #size-cells, fixes the issue. >> >> Now, fdtdec_get_addr_size_auto_parent() works properly. > > Sorry about that. This patch, Don't worry, no one is infallible :) > > Acked-by: Stephen Warren <swarren@nvidia.com> > > (but not tested, but since this allows a previously failing case, it's > hard to see how this patch could cause any problems.) > This just fixes the problem, which I noticed, but it looks, that it shouldn't break other things. Best regards,
On 09/25/2015 02:35 AM, Przemyslaw Marczak wrote: > Hello Stephen, > > On 09/24/2015 07:14 PM, Stephen Warren wrote: >> On 09/24/2015 09:29 AM, Przemyslaw Marczak wrote: >>> After rework of lib/fdtdec.c by commit: >>> >>> commit 02464e386bb5f0a022c121f95ae75cf583759d95 >>> Author: Stephen Warren <swarren@nvidia.com> >>> Date: Thu Aug 6 15:31:02 2015 -0600 >> >> That'd usually be abbreviated as: >> >> Commit 02464e386bb5 "fdt: add new fdt address parsing functions". > > Ok, I will update the commit message. > >> Of course, if you want to shame me that's justified too:-) Tracking down >> regressions sucks:-( > > Oh no no... maybe a little :) > >>> the function fdtdec_get_addr() doesn't work as previous, >>> because the implementation assumes that properties '#address-cells' >>> and '#size-cells' are equal to 1, which can be not true sometimes. >> >> "are equal to" should be "is at least"; the purpose of that rework was >> to support values greater than one. >> > > But it describe the fdtdec_get_addr(), which calls > > fdtdec_get_addr_size_fixed(...) > > and for this call we have: > > na = sizeof(fdt_addr_t) / sizeof(fdt32_t) == 1 > > ns = sizeof(fdt_size_t) / sizeof(fdt32_t) == 1 > > This is consistent with the description for this function in > include/fdtdec.h. Ah yes; I was thinking of the core function fdtdec_get_addr_size_fixed(). The description you gave seems correct.
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 9f0b65d..9cf57b9 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -149,7 +149,7 @@ fdt_addr_t fdtdec_get_addr_size_auto_parent(const void *blob, int parent, } ns = fdt_size_cells(blob, parent); - if (ns < 1) { + if (ns < 0) { debug("(bad #size-cells)\n"); return FDT_ADDR_T_NONE; }
After rework of lib/fdtdec.c by commit: commit 02464e386bb5f0a022c121f95ae75cf583759d95 Author: Stephen Warren <swarren@nvidia.com> Date: Thu Aug 6 15:31:02 2015 -0600 the function fdtdec_get_addr() doesn't work as previous, because the implementation assumes that properties '#address-cells' and '#size-cells' are equal to 1, which can be not true sometimes. The new API introduced fdtdec_get_addr_size_auto_parent() for the 'reg' property parsing, but the implementation assumes, that #size-cells can't be less than 1. This causes that the following children's 'reg' property can't be reached: parent@0x0 { #address-cells = <1>; #size-cells = <0>; children@0x100 { reg = < 0x100 >; }; }; Change the condition value from '1' to '0', which allows parsing property with at least zero #size-cells, fixes the issue. Now, fdtdec_get_addr_size_auto_parent() works properly. Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com> --- lib/fdtdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)