[RFC] drm: implement generic firmware eviction

[David Herrmann]

Hi

On Tue, Aug 30, 2016 at 10:58 PM, Rob Herring <robh+dt at kernel.org> wrote:
> On Tue, Aug 30, 2016 at 2:30 PM, David Herrmann <dh.herrmann at 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