Re: [PATCH] fbcon: Fix delayed takeover locking

From: Javier Martinez Canillas <javierm@redhat.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Du Cheng <ducheng2@gmail.com>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Zheyu Ma <zheyuma97@gmail.com>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Claudio Suarez <cssk@net-c.es>,
	Matthew Wilcox <willy@infradead.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Sam Ravnborg <sam@ravnborg.org>, Helge Deller <deller@gmx.de>,
	Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH] fbcon: Fix delayed takeover locking
Date: Wed, 13 Apr 2022 11:31:17 +0200	[thread overview]
Message-ID: <85a3bd58-7c7c-051d-665d-68d00e4b3ee0@redhat.com> (raw)
In-Reply-To: <YlaWE+/YBtcrnLHY@phenom.ffwll.local>

On 4/13/22 11:21, Daniel Vetter wrote:
> On Wed, Apr 13, 2022 at 11:16:08AM +0200, Javier Martinez Canillas wrote:
>> 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.
> 
> Will add.
>>

Thanks.

[snip]  

>> 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.
> 
> Yeah I forgot to mention that in the commit message - while reviewing all
> the locking to figure out the bug I also noticed that I didn't update the
> comments, and since it's all issues in the same patch I figured I'll do it
> all in one go.
> 
> Ok if I explain that and then keep the comment removals?
Yes, I'm OK with that and agreed that all changes are to fix the same commit.

It's just that it took me some time to figure out why you were removing those.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat