Hi
On Tue, Aug 30, 2016 at 10:58 PM, Rob Herring robh+dt@kernel.org wrote:
On Tue, Aug 30, 2016 at 2:30 PM, David Herrmann dh.herrmann@gmail.com wrote:
Sure, all those functions are not meant to be called in parallel by multiple tasks. They are rather meant to have a single holder which preferably is the one instantiating and destroying the node/device/foobar. But the framebuffer eviction code somehow needs to trigger the removal, and thus needs some hook, that can be called in parallel by any driver that is loaded.
I can make sure the simple-framebuffer eviction code is locked properly. That'll work, for now. But if someone ends up providing simple-framebuffer devices via overlays or any other dynamic mechanism, they will race the removal.
No doubt someone will come up with some usecase wanting to do that, but IMO that is not a supported usecase and should not be. simple-framebuffer is for providing a firmware setup framebuffer.
Sure. Any sensible simple-framebuffer use would follow what we have now. But it feels wrong to me that if some node is added that just happens to have "simple-framebuffer" as name, suddenly things will go wrong. I mean, yeah, DT is not a userspace API, but I still would like the code to catch misuses rather than fail. It is an API after all. Or is that being overly pedantic?
And it will be completely non-obvious to them. I really don't want to be responsible to have submitted that code. If anyone cares for proper device hand-over on ARM, please come up with an idea to fix it. Otherwise, I will just limit the simpledrm code to x86.
IOW the device handover code somehow needs to know who was responsible for the instantiation of the simple-framebuffer device, so it can tell them to remove it again. On x86 there is only one place where those can be instantiated. But on OF-based systems, it can be dynamically instantiated in many places right now.
What do you mean by all over the place? It is only in simplefb_init ATM. I haven't looked at what simpledrm is doing, but we can move the device creation to of_platform_default_populate_init if we need a single spot.
Currently I see at least 3 paths that might add such nodes:
- of_platform_populate() - of_node_attach() (via the notifier) - simplefb_init()
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?
Thanks David