Hi all,
I fixed the fbcon_exited mess that CI spotted (hopefully it works now, the
code is still rather brittle imo). Plus all the little nits 0day and
people found.
Maarten slapped an rb onto the entire pile, but I feel like enough has
changed that a 2nd look from him is warranted.
I also added a backlight patch on top, I think that nicely highlights how
the fb notifier is now only used for backlight notifications. Maybe we
could rename it as a follow-up to make that clear.
Oh and one rather badly looking thing: am200epd is abusing the notifier in
very interesting ways. I guess a proper fix would be to figure out where
the display boot memory reservation is some more direct way, instead of
listening to the fw fb driver. But that's definitely outside of my
knowledge, so I left it at a bunch of #ifdef and comments.
I think we also still need an ack from Greg KH for the vt and staging
bits.
As usual, comments, review and testing very much welcome.
btw for future plans: I think this is tricky enough (it's old code and all
that) that we should let this soak for 2-3 kernel releases. I think the
following would be nice subsequent cleanup/fixes:
- push the console lock completely from fbmem.c to fbcon.c. I think we're
mostly there with prep, but needs to pondering of corner cases.
- move fbcon.c from using indices for tracking fb_info (and accessing
registered_fbs without proper locking all the time) to real fb_info
pointers with the right amount of refcounting. Mostly motivated by the
fun I had trying to simplify fbcon_exit().
- make sure that fbcon call lock/unlock_fb when it calls fbmem.c
functions, and sprinkle assert_lockdep_held around in fbmem.c. This
needs the console_lock cleanups first.
But I think that's material for maybe next year or so.
Cheers, Daniel
Daniel Vetter (33):
dummycon: Sprinkle locking checks
fbdev: locking check for fb_set_suspend
vt: might_sleep() annotation for do_blank_screen
vt: More locking checks
fbdev/sa1100fb: Remove dead code
fbdev/cyber2000: Remove struct display
fbdev/aty128fb: Remove dead code
fbcon: s/struct display/struct fbcon_display/
fbcon: Remove fbcon_has_exited
fbcon: call fbcon_fb_(un)registered directly
fbdev/sh_mobile: remove sh_mobile_lcdc_display_notify
fbdev/omap: sysfs files can't disappear before the device is gone
fbdev: sysfs files can't disappear before the device is gone
staging/olpc: lock_fb_info can't fail
fbdev/atyfb: lock_fb_info can't fail
fbdev: lock_fb_info cannot fail
fbcon: call fbcon_fb_bind directly
fbdev: make unregister/unlink functions not fail
fbdev: unify unlink_framebuffer paths
fbdev/sh_mob: Remove fb notifier callback
fbdev: directly call fbcon_suspended/resumed
fbcon: Call fbcon_mode_deleted/new_modelist directly
fbdev: Call fbcon_get_requirement directly
Revert "backlight/fbcon: Add FB_EVENT_CONBLANK"
fbmem: pull fbcon_fb_blanked out of fb_blank
fbdev: remove FBINFO_MISC_USEREVENT around fb_blank
fb: Flatten control flow in fb_set_var
fbcon: replace FB_EVENT_MODE_CHANGE/_ALL with direct calls
vgaswitcheroo: call fbcon_remap_all directly
fbcon: Call con2fb_map functions directly
fbcon: Document what I learned about fbcon locking
staging/olpc_dcon: Add drm conversion to TODO
backlight: simplify lcd notifier
arch/arm/mach-pxa/am200epd.c | 13 +-
drivers/gpu/vga/vga_switcheroo.c | 11 +-
drivers/media/pci/ivtv/ivtvfb.c | 6 +-
drivers/staging/fbtft/fbtft-core.c | 4 +-
drivers/staging/olpc_dcon/TODO | 7 +
drivers/staging/olpc_dcon/olpc_dcon.c | 6 +-
drivers/tty/vt/vt.c | 18 +
drivers/video/backlight/backlight.c | 2 +-
drivers/video/backlight/lcd.c | 12 -
drivers/video/console/dummycon.c | 6 +
drivers/video/fbdev/aty/aty128fb.c | 64 ---
drivers/video/fbdev/aty/atyfb_base.c | 3 +-
drivers/video/fbdev/core/fbcmap.c | 6 +-
drivers/video/fbdev/core/fbcon.c | 311 ++++++--------
drivers/video/fbdev/core/fbcon.h | 6 +-
drivers/video/fbdev/core/fbmem.c | 399 +++++++-----------
drivers/video/fbdev/core/fbsysfs.c | 20 +-
drivers/video/fbdev/cyber2000fb.c | 1 -
drivers/video/fbdev/neofb.c | 9 +-
.../video/fbdev/omap2/omapfb/omapfb-sysfs.c | 21 +-
drivers/video/fbdev/sa1100fb.c | 25 --
drivers/video/fbdev/savage/savagefb_driver.c | 9 +-
drivers/video/fbdev/sh_mobile_lcdcfb.c | 112 +----
drivers/video/fbdev/sh_mobile_lcdcfb.h | 5 -
include/linux/console_struct.h | 5 +-
include/linux/fb.h | 45 +-
include/linux/fbcon.h | 30 ++
27 files changed, 394 insertions(+), 762 deletions(-)
--
2.20.1
Simply because olpc never unregisters the damn thing. It also
registers the framebuffer directly by poking around in fbdev
core internals, so it's all around rather broken.
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Jens Frederich <[email protected]>
Cc: Daniel Drake <[email protected]>
Cc: Jon Nettleton <[email protected]>
---
drivers/staging/olpc_dcon/olpc_dcon.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/staging/olpc_dcon/olpc_dcon.c b/drivers/staging/olpc_dcon/olpc_dcon.c
index 6b714f740ac3..a254238be181 100644
--- a/drivers/staging/olpc_dcon/olpc_dcon.c
+++ b/drivers/staging/olpc_dcon/olpc_dcon.c
@@ -250,11 +250,7 @@ static bool dcon_blank_fb(struct dcon_priv *dcon, bool blank)
int err;
console_lock();
- if (!lock_fb_info(dcon->fbinfo)) {
- console_unlock();
- dev_err(&dcon->client->dev, "unable to lock framebuffer\n");
- return false;
- }
+ lock_fb_info(dcon->fbinfo);
dcon->ignore_fb_events = true;
err = fb_blank(dcon->fbinfo,
--
2.20.1
Which means lock_fb_info can never fail. Remove the error handling.
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Daniel Vetter <[email protected]>
---
.../video/fbdev/omap2/omapfb/omapfb-sysfs.c | 21 +++++++------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-sysfs.c b/drivers/video/fbdev/omap2/omapfb/omapfb-sysfs.c
index 8087a009c54f..bd0d20283372 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-sysfs.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-sysfs.c
@@ -60,8 +60,7 @@ static ssize_t store_rotate_type(struct device *dev,
if (rot_type != OMAP_DSS_ROT_DMA && rot_type != OMAP_DSS_ROT_VRFB)
return -EINVAL;
- if (!lock_fb_info(fbi))
- return -ENODEV;
+ lock_fb_info(fbi);
r = 0;
if (rot_type == ofbi->rotation_type)
@@ -112,8 +111,7 @@ static ssize_t store_mirror(struct device *dev,
if (r)
return r;
- if (!lock_fb_info(fbi))
- return -ENODEV;
+ lock_fb_info(fbi);
ofbi->mirror = mirror;
@@ -149,8 +147,7 @@ static ssize_t show_overlays(struct device *dev,
ssize_t l = 0;
int t;
- if (!lock_fb_info(fbi))
- return -ENODEV;
+ lock_fb_info(fbi);
omapfb_lock(fbdev);
for (t = 0; t < ofbi->num_overlays; t++) {
@@ -208,8 +205,7 @@ static ssize_t store_overlays(struct device *dev, struct device_attribute *attr,
if (buf[len - 1] == '\n')
len = len - 1;
- if (!lock_fb_info(fbi))
- return -ENODEV;
+ lock_fb_info(fbi);
omapfb_lock(fbdev);
if (len > 0) {
@@ -340,8 +336,7 @@ static ssize_t show_overlays_rotate(struct device *dev,
ssize_t l = 0;
int t;
- if (!lock_fb_info(fbi))
- return -ENODEV;
+ lock_fb_info(fbi);
for (t = 0; t < ofbi->num_overlays; t++) {
l += snprintf(buf + l, PAGE_SIZE - l, "%s%d",
@@ -369,8 +364,7 @@ static ssize_t store_overlays_rotate(struct device *dev,
if (buf[len - 1] == '\n')
len = len - 1;
- if (!lock_fb_info(fbi))
- return -ENODEV;
+ lock_fb_info(fbi);
if (len > 0) {
char *p = (char *)buf;
@@ -453,8 +447,7 @@ static ssize_t store_size(struct device *dev, struct device_attribute *attr,
size = PAGE_ALIGN(size);
- if (!lock_fb_info(fbi))
- return -ENODEV;
+ lock_fb_info(fbi);
if (display && display->driver->sync)
display->driver->sync(display);
--
2.20.1
Which means lock_fb_info can never fail. Remove the error handling.
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Bartlomiej Zolnierkiewicz <[email protected]>
Cc: Rob Clark <[email protected]>
---
drivers/video/fbdev/core/fbsysfs.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index 44cca39f2b51..5f329278e55f 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -179,10 +179,7 @@ static ssize_t store_modes(struct device *device,
return -EINVAL;
console_lock();
- if (!lock_fb_info(fb_info)) {
- console_unlock();
- return -ENODEV;
- }
+ lock_fb_info(fb_info);
list_splice(&fb_info->modelist, &old_list);
fb_videomode_to_modelist((const struct fb_videomode *)buf, i,
@@ -409,10 +406,7 @@ static ssize_t store_fbstate(struct device *device,
state = simple_strtoul(buf, &last, 0);
console_lock();
- if (!lock_fb_info(fb_info)) {
- console_unlock();
- return -ENODEV;
- }
+ lock_fb_info(fb_info);
fb_set_suspend(fb_info, (int)state);
--
2.20.1
For symmetry reasons with do_unblank_screen, except without the
oops_in_progress special case.
Just a drive-by annotation while I'm trying to untangle the fbcon vs.
fbdev screen blank/unblank maze.
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Nicolas Pitre <[email protected]>
Cc: Adam Borowski <[email protected]>
Cc: Martin Hostettler <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Mikulas Patocka <[email protected]>
---
drivers/tty/vt/vt.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index fdd12f8c3deb..bc9813b14c58 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4159,6 +4159,8 @@ void do_blank_screen(int entering_gfx)
struct vc_data *vc = vc_cons[fg_console].d;
int i;
+ might_sleep();
+
WARN_CONSOLE_UNLOCKED();
if (console_blanked) {
--
2.20.1
As part of trying to understand the locking (or lack thereof) in the
fbcon/vt/fbdev maze, annotate everything.
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Bartlomiej Zolnierkiewicz <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Nicolas Pitre <[email protected]>
---
drivers/video/console/dummycon.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/video/console/dummycon.c b/drivers/video/console/dummycon.c
index 45ad925ad5f8..2352b4c37826 100644
--- a/drivers/video/console/dummycon.c
+++ b/drivers/video/console/dummycon.c
@@ -33,6 +33,8 @@ static bool dummycon_putc_called;
void dummycon_register_output_notifier(struct notifier_block *nb)
{
+ WARN_CONSOLE_UNLOCKED();
+
raw_notifier_chain_register(&dummycon_output_nh, nb);
if (dummycon_putc_called)
@@ -41,11 +43,15 @@ void dummycon_register_output_notifier(struct notifier_block *nb)
void dummycon_unregister_output_notifier(struct notifier_block *nb)
{
+ WARN_CONSOLE_UNLOCKED();
+
raw_notifier_chain_unregister(&dummycon_output_nh, nb);
}
static void dummycon_putc(struct vc_data *vc, int c, int ypos, int xpos)
{
+ WARN_CONSOLE_UNLOCKED();
+
dummycon_putc_called = true;
raw_notifier_call_chain(&dummycon_output_nh, 0, NULL);
}
--
2.20.1
Hi Daniel.
Good work, nice cleanup all over.
A few comments to a few patches - not something that warrant a
new series to be posted as long as it is fixed before the patches are
applied.
> btw for future plans: I think this is tricky enough (it's old code and all
> that) that we should let this soak for 2-3 kernel releases. I think the
> following would be nice subsequent cleanup/fixes:
>
> - push the console lock completely from fbmem.c to fbcon.c. I think we're
> mostly there with prep, but needs to pondering of corner cases.
I wonder - should this code consistently use __acquire() etc so we could
get a little static analysis help for the locking?
I have not played with this for several years and I do not know the
maturity of this today.
> - move fbcon.c from using indices for tracking fb_info (and accessing
> registered_fbs without proper locking all the time) to real fb_info
> pointers with the right amount of refcounting. Mostly motivated by the
> fun I had trying to simplify fbcon_exit().
>
> - make sure that fbcon call lock/unlock_fb when it calls fbmem.c
> functions, and sprinkle assert_lockdep_held around in fbmem.c. This
> needs the console_lock cleanups first.
>
> But I think that's material for maybe next year or so.
Or maybe after next kernel release.
Could we put this nice plan into todo.rst or similar so we do not have
to hunt it down by asking google?
For the whole series you can add my:
Reviewed-by: Sam Ravnborg <[email protected]>
Some parts are reviewed as "this looks entirely correct", other parts
I would claim that I actually understood.
And after having spend some hours on this a r-b seems in order.
Sam
On Fri, May 24, 2019 at 10:53:35AM +0200, Daniel Vetter wrote:
> Simply because olpc never unregisters the damn thing. It also
> registers the framebuffer directly by poking around in fbdev
> core internals, so it's all around rather broken.
>
> Signed-off-by: Daniel Vetter <[email protected]>
> Cc: Jens Frederich <[email protected]>
> Cc: Daniel Drake <[email protected]>
> Cc: Jon Nettleton <[email protected]>
Hi Greg,
Somehow get_maintainers didn't pick you up for this. Ack for merging this
through drm/fbdev? It's part of a bigger series to rework fbdev/fbcon
interactions.
Thanks, Daniel
> ---
> drivers/staging/olpc_dcon/olpc_dcon.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/staging/olpc_dcon/olpc_dcon.c b/drivers/staging/olpc_dcon/olpc_dcon.c
> index 6b714f740ac3..a254238be181 100644
> --- a/drivers/staging/olpc_dcon/olpc_dcon.c
> +++ b/drivers/staging/olpc_dcon/olpc_dcon.c
> @@ -250,11 +250,7 @@ static bool dcon_blank_fb(struct dcon_priv *dcon, bool blank)
> int err;
>
> console_lock();
> - if (!lock_fb_info(dcon->fbinfo)) {
> - console_unlock();
> - dev_err(&dcon->client->dev, "unable to lock framebuffer\n");
> - return false;
> - }
> + lock_fb_info(dcon->fbinfo);
>
> dcon->ignore_fb_events = true;
> err = fb_blank(dcon->fbinfo,
> --
> 2.20.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Mon, May 27, 2019 at 09:10:10AM +0200, Daniel Vetter wrote:
> On Fri, May 24, 2019 at 10:53:35AM +0200, Daniel Vetter wrote:
> > Simply because olpc never unregisters the damn thing. It also
> > registers the framebuffer directly by poking around in fbdev
> > core internals, so it's all around rather broken.
> >
> > Signed-off-by: Daniel Vetter <[email protected]>
> > Cc: Jens Frederich <[email protected]>
> > Cc: Daniel Drake <[email protected]>
> > Cc: Jon Nettleton <[email protected]>
>
> Hi Greg,
>
> Somehow get_maintainers didn't pick you up for this. Ack for merging this
> through drm/fbdev? It's part of a bigger series to rework fbdev/fbcon
> interactions.
Again, all good for you to take:
Acked-by: Greg Kroah-Hartman <[email protected]>
On Sat, May 25, 2019 at 07:19:28PM +0200, Sam Ravnborg wrote:
> Hi Daniel.
>
> Good work, nice cleanup all over.
>
> A few comments to a few patches - not something that warrant a
> new series to be posted as long as it is fixed before the patches are
> applied.
Hm yeah good idea, I'll add that to the next version.
> > btw for future plans: I think this is tricky enough (it's old code and all
> > that) that we should let this soak for 2-3 kernel releases. I think the
> > following would be nice subsequent cleanup/fixes:
> >
> > - push the console lock completely from fbmem.c to fbcon.c. I think we're
> > mostly there with prep, but needs to pondering of corner cases.
> I wonder - should this code consistently use __acquire() etc so we could
> get a little static analysis help for the locking?
>
> I have not played with this for several years and I do not know the
> maturity of this today.
Ime these sparse annotations are pretty useless because too inflexible. I
tend to use runtime locking checks based on lockdep. Those are more
accurate, and work even when the place the lock is taken is very far away
from where you're checking, without having to annoate all functions in
between. We need that for something like console_lock which is a very big
lock. Only downside is that only paths you hit at runtime are checked.
> > - move fbcon.c from using indices for tracking fb_info (and accessing
> > registered_fbs without proper locking all the time) to real fb_info
> > pointers with the right amount of refcounting. Mostly motivated by the
> > fun I had trying to simplify fbcon_exit().
> >
> > - make sure that fbcon call lock/unlock_fb when it calls fbmem.c
> > functions, and sprinkle assert_lockdep_held around in fbmem.c. This
> > needs the console_lock cleanups first.
> >
> > But I think that's material for maybe next year or so.
> Or maybe after next kernel release.
> Could we put this nice plan into todo.rst or similar so we do not have
> to hunt it down by asking google?
>
> For the whole series you can add my:
> Reviewed-by: Sam Ravnborg <[email protected]>
>
> Some parts are reviewed as "this looks entirely correct", other parts
> I would claim that I actually understood.
> And after having spend some hours on this a r-b seems in order.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Mon, May 27, 2019 at 9:17 AM Daniel Vetter <[email protected]> wrote:
>
> On Sat, May 25, 2019 at 07:19:28PM +0200, Sam Ravnborg wrote:
> > Hi Daniel.
> >
> > Good work, nice cleanup all over.
> >
> > A few comments to a few patches - not something that warrant a
> > new series to be posted as long as it is fixed before the patches are
> > applied.
>
> Hm yeah good idea, I'll add that to the next version.
Actually I thought a bit more about the locking story, and it's a bit
worse than my simple plan here. I think better to just have that
floating around, and not make it look like it's an easy simple
cleanup.
The case I forgot about is that any places that has a printk can
recurse through the console_lock, which means we need a lot more
try_lock than I originally thought we'd need.
-Daniel
>
> > > btw for future plans: I think this is tricky enough (it's old code and all
> > > that) that we should let this soak for 2-3 kernel releases. I think the
> > > following would be nice subsequent cleanup/fixes:
> > >
> > > - push the console lock completely from fbmem.c to fbcon.c. I think we're
> > > mostly there with prep, but needs to pondering of corner cases.
> > I wonder - should this code consistently use __acquire() etc so we could
> > get a little static analysis help for the locking?
> >
> > I have not played with this for several years and I do not know the
> > maturity of this today.
>
> Ime these sparse annotations are pretty useless because too inflexible. I
> tend to use runtime locking checks based on lockdep. Those are more
> accurate, and work even when the place the lock is taken is very far away
> from where you're checking, without having to annoate all functions in
> between. We need that for something like console_lock which is a very big
> lock. Only downside is that only paths you hit at runtime are checked.
>
> > > - move fbcon.c from using indices for tracking fb_info (and accessing
> > > registered_fbs without proper locking all the time) to real fb_info
> > > pointers with the right amount of refcounting. Mostly motivated by the
> > > fun I had trying to simplify fbcon_exit().
> > >
> > > - make sure that fbcon call lock/unlock_fb when it calls fbmem.c
> > > functions, and sprinkle assert_lockdep_held around in fbmem.c. This
> > > needs the console_lock cleanups first.
> > >
> > > But I think that's material for maybe next year or so.
> > Or maybe after next kernel release.
> > Could we put this nice plan into todo.rst or similar so we do not have
> > to hunt it down by asking google?
> >
> > For the whole series you can add my:
> > Reviewed-by: Sam Ravnborg <[email protected]>
> >
> > Some parts are reviewed as "this looks entirely correct", other parts
> > I would claim that I actually understood.
> > And after having spend some hours on this a r-b seems in order.
>
> Thanks, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch