Hey Rob!
On Wed, Aug 31, 2016 at 1:01 AM, Rob Herring robh+dt@kernel.org wrote:
On Tue, Aug 30, 2016 at 4:12 PM, David Herrmann dh.herrmann@gmail.com wrote:
Currently I see at least 3 paths that might add such nodes:
- of_platform_populate()
If we assume the node is only under chosen, then that would require a call to of_platform_populate with /chosen as the root which shouldn't happen. More generally the assumption is of_platform_populate is only called once from for the root node and only on sub-trees by the driver of sub-tree's root device.
I see.
- of_node_attach() (via the notifier)
A node getting attached would be harmless. Nothing really would happen until a handler calls of_platform_populate. So this is the same as the 1st case.
drivers/of/platform.c has a notifier callback and registers the platform devices when added. It has a check that the parent is a populated bus, which I guess is what you refer to?
- simplefb_init()
Actually, I'd prefer to move it out of there and into the core. I don't think drivers should look at /chosen and only the core and arch code should. Of course, the only enforcing of that is policy. For overlays though, there's been some discussion on limiting where overlays can be applied.
Should I just ignore anything but simplefb_init()? I understand that it's the only one used by normal code-paths, but isn't it kinda ugly to silently introduce race conditions if a node just happens to be introduced via one of the other methods? Or are errors in the DT not meant to be caught?
In general there's no limit to how wrong a DT could be. I could write a DT that causes every DT enabled driver in the kernel to probe (that would be an interesting test case). The kernel is limited in knowing what is correct, and the whole point of DT is to move that information out of the kernel. This is case is just one compatible string out of thousands and location in the tree is just one thing to check.
This is a major reason why there is not yet a userspace interface for applying DT overlays as who knows what random crap could be in the DT. We're nervous about what could happen from frying h/w to creating security holes. Evidently the ACPI folks were not so nervous and added an interface.
I see. Thanks a lot for the clarifications! I will put the of_platform_device_destroy() right next to the x86 handler, making sure it is properly locked. I'm not so sure about the of_chosen instantiation, though. x86 has the instantiation in arch/x86/kernel/sysfb_simplefb.c, so maybe an equivalent for arm would be fine?
Thanks David