| Message ID | 20220208210824.2238981-7-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | fbcon patches, take two | expand |
Hello Daniel, On 2/8/22 22:08, Daniel Vetter wrote: > Allows us to delete a bunch of hand-rolled stuff. Also to simplify the > code we initialize the cursor_work completely when we allocate the > fbcon_ops structure, instead of trying to cope with console > re-initialization. > Maybe also make it more explicit in the commit message that the delayed work is replacing a timer that was used before for the cursor ? > The motiviation here is that fbcon code stops using the fb_info.queue, motivation [snip] > /* > * This is the interface between the low-level console driver and the > @@ -68,7 +68,7 @@ struct fbcon_ops { > int (*update_start)(struct fb_info *info); > int (*rotate_font)(struct fb_info *info, struct vc_data *vc); > struct fb_var_screeninfo var; /* copy of the current fb_var_screeninfo */ > - struct timer_list cursor_timer; /* Cursor timer */ > + struct delayed_work cursor_work; /* Cursor timer */ A delayed_work uses a timer underneath but I wonder if the comment also needs to be updated since technically isn't a timer anymore but deferred work that gets re-scheduled each time on fb_flashcursor(). The patch looks good to me and makes the logic much simpler than before. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Best regards,
Am 08.02.22 um 22:08 schrieb Daniel Vetter: > Allows us to delete a bunch of hand-rolled stuff. Also to simplify the > code we initialize the cursor_work completely when we allocate the > fbcon_ops structure, instead of trying to cope with console > re-initialization. > > The motiviation here is that fbcon code stops using the fb_info.queue, > which helps with locking issues around cleanup and all that in a later > patch. > > Also note that this allows us to ditch the hand-rolled work cleanup in > fbcon_exit - we already call fbcon_del_cursor_timer, which takes care > of everything. Plus this was racy anyway. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Claudio Suarez <cssk@net-c.es> > Cc: Du Cheng <ducheng2@gmail.com> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > drivers/video/fbdev/core/fbcon.c | 85 +++++++++++++------------------- > drivers/video/fbdev/core/fbcon.h | 4 +- > 2 files changed, 35 insertions(+), 54 deletions(-) > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > index 83f0223f5333..a368ed602e2e 100644 > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -350,8 +350,8 @@ static int get_color(struct vc_data *vc, struct fb_info *info, > > static void fb_flashcursor(struct work_struct *work) > { > - struct fb_info *info = container_of(work, struct fb_info, queue); > - struct fbcon_ops *ops = info->fbcon_par; > + struct fbcon_ops *ops = container_of(work, struct fbcon_ops, cursor_work.work); > + struct fb_info *info; > struct vc_data *vc = NULL; > int c; > int mode; > @@ -364,7 +364,10 @@ static void fb_flashcursor(struct work_struct *work) > if (ret == 0) > return; > > - if (ops && ops->currcon != -1) > + /* protected by console_lock */ > + info = ops->info; > + > + if (ops->currcon != -1) > vc = vc_cons[ops->currcon].d; > > if (!vc || !con_is_visible(vc) || > @@ -380,42 +383,25 @@ static void fb_flashcursor(struct work_struct *work) > ops->cursor(vc, info, mode, get_color(vc, info, c, 1), > get_color(vc, info, c, 0)); > console_unlock(); > -} > > -static void cursor_timer_handler(struct timer_list *t) > -{ > - struct fbcon_ops *ops = from_timer(ops, t, cursor_timer); > - struct fb_info *info = ops->info; > - > - queue_work(system_power_efficient_wq, &info->queue); > - mod_timer(&ops->cursor_timer, jiffies + ops->cur_blink_jiffies); > + queue_delayed_work(system_power_efficient_wq, &ops->cursor_work, > + ops->cur_blink_jiffies); > } > > -static void fbcon_add_cursor_timer(struct fb_info *info) > +static void fbcon_add_cursor_work(struct fb_info *info) > { > struct fbcon_ops *ops = info->fbcon_par; > > - if ((!info->queue.func || info->queue.func == fb_flashcursor) && > - !(ops->flags & FBCON_FLAGS_CURSOR_TIMER) && > - !fbcon_cursor_noblink) { > - if (!info->queue.func) > - INIT_WORK(&info->queue, fb_flashcursor); > - > - timer_setup(&ops->cursor_timer, cursor_timer_handler, 0); > - mod_timer(&ops->cursor_timer, jiffies + ops->cur_blink_jiffies); > - ops->flags |= FBCON_FLAGS_CURSOR_TIMER; > - } > + if (!fbcon_cursor_noblink) > + queue_delayed_work(system_power_efficient_wq, &ops->cursor_work, > + ops->cur_blink_jiffies); > } > > -static void fbcon_del_cursor_timer(struct fb_info *info) > +static void fbcon_del_cursor_work(struct fb_info *info) > { > struct fbcon_ops *ops = info->fbcon_par; > > - if (info->queue.func == fb_flashcursor && > - ops->flags & FBCON_FLAGS_CURSOR_TIMER) { > - del_timer_sync(&ops->cursor_timer); > - ops->flags &= ~FBCON_FLAGS_CURSOR_TIMER; > - } > + cancel_delayed_work_sync(&ops->cursor_work); > } > > #ifndef MODULE > @@ -714,6 +700,8 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > if (!ops) > err = -ENOMEM; > + > + INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor); There's similar code in fbcon_startup() when there should be a single init function for fbcon_ops. Maybe something for later. Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > } > > if (!err) { > @@ -751,7 +739,7 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, > } > > if (!err) { > - fbcon_del_cursor_timer(oldinfo); > + fbcon_del_cursor_work(oldinfo); > kfree(ops->cursor_state.mask); > kfree(ops->cursor_data); > kfree(ops->cursor_src); > @@ -867,7 +855,7 @@ static int set_con2fb_map(int unit, int newidx, int user) > logo_shown != FBCON_LOGO_DONTSHOW); > > if (!found) > - fbcon_add_cursor_timer(info); > + fbcon_add_cursor_work(info); > con2fb_map_boot[unit] = newidx; > con2fb_init_display(vc, info, unit, show_logo); > } > @@ -964,6 +952,8 @@ static const char *fbcon_startup(void) > return NULL; > } > > + INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor); > + > ops->currcon = -1; > ops->graphics = 1; > ops->cur_rotate = -1; > @@ -1006,7 +996,7 @@ static const char *fbcon_startup(void) > info->var.yres, > info->var.bits_per_pixel); > > - fbcon_add_cursor_timer(info); > + fbcon_add_cursor_work(info); > return display_desc; > } > > @@ -1194,7 +1184,7 @@ static void fbcon_deinit(struct vc_data *vc) > goto finished; > > if (con_is_visible(vc)) > - fbcon_del_cursor_timer(info); > + fbcon_del_cursor_work(info); > > ops->flags &= ~FBCON_FLAGS_INIT; > finished: > @@ -1320,9 +1310,9 @@ static void fbcon_cursor(struct vc_data *vc, int mode) > return; > > if (vc->vc_cursor_type & CUR_SW) > - fbcon_del_cursor_timer(info); > + fbcon_del_cursor_work(info); > else > - fbcon_add_cursor_timer(info); > + fbcon_add_cursor_work(info); > > ops->cursor_flash = (mode == CM_ERASE) ? 0 : 1; > > @@ -2132,14 +2122,14 @@ static int fbcon_switch(struct vc_data *vc) > } > > if (old_info != info) > - fbcon_del_cursor_timer(old_info); > + fbcon_del_cursor_work(old_info); > } > > if (fbcon_is_inactive(vc, info) || > ops->blank_state != FB_BLANK_UNBLANK) > - fbcon_del_cursor_timer(info); > + fbcon_del_cursor_work(info); > else > - fbcon_add_cursor_timer(info); > + fbcon_add_cursor_work(info); > > set_blitting_type(vc, info); > ops->cursor_reset = 1; > @@ -2247,9 +2237,9 @@ static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch) > > if (mode_switch || fbcon_is_inactive(vc, info) || > ops->blank_state != FB_BLANK_UNBLANK) > - fbcon_del_cursor_timer(info); > + fbcon_del_cursor_work(info); > else > - fbcon_add_cursor_timer(info); > + fbcon_add_cursor_work(info); > > return 0; > } > @@ -3181,7 +3171,7 @@ static ssize_t show_cursor_blink(struct device *device, > if (!ops) > goto err; > > - blink = (ops->flags & FBCON_FLAGS_CURSOR_TIMER) ? 1 : 0; > + blink = delayed_work_pending(&ops->cursor_work); > err: > console_unlock(); > return snprintf(buf, PAGE_SIZE, "%d\n", blink); > @@ -3210,10 +3200,10 @@ static ssize_t store_cursor_blink(struct device *device, > > if (blink) { > fbcon_cursor_noblink = 0; > - fbcon_add_cursor_timer(info); > + fbcon_add_cursor_work(info); > } else { > fbcon_cursor_noblink = 1; > - fbcon_del_cursor_timer(info); > + fbcon_del_cursor_work(info); > } > > err: > @@ -3314,15 +3304,9 @@ static void fbcon_exit(void) > #endif > > for_each_registered_fb(i) { > - int pending = 0; > - > mapped = 0; > info = registered_fb[i]; > > - if (info->queue.func) > - pending = cancel_work_sync(&info->queue); > - pr_debug("fbcon: %s pending work\n", (pending ? "canceled" : "no")); > - > for (j = first_fb_vc; j <= last_fb_vc; j++) { > if (con2fb_map[j] == i) { > mapped = 1; > @@ -3338,15 +3322,12 @@ static void fbcon_exit(void) > if (info->fbcon_par) { > struct fbcon_ops *ops = info->fbcon_par; > > - fbcon_del_cursor_timer(info); > + fbcon_del_cursor_work(info); > kfree(ops->cursor_src); > kfree(ops->cursor_state.mask); > kfree(info->fbcon_par); > info->fbcon_par = NULL; > } > - > - if (info->queue.func == fb_flashcursor) > - info->queue.func = NULL; > } > } > } > diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h > index 969d41ecede5..6708ca0048aa 100644 > --- a/drivers/video/fbdev/core/fbcon.h > +++ b/drivers/video/fbdev/core/fbcon.h > @@ -14,11 +14,11 @@ > #include <linux/types.h> > #include <linux/vt_buffer.h> > #include <linux/vt_kern.h> > +#include <linux/workqueue.h> > > #include <asm/io.h> > > #define FBCON_FLAGS_INIT 1 > -#define FBCON_FLAGS_CURSOR_TIMER 2 > > /* > * This is the interface between the low-level console driver and the > @@ -68,7 +68,7 @@ struct fbcon_ops { > int (*update_start)(struct fb_info *info); > int (*rotate_font)(struct fb_info *info, struct vc_data *vc); > struct fb_var_screeninfo var; /* copy of the current fb_var_screeninfo */ > - struct timer_list cursor_timer; /* Cursor timer */ > + struct delayed_work cursor_work; /* Cursor timer */ > struct fb_cursor cursor_state; > struct fbcon_display *p; > struct fb_info *info;
On 2022/02/09 6:08, Daniel Vetter wrote: > @@ -714,6 +700,8 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > if (!ops) > err = -ENOMEM; > + > + INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor); > } > > if (!err) { Memory allocation fault injection will hit NULL pointer dereference.
On Thu, Feb 10, 2022 at 08:43:36PM +0900, Tetsuo Handa wrote: > On 2022/02/09 6:08, Daniel Vetter wrote: > > @@ -714,6 +700,8 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > > if (!ops) > > err = -ENOMEM; > > + > > + INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor); > > } > > > > if (!err) { > > Memory allocation fault injection will hit NULL pointer dereference. The error handling here is convoluted and I got this wrong, but a later patch to extract an fbcon_open() helper fixes it. I'll fix this small bisect issue for v3 anyway, thanks for taking a look at the patches. -Daniel
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 83f0223f5333..a368ed602e2e 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -350,8 +350,8 @@ static int get_color(struct vc_data *vc, struct fb_info *info, static void fb_flashcursor(struct work_struct *work) { - struct fb_info *info = container_of(work, struct fb_info, queue); - struct fbcon_ops *ops = info->fbcon_par; + struct fbcon_ops *ops = container_of(work, struct fbcon_ops, cursor_work.work); + struct fb_info *info; struct vc_data *vc = NULL; int c; int mode; @@ -364,7 +364,10 @@ static void fb_flashcursor(struct work_struct *work) if (ret == 0) return; - if (ops && ops->currcon != -1) + /* protected by console_lock */ + info = ops->info; + + if (ops->currcon != -1) vc = vc_cons[ops->currcon].d; if (!vc || !con_is_visible(vc) || @@ -380,42 +383,25 @@ static void fb_flashcursor(struct work_struct *work) ops->cursor(vc, info, mode, get_color(vc, info, c, 1), get_color(vc, info, c, 0)); console_unlock(); -} -static void cursor_timer_handler(struct timer_list *t) -{ - struct fbcon_ops *ops = from_timer(ops, t, cursor_timer); - struct fb_info *info = ops->info; - - queue_work(system_power_efficient_wq, &info->queue); - mod_timer(&ops->cursor_timer, jiffies + ops->cur_blink_jiffies); + queue_delayed_work(system_power_efficient_wq, &ops->cursor_work, + ops->cur_blink_jiffies); } -static void fbcon_add_cursor_timer(struct fb_info *info) +static void fbcon_add_cursor_work(struct fb_info *info) { struct fbcon_ops *ops = info->fbcon_par; - if ((!info->queue.func || info->queue.func == fb_flashcursor) && - !(ops->flags & FBCON_FLAGS_CURSOR_TIMER) && - !fbcon_cursor_noblink) { - if (!info->queue.func) - INIT_WORK(&info->queue, fb_flashcursor); - - timer_setup(&ops->cursor_timer, cursor_timer_handler, 0); - mod_timer(&ops->cursor_timer, jiffies + ops->cur_blink_jiffies); - ops->flags |= FBCON_FLAGS_CURSOR_TIMER; - } + if (!fbcon_cursor_noblink) + queue_delayed_work(system_power_efficient_wq, &ops->cursor_work, + ops->cur_blink_jiffies); } -static void fbcon_del_cursor_timer(struct fb_info *info) +static void fbcon_del_cursor_work(struct fb_info *info) { struct fbcon_ops *ops = info->fbcon_par; - if (info->queue.func == fb_flashcursor && - ops->flags & FBCON_FLAGS_CURSOR_TIMER) { - del_timer_sync(&ops->cursor_timer); - ops->flags &= ~FBCON_FLAGS_CURSOR_TIMER; - } + cancel_delayed_work_sync(&ops->cursor_work); } #ifndef MODULE @@ -714,6 +700,8 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); if (!ops) err = -ENOMEM; + + INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor); } if (!err) { @@ -751,7 +739,7 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, } if (!err) { - fbcon_del_cursor_timer(oldinfo); + fbcon_del_cursor_work(oldinfo); kfree(ops->cursor_state.mask); kfree(ops->cursor_data); kfree(ops->cursor_src); @@ -867,7 +855,7 @@ static int set_con2fb_map(int unit, int newidx, int user) logo_shown != FBCON_LOGO_DONTSHOW); if (!found) - fbcon_add_cursor_timer(info); + fbcon_add_cursor_work(info); con2fb_map_boot[unit] = newidx; con2fb_init_display(vc, info, unit, show_logo); } @@ -964,6 +952,8 @@ static const char *fbcon_startup(void) return NULL; } + INIT_DELAYED_WORK(&ops->cursor_work, fb_flashcursor); + ops->currcon = -1; ops->graphics = 1; ops->cur_rotate = -1; @@ -1006,7 +996,7 @@ static const char *fbcon_startup(void) info->var.yres, info->var.bits_per_pixel); - fbcon_add_cursor_timer(info); + fbcon_add_cursor_work(info); return display_desc; } @@ -1194,7 +1184,7 @@ static void fbcon_deinit(struct vc_data *vc) goto finished; if (con_is_visible(vc)) - fbcon_del_cursor_timer(info); + fbcon_del_cursor_work(info); ops->flags &= ~FBCON_FLAGS_INIT; finished: @@ -1320,9 +1310,9 @@ static void fbcon_cursor(struct vc_data *vc, int mode) return; if (vc->vc_cursor_type & CUR_SW) - fbcon_del_cursor_timer(info); + fbcon_del_cursor_work(info); else - fbcon_add_cursor_timer(info); + fbcon_add_cursor_work(info); ops->cursor_flash = (mode == CM_ERASE) ? 0 : 1; @@ -2132,14 +2122,14 @@ static int fbcon_switch(struct vc_data *vc) } if (old_info != info) - fbcon_del_cursor_timer(old_info); + fbcon_del_cursor_work(old_info); } if (fbcon_is_inactive(vc, info) || ops->blank_state != FB_BLANK_UNBLANK) - fbcon_del_cursor_timer(info); + fbcon_del_cursor_work(info); else - fbcon_add_cursor_timer(info); + fbcon_add_cursor_work(info); set_blitting_type(vc, info); ops->cursor_reset = 1; @@ -2247,9 +2237,9 @@ static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch) if (mode_switch || fbcon_is_inactive(vc, info) || ops->blank_state != FB_BLANK_UNBLANK) - fbcon_del_cursor_timer(info); + fbcon_del_cursor_work(info); else - fbcon_add_cursor_timer(info); + fbcon_add_cursor_work(info); return 0; } @@ -3181,7 +3171,7 @@ static ssize_t show_cursor_blink(struct device *device, if (!ops) goto err; - blink = (ops->flags & FBCON_FLAGS_CURSOR_TIMER) ? 1 : 0; + blink = delayed_work_pending(&ops->cursor_work); err: console_unlock(); return snprintf(buf, PAGE_SIZE, "%d\n", blink); @@ -3210,10 +3200,10 @@ static ssize_t store_cursor_blink(struct device *device, if (blink) { fbcon_cursor_noblink = 0; - fbcon_add_cursor_timer(info); + fbcon_add_cursor_work(info); } else { fbcon_cursor_noblink = 1; - fbcon_del_cursor_timer(info); + fbcon_del_cursor_work(info); } err: @@ -3314,15 +3304,9 @@ static void fbcon_exit(void) #endif for_each_registered_fb(i) { - int pending = 0; - mapped = 0; info = registered_fb[i]; - if (info->queue.func) - pending = cancel_work_sync(&info->queue); - pr_debug("fbcon: %s pending work\n", (pending ? "canceled" : "no")); - for (j = first_fb_vc; j <= last_fb_vc; j++) { if (con2fb_map[j] == i) { mapped = 1; @@ -3338,15 +3322,12 @@ static void fbcon_exit(void) if (info->fbcon_par) { struct fbcon_ops *ops = info->fbcon_par; - fbcon_del_cursor_timer(info); + fbcon_del_cursor_work(info); kfree(ops->cursor_src); kfree(ops->cursor_state.mask); kfree(info->fbcon_par); info->fbcon_par = NULL; } - - if (info->queue.func == fb_flashcursor) - info->queue.func = NULL; } } } diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h index 969d41ecede5..6708ca0048aa 100644 --- a/drivers/video/fbdev/core/fbcon.h +++ b/drivers/video/fbdev/core/fbcon.h @@ -14,11 +14,11 @@ #include <linux/types.h> #include <linux/vt_buffer.h> #include <linux/vt_kern.h> +#include <linux/workqueue.h> #include <asm/io.h> #define FBCON_FLAGS_INIT 1 -#define FBCON_FLAGS_CURSOR_TIMER 2 /* * This is the interface between the low-level console driver and the @@ -68,7 +68,7 @@ struct fbcon_ops { int (*update_start)(struct fb_info *info); int (*rotate_font)(struct fb_info *info, struct vc_data *vc); struct fb_var_screeninfo var; /* copy of the current fb_var_screeninfo */ - struct timer_list cursor_timer; /* Cursor timer */ + struct delayed_work cursor_work; /* Cursor timer */ struct fb_cursor cursor_state; struct fbcon_display *p; struct fb_info *info;
Allows us to delete a bunch of hand-rolled stuff. Also to simplify the code we initialize the cursor_work completely when we allocate the fbcon_ops structure, instead of trying to cope with console re-initialization. The motiviation here is that fbcon code stops using the fb_info.queue, which helps with locking issues around cleanup and all that in a later patch. Also note that this allows us to ditch the hand-rolled work cleanup in fbcon_exit - we already call fbcon_del_cursor_timer, which takes care of everything. Plus this was racy anyway. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Claudio Suarez <cssk@net-c.es> Cc: Du Cheng <ducheng2@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- drivers/video/fbdev/core/fbcon.c | 85 +++++++++++++------------------- drivers/video/fbdev/core/fbcon.h | 4 +- 2 files changed, 35 insertions(+), 54 deletions(-)