Re: [PATCH 19/21] fbcon: Maintain a private array of fb_info

From: Sam Ravnborg
Date: Sat Feb 05 2022 - 00:43:23 EST

Next message: Ira Weiny: "Re: [PATCH V8 36/44] memremap_pages: Reserve a PKS PKey for eventual use by PMEM"
Previous message: Sam Ravnborg: "Re: [PATCH 01/21] MAINTAINERS: Add entry for fbdev core"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Daniel,

On Mon, Jan 31, 2022 at 10:05:50PM +0100, Daniel Vetter wrote:
> Accessing the one in fbmem.c without taking the right locks is a bad
> idea. Instead maintain our own private copy, which is fully protected
> by console_lock() (like everything else in fbcon.c). That copy is
> serialized through fbcon_fb_registered/unregistered() calls.

I fail to see why we can make a private copy of registered_fb
just like that - are they not somehow shared between fbcon and fbmem.
So when fbmem updates it, then fbcon will use the entry or such?

I guess I am just ignorant of how registered_fb is used - but please
explain.

Sam

>
> Also this means we do not need to hold a full fb_info reference, which
> is nice because doing so would mean a refcount loop between the
> console and the fb_info. But it's also not nice since it means
> console_lock() must be held absolutely everywhere. Well strictly
> speaking we could still try to do some refcounting games again by
> calling get_fb_info before we drop the console_lock. But things will
> get tricky.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Cc: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Cc: Claudio Suarez <cssk@xxxxxxxx>
> Cc: Du Cheng <ducheng2@xxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
> drivers/video/fbdev/core/fbcon.c | 82 +++++++++++++++++---------------
> 1 file changed, 43 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 22581952b4fd..a0ca34b29615 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -86,10 +86,6 @@
> * - fbcon state itself is protected by the console_lock, and the code does a
> * pretty good job at making sure that lock is held everywhere it's needed.
> *
> - * - access to the registered_fb array is entirely unprotected. This should use
> - * proper object lifetime handling, i.e. get/put_fb_info. This also means
> - * switching from indices to proper pointers for fb_info everywhere.
> - *
> * - fbcon doesn't bother with fb_lock/unlock at all. This is buggy, since it
> * means concurrent access to the same fbdev from both fbcon and userspace
> * will blow up. To fix this all fbcon calls from fbmem.c need to be moved out
> @@ -107,6 +103,13 @@ enum {
>
> static struct fbcon_display fb_display[MAX_NR_CONSOLES];
>
> +struct fb_info *fbcon_registered_fb[FB_MAX];
> +int fbcon_num_registered_fb;
> +
> +#define fbcon_for_each_registered_fb(i) \
> + for (i = 0; WARN_CONSOLE_UNLOCKED(), i < FB_MAX; i++) \
> + if (!fbcon_registered_fb[i]) {} else
> +
> static signed char con2fb_map[MAX_NR_CONSOLES];
> static signed char con2fb_map_boot[MAX_NR_CONSOLES];
>
> @@ -114,12 +117,7 @@ static struct fb_info *fbcon_info_from_console(int console)
> {
> WARN_CONSOLE_UNLOCKED();
>
> - /*
> - * Note that only con2fb_map is protected by the console lock,
> - * registered_fb is protected by a separate mutex. This lookup can
> - * therefore race.
> - */
> - return registered_fb[con2fb_map[console]];
> + return fbcon_registered_fb[con2fb_map[console]];
> }
>
> static int logo_lines;
> @@ -516,7 +514,7 @@ static int do_fbcon_takeover(int show_logo)
> {
> int err, i;
>
> - if (!num_registered_fb)
> + if (!fbcon_num_registered_fb)
> return -ENODEV;
>
> if (!show_logo)
> @@ -822,7 +820,7 @@ static int set_con2fb_map(int unit, int newidx, int user)
> {
> struct vc_data *vc = vc_cons[unit].d;
> int oldidx = con2fb_map[unit];
> - struct fb_info *info = registered_fb[newidx];
> + struct fb_info *info = fbcon_registered_fb[newidx];
> struct fb_info *oldinfo = NULL;
> int found, err = 0, show_logo;
>
> @@ -840,7 +838,7 @@ static int set_con2fb_map(int unit, int newidx, int user)
> }
>
> if (oldidx != -1)
> - oldinfo = registered_fb[oldidx];
> + oldinfo = fbcon_registered_fb[oldidx];
>
> found = search_fb_in_map(newidx);
>
> @@ -932,13 +930,13 @@ static const char *fbcon_startup(void)
> * If num_registered_fb is zero, this is a call for the dummy part.
> * The frame buffer devices weren't initialized yet.
> */
> - if (!num_registered_fb || info_idx == -1)
> + if (!fbcon_num_registered_fb || info_idx == -1)
> return display_desc;
> /*
> * Instead of blindly using registered_fb[0], we use info_idx, set by
> * fbcon_fb_registered();
> */
> - info = registered_fb[info_idx];
> + info = fbcon_registered_fb[info_idx];
> if (!info)
> return NULL;
>
> @@ -1153,9 +1151,9 @@ static void fbcon_release_all(void)
> struct fb_info *info;
> int i, j, mapped;
>
> - for_each_registered_fb(i) {
> + fbcon_for_each_registered_fb(i) {
> mapped = 0;
> - info = registered_fb[i];
> + info = fbcon_registered_fb[i];
>
> for (j = first_fb_vc; j <= last_fb_vc; j++) {
> if (con2fb_map[j] == i) {
> @@ -1182,7 +1180,7 @@ static void fbcon_deinit(struct vc_data *vc)
> if (idx == -1)
> goto finished;
>
> - info = registered_fb[idx];
> + info = fbcon_registered_fb[idx];
>
> if (!info)
> goto finished;
> @@ -2118,9 +2116,9 @@ static int fbcon_switch(struct vc_data *vc)
> *
> * info->currcon = vc->vc_num;
> */
> - for_each_registered_fb(i) {
> - if (registered_fb[i]->fbcon_par) {
> - struct fbcon_ops *o = registered_fb[i]->fbcon_par;
> + fbcon_for_each_registered_fb(i) {
> + if (fbcon_registered_fb[i]->fbcon_par) {
> + struct fbcon_ops *o = fbcon_registered_fb[i]->fbcon_par;
>
> o->currcon = vc->vc_num;
> }
> @@ -2765,7 +2763,7 @@ int fbcon_mode_deleted(struct fb_info *info,
> j = con2fb_map[i];
> if (j == -1)
> continue;
> - fb_info = registered_fb[j];
> + fb_info = fbcon_registered_fb[j];
> if (fb_info != info)
> continue;
> p = &fb_display[i];
> @@ -2821,7 +2819,7 @@ void fbcon_fb_unbind(struct fb_info *info)
> set_con2fb_map(i, new_idx, 0);
> }
> } else {
> - struct fb_info *info = registered_fb[idx];
> + struct fb_info *info = fbcon_registered_fb[idx];
>
> /* This is sort of like set_con2fb_map, except it maps
> * the consoles to no device and then releases the
> @@ -2851,6 +2849,9 @@ void fbcon_fb_unregistered(struct fb_info *info)
>
> console_lock();
>
> + fbcon_registered_fb[info->node] = NULL;
> + fbcon_num_registered_fb--;
> +
> if (deferred_takeover) {
> console_unlock();
> return;
> @@ -2865,7 +2866,7 @@ void fbcon_fb_unregistered(struct fb_info *info)
> if (idx == info_idx) {
> info_idx = -1;
>
> - for_each_registered_fb(i) {
> + fbcon_for_each_registered_fb(i) {
> info_idx = i;
> break;
> }
> @@ -2881,7 +2882,7 @@ void fbcon_fb_unregistered(struct fb_info *info)
> if (primary_device == idx)
> primary_device = -1;
>
> - if (!num_registered_fb)
> + if (!fbcon_num_registered_fb)
> do_unregister_con_driver(&fb_con);
> console_unlock();
> }
> @@ -2956,6 +2957,9 @@ int fbcon_fb_registered(struct fb_info *info)
> else
> atomic_inc(&ignore_console_lock_warning);
>
> + fbcon_registered_fb[info->node] = info;
> + fbcon_num_registered_fb++;
> +
> idx = info->node;
> fbcon_select_primary(info);
>
> @@ -3075,9 +3079,9 @@ int fbcon_set_con2fb_map_ioctl(void __user *argp)
> return -EINVAL;
> if (con2fb.framebuffer >= FB_MAX)
> return -EINVAL;
> - if (!registered_fb[con2fb.framebuffer])
> + if (!fbcon_registered_fb[con2fb.framebuffer])
> request_module("fb%d", con2fb.framebuffer);
> - if (!registered_fb[con2fb.framebuffer]) {
> + if (!fbcon_registered_fb[con2fb.framebuffer]) {
> return -EINVAL;
> }
>
> @@ -3144,10 +3148,10 @@ static ssize_t store_rotate(struct device *device,
> console_lock();
> idx = con2fb_map[fg_console];
>
> - if (idx == -1 || registered_fb[idx] == NULL)
> + if (idx == -1 || fbcon_registered_fb[idx] == NULL)
> goto err;
>
> - info = registered_fb[idx];
> + info = fbcon_registered_fb[idx];
> rotate = simple_strtoul(buf, last, 0);
> fbcon_rotate(info, rotate);
> err:
> @@ -3166,10 +3170,10 @@ static ssize_t store_rotate_all(struct device *device,
> console_lock();
> idx = con2fb_map[fg_console];
>
> - if (idx == -1 || registered_fb[idx] == NULL)
> + if (idx == -1 || fbcon_registered_fb[idx] == NULL)
> goto err;
>
> - info = registered_fb[idx];
> + info = fbcon_registered_fb[idx];
> rotate = simple_strtoul(buf, last, 0);
> fbcon_rotate_all(info, rotate);
> err:
> @@ -3186,10 +3190,10 @@ static ssize_t show_rotate(struct device *device,
> console_lock();
> idx = con2fb_map[fg_console];
>
> - if (idx == -1 || registered_fb[idx] == NULL)
> + if (idx == -1 || fbcon_registered_fb[idx] == NULL)
> goto err;
>
> - info = registered_fb[idx];
> + info = fbcon_registered_fb[idx];
> rotate = fbcon_get_rotate(info);
> err:
> console_unlock();
> @@ -3206,10 +3210,10 @@ static ssize_t show_cursor_blink(struct device *device,
> console_lock();
> idx = con2fb_map[fg_console];
>
> - if (idx == -1 || registered_fb[idx] == NULL)
> + if (idx == -1 || fbcon_registered_fb[idx] == NULL)
> goto err;
>
> - info = registered_fb[idx];
> + info = fbcon_registered_fb[idx];
> ops = info->fbcon_par;
>
> if (!ops)
> @@ -3232,10 +3236,10 @@ static ssize_t store_cursor_blink(struct device *device,
> console_lock();
> idx = con2fb_map[fg_console];
>
> - if (idx == -1 || registered_fb[idx] == NULL)
> + if (idx == -1 || fbcon_registered_fb[idx] == NULL)
> goto err;
>
> - info = registered_fb[idx];
> + info = fbcon_registered_fb[idx];
>
> if (!info->fbcon_par)
> goto err;
> @@ -3295,8 +3299,8 @@ static void fbcon_register_existing_fbs(struct work_struct *work)
> deferred_takeover = false;
> logo_shown = FBCON_LOGO_DONTSHOW;
>
> - for_each_registered_fb(i)
> - fbcon_fb_registered(registered_fb[i]);
> + fbcon_for_each_registered_fb(i)
> + fbcon_fb_registered(fbcon_registered_fb[i]);
>
> console_unlock();
> }
> --
> 2.33.0

Next message: Ira Weiny: "Re: [PATCH V8 36/44] memremap_pages: Reserve a PKS PKey for eventual use by PMEM"
Previous message: Sam Ravnborg: "Re: [PATCH 01/21] MAINTAINERS: Add entry for fbdev core"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]