| Message ID | 20220703022458.1.Ib77c21bb412664634b823b243f6edb9453f3e845@changeid |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ" | expand |
Hi, On Sun, 3 Jul 2022 at 02:25, Simon Glass <sjg@chromium.org> wrote: > > The fdt_path_offset() function is slow since it must scan the tree. > This substantial overhead now applies to all boards. > > The original code may not be ideal but it is fit for purpose and is only > needed on a few boards. > > We should revert this in time for the release. > > This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > configs/am65x_evm_a53_defconfig | 1 + > configs/evb-ast2600_defconfig | 1 + > configs/sama7g5ek_mmc1_defconfig | 1 + > configs/sama7g5ek_mmc_defconfig | 1 + > lib/Kconfig | 7 +++++++ > lib/fdtdec.c | 7 +++++-- > 6 files changed, 16 insertions(+), 2 deletions(-) Please also see the context here: https://patchwork.ozlabs.org/project/uboot/patch/20201111142605.17034-1-a-govindraju@ti.com/ Regards, Simon
On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: > Hi, > > On Sun, 3 Jul 2022 at 02:25, Simon Glass <sjg@chromium.org> wrote: > > > > The fdt_path_offset() function is slow since it must scan the tree. > > This substantial overhead now applies to all boards. > > > > The original code may not be ideal but it is fit for purpose and is only > > needed on a few boards. > > > > We should revert this in time for the release. > > > > This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > configs/am65x_evm_a53_defconfig | 1 + > > configs/evb-ast2600_defconfig | 1 + > > configs/sama7g5ek_mmc1_defconfig | 1 + > > configs/sama7g5ek_mmc_defconfig | 1 + > > lib/Kconfig | 7 +++++++ > > lib/fdtdec.c | 7 +++++-- > > 6 files changed, 16 insertions(+), 2 deletions(-) > > Please also see the context here: > > https://patchwork.ozlabs.org/project/uboot/patch/20201111142605.17034-1-a-govindraju@ti.com/ Previously when we've had issues of making the fast path slow, people have posted measurements of before/after in order to demonstrate the problem. Can we please get some logs of before/after and various possible solutions? Thanks.
Hi Tom, On Sun, 3 Jul 2022 at 06:43, Tom Rini <trini@konsulko.com> wrote: > > On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: > > Hi, > > > > On Sun, 3 Jul 2022 at 02:25, Simon Glass <sjg@chromium.org> wrote: > > > > > > The fdt_path_offset() function is slow since it must scan the tree. > > > This substantial overhead now applies to all boards. > > > > > > The original code may not be ideal but it is fit for purpose and is only > > > needed on a few boards. > > > > > > We should revert this in time for the release. > > > > > > This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > --- > > > > > > configs/am65x_evm_a53_defconfig | 1 + > > > configs/evb-ast2600_defconfig | 1 + > > > configs/sama7g5ek_mmc1_defconfig | 1 + > > > configs/sama7g5ek_mmc_defconfig | 1 + > > > lib/Kconfig | 7 +++++++ > > > lib/fdtdec.c | 7 +++++-- > > > 6 files changed, 16 insertions(+), 2 deletions(-) > > > > Please also see the context here: > > > > https://patchwork.ozlabs.org/project/uboot/patch/20201111142605.17034-1-a-govindraju@ti.com/ > > Previously when we've had issues of making the fast path slow, people > have posted measurements of before/after in order to demonstrate the > problem. Can we please get some logs of before/after and various > possible solutions? Thanks. Well this code is not needed at all for all but four boards. It does a very expensive check of the DT and this can happen before relocation, or is SPL. I don't have a board to test with at present, but I expect it would cost 10s of milliseconds on AT91, for example. Also, it just isn't needed, which is why I suggest a revert. Regards, Simon
On 05/07/2022 11.47, Simon Glass wrote: > Hi Tom, > > On Sun, 3 Jul 2022 at 06:43, Tom Rini <trini@konsulko.com> wrote: >> >> On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: >>> Hi, >>> >>> On Sun, 3 Jul 2022 at 02:25, Simon Glass <sjg@chromium.org> wrote: >>>> >>>> The fdt_path_offset() function is slow since it must scan the tree. >>>> This substantial overhead now applies to all boards. >>>> >>>> The original code may not be ideal but it is fit for purpose and is only >>>> needed on a few boards. >>>> >>>> We should revert this in time for the release. >>>> >>>> This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>> --- >>>> >>>> configs/am65x_evm_a53_defconfig | 1 + >>>> configs/evb-ast2600_defconfig | 1 + >>>> configs/sama7g5ek_mmc1_defconfig | 1 + >>>> configs/sama7g5ek_mmc_defconfig | 1 + >>>> lib/Kconfig | 7 +++++++ >>>> lib/fdtdec.c | 7 +++++-- >>>> 6 files changed, 16 insertions(+), 2 deletions(-) >>> >>> Please also see the context here: >>> >>> https://patchwork.ozlabs.org/project/uboot/patch/20201111142605.17034-1-a-govindraju@ti.com/ >> >> Previously when we've had issues of making the fast path slow, people >> have posted measurements of before/after in order to demonstrate the >> problem. Can we please get some logs of before/after and various >> possible solutions? Thanks. > > Well this code is not needed at all for all but four boards. Three. One board seems to enable that config for no reason at all. And it wouldn't work on that board, because the code was fragile and error prone. See my detailed explanations in the original patch thread. It does a > very expensive check of the DT and this can happen before relocation, > or is SPL. I don't have a board to test with at present, but I expect > it would cost 10s of milliseconds on AT91, for example. As I've said before, I prefer code which is correct out-of-the-box. I also prefer simpler code with less ifdefs (yeah yeah, IS_ENABLED...) and fewer configuration options to worry about. The three boards which have aliases where the leaf nodes have duplicate names also happen to enable some other unrelated magic config knob which happens to make the phandle comparison work. So at the very least the code should stop comparing phandles and just compare the node offsets; whether that check should be under a config knob can be discussed (certainly that config knob should not have PHANDLE in it), but, again, as I've said, it should be opt-out, and preferably with a build-time check that verifies that no two aliases point at same basenames. _If_ that 10s of milliseconds figure is true, there are other things one could do at build time. Say have some pass over the dtb which simply adds "u-boot,seq" properties to the target nodes of aliases, using that if present, or add a "back pointer" "u-boot,alias" property one could compare to name. Rasmus
Hi Rasmus, On Tue, 5 Jul 2022 at 07:47, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 05/07/2022 11.47, Simon Glass wrote: > > Hi Tom, > > > > On Sun, 3 Jul 2022 at 06:43, Tom Rini <trini@konsulko.com> wrote: > >> > >> On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: > >>> Hi, > >>> > >>> On Sun, 3 Jul 2022 at 02:25, Simon Glass <sjg@chromium.org> wrote: > >>>> > >>>> The fdt_path_offset() function is slow since it must scan the tree. > >>>> This substantial overhead now applies to all boards. > >>>> > >>>> The original code may not be ideal but it is fit for purpose and is only > >>>> needed on a few boards. > >>>> > >>>> We should revert this in time for the release. > >>>> > >>>> This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > >>>> > >>>> Signed-off-by: Simon Glass <sjg@chromium.org> > >>>> --- > >>>> > >>>> configs/am65x_evm_a53_defconfig | 1 + > >>>> configs/evb-ast2600_defconfig | 1 + > >>>> configs/sama7g5ek_mmc1_defconfig | 1 + > >>>> configs/sama7g5ek_mmc_defconfig | 1 + > >>>> lib/Kconfig | 7 +++++++ > >>>> lib/fdtdec.c | 7 +++++-- > >>>> 6 files changed, 16 insertions(+), 2 deletions(-) > >>> > >>> Please also see the context here: > >>> > >>> https://patchwork.ozlabs.org/project/uboot/patch/20201111142605.17034-1-a-govindraju@ti.com/ > >> > >> Previously when we've had issues of making the fast path slow, people > >> have posted measurements of before/after in order to demonstrate the > >> problem. Can we please get some logs of before/after and various > >> possible solutions? Thanks. > > > > Well this code is not needed at all for all but four boards. > > Three. One board seems to enable that config for no reason at all. And > it wouldn't work on that board, because the code was fragile and error > prone. See my detailed explanations in the original patch thread. > > It does a > > very expensive check of the DT and this can happen before relocation, > > or is SPL. I don't have a board to test with at present, but I expect > > it would cost 10s of milliseconds on AT91, for example. > > As I've said before, I prefer code which is correct out-of-the-box. I > also prefer simpler code with less ifdefs (yeah yeah, IS_ENABLED...) and > fewer configuration options to worry about. The three boards which have > aliases where the leaf nodes have duplicate names also happen to enable > some other unrelated magic config knob which happens to make the phandle > comparison work. So at the very least the code should stop comparing > phandles and just compare the node offsets; whether that check should be I don't disagree that this is a bit odd, but it is efficient. Another approach here would be to add better documentation since the option doesn't quite work as advertised. > under a config knob can be discussed (certainly that config knob should > not have PHANDLE in it), but, again, as I've said, it should be opt-out, > and preferably with a build-time check that verifies that no two aliases > point at same basenames. Opt-out means that everything pays the penalty, though. This is real corner case. Arguably the device tree should be updated to avoid this problem. > > _If_ that 10s of milliseconds figure is true, there are other things one > could do at build time. Say have some pass over the dtb which simply > adds "u-boot,seq" properties to the target nodes of aliases, using that > if present, or add a "back pointer" "u-boot,alias" property one could > compare to name. That's a nice idea. The point of my revert was to get something in for the release. I fully expect some sort of change to go in afterwards, but I don't think people were aware of the impact of your patch (see context link above). So I still favour a revert, for now. Regards, Simon
Hi Tom, On Tue, 5 Jul 2022 at 10:42, Simon Glass <sjg@chromium.org> wrote: > > Hi Rasmus, > > On Tue, 5 Jul 2022 at 07:47, Rasmus Villemoes > <rasmus.villemoes@prevas.dk> wrote: > > > > On 05/07/2022 11.47, Simon Glass wrote: > > > Hi Tom, > > > > > > On Sun, 3 Jul 2022 at 06:43, Tom Rini <trini@konsulko.com> wrote: > > >> > > >> On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: > > >>> Hi, > > >>> > > >>> On Sun, 3 Jul 2022 at 02:25, Simon Glass <sjg@chromium.org> wrote: > > >>>> > > >>>> The fdt_path_offset() function is slow since it must scan the tree. > > >>>> This substantial overhead now applies to all boards. > > >>>> > > >>>> The original code may not be ideal but it is fit for purpose and is only > > >>>> needed on a few boards. > > >>>> > > >>>> We should revert this in time for the release. > > >>>> > > >>>> This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > > >>>> > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org> > > >>>> --- > > >>>> > > >>>> configs/am65x_evm_a53_defconfig | 1 + > > >>>> configs/evb-ast2600_defconfig | 1 + > > >>>> configs/sama7g5ek_mmc1_defconfig | 1 + > > >>>> configs/sama7g5ek_mmc_defconfig | 1 + > > >>>> lib/Kconfig | 7 +++++++ > > >>>> lib/fdtdec.c | 7 +++++-- > > >>>> 6 files changed, 16 insertions(+), 2 deletions(-) > > >>> > > >>> Please also see the context here: > > >>> > > >>> https://patchwork.ozlabs.org/project/uboot/patch/20201111142605.17034-1-a-govindraju@ti.com/ > > >> > > >> Previously when we've had issues of making the fast path slow, people > > >> have posted measurements of before/after in order to demonstrate the > > >> problem. Can we please get some logs of before/after and various > > >> possible solutions? Thanks. > > > > > > Well this code is not needed at all for all but four boards. > > > > Three. One board seems to enable that config for no reason at all. And > > it wouldn't work on that board, because the code was fragile and error > > prone. See my detailed explanations in the original patch thread. > > > > It does a > > > very expensive check of the DT and this can happen before relocation, > > > or is SPL. I don't have a board to test with at present, but I expect > > > it would cost 10s of milliseconds on AT91, for example. > > > > As I've said before, I prefer code which is correct out-of-the-box. I > > also prefer simpler code with less ifdefs (yeah yeah, IS_ENABLED...) and > > fewer configuration options to worry about. The three boards which have > > aliases where the leaf nodes have duplicate names also happen to enable > > some other unrelated magic config knob which happens to make the phandle > > comparison work. So at the very least the code should stop comparing > > phandles and just compare the node offsets; whether that check should be > > I don't disagree that this is a bit odd, but it is efficient. > > Another approach here would be to add better documentation since the > option doesn't quite work as advertised. > > > under a config knob can be discussed (certainly that config knob should > > not have PHANDLE in it), but, again, as I've said, it should be opt-out, > > and preferably with a build-time check that verifies that no two aliases > > point at same basenames. > > Opt-out means that everything pays the penalty, though. This is real > corner case. Arguably the device tree should be updated to avoid this > problem. > > > > > _If_ that 10s of milliseconds figure is true, there are other things one > > could do at build time. Say have some pass over the dtb which simply > > adds "u-boot,seq" properties to the target nodes of aliases, using that > > if present, or add a "back pointer" "u-boot,alias" property one could > > compare to name. > > That's a nice idea. > > The point of my revert was to get something in for the release. I > fully expect some sort of change to go in afterwards, but I don't > think people were aware of the impact of your patch (see context link > above). > > So I still favour a revert, for now. It looks like this didn't make it for the release. I'm not sure if this is causing problems. Shall I pick it up for the upcoming release? Regards, Simon
On Sat, Jul 30, 2022 at 07:27:26PM -0600, Simon Glass wrote: > Hi Tom, > > On Tue, 5 Jul 2022 at 10:42, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Rasmus, > > > > On Tue, 5 Jul 2022 at 07:47, Rasmus Villemoes > > <rasmus.villemoes@prevas.dk> wrote: > > > > > > On 05/07/2022 11.47, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Sun, 3 Jul 2022 at 06:43, Tom Rini <trini@konsulko.com> wrote: > > > >> > > > >> On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: > > > >>> Hi, > > > >>> > > > >>> On Sun, 3 Jul 2022 at 02:25, Simon Glass <sjg@chromium.org> wrote: > > > >>>> > > > >>>> The fdt_path_offset() function is slow since it must scan the tree. > > > >>>> This substantial overhead now applies to all boards. > > > >>>> > > > >>>> The original code may not be ideal but it is fit for purpose and is only > > > >>>> needed on a few boards. > > > >>>> > > > >>>> We should revert this in time for the release. > > > >>>> > > > >>>> This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > > > >>>> > > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org> > > > >>>> --- > > > >>>> > > > >>>> configs/am65x_evm_a53_defconfig | 1 + > > > >>>> configs/evb-ast2600_defconfig | 1 + > > > >>>> configs/sama7g5ek_mmc1_defconfig | 1 + > > > >>>> configs/sama7g5ek_mmc_defconfig | 1 + > > > >>>> lib/Kconfig | 7 +++++++ > > > >>>> lib/fdtdec.c | 7 +++++-- > > > >>>> 6 files changed, 16 insertions(+), 2 deletions(-) > > > >>> > > > >>> Please also see the context here: > > > >>> > > > >>> https://patchwork.ozlabs.org/project/uboot/patch/20201111142605.17034-1-a-govindraju@ti.com/ > > > >> > > > >> Previously when we've had issues of making the fast path slow, people > > > >> have posted measurements of before/after in order to demonstrate the > > > >> problem. Can we please get some logs of before/after and various > > > >> possible solutions? Thanks. > > > > > > > > Well this code is not needed at all for all but four boards. > > > > > > Three. One board seems to enable that config for no reason at all. And > > > it wouldn't work on that board, because the code was fragile and error > > > prone. See my detailed explanations in the original patch thread. > > > > > > It does a > > > > very expensive check of the DT and this can happen before relocation, > > > > or is SPL. I don't have a board to test with at present, but I expect > > > > it would cost 10s of milliseconds on AT91, for example. > > > > > > As I've said before, I prefer code which is correct out-of-the-box. I > > > also prefer simpler code with less ifdefs (yeah yeah, IS_ENABLED...) and > > > fewer configuration options to worry about. The three boards which have > > > aliases where the leaf nodes have duplicate names also happen to enable > > > some other unrelated magic config knob which happens to make the phandle > > > comparison work. So at the very least the code should stop comparing > > > phandles and just compare the node offsets; whether that check should be > > > > I don't disagree that this is a bit odd, but it is efficient. > > > > Another approach here would be to add better documentation since the > > option doesn't quite work as advertised. > > > > > under a config knob can be discussed (certainly that config knob should > > > not have PHANDLE in it), but, again, as I've said, it should be opt-out, > > > and preferably with a build-time check that verifies that no two aliases > > > point at same basenames. > > > > Opt-out means that everything pays the penalty, though. This is real > > corner case. Arguably the device tree should be updated to avoid this > > problem. > > > > > > > > _If_ that 10s of milliseconds figure is true, there are other things one > > > could do at build time. Say have some pass over the dtb which simply > > > adds "u-boot,seq" properties to the target nodes of aliases, using that > > > if present, or add a "back pointer" "u-boot,alias" property one could > > > compare to name. > > > > That's a nice idea. > > > > The point of my revert was to get something in for the release. I > > fully expect some sort of change to go in afterwards, but I don't > > think people were aware of the impact of your patch (see context link > > above). > > > > So I still favour a revert, for now. > > It looks like this didn't make it for the release. I'm not sure if > this is causing problems. > > Shall I pick it up for the upcoming release? I don't think we should pick up the revert as I don't think there's agreement that reverting this is the right step forward among all of the interested parties. One thing I want to know at this point is, was the problematic device tree accepted upstream, or did they also say that 2 nodes with the same name but different paths is wrong, don't do that?
On 31/07/2022 15.28, Tom Rini wrote: > On Sat, Jul 30, 2022 at 07:27:26PM -0600, Simon Glass wrote: >> Hi Tom, >> >> Shall I pick it up for the upcoming release? > > I don't think we should pick up the revert as I don't think there's > agreement that reverting this is the right step forward among all of the > interested parties. > > One thing I want to know at this point is, was the problematic device > tree accepted upstream, or did they also say that 2 nodes with the same > name but different paths is wrong, don't do that? There's no problematic device tree, having two nodes with the same (base)name is perfectly fine and in fact sometimes expected/required (in the cases where a node name is standardized; e.g. having two identical eeproms on different i2c buses both would/should be called eeprom@50). What Simon is concerned about is that the translation from full path to blob offset is now done unconditionally, and that might be costly. I'm not sure it is (and didn't know that it could be), but as I've said repeatedly, I prefer code that works out-of-the-box, and for the boards that do need this extra check (because just looking at the basename is not enough), the fact that the previous code worked seemed to be pure luck - because those dtbs were compiled with -@ due to some other CONFIG_ knob being set. And again, involving phandles at all is what make the code fragile, so a revert that reinstates a CONFIG_ knob with PHANDLE in the name is not a good way forward, assuming there is even anything to fix. So if the performance thing is real, sure, we can introduce a CONFIG_something, but only if at the same time we have a sanity check at build-time that detects if one should enable/disable that option (depending on whether we make it "positive" or "negative"). Something like this seems to do the trick, but I haven't looked at hooking it up in the build system: === scripts/check-alias-homonyms.py === #!/usr/bin/python3 import sys sys.path.insert(0, 'scripts/dtc/pylibfdt') sys.path.insert(1, 'tools') from dtoc import fdt ret = 0 for name in sys.argv[1:]: dtb = fdt.FdtScan(name) aliasnode = dtb.GetNode("/aliases") if not aliasnode: continue basenames = dict() for prop in aliasnode.props.values(): alias = prop.name path = prop.value base = path[path.rfind('/')+1:] if base in basenames: basenames[base].append(alias) else: basenames[base] = [alias] for base, aliases in basenames.items(): if len(aliases) == 1: continue print("Warning: In %s, the aliases %s all point at nodes with the basename '%s' - consider (en/dis)abling CONFIG_..." % (name, ",".join(aliases), base)) ret = 1 sys.exit(ret) === I imagine this being run over CONFIG_OF_LIST when CONFIG_... has the value that disables the fdt_path_offset() call. For concreteness, something like config ASSUME_ALIAS_HOMONYMS # or whatever better name one can come up with bool "Assume there may be nodes pointed to by aliases in DT that have identical basenames" help In most cases, the nodes pointed to by aliases in the device tree all have different basenames. When this is satisfied, the fdtdec_get_alias_seq() function can avoid a somewhat expensive full walk of the dtb when looking for an alias for a given node. If the device tree for your board does have aliases pointing at nodes with the same basenames (but of course different full paths), that full walk is necessary for correctness, and you can then enable this option. plus - if (offset != fdt_path_offset(blob, prop)) + if (IS_ENABLED(CONFIG_ASSUME_ALIAS_HOMONYMS) && offset != fdt_path_offset(blob, prop)) I have no idea what running the above python script on the dtb(s) in the common case of !CONFIG_ASSUME_ALIAS_HOMONYMS adds to buildtime, but I do believe that before eliding some code that is necessary for correctness in the general case, we need buildtime verification that it's ok. Rasmus
Hi Rasmus, On Mon, 1 Aug 2022 at 07:13, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 31/07/2022 15.28, Tom Rini wrote: > > On Sat, Jul 30, 2022 at 07:27:26PM -0600, Simon Glass wrote: > >> Hi Tom, > >> > > >> Shall I pick it up for the upcoming release? > > > > I don't think we should pick up the revert as I don't think there's > > agreement that reverting this is the right step forward among all of the > > interested parties. > > > > One thing I want to know at this point is, was the problematic device > > tree accepted upstream, or did they also say that 2 nodes with the same > > name but different paths is wrong, don't do that? Returning to this old thread... > > There's no problematic device tree, having two nodes with the same > (base)name is perfectly fine and in fact sometimes expected/required (in > the cases where a node name is standardized; e.g. having two identical > eeproms on different i2c buses both would/should be called eeprom@50). > > What Simon is concerned about is that the translation from full path to > blob offset is now done unconditionally, and that might be costly. I'm > not sure it is (and didn't know that it could be), but as I've said > repeatedly, I prefer code that works out-of-the-box, and for the boards > that do need this extra check (because just looking at the basename is > not enough), the fact that the previous code worked seemed to be pure > luck - because those dtbs were compiled with -@ due to some other > CONFIG_ knob being set. And again, involving phandles at all is what > make the code fragile, so a revert that reinstates a CONFIG_ knob with > PHANDLE in the name is not a good way forward, assuming there is even > anything to fix. > > So if the performance thing is real, sure, we can introduce a > CONFIG_something, but only if at the same time we have a sanity check at > build-time that detects if one should enable/disable that option > (depending on whether we make it "positive" or "negative"). Something > like this seems to do the trick, but I haven't looked at hooking it up > in the build system: > > === scripts/check-alias-homonyms.py === > #!/usr/bin/python3 > > import sys > > sys.path.insert(0, 'scripts/dtc/pylibfdt') > sys.path.insert(1, 'tools') > > from dtoc import fdt > > ret = 0 > > for name in sys.argv[1:]: > dtb = fdt.FdtScan(name) > aliasnode = dtb.GetNode("/aliases") > if not aliasnode: > continue > basenames = dict() > for prop in aliasnode.props.values(): > alias = prop.name > path = prop.value > base = path[path.rfind('/')+1:] > if base in basenames: > basenames[base].append(alias) > else: > basenames[base] = [alias] > for base, aliases in basenames.items(): > if len(aliases) == 1: > continue > print("Warning: In %s, the aliases %s all point at nodes with > the basename '%s' - consider (en/dis)abling CONFIG_..." % (name, > ",".join(aliases), base)) > ret = 1 > > sys.exit(ret) > === > > I imagine this being run over CONFIG_OF_LIST when CONFIG_... has the > value that disables the fdt_path_offset() call. For concreteness, > something like > > config ASSUME_ALIAS_HOMONYMS # or whatever better name one can come up with > bool "Assume there may be nodes pointed to by aliases in DT that have > identical basenames" > help > In most cases, the nodes pointed to by aliases in the device tree > all have different basenames. When this is satisfied, the > fdtdec_get_alias_seq() function can avoid a somewhat expensive full walk > of the dtb when looking for an alias for a given node. If the device > tree for your board does have aliases pointing at nodes with the same > basenames (but of course different full paths), that full walk is > necessary for correctness, and you can then enable this option. > > plus > > - if (offset != fdt_path_offset(blob, prop)) > + if (IS_ENABLED(CONFIG_ASSUME_ALIAS_HOMONYMS) && offset != > fdt_path_offset(blob, prop)) > > I have no idea what running the above python script on the dtb(s) in the > common case of !CONFIG_ASSUME_ALIAS_HOMONYMS adds to buildtime, but I do > believe that before eliding some code that is necessary for correctness > in the general case, we need buildtime verification that it's ok. The runtime performance impact is real. We actually have quite a problem in general, e.g. the pinctrl drivers can take ages to operate before relocation because they read the DT multiple times. Your script seems like a great idea to me. Perhaps it should produce an error if the CONFIG is not enabled? Regards, Simon
On 19/04/2023 03.49, Simon Glass wrote: > Hi Rasmus, > > Returning to this old thread... > >> >> There's no problematic device tree, having two nodes with the same >> (base)name is perfectly fine and in fact sometimes expected/required (in >> the cases where a node name is standardized; e.g. having two identical >> eeproms on different i2c buses both would/should be called eeprom@50). >> >> What Simon is concerned about is that the translation from full path to >> blob offset is now done unconditionally, and that might be costly. I'm >> not sure it is (and didn't know that it could be), but as I've said >> repeatedly, I prefer code that works out-of-the-box, and for the boards >> that do need this extra check (because just looking at the basename is >> not enough), the fact that the previous code worked seemed to be pure >> luck - because those dtbs were compiled with -@ due to some other >> CONFIG_ knob being set. And again, involving phandles at all is what >> make the code fragile, so a revert that reinstates a CONFIG_ knob with >> PHANDLE in the name is not a good way forward, assuming there is even >> anything to fix. >> >> So if the performance thing is real, sure, we can introduce a >> CONFIG_something, but only if at the same time we have a sanity check at >> build-time that detects if one should enable/disable that option >> (depending on whether we make it "positive" or "negative"). Something >> like this seems to do the trick, but I haven't looked at hooking it up >> in the build system: >> >> === scripts/check-alias-homonyms.py === >> #!/usr/bin/python3 >> >> import sys >> >> sys.path.insert(0, 'scripts/dtc/pylibfdt') >> sys.path.insert(1, 'tools') >> >> from dtoc import fdt >> >> ret = 0 >> >> for name in sys.argv[1:]: >> dtb = fdt.FdtScan(name) >> aliasnode = dtb.GetNode("/aliases") >> if not aliasnode: >> continue >> basenames = dict() >> for prop in aliasnode.props.values(): >> alias = prop.name >> path = prop.value >> base = path[path.rfind('/')+1:] >> if base in basenames: >> basenames[base].append(alias) >> else: >> basenames[base] = [alias] >> for base, aliases in basenames.items(): >> if len(aliases) == 1: >> continue >> print("Warning: In %s, the aliases %s all point at nodes with >> the basename '%s' - consider (en/dis)abling CONFIG_..." % (name, >> ",".join(aliases), base)) >> ret = 1 >> >> sys.exit(ret) >> === >> >> I imagine this being run over CONFIG_OF_LIST when CONFIG_... has the >> value that disables the fdt_path_offset() call. For concreteness, >> something like >> >> config ASSUME_ALIAS_HOMONYMS # or whatever better name one can come up with >> bool "Assume there may be nodes pointed to by aliases in DT that have >> identical basenames" >> help >> In most cases, the nodes pointed to by aliases in the device tree >> all have different basenames. When this is satisfied, the >> fdtdec_get_alias_seq() function can avoid a somewhat expensive full walk >> of the dtb when looking for an alias for a given node. If the device >> tree for your board does have aliases pointing at nodes with the same >> basenames (but of course different full paths), that full walk is >> necessary for correctness, and you can then enable this option. >> >> plus >> >> - if (offset != fdt_path_offset(blob, prop)) >> + if (IS_ENABLED(CONFIG_ASSUME_ALIAS_HOMONYMS) && offset != >> fdt_path_offset(blob, prop)) >> >> I have no idea what running the above python script on the dtb(s) in the >> common case of !CONFIG_ASSUME_ALIAS_HOMONYMS adds to buildtime, but I do >> believe that before eliding some code that is necessary for correctness >> in the general case, we need buildtime verification that it's ok. > > The runtime performance impact is real. We actually have quite a > problem in general, e.g. the pinctrl drivers can take ages to operate > before relocation because they read the DT multiple times. OK. For other reasons, I'm looking at the upstream dtc code, and have some ideas for performance improvements to the "raw blob walking" code. But I don't have any idea how large an impact it could have, if it would help that pinctrl case, or when I'll actually find some spare cycles to work on it. > Your script seems like a great idea to me. Perhaps it should produce > an error if the CONFIG is not enabled? I was imagining that it would only run when the config is not enabled (because when it is enabled, correctness is guaranteed at runtime), and AFAICT it already does produce an error when it finds homonyms. But it really was just something I wrote in five minutes, I have no idea how to hook it up in the build system or how to propagate that error. Anybody is welcome to take that code and polish it up and make a real patch out of it. I also think I suggested doing something else at build time, something along the lines of 'for each alias, inject a property in the target node with name "u-boot,dm-alias" and value the name of the alias'. Then the run-time code could simply start by fetching the node's u-boot,dm-alias property, and if present, check if its value starts with the required stem. That would likely be even faster than what we do today, but the current code would probably need to be kept as a fallback. Of course that somewhat assumes that no node has two aliases, but the semantics of that is already ill-defined, and I don't think it's a real issue (and the code doing the injection could easily detect it and warn or bail). Rasmus
Hi Rasmus, On Tue, 18 Apr 2023 at 23:30, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 19/04/2023 03.49, Simon Glass wrote: > > Hi Rasmus, > > > > Returning to this old thread... > > > >> > >> There's no problematic device tree, having two nodes with the same > >> (base)name is perfectly fine and in fact sometimes expected/required (in > >> the cases where a node name is standardized; e.g. having two identical > >> eeproms on different i2c buses both would/should be called eeprom@50). > >> > >> What Simon is concerned about is that the translation from full path to > >> blob offset is now done unconditionally, and that might be costly. I'm > >> not sure it is (and didn't know that it could be), but as I've said > >> repeatedly, I prefer code that works out-of-the-box, and for the boards > >> that do need this extra check (because just looking at the basename is > >> not enough), the fact that the previous code worked seemed to be pure > >> luck - because those dtbs were compiled with -@ due to some other > >> CONFIG_ knob being set. And again, involving phandles at all is what > >> make the code fragile, so a revert that reinstates a CONFIG_ knob with > >> PHANDLE in the name is not a good way forward, assuming there is even > >> anything to fix. > >> > >> So if the performance thing is real, sure, we can introduce a > >> CONFIG_something, but only if at the same time we have a sanity check at > >> build-time that detects if one should enable/disable that option > >> (depending on whether we make it "positive" or "negative"). Something > >> like this seems to do the trick, but I haven't looked at hooking it up > >> in the build system: > >> > >> === scripts/check-alias-homonyms.py === > >> #!/usr/bin/python3 > >> > >> import sys > >> > >> sys.path.insert(0, 'scripts/dtc/pylibfdt') > >> sys.path.insert(1, 'tools') > >> > >> from dtoc import fdt > >> > >> ret = 0 > >> > >> for name in sys.argv[1:]: > >> dtb = fdt.FdtScan(name) > >> aliasnode = dtb.GetNode("/aliases") > >> if not aliasnode: > >> continue > >> basenames = dict() > >> for prop in aliasnode.props.values(): > >> alias = prop.name > >> path = prop.value > >> base = path[path.rfind('/')+1:] > >> if base in basenames: > >> basenames[base].append(alias) > >> else: > >> basenames[base] = [alias] > >> for base, aliases in basenames.items(): > >> if len(aliases) == 1: > >> continue > >> print("Warning: In %s, the aliases %s all point at nodes with > >> the basename '%s' - consider (en/dis)abling CONFIG_..." % (name, > >> ",".join(aliases), base)) > >> ret = 1 > >> > >> sys.exit(ret) > >> === > >> > >> I imagine this being run over CONFIG_OF_LIST when CONFIG_... has the > >> value that disables the fdt_path_offset() call. For concreteness, > >> something like > >> > >> config ASSUME_ALIAS_HOMONYMS # or whatever better name one can come up with > >> bool "Assume there may be nodes pointed to by aliases in DT that have > >> identical basenames" > >> help > >> In most cases, the nodes pointed to by aliases in the device tree > >> all have different basenames. When this is satisfied, the > >> fdtdec_get_alias_seq() function can avoid a somewhat expensive full walk > >> of the dtb when looking for an alias for a given node. If the device > >> tree for your board does have aliases pointing at nodes with the same > >> basenames (but of course different full paths), that full walk is > >> necessary for correctness, and you can then enable this option. > >> > >> plus > >> > >> - if (offset != fdt_path_offset(blob, prop)) > >> + if (IS_ENABLED(CONFIG_ASSUME_ALIAS_HOMONYMS) && offset != > >> fdt_path_offset(blob, prop)) > >> > >> I have no idea what running the above python script on the dtb(s) in the > >> common case of !CONFIG_ASSUME_ALIAS_HOMONYMS adds to buildtime, but I do > >> believe that before eliding some code that is necessary for correctness > >> in the general case, we need buildtime verification that it's ok. > > > > The runtime performance impact is real. We actually have quite a > > problem in general, e.g. the pinctrl drivers can take ages to operate > > before relocation because they read the DT multiple times. > > OK. For other reasons, I'm looking at the upstream dtc code, and have > some ideas for performance improvements to the "raw blob walking" code. > But I don't have any idea how large an impact it could have, if it would > help that pinctrl case, or when I'll actually find some spare cycles to > work on it. OK. Part of the issue with pinctrl might be that drivers should be caching the DT in their internal structures, rather than scanning it each time pinctrl_select_state() is called. > > > Your script seems like a great idea to me. Perhaps it should produce > > an error if the CONFIG is not enabled? > > I was imagining that it would only run when the config is not enabled > (because when it is enabled, correctness is guaranteed at runtime), and > AFAICT it already does produce an error when it finds homonyms. But it > really was just something I wrote in five minutes, I have no idea how to > hook it up in the build system or how to propagate that error. Anybody > is welcome to take that code and polish it up and make a real patch out > of it. Probably it just needs to go in Makefile: u-boot.dtb: dts/dt.dtb python <your_script.py> $< $(call cmd,copy) > > I also think I suggested doing something else at build time, something > along the lines of 'for each alias, inject a property in the target node > with name "u-boot,dm-alias" and value the name of the alias'. Then the > run-time code could simply start by fetching the node's u-boot,dm-alias > property, and if present, check if its value starts with the required > stem. That would likely be even faster than what we do today, but the > current code would probably need to be kept as a fallback. I've thought for a long time that it would be great to have a table of phandles (array of node offsets in a property in a suitable node) and a second 'aliases' node (with a different name) that uses node offsets instead of strings. Haven't done it though... > > Of course that somewhat assumes that no node has two aliases, but the > semantics of that is already ill-defined, and I don't think it's a real > issue (and the code doing the injection could easily detect it and warn > or bail). OK Regards, Simon
diff --git a/configs/am65x_evm_a53_defconfig b/configs/am65x_evm_a53_defconfig index 65e41e5b6af..9c088e1d02e 100644 --- a/configs/am65x_evm_a53_defconfig +++ b/configs/am65x_evm_a53_defconfig @@ -180,3 +180,4 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x0451 CONFIG_USB_GADGET_PRODUCT_NUM=0x6162 CONFIG_USB_GADGET_DOWNLOAD=y CONFIG_OF_LIBFDT_OVERLAY=y +CONFIG_PHANDLE_CHECK_SEQ=y diff --git a/configs/evb-ast2600_defconfig b/configs/evb-ast2600_defconfig index 5c4d8426607..f337128abb3 100644 --- a/configs/evb-ast2600_defconfig +++ b/configs/evb-ast2600_defconfig @@ -90,3 +90,4 @@ CONFIG_WDT=y CONFIG_SHA384=y CONFIG_HEXDUMP=y # CONFIG_EFI_LOADER is not set +CONFIG_PHANDLE_CHECK_SEQ=y diff --git a/configs/sama7g5ek_mmc1_defconfig b/configs/sama7g5ek_mmc1_defconfig index 20ca98821a0..16b138ff93d 100644 --- a/configs/sama7g5ek_mmc1_defconfig +++ b/configs/sama7g5ek_mmc1_defconfig @@ -80,3 +80,4 @@ CONFIG_TIMER=y CONFIG_MCHP_PIT64B_TIMER=y CONFIG_OF_LIBFDT_OVERLAY=y # CONFIG_EFI_LOADER_HII is not set +CONFIG_PHANDLE_CHECK_SEQ=y diff --git a/configs/sama7g5ek_mmc_defconfig b/configs/sama7g5ek_mmc_defconfig index c9f62a8ebe5..c08c889c04f 100644 --- a/configs/sama7g5ek_mmc_defconfig +++ b/configs/sama7g5ek_mmc_defconfig @@ -80,3 +80,4 @@ CONFIG_TIMER=y CONFIG_MCHP_PIT64B_TIMER=y CONFIG_OF_LIBFDT_OVERLAY=y # CONFIG_EFI_LOADER_HII is not set +CONFIG_PHANDLE_CHECK_SEQ=y diff --git a/lib/Kconfig b/lib/Kconfig index c9f9ddce7d0..223b63cff52 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -960,4 +960,11 @@ config LMB_RESERVED_REGIONS Define the number of supported reserved regions in the library logical memory blocks. +config PHANDLE_CHECK_SEQ + bool "Enable phandle check while getting sequence number" + help + When there are multiple device tree nodes with same name, + enable this config option to distinguish them using + phandles in fdtdec_get_alias_seq() function. + endmenu diff --git a/lib/fdtdec.c b/lib/fdtdec.c index ffa78f97ca0..e20f6aad9c2 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -516,8 +516,11 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset, * Adding an extra check to distinguish DT nodes with * same name */ - if (offset != fdt_path_offset(blob, prop)) - continue; + if (IS_ENABLED(CONFIG_PHANDLE_CHECK_SEQ)) { + if (fdt_get_phandle(blob, offset) != + fdt_get_phandle(blob, fdt_path_offset(blob, prop))) + continue; + } val = trailing_strtol(name); if (val != -1) {
The fdt_path_offset() function is slow since it must scan the tree. This substantial overhead now applies to all boards. The original code may not be ideal but it is fit for purpose and is only needed on a few boards. We should revert this in time for the release. This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. Signed-off-by: Simon Glass <sjg@chromium.org> --- configs/am65x_evm_a53_defconfig | 1 + configs/evb-ast2600_defconfig | 1 + configs/sama7g5ek_mmc1_defconfig | 1 + configs/sama7g5ek_mmc_defconfig | 1 + lib/Kconfig | 7 +++++++ lib/fdtdec.c | 7 +++++-- 6 files changed, 16 insertions(+), 2 deletions(-)