Hello Daniel,
On 4/13/22 10:21, Daniel Vetter wrote:
I messed up the delayed takover path in the locking conversion in 6e7da3af008b ("fbcon: Move console_lock for register/unlink/unregister").
Maybe a few more words of what the issue is ? Something like the following:
If CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER is enabled, fbcon take-over doesn't take place when calling fbcon_fb_registered(). Instead, is deferred using a workqueue and its fbcon_register_existing_fbs() function calls to fbcon_fb_registered() again for each registered fbcon fb.
This leads to the console_lock tried to be held twice, causing a deadlock.
Fix it by re-extracting the lockless function and using it in the delayed takeover path, where we need to hold the lock already to iterate over the list of already registered fb. Well the current code still is broken in there (since the list is protected by a registration_lock, which we can't take here because it nests the other way round with console_lock), but in the future this will be a list protected by console_lock when this is all sorted out.
[snip]
-/* called with console_lock held */ void fbcon_fb_unbind(struct fb_info *info) { int i, new_idx = -1; @@ -2822,7 +2821,6 @@ void fbcon_fb_unbind(struct fb_info *info) console_unlock(); }
-/* called with console_lock held */ void fbcon_fb_unregistered(struct fb_info *info) {
Removing these comments feels like should be in a separate patch or at least mention in the patch description that should had been removed in the commit 6e7da3af008b ("fbcon: Move console_lock for register/unlink/unregister"), that made these functions to be called without the console_lock being held.
The changes themselves look good to me.
Reviewed-by: Javier Martinez Canillas javierm@redhat.com