On Thu, Oct 11, 2018 at 12:39 AM Frank Rowand [off-list ref] wrote:

[resend of my messed up rejected email of a minute ago, sorry]

On 10/10/18 14:03, Frank Rowand wrote:

quoted

On 10/10/18 13:40, Alan Tull wrote:

quoted

On Wed, Oct 10, 2018 at 1:49 AM Frank Rowand [off-list ref] wrote:

quoted

On 10/09/18 23:04, frowand.list@gmail.com wrote:

quoted

From: Frank Rowand <redacted>


"of: overlay: use prop add changeset entry for property in new nodes"
fixed a problem where an 'update property' changeset entry was
created for properties contained in nodes added by a changeset.
The fix was to use an 'add property' changeset entry.

This exposed more bugs in the apply overlay code.  The properties
'name', 'phandle', and 'linux,phandle' were filtered out by
add_changeset_property() as special properties.  Change the filter
to be only for existing nodes, not newly added nodes.

The second bug is that the 'name' property does not exist in the
newest FDT version, and has to be constructed from the node's
full_name.  Construct an 'add property' changeset entry for
newly added nodes.

Signed-off-by: Frank Rowand <redacted>
---


Hi Alan,

Thanks for reporting the problem with missing node names.

I was able to replicate the problem, and have created this preliminary
version of a patch to fix the problem.

I have not extensively reviewed the patch yet, but would appreciate
if you can confirm this fixes your problem.

I created this patch as patch 17 of the series, but have also
applied it as patch 05.1, immediately after patch 05/16, and
built the kernel, booted, and verified name and phandle for
one of the nodes in a unittest overlay for both cases.  So
minimal testing so far on my part.

I have not verified whether the series builds and boots after
each of patches 06..16 if this patch is applied as patch 05.1.

There is definitely more work needed for me to complete this
patch because it allocates some more memory, but does not yet
free it when the overlay is released.

-Frank


 drivers/of/overlay.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 67 insertions(+), 5 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 0b0904f44bc7..9746cea2aa91 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c

@@ -301,10 +301,11 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
      struct property *new_prop = NULL, *prop;
      int ret = 0;

-     if (!of_prop_cmp(overlay_prop->name, "name") ||
-         !of_prop_cmp(overlay_prop->name, "phandle") ||
-         !of_prop_cmp(overlay_prop->name, "linux,phandle"))
-             return 0;
+     if (target->in_livetree)
+             if (!of_prop_cmp(overlay_prop->name, "name") ||
+                 !of_prop_cmp(overlay_prop->name, "phandle") ||
+                 !of_prop_cmp(overlay_prop->name, "linux,phandle"))
+                     return 0;

This is a big hammer patch.

Nobody should waste time reviewing this patch.

I wasn't clear if you still could use the testing so I did re-run my
test.  This patch adds back some of the missing properties, but the
the kobject names aren't set as dev_name() returns NULL:

* without this patch some of_node properties don't show up in sysfs:
root@arria10:~# ls
/sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node
clocks            compatible        interrupt-parent  interrupts        reg

* with this patch, the of_node properties phandle and name are back:
root@arria10:~#  ls
/sys/bus/platform/drivers/altera_freeze_br/ff200450.\<NULL\>/of_node
clocks            compatible        interrupt-parent  interrupts
 name              phandle           reg

Thanks for the testing.  I'll keep chasing after this problem today.

Glad to help!  Thanks for all your work in this area!

quoted

This is useful data for me as I was not looking under the /sys/bus/...
tree that you reported, but was instead looking at /proc/device-tree/...
which showed the same type of problem since the overlay I was using
does not show up under /sys/bus/...

I'll have to create a useful overlay test case that will show up under
/sys/bus/...

In the meantime, can you send me the base FDT and the overlay FDT for
your test case?

I now have a test case that shows the problem under /sys/bus/... so I
no longer need the base FDT and overlay FDT for your test case.

I have determined the location that sets the name to "<NULL>" but do
not have the fix yet.  Still working on that.

I understand you're quite busy with all this, but I'm wondering
whether it might be worth it go ahead and make the properties be
kernel objects also at this point.  That would be an improvement for
the case of overlay properties added to non-overlay nodes, so the
lifespan of the overlay property memory can be coupled with the
properties kobj's instead of the node kobj's.

Alan