Hello Thomas,
On 4/13/22 20:09, Thomas Zimmermann wrote:
[snip]
>>>> index bc6ed750e915..bdd00d381bbc 100644
>>>> --- a/drivers/video/fbdev/core/fbmem.c
>>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>>> @@ -1579,14 +1579,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
>>>> * If it's not a platform device, at least print a warning. A
>>>> * fix would add code to remove the device from the system.
>>>> */
>>>> - if (!device) {
>>>> - /* TODO: Represent each OF framebuffer as its own
>>>> - * device in the device hierarchy. For now, offb
>>>> - * doesn't have such a device, so unregister the
>>>> - * framebuffer as before without warning.
>>>> - */
>>>> - do_unregister_framebuffer(registered_fb[i]);
>>>
>>> Maybe we could still keep this for a couple of releases but with a big
>>> warning that's not supported in case there are out-of-tree drivers out
>>> there that still do this ?
>>>
>>> Or at least a warning if the do_unregister_framebuffer() call is removed.
>>
>> Yeah dying while holding console_lock isn't fun, and not having a WARN_ON
>> + bail-out code pretty much forces bug reporters to do a bisect here to
>> give us something more than "machine dies at boot with no messages".
>>
>> I'd just outright keep the WARN_ON here for 1-2 years even to really make
>> sure we got all the bug reports, since often these older machines only
>> update onto LTS releases.
>
> If that's what the consent is, I'll go with it.
>
> I'm just not sure if we talk about the same problem. offb didn't have a
> platform device, so we recently added this workaround with 'if
> (!device)'. All the other fbdev drivers have a platform device; and
> anything else that could fail is out-of-tree. We don't really care about
> those AFAIK.
>
Yes, agreed on the offb change but I'm not really sure if we don't care
about out-of-tree modules. I mean, you are right in theory but I still
feel that we are changing a core behavior without giving people time to
sort out if needed.
Since before commit 27599aacbaef ("fbdev: Hot-unplug firmware fb devices
on forced removal") registered FBs didn't need to have a device, but now
that will lead to a NULL pointer dereference in dev_is_platform(device).
And that change only landed in v5.18-rc1, so it is fairly recent.
I know that we follow https://www.kernel.org/doc/Documentation/process/stable-api-nonsense.rst
but still my opinion is that having a warning for a couple of releases
if registered_fb[i]->device is NULL, instead of just crashing would be
a better way to handle this.
> With offb converted, we could practically remove all of the checks here
> and call platform_device_unregister() unconditionally.
>
Yes for mainline, but as mentioned I thought mostly about out-of-tree. If
folks agree that we shouldn't care about these, I'm Ok with that as well.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
