Lopper removes "reserved-memory" nodes because it's not referenced
deathjest3r opened this issue · 6 comments
Hello,
I'm not sure if there is a bug. But to me it seems lopper is stripping the reserved-memory node even if there are sub-nodes that are referenced. To me it feels like this commit 9005018 is introducing this problem. Before that the function resolve_all_refs() was correctly returning all the nodes and incremented their ref count by 1. After this commit the function returns just a very short list which to me looks like are the nodes from the domain file, containing only the following four entries:
/partition/linux_standalone
/partition
/
/cpus-cluster@0
So, all other nodes have a ref count of 0. This is not a problem for most of the nodes because if they are referenced their ref count will be incremented later. But the reserved-memory node is a special case, because only its sub-nodes are actually checked and their ref count incremented. But then in the process_domain() function also the reserved-memory node is checked and then directly removed because its ref count is 0. To fix this I added the following in the process_domain() function:
--- a/lopper/assists/openamp.py
+++ b/lopper/assists/openamp.py
@@ -295,6 +295,7 @@ def process_domain( tgt_node, sdt, options ):
# to a list of nodes that we'll use to check for referenced children later. Anything
# with no reference, will be removed.
anode.ref = 1
+ node_parent.ref = 1
# filter #1:
# - starting at /
Any idea on the matter?
None of the sanity of commonly run workflows are seeing issues (and that commit isn't recent), I've been running some of the openamp flows just in the past few days.
That being said, I'm missing context to run some more detailed tests.
IS this happening on a simple read -> write of a device tree with a reserved memory node ? Or is it just in the openamp workflow ?
Can you provide a sample input file and lopper command line that shows the issue ?
Hello,
thank you for your quick reply. So, yes here is a very simple example. But it seems the openamp (from lops/lop-load.dts) is causing the reserved memory nodes to disappear (but only in a specific case):
Lopper cmd:
./lopper/lopper.py -f --enhanced -i lopper/lopper/lops/lop-load.dts -i node_dtsi_config.dts -i cpu_tree.dts -O output node_dtsi_domain.dts output_dts.dts
node_dtsi_config.dts:
/dts-v1/;
/ {
compatible = "system-device-tree-v1,lop";
lops {
compatible = "system-device-tree-v1,lop";
lop_1 {
compatible = "system-device-tree-v1,lop,assist-v1";
node = "/partition/linux_standalone";
id = "openamp,domain-v1";
};
lop_2 {
compatible = "system-device-tree-v1,lop,modify";
modify = "/cpus-cluster@[0-9a-fA-F]+$::/cpus";
};
lop_3 {
compatible = "system-device-tree-v1,lop,modify";
modify = "/partition/::";
};
lop_4 {
compatible = "system-device-tree-v1,lop,modify";
modify = "/l2_cache@[0-9a-fA-F]+$:status:okay";
};
lop_5 {
compatible = "system-device-tree-v1,lop,add";
node_src = "chosen";
node_dest = "/chosen";
chosen {
bootargs = "earlycon norandmaps console=ttyS0,115200";
stdout-path = "serial0:115200n8";
};
};
};
};
node_dtsi_domain.dts:
/ {
partition {
#address-cells = <0x2>;
#size-cells = <0x2>;
linux_standalone {
compatible = "openamp,domain-v1";
#address-cells = <0x2>;
#size-cells = <0x2>;
memory = < 0x00000000 0x00000000 0x00000000 0x00400000 >;
cpus = <&cpus_cc0 0xFFFF 0x0>;
access =<&l2_data 0x1>, <&smem 0x1>; # <--- Non-existent reference to smem!
};
};
};
cpu_tree.dts
/dts-v1/;
/ {
cpus_cc0: cpus-cluster@0 {
#address-cells = <0x1>;
#size-cells = <0x0>;
#cpus-mask-cells = <0x1>;
compatible = "cpus,cluster";
#ranges-address-cells = <0x2>;
#ranges-size-cells = <0x2>;
cpu@0 {
compatible = "pe";
device_type = "cpu";
reg = <0x0>;
};
cpu@1 {
compatible = "pe";
device_type = "cpu";
reg = <0x1>;
};
};
reserved-memory {
#address-cells = <0x2>;
#size-cells = <0x2>;
ranges;
l2_data: l2_data@10000000 {
reg = <0x0 0x10000000 0x0 0x200000>;
no-map;
};
};
l2_cache: l2_cache@28000 {
compatible = "l2-cache";
status = "disabled";
reg = <0x0 0x28000 0x0 0x1000>;
l2-size = <0x100000>;
memory-region = <&l2_data>;
};
};
As you can see there is a l2_cache node which references the l2_data. But the whole reserved-memory node will dissapear, even thought there is a reference to the l2_data.
But I have identified the problem. If the access entry in the domain specification contains an entry that does not exist (in our case the &smem) it doesn't throw an error it just causes the reserved-memory node to disappear. So, it was a fault on my side. Sorry for that. But still strange behavior, no?
By default Lopper removes properties that have invalid/undefined phandle references, and if there's a bug in that processing, it might be removing all of the access entries, which then removes the reference to reserved-memory.
There is also secondary processing that cleans up "unreferenced nodes", which the problem above could easily chain to.
I do think it is a bug, and with the simple reproducer (many thanks!), I'll do a deeper dive shortly and confirm a fix.
curiously, I'm not seeing the same behaviour. Here's my run with those same files. Can you retry with the tip of lopper's master branch ?
I can't confirm if the rest of the output is correct, but I do see the reserved-memory node.
% ./lopper.py -f --enhanced -i lopper/lops/lop-load.dts -i github-bug/node_dtsi_config.dts -i github-bug/cpu_tree.dts github-bug/node_dtsi_domain.dts output_dts.dts
% cat output_dts.dts
/dts-v1/;
/ {
cpus_cc0: cpus {
#address-cells = <0x1>;
#size-cells = <0x0>;
#cpus-mask-cells = <0x1>;
compatible = "cpus,cluster";
#ranges-address-cells = <0x2>;
#ranges-size-cells = <0x2>;
phandle = <0x1>;
cpu@0 {
compatible = "pe";
device_type = "cpu";
reg = <0x0>;
};
cpu@1 {
compatible = "pe";
device_type = "cpu";
reg = <0x1>;
};
};
reserved-memory {
#address-cells = <0x2>;
#size-cells = <0x2>;
ranges;
l2_data: l2_data@10000000 {
reg = <0x0 0x10000000 0x0 0x200000>;
no-map;
phandle = <0x2>;
};
};
l2_cache: l2_cache@28000 {
compatible = "l2-cache";
status = "okay";
reg = <0x0 0x28000 0x0 0x1000>;
l2-size = <0x100000>;
memory-region = <0x2>;
};
chosen {
stdout-path = "serial0:115200n8";
bootargs = "earlycon norandmaps console=ttyS0,115200";
};
};
Hello @zeddii, thank you for your reply. Indeed I can't reproduce it myself at the moment. I think we can close this ticket. I will have an other look at the problem with my more complex device tree (where the problem actually showed up). But thank you for your support and patience!
Thanks for the update.
Definitely do re-open it if the problem shows up in your more complex device tree. I'll continue to poke at what I have here, but we'll just figure out how to get a similar setup to the complex case, if it does come back!