Hello Thomas,
Thanks for the feedback.
On 4/25/22 10:27, Thomas Zimmermann wrote:
Hi
Am 20.04.22 um 10:53 schrieb Javier Martinez Canillas:
Drivers that want to remove registered conflicting framebuffers prior to register their own framebuffer, calls remove_conflicting_framebuffers().
This function takes the registration_lock mutex, to prevent a races when drivers register framebuffer devices. But if a conflicting framebuffer device is found, the underlaying platform device is unregistered and this will lead to the platform driver .remove callback to be called, which in turn will call to the unregister_framebuffer() that takes the same lock.
To prevent this, a struct fb_info.forced_out field was used as indication to unregister_framebuffer() whether the mutex has to be grabbed or not.
A cleaner solution is to drop the lock before platform_device_unregister() so unregister_framebuffer() can take it when called from the fbdev driver, and just grab the lock again after the device has been registered and do a removal loop restart.
I don't see how this patch improves the situation. So far, do_remove_conflicting_framebuffers() had no business in maintaining locks. And now it's doing this in in a goto-loop where it keeps getting/dropping locks. That's asking for bugs IMHO.
It's true that do_remove_conflicting_framebuffers() gets more complicated with all the locks release/re-acquire but OTOH unregister_framebuffer() doesn't do conditionally locking, and more importantly the drivers .remove callback isn't called with the lock held, which IMHO is also quite fragile.