Re: [PATCH 2/2] fbdev: Remove hot-unplug workaround for framebuffers without device

From: Daniel Vetter <daniel@ffwll.ch>
To: Javier Martinez Canillas <javierm@redhat.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>,
	robh+dt@kernel.org, frowand.list@gmail.com, daniel@ffwll.ch,
	deller@gmx.de, sam@ravnborg.org, linux@roeck-us.net,
	mpe@ellerman.id.au, benh@kernel.crashing.org, paulus@samba.org,
	devicetree@vger.kernel.org, linux-fbdev@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/2] fbdev: Remove hot-unplug workaround for framebuffers without device
Date: Wed, 13 Apr 2022 18:05:51 +0200	[thread overview]
Message-ID: <Ylb0316ABOhOe1Rb@phenom.ffwll.local> (raw)
In-Reply-To: <2e183cc9-603d-f038-54aa-5601f11b0484@redhat.com>

On Wed, Apr 13, 2022 at 12:50:50PM +0200, Javier Martinez Canillas wrote:
> On 4/13/22 11:24, Thomas Zimmermann wrote:
> > A workaround makes fbdev hot-unplugging work for framebuffers without
> > device. The only user for this feature was offb. As each OF framebuffer
> > now has an associated platform device, the workaround is no longer
> > needed. Remove it. Effectively reverts commit 0f525289ff0d ("fbdev: Fix
> > unregistering of framebuffers without device").
> > 
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > ---
> >  drivers/video/fbdev/core/fbmem.c | 9 +--------
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> > index bc6ed750e915..bdd00d381bbc 100644
> > --- a/drivers/video/fbdev/core/fbmem.c
> > +++ b/drivers/video/fbdev/core/fbmem.c
> > @@ -1579,14 +1579,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
> >  			 * If it's not a platform device, at least print a warning. A
> >  			 * fix would add code to remove the device from the system.
> >  			 */
> > -			if (!device) {
> > -				/* TODO: Represent each OF framebuffer as its own
> > -				 * device in the device hierarchy. For now, offb
> > -				 * doesn't have such a device, so unregister the
> > -				 * framebuffer as before without warning.
> > -				 */
> > -				do_unregister_framebuffer(registered_fb[i]);
> 
> Maybe we could still keep this for a couple of releases but with a big
> warning that's not supported in case there are out-of-tree drivers out
> there that still do this ?
> 
> Or at least a warning if the do_unregister_framebuffer() call is removed.

Yeah dying while holding console_lock isn't fun, and not having a WARN_ON
+ bail-out code pretty much forces bug reporters to do a bisect here to
give us something more than "machine dies at boot with no messages".

I'd just outright keep the WARN_ON here for 1-2 years even to really make
sure we got all the bug reports, since often these older machines only
update onto LTS releases.

And it needs to be a WARN_ON + bail out since BUG_ON is as bad as just
oopsing.
-Daniel

> 
> Regardless of what you chose to do, the patch looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch