Re: [PATCH 1/5] fbdev: Hot-unplug firmware fb devices on forced removal

[Javier Martinez Canillas <javierm@redhat.com>]

On 1/24/22 15:19, Thomas Zimmermann wrote:

[snip]

>>> +			if (dev_is_platform(dev)) {
>>
>> In do_register_framebuffer() creating the fb%d is not a fatal error. It would
>> be safer to do if (!IS_ERR_OR_NULL(dev) && dev_is_platform(dev)) instead here.
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/video/fbdev/core/fbmem.c#L1605
> 
> 'dev' here refers to 'fb_info->device', which is the underlying device 
> created by the sysfb code.  fb_info->dev is something different.
>

oh, indeed. I conflated the two.

Maybe the local variable could be renamed to 'device' just to avoid confusion ?

[snip]

>> I'm not sure to follow the logic here. The forced_out bool is set when the
>> platform device is unregistered in do_remove_conflicting_framebuffers(),
>> but shouldn't the struct platform_driver .remove callback be executed even
>> in this case ?
>>
>> That is, the platform_device_unregister() will trigger the call to the
>> .remove callback that in turn will call unregister_framebuffer().
>>
>> Shouldn't we always hold the mutex when calling do_unregister_framebuffer() ?
> 
> Doing the hot-unplug will end up in unregister_framebuffer(), but we 
> already hold the lock from the do_remove_conflicting_framebuffer() code.
>

Yes, I realized that just after sending the first email. Sorry for the noise.
 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat