All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	LKML <linux-kernel@vger.kernel.org>,
	Prarit Bhargava <prarit@redhat.com>,
	Kees Cook <keescook@chromium.org>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Yisheng Xie <ysxie@foxmail.com>,
	Hans de Goede <hdegoede@redhat.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Konstantin Khorenko <khorenko@virtuozzo.com>
Subject: Re: [PATCH 09/33] fbcon: Remove fbcon_has_exited
Date: Mon, 27 May 2019 08:10:42 +0200	[thread overview]
Message-ID: <20190527061042.GF21222@phenom.ffwll.local> (raw)
In-Reply-To: <20190525153826.GA8661@ravnborg.org>

On Sat, May 25, 2019 at 05:38:26PM +0200, Sam Ravnborg wrote:
> Hi Daniel.
> 
> One detail I noticed while brosing the changes.
> 
> >  
> > @@ -1064,9 +1062,13 @@ static void fbcon_init(struct vc_data *vc, int init)
> >  	int logo = 1, new_rows, new_cols, rows, cols, charcnt = 256;
> >  	int cap, ret;
> >  
> > -	if (info_idx == -1 || info == NULL)
> > +	if (WARN_ON(info_idx == -1))
> >  	    return;
> >  
> > +	if (con2fb_map[vc->vc_num] == -1)
> > +		con2fb_map[vc->vc_num] = info_idx;
> > +
> > +	info = registered_fb[con2fb_map[vc->vc_num]];
> >  	cap = info->flags;
> 
> When info is defined it is also assigned:
> struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> 
> As the test for info is gone this assignment is no longer
> requrired and can be deleted.

We use it later on, so we definitely still need info. But I indeed forgot
to delete the initial assignment of info at the function start. Is that
what you mean here?
>
> The code now assumes that there is always an fb_info if con2fb_map[]
> is not set to -1. I could not determine if this is OK, but this
> likely boils down to your locking concern of registered_fb.

Yup, see how fb_registered/unregistered manage this. I think that part
actually all works out correctly (as long as everyone is holding
console_lock). But it's a bit unpretty that fbcon digs around in the raw
registered_fb array instead of going through the refcounted helpers, and
having a full refcounted pointer. Much easier to convince yourself of that
than chasing indices assigments all over imo.
-Daniel

> 
> 	Sam
> 
> >  
> >  	if (logo_shown < 0 && console_loglevel <= CONSOLE_LOGLEVEL_QUIET)
> > @@ -3336,14 +3338,6 @@ static int fbcon_event_notify(struct notifier_block *self,
> >  	struct fb_blit_caps *caps;
> >  	int idx, ret = 0;
> >  
> > -	/*
> > -	 * ignore all events except driver registration and deregistration
> > -	 * if fbcon is not active
> > -	 */
> > -	if (fbcon_has_exited && !(action == FB_EVENT_FB_REGISTERED ||
> > -				  action == FB_EVENT_FB_UNREGISTERED))
> > -		goto done;
> > -
> >  	switch(action) {
> >  	case FB_EVENT_SUSPEND:
> >  		fbcon_suspended(info);
> > @@ -3396,7 +3390,6 @@ static int fbcon_event_notify(struct notifier_block *self,
> >  		fbcon_remap_all(idx);
> >  		break;
> >  	}
> > -done:
> >  	return ret;
> >  }
> >  
> > @@ -3443,9 +3436,6 @@ static ssize_t store_rotate(struct device *device,
> >  	int rotate, idx;
> >  	char **last = NULL;
> >  
> > -	if (fbcon_has_exited)
> > -		return count;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3468,9 +3458,6 @@ static ssize_t store_rotate_all(struct device *device,
> >  	int rotate, idx;
> >  	char **last = NULL;
> >  
> > -	if (fbcon_has_exited)
> > -		return count;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3491,9 +3478,6 @@ static ssize_t show_rotate(struct device *device,
> >  	struct fb_info *info;
> >  	int rotate = 0, idx;
> >  
> > -	if (fbcon_has_exited)
> > -		return 0;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3514,9 +3498,6 @@ static ssize_t show_cursor_blink(struct device *device,
> >  	struct fbcon_ops *ops;
> >  	int idx, blink = -1;
> >  
> > -	if (fbcon_has_exited)
> > -		return 0;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3543,9 +3524,6 @@ static ssize_t store_cursor_blink(struct device *device,
> >  	int blink, idx;
> >  	char **last = NULL;
> >  
> > -	if (fbcon_has_exited)
> > -		return count;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3668,9 +3646,6 @@ static void fbcon_exit(void)
> >  	struct fb_info *info;
> >  	int i, j, mapped;
> >  
> > -	if (fbcon_has_exited)
> > -		return;
> > -
> >  #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> >  	if (deferred_takeover) {
> >  		dummycon_unregister_output_notifier(&fbcon_output_nb);
> > @@ -3695,7 +3670,7 @@ static void fbcon_exit(void)
> >  		for (j = first_fb_vc; j <= last_fb_vc; j++) {
> >  			if (con2fb_map[j] == i) {
> >  				mapped = 1;
> > -				break;
> > +				con2fb_map[j] = -1;
> >  			}
> >  		}
> >  
> > @@ -3718,8 +3693,6 @@ static void fbcon_exit(void)
> >  				info->queue.func = NULL;
> >  		}
> >  	}
> > -
> > -	fbcon_has_exited = 1;
> >  }
> >  
> >  void __init fb_console_init(void)
> > -- 
> > 2.20.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Prarit Bhargava <prarit@redhat.com>,
	Kees Cook <keescook@chromium.org>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	LKML <linux-kernel@vger.kernel.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Yisheng Xie <ysxie@foxmail.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Konstantin Khorenko <khorenko@virtuozzo.com>
Subject: Re: [PATCH 09/33] fbcon: Remove fbcon_has_exited
Date: Mon, 27 May 2019 08:10:42 +0200	[thread overview]
Message-ID: <20190527061042.GF21222@phenom.ffwll.local> (raw)
In-Reply-To: <20190525153826.GA8661@ravnborg.org>

On Sat, May 25, 2019 at 05:38:26PM +0200, Sam Ravnborg wrote:
> Hi Daniel.
> 
> One detail I noticed while brosing the changes.
> 
> >  
> > @@ -1064,9 +1062,13 @@ static void fbcon_init(struct vc_data *vc, int init)
> >  	int logo = 1, new_rows, new_cols, rows, cols, charcnt = 256;
> >  	int cap, ret;
> >  
> > -	if (info_idx == -1 || info == NULL)
> > +	if (WARN_ON(info_idx == -1))
> >  	    return;
> >  
> > +	if (con2fb_map[vc->vc_num] == -1)
> > +		con2fb_map[vc->vc_num] = info_idx;
> > +
> > +	info = registered_fb[con2fb_map[vc->vc_num]];
> >  	cap = info->flags;
> 
> When info is defined it is also assigned:
> struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> 
> As the test for info is gone this assignment is no longer
> requrired and can be deleted.

We use it later on, so we definitely still need info. But I indeed forgot
to delete the initial assignment of info at the function start. Is that
what you mean here?
>
> The code now assumes that there is always an fb_info if con2fb_map[]
> is not set to -1. I could not determine if this is OK, but this
> likely boils down to your locking concern of registered_fb.

Yup, see how fb_registered/unregistered manage this. I think that part
actually all works out correctly (as long as everyone is holding
console_lock). But it's a bit unpretty that fbcon digs around in the raw
registered_fb array instead of going through the refcounted helpers, and
having a full refcounted pointer. Much easier to convince yourself of that
than chasing indices assigments all over imo.
-Daniel

> 
> 	Sam
> 
> >  
> >  	if (logo_shown < 0 && console_loglevel <= CONSOLE_LOGLEVEL_QUIET)
> > @@ -3336,14 +3338,6 @@ static int fbcon_event_notify(struct notifier_block *self,
> >  	struct fb_blit_caps *caps;
> >  	int idx, ret = 0;
> >  
> > -	/*
> > -	 * ignore all events except driver registration and deregistration
> > -	 * if fbcon is not active
> > -	 */
> > -	if (fbcon_has_exited && !(action == FB_EVENT_FB_REGISTERED ||
> > -				  action == FB_EVENT_FB_UNREGISTERED))
> > -		goto done;
> > -
> >  	switch(action) {
> >  	case FB_EVENT_SUSPEND:
> >  		fbcon_suspended(info);
> > @@ -3396,7 +3390,6 @@ static int fbcon_event_notify(struct notifier_block *self,
> >  		fbcon_remap_all(idx);
> >  		break;
> >  	}
> > -done:
> >  	return ret;
> >  }
> >  
> > @@ -3443,9 +3436,6 @@ static ssize_t store_rotate(struct device *device,
> >  	int rotate, idx;
> >  	char **last = NULL;
> >  
> > -	if (fbcon_has_exited)
> > -		return count;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3468,9 +3458,6 @@ static ssize_t store_rotate_all(struct device *device,
> >  	int rotate, idx;
> >  	char **last = NULL;
> >  
> > -	if (fbcon_has_exited)
> > -		return count;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3491,9 +3478,6 @@ static ssize_t show_rotate(struct device *device,
> >  	struct fb_info *info;
> >  	int rotate = 0, idx;
> >  
> > -	if (fbcon_has_exited)
> > -		return 0;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3514,9 +3498,6 @@ static ssize_t show_cursor_blink(struct device *device,
> >  	struct fbcon_ops *ops;
> >  	int idx, blink = -1;
> >  
> > -	if (fbcon_has_exited)
> > -		return 0;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3543,9 +3524,6 @@ static ssize_t store_cursor_blink(struct device *device,
> >  	int blink, idx;
> >  	char **last = NULL;
> >  
> > -	if (fbcon_has_exited)
> > -		return count;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3668,9 +3646,6 @@ static void fbcon_exit(void)
> >  	struct fb_info *info;
> >  	int i, j, mapped;
> >  
> > -	if (fbcon_has_exited)
> > -		return;
> > -
> >  #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> >  	if (deferred_takeover) {
> >  		dummycon_unregister_output_notifier(&fbcon_output_nb);
> > @@ -3695,7 +3670,7 @@ static void fbcon_exit(void)
> >  		for (j = first_fb_vc; j <= last_fb_vc; j++) {
> >  			if (con2fb_map[j] == i) {
> >  				mapped = 1;
> > -				break;
> > +				con2fb_map[j] = -1;
> >  			}
> >  		}
> >  
> > @@ -3718,8 +3693,6 @@ static void fbcon_exit(void)
> >  				info->queue.func = NULL;
> >  		}
> >  	}
> > -
> > -	fbcon_has_exited = 1;
> >  }
> >  
> >  void __init fb_console_init(void)
> > -- 
> > 2.20.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2019-05-27  6:10 UTC|newest]

Thread overview: 106+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-24  8:53 [PATCH 00/33] fbcon notifier begone! Daniel Vetter
2019-05-24  8:53 ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 01/33] dummycon: Sprinkle locking checks Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 02/33] fbdev: locking check for fb_set_suspend Daniel Vetter
2019-05-24  8:53 ` [PATCH 03/33] vt: might_sleep() annotation for do_blank_screen Daniel Vetter
2019-05-24  8:53 ` [PATCH 04/33] vt: More locking checks Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-27  7:08   ` Daniel Vetter
2019-05-27  7:08     ` Daniel Vetter
2019-05-27  7:22     ` Greg Kroah-Hartman
2019-05-24  8:53 ` [PATCH 05/33] fbdev/sa1100fb: Remove dead code Daniel Vetter
2019-05-24  8:53 ` [PATCH 06/33] fbdev/cyber2000: Remove struct display Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 07/33] fbdev/aty128fb: Remove dead code Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 08/33] fbcon: s/struct display/struct fbcon_display/ Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 09/33] fbcon: Remove fbcon_has_exited Daniel Vetter
2019-05-25 15:38   ` Sam Ravnborg
2019-05-25 15:38     ` Sam Ravnborg
2019-05-27  6:10     ` Daniel Vetter [this message]
2019-05-27  6:10       ` Daniel Vetter
2019-05-27  6:51       ` Sam Ravnborg
2019-05-27  6:51         ` Sam Ravnborg
2019-05-24  8:53 ` [PATCH 10/33] fbcon: call fbcon_fb_(un)registered directly Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 11/33] fbdev/sh_mobile: remove sh_mobile_lcdc_display_notify Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-25 15:01   ` Sam Ravnborg
2019-05-25 15:01     ` Sam Ravnborg
2019-05-27  6:13     ` Daniel Vetter
2019-05-27  6:52       ` Sam Ravnborg
2019-05-24  8:53 ` [PATCH 12/33] fbdev/omap: sysfs files can't disappear before the device is gone Daniel Vetter
2019-05-24  8:53 ` [PATCH 13/33] fbdev: " Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 14/33] staging/olpc: lock_fb_info can't fail Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-27  7:10   ` Daniel Vetter
2019-05-27  7:22     ` Greg KH
2019-05-27  7:22       ` Greg KH
2019-05-24  8:53 ` [PATCH 15/33] fbdev/atyfb: " Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 16/33] fbdev: lock_fb_info cannot fail Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 17/33] fbcon: call fbcon_fb_bind directly Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 18/33] fbdev: make unregister/unlink functions not fail Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 19/33] fbdev: unify unlink_framebuffer paths Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 20/33] fbdev/sh_mob: Remove fb notifier callback Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 21/33] fbdev: directly call fbcon_suspended/resumed Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 22/33] fbcon: Call fbcon_mode_deleted/new_modelist directly Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 23/33] fbdev: Call fbcon_get_requirement directly Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 24/33] Revert "backlight/fbcon: Add FB_EVENT_CONBLANK" Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24 13:14   ` Daniel Thompson
2019-05-24 13:14     ` Daniel Thompson
2019-05-24 13:14     ` Daniel Thompson
2019-05-24 15:28     ` Daniel Vetter
2019-05-24 15:28       ` Daniel Vetter
2019-05-27 10:59       ` Bartlomiej Zolnierkiewicz
2019-05-27 10:59         ` Bartlomiej Zolnierkiewicz
2019-05-24  8:53 ` [PATCH 25/33] fbmem: pull fbcon_fb_blanked out of fb_blank Daniel Vetter
2019-05-25 17:00   ` Sam Ravnborg
2019-05-25 17:00     ` Sam Ravnborg
2019-05-24  8:53 ` [PATCH 26/33] fbdev: remove FBINFO_MISC_USEREVENT around fb_blank Daniel Vetter
2019-05-24  8:53 ` [PATCH 27/33] fb: Flatten control flow in fb_set_var Daniel Vetter
2019-05-24  8:53 ` [PATCH 28/33] fbcon: replace FB_EVENT_MODE_CHANGE/_ALL with direct calls Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 29/33] vgaswitcheroo: call fbcon_remap_all directly Daniel Vetter
2019-05-24  8:53   ` Daniel Vetter
2019-05-24  8:53 ` [PATCH 30/33] fbcon: Call con2fb_map functions directly Daniel Vetter
2019-05-24  8:53 ` [PATCH 31/33] fbcon: Document what I learned about fbcon locking Daniel Vetter
2019-05-24  8:53 ` [PATCH 32/33] staging/olpc_dcon: Add drm conversion to TODO Daniel Vetter
2019-05-27  7:11   ` Daniel Vetter
2019-05-27  7:11     ` Daniel Vetter
2019-05-27  7:22     ` Greg KH
2019-05-24  8:53 ` [PATCH 33/33] backlight: simplify lcd notifier Daniel Vetter
2019-05-24 11:03 ` ✗ Fi.CI.CHECKPATCH: warning for fbcon notifier begone! (rev3) Patchwork
2019-05-24 11:18 ` ✗ Fi.CI.SPARSE: " Patchwork
2019-05-24 11:23 ` ✓ Fi.CI.BAT: success " Patchwork
2019-05-25 17:19 ` [PATCH 00/33] fbcon notifier begone! Sam Ravnborg
2019-05-27  7:17   ` Daniel Vetter
2019-05-27 11:56     ` Daniel Vetter
2019-05-25 17:24 ` ✗ Fi.CI.IGT: failure for fbcon notifier begone! (rev3) Patchwork
2019-05-26 15:34 ` ✗ Fi.CI.CHECKPATCH: warning for fbcon notifier begone! (rev4) Patchwork
2019-05-26 15:49 ` ✗ Fi.CI.SPARSE: " Patchwork
2019-05-26 15:53 ` ✓ Fi.CI.BAT: success " Patchwork
2019-05-27 11:02 ` ✓ Fi.CI.IGT: " Patchwork
  -- strict thread matches above, loose matches on Subject: below --
2019-05-28  9:02 [PATCH 00/33] fbcon notifier begone v3! Daniel Vetter
2019-05-28  9:02 ` [PATCH 09/33] fbcon: Remove fbcon_has_exited Daniel Vetter
2019-05-28  9:02   ` Daniel Vetter
2019-05-28  9:02   ` Daniel Vetter
2019-05-20  8:21 [PATCH 00/33] fbcon notifier begone! Daniel Vetter
2019-05-20  8:21 ` [PATCH 09/33] fbcon: Remove fbcon_has_exited Daniel Vetter
2019-05-20  8:21   ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190527061042.GF21222@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=b.zolnierkie@samsung.com \
    --cc=daniel.vetter@ffwll.ch \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hdegoede@redhat.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=keescook@chromium.org \
    --cc=khorenko@virtuozzo.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=prarit@redhat.com \
    --cc=sam@ravnborg.org \
    --cc=ysxie@foxmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.