| Message ID | 20220131210552.482606-15-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | some fbcon patches, mostly locking | expand |
On Mon, Jan 31, 2022 at 10:05:45PM +0100, Daniel Vetter wrote: > Now we get to the real motiviation, because fbmem.c insists that > that's the right lock for these. > > Ofc fbcon.c has a lot more places where it probably should call > lock_fb_info(). But looking at fbmem.c at least most of these seem to > be protected by console_lock() too, which is probably what papers over > any issues. > > Note that this means we're shuffling around a bit the locking sections > for some of the console takeover and unbind paths, but not all: > - console binding/unbinding from the console layer never with > lock_fb_info > - unbind (as opposed to unlink) never bother with lock_fb_info > > Also the real serialization against set_par and set_pan are still > doing by wrapping the entire ioctl code in console_lock(). So this > shuffling shouldn't be worse than what we had from a "can you trigger > races?" pov, but it's at least clearer. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Claudio Suarez <cssk@net-c.es> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Du Cheng <ducheng2@gmail.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: William Kucharski <william.kucharski@oracle.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Zheyu Ma <zheyuma97@gmail.com> > Cc: Zhen Lei <thunder.leizhen@huawei.com> > Cc: Xiyu Yang <xiyuyang19@fudan.edu.cn> Well, the patch does what the commit log says. Acked-by: Sam Ravnborg <sam@ravnborg.org>
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 5a3391ff038d..496bc5f2133e 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -682,8 +682,10 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount) static void fbcon_release(struct fb_info *info) { + lock_fb_info(info); if (info->fbops->fb_release) info->fbops->fb_release(info, 0); + unlock_fb_info(info); module_put(info->fbops->owner); } @@ -695,11 +697,14 @@ static int fbcon_open(struct fb_info *info) if (!try_module_get(info->fbops->owner)) return -ENODEV; + lock_fb_info(info); if (info->fbops->fb_open && info->fbops->fb_open(info, 0)) { + unlock_fb_info(info); module_put(info->fbops->owner); return -ENODEV; } + unlock_fb_info(info); ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); if (!ops) { diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 0fa7ede94fa6..fd51d12f2702 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1653,9 +1653,7 @@ static int do_register_framebuffer(struct fb_info *fb_info) console_lock(); else atomic_inc(&ignore_console_lock_warning); - lock_fb_info(fb_info); ret = fbcon_fb_registered(fb_info); - unlock_fb_info(fb_info); if (!lockless_register_fb) console_unlock(); @@ -1672,9 +1670,7 @@ static void unbind_console(struct fb_info *fb_info) return; console_lock(); - lock_fb_info(fb_info); fbcon_fb_unbind(fb_info); - unlock_fb_info(fb_info); console_unlock(); }
Now we get to the real motiviation, because fbmem.c insists that that's the right lock for these. Ofc fbcon.c has a lot more places where it probably should call lock_fb_info(). But looking at fbmem.c at least most of these seem to be protected by console_lock() too, which is probably what papers over any issues. Note that this means we're shuffling around a bit the locking sections for some of the console takeover and unbind paths, but not all: - console binding/unbinding from the console layer never with lock_fb_info - unbind (as opposed to unlink) never bother with lock_fb_info Also the real serialization against set_par and set_pan are still doing by wrapping the entire ioctl code in console_lock(). So this shuffling shouldn't be worse than what we had from a "can you trigger races?" pov, but it's at least clearer. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Claudio Suarez <cssk@net-c.es> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Du Cheng <ducheng2@gmail.com> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Matthew Wilcox <willy@infradead.org> Cc: William Kucharski <william.kucharski@oracle.com> Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Zheyu Ma <zheyuma97@gmail.com> Cc: Zhen Lei <thunder.leizhen@huawei.com> Cc: Xiyu Yang <xiyuyang19@fudan.edu.cn> --- drivers/video/fbdev/core/fbcon.c | 5 +++++ drivers/video/fbdev/core/fbmem.c | 4 ---- 2 files changed, 5 insertions(+), 4 deletions(-)