| Message ID | 20220413092454.1073-1-tzimmermann@suse.de |
|---|---|
| Headers | show |
| Series | of: Register platform device for each framebuffer | expand |
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. Regardless of what you chose to do, the patch looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@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 >
Hi Am 13.04.22 um 18:05 schrieb Daniel Vetter: > 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. If that's what the consent is, I'll go with it. I'm just not sure if we talk about the same problem. offb didn't have a platform device, so we recently added this workaround with 'if (!device)'. All the other fbdev drivers have a platform device; and anything else that could fail is out-of-tree. We don't really care about those AFAIK. With offb converted, we could practically remove all of the checks here and call platform_device_unregister() unconditionally. Best regards Thomas > > 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 >> >
Hello Thomas, On 4/13/22 20:09, Thomas Zimmermann wrote: [snip] >>>> 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. > > If that's what the consent is, I'll go with it. > > I'm just not sure if we talk about the same problem. offb didn't have a > platform device, so we recently added this workaround with 'if > (!device)'. All the other fbdev drivers have a platform device; and > anything else that could fail is out-of-tree. We don't really care about > those AFAIK. > Yes, agreed on the offb change but I'm not really sure if we don't care about out-of-tree modules. I mean, you are right in theory but I still feel that we are changing a core behavior without giving people time to sort out if needed. Since before commit 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal") registered FBs didn't need to have a device, but now that will lead to a NULL pointer dereference in dev_is_platform(device). And that change only landed in v5.18-rc1, so it is fairly recent. I know that we follow https://www.kernel.org/doc/Documentation/process/stable-api-nonsense.rst but still my opinion is that having a warning for a couple of releases if registered_fb[i]->device is NULL, instead of just crashing would be a better way to handle this. > With offb converted, we could practically remove all of the checks here > and call platform_device_unregister() unconditionally. > Yes for mainline, but as mentioned I thought mostly about out-of-tree. If folks agree that we shouldn't care about these, I'm Ok with that as well.