Re: [PATCH] fbcon: Fix delayed takeover locking

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

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>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat