| Message ID | 20220413092454.1073-3-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| 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.
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]); - } else if (dev_is_platform(device)) { + if (dev_is_platform(device)) { registered_fb[i]->forced_out = true; platform_device_unregister(to_platform_device(device)); } else {
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(-)