* [PATCH 0/2] fbcon+i915 locking fixes @ 2013-01-23 16:25 Daniel Vetter 2013-01-23 16:25 ` [PATCH 1/2] fb: Rework locking to fix lock ordering on takeover Daniel Vetter 2013-01-23 16:25 ` [PATCH 2/2] drm/i915: fixup per-crtc locking in intel_release_load_detect_pipe Daniel Vetter 0 siblings, 2 replies; 6+ messages in thread From: Daniel Vetter @ 2013-01-23 16:25 UTC (permalink / raw) To: Dave Airlie Cc: Daniel Vetter, Intel Graphics Development, linux-fbdev-devel, LKML, DRI Development Hi Dave, First patch is a resend of Alan's fbcon vs. fb notifier deadlock fix. Without this lockdep is pretty much useless for debugging drm locking issues, and it looks like quite a few bootup hangs could be blamed on this one, too. Since the fbdev maintainer seems to be awol, please merge this through your drm tree. I've looked through the patch and the new locking seems to be sane so feel free to smash a Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> onto it. 2nd patch is a locking bug in the i915 driver with the new kms locking. Since the bug only happens in your drm-next tree, please apply the patch there directly. Thanks, Daniel Alan Cox (1): fb: Rework locking to fix lock ordering on takeover Daniel Vetter (1): drm/i915: fixup crtc locking in intel_release_load_detect_pipe drivers/gpu/drm/i915/intel_display.c | 1 + drivers/tty/vt/vt.c | 81 ++++++++++++++++++++++++++-------- drivers/video/console/fbcon.c | 30 ++++++++++++- drivers/video/fbmem.c | 5 +-- drivers/video/fbsysfs.c | 3 ++ include/linux/console.h | 1 + 6 files changed, 98 insertions(+), 23 deletions(-) -- 1.7.10.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] fb: Rework locking to fix lock ordering on takeover 2013-01-23 16:25 [PATCH 0/2] fbcon+i915 locking fixes Daniel Vetter @ 2013-01-23 16:25 ` Daniel Vetter 2013-01-23 16:38 ` Takashi Iwai 2013-01-23 16:25 ` [PATCH 2/2] drm/i915: fixup per-crtc locking in intel_release_load_detect_pipe Daniel Vetter 1 sibling, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2013-01-23 16:25 UTC (permalink / raw) To: Dave Airlie Cc: Intel Graphics Development, DRI Development, LKML, linux-fbdev-devel, Alan Cox, Alan Cox, Daniel Vetter From: Alan Cox <alan@lxorguk.ukuu.org.uk> Adjust the console layer to allow a take over call where the caller already holds the locks. Make the fb layer lock in order. This s partly a band aid, the fb layer is terminally confused about the locking rules it uses for its notifiers it seems. Signed-off-by: Alan Cox <alan@linux.intel.com> [danvet: Tiny whitespace cleanup.] Reported-and-tested-by: Hugh Dickins <hughd@google.com> Reported-and-tested-by: Sasha Levin <levinsasha928@gmail.com> References: https://lkml.org/lkml/2012/10/25/516 Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/tty/vt/vt.c | 81 +++++++++++++++++++++++++++++++---------- drivers/video/console/fbcon.c | 30 ++++++++++++++- drivers/video/fbmem.c | 5 +-- drivers/video/fbsysfs.c | 3 ++ include/linux/console.h | 1 + 5 files changed, 97 insertions(+), 23 deletions(-) diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c index 8fd8968..a38d9d1 100644 --- a/drivers/tty/vt/vt.c +++ b/drivers/tty/vt/vt.c @@ -2987,7 +2987,7 @@ int __init vty_init(const struct file_operations *console_fops) static struct class *vtconsole_class; -static int bind_con_driver(const struct consw *csw, int first, int last, +static int do_bind_con_driver(const struct consw *csw, int first, int last, int deflt) { struct module *owner = csw->owner; @@ -2998,7 +2998,7 @@ static int bind_con_driver(const struct consw *csw, int first, int last, if (!try_module_get(owner)) return -ENODEV; - console_lock(); + WARN_CONSOLE_UNLOCKED(); /* check if driver is registered */ for (i = 0; i < MAX_NR_CON_DRIVER; i++) { @@ -3083,11 +3083,22 @@ static int bind_con_driver(const struct consw *csw, int first, int last, retval = 0; err: - console_unlock(); module_put(owner); return retval; }; + +static int bind_con_driver(const struct consw *csw, int first, int last, + int deflt) +{ + int ret; + + console_lock(); + ret = do_bind_con_driver(csw, first, last, deflt); + console_unlock(); + return ret; +} + #ifdef CONFIG_VT_HW_CONSOLE_BINDING static int con_is_graphics(const struct consw *csw, int first, int last) { @@ -3199,9 +3210,9 @@ int unbind_con_driver(const struct consw *csw, int first, int last, int deflt) if (!con_is_bound(csw)) con_driver->flag &= ~CON_DRIVER_FLAG_INIT; - console_unlock(); /* ignore return value, binding should not fail */ - bind_con_driver(defcsw, first, last, deflt); + do_bind_con_driver(defcsw, first, last, deflt); + console_unlock(); err: module_put(owner); return retval; @@ -3492,28 +3503,18 @@ int con_debug_leave(void) } EXPORT_SYMBOL_GPL(con_debug_leave); -/** - * register_con_driver - register console driver to console layer - * @csw: console driver - * @first: the first console to take over, minimum value is 0 - * @last: the last console to take over, maximum value is MAX_NR_CONSOLES -1 - * - * DESCRIPTION: This function registers a console driver which can later - * bind to a range of consoles specified by @first and @last. It will - * also initialize the console driver by calling con_startup(). - */ -int register_con_driver(const struct consw *csw, int first, int last) +static int do_register_con_driver(const struct consw *csw, int first, int last) { struct module *owner = csw->owner; struct con_driver *con_driver; const char *desc; int i, retval = 0; + WARN_CONSOLE_UNLOCKED(); + if (!try_module_get(owner)) return -ENODEV; - console_lock(); - for (i = 0; i < MAX_NR_CON_DRIVER; i++) { con_driver = ®istered_con_driver[i]; @@ -3566,10 +3567,29 @@ int register_con_driver(const struct consw *csw, int first, int last) } err: - console_unlock(); module_put(owner); return retval; } + +/** + * register_con_driver - register console driver to console layer + * @csw: console driver + * @first: the first console to take over, minimum value is 0 + * @last: the last console to take over, maximum value is MAX_NR_CONSOLES -1 + * + * DESCRIPTION: This function registers a console driver which can later + * bind to a range of consoles specified by @first and @last. It will + * also initialize the console driver by calling con_startup(). + */ +int register_con_driver(const struct consw *csw, int first, int last) +{ + int retval; + + console_lock(); + retval = do_register_con_driver(csw, first, last); + console_unlock(); + return retval; +} EXPORT_SYMBOL(register_con_driver); /** @@ -3625,6 +3645,29 @@ EXPORT_SYMBOL(unregister_con_driver); * * take_over_console is basically a register followed by unbind */ +int do_take_over_console(const struct consw *csw, int first, int last, int deflt) +{ + int err; + + err = do_register_con_driver(csw, first, last); + /* if we get an busy error we still want to bind the console driver + * and return success, as we may have unbound the console driver + * but not unregistered it. + */ + if (err == -EBUSY) + err = 0; + if (!err) + do_bind_con_driver(csw, first, last, deflt); + + return err; +} +/* + * If we support more console drivers, this function is used + * when a driver wants to take over some existing consoles + * and become default driver for newly opened ones. + * + * take_over_console is basically a register followed by unbind + */ int take_over_console(const struct consw *csw, int first, int last, int deflt) { int err; diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c index fdefa8f..c75f8ce 100644 --- a/drivers/video/console/fbcon.c +++ b/drivers/video/console/fbcon.c @@ -529,6 +529,34 @@ static int search_for_mapped_con(void) return retval; } +static int do_fbcon_takeover(int show_logo) +{ + int err, i; + + if (!num_registered_fb) + return -ENODEV; + + if (!show_logo) + logo_shown = FBCON_LOGO_DONTSHOW; + + for (i = first_fb_vc; i <= last_fb_vc; i++) + con2fb_map[i] = info_idx; + + err = do_take_over_console(&fb_con, first_fb_vc, last_fb_vc, + fbcon_is_default); + + if (err) { + for (i = first_fb_vc; i <= last_fb_vc; i++) { + con2fb_map[i] = -1; + } + info_idx = -1; + } else { + fbcon_has_console_bind = 1; + } + + return err; +} + static int fbcon_takeover(int show_logo) { int err, i; @@ -3115,7 +3143,7 @@ static int fbcon_fb_registered(struct fb_info *info) } if (info_idx != -1) - ret = fbcon_takeover(1); + ret = do_fbcon_takeover(1); } else { for (i = first_fb_vc; i <= last_fb_vc; i++) { if (con2fb_map_boot[i] == idx) diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c index 3ff0105..d8d9831 100644 --- a/drivers/video/fbmem.c +++ b/drivers/video/fbmem.c @@ -1650,7 +1650,9 @@ static int do_register_framebuffer(struct fb_info *fb_info) event.info = fb_info; if (!lock_fb_info(fb_info)) return -ENODEV; + console_lock(); fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event); + console_unlock(); unlock_fb_info(fb_info); return 0; } @@ -1853,11 +1855,8 @@ int fb_new_modelist(struct fb_info *info) err = 1; if (!list_empty(&info->modelist)) { - if (!lock_fb_info(info)) - return -ENODEV; event.info = info; err = fb_notifier_call_chain(FB_EVENT_NEW_MODELIST, &event); - unlock_fb_info(info); } return err; diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c index a55e366..ef476b0 100644 --- a/drivers/video/fbsysfs.c +++ b/drivers/video/fbsysfs.c @@ -177,6 +177,8 @@ static ssize_t store_modes(struct device *device, if (i * sizeof(struct fb_videomode) != count) return -EINVAL; + if (!lock_fb_info(fb_info)) + return -ENODEV; console_lock(); list_splice(&fb_info->modelist, &old_list); fb_videomode_to_modelist((const struct fb_videomode *)buf, i, @@ -188,6 +190,7 @@ static ssize_t store_modes(struct device *device, fb_destroy_modelist(&old_list); console_unlock(); + unlock_fb_info(fb_info); return 0; } diff --git a/include/linux/console.h b/include/linux/console.h index dedb082..4ef4307 100644 --- a/include/linux/console.h +++ b/include/linux/console.h @@ -78,6 +78,7 @@ int con_is_bound(const struct consw *csw); int register_con_driver(const struct consw *csw, int first, int last); int unregister_con_driver(const struct consw *csw); int take_over_console(const struct consw *sw, int first, int last, int deflt); +int do_take_over_console(const struct consw *sw, int first, int last, int deflt); void give_up_console(const struct consw *sw); #ifdef CONFIG_HW_CONSOLE int con_debug_enter(struct vc_data *vc); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] fb: Rework locking to fix lock ordering on takeover 2013-01-23 16:25 ` [PATCH 1/2] fb: Rework locking to fix lock ordering on takeover Daniel Vetter @ 2013-01-23 16:38 ` Takashi Iwai 2013-01-23 16:49 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2013-01-23 16:38 UTC (permalink / raw) To: Daniel Vetter Cc: Dave Airlie, Intel Graphics Development, DRI Development, LKML, linux-fbdev-devel, Alan Cox, Alan Cox At Wed, 23 Jan 2013 17:25:08 +0100, Daniel Vetter wrote: > > From: Alan Cox <alan@lxorguk.ukuu.org.uk> > > Adjust the console layer to allow a take over call where the caller already > holds the locks. Make the fb layer lock in order. > > This s partly a band aid, the fb layer is terminally confused about the > locking rules it uses for its notifiers it seems. > > Signed-off-by: Alan Cox <alan@linux.intel.com> > [danvet: Tiny whitespace cleanup.] > Reported-and-tested-by: Hugh Dickins <hughd@google.com> > Reported-and-tested-by: Sasha Levin <levinsasha928@gmail.com> > References: https://lkml.org/lkml/2012/10/25/516 > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> FYI, the latest patch of this is found in mm tree: http://ozlabs.org/~akpm/mmots/broken-out/fb-rework-locking-to-fix-lock-ordering-on-takeover.patch Also I hit the same problem in another code paths (for unbind and unregister): http://marc.info/?t=135309396400003&r=1&w=2 My additional patch is found in mm tree, too: http://ozlabs.org/~akpm/mmots/broken-out/fb-yet-another-band-aid-for-fixing-lockdep-mess.patch Takashi ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] fb: Rework locking to fix lock ordering on takeover 2013-01-23 16:38 ` Takashi Iwai @ 2013-01-23 16:49 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2013-01-23 16:49 UTC (permalink / raw) To: Takashi Iwai Cc: linux-fbdev-devel, Intel Graphics Development, LKML, DRI Development, Andrew Morton, Alan Cox, Alan Cox On Wed, Jan 23, 2013 at 5:38 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Wed, 23 Jan 2013 17:25:08 +0100, > Daniel Vetter wrote: >> >> From: Alan Cox <alan@lxorguk.ukuu.org.uk> >> >> Adjust the console layer to allow a take over call where the caller already >> holds the locks. Make the fb layer lock in order. >> >> This s partly a band aid, the fb layer is terminally confused about the >> locking rules it uses for its notifiers it seems. >> >> Signed-off-by: Alan Cox <alan@linux.intel.com> >> [danvet: Tiny whitespace cleanup.] >> Reported-and-tested-by: Hugh Dickins <hughd@google.com> >> Reported-and-tested-by: Sasha Levin <levinsasha928@gmail.com> >> References: https://lkml.org/lkml/2012/10/25/516 >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > FYI, the latest patch of this is found in mm tree: > http://ozlabs.org/~akpm/mmots/broken-out/fb-rework-locking-to-fix-lock-ordering-on-takeover.patch > > Also I hit the same problem in another code paths (for unbind and > unregister): > http://marc.info/?t=135309396400003&r=1&w=2 > > My additional patch is found in mm tree, too: > http://ozlabs.org/~akpm/mmots/broken-out/fb-yet-another-band-aid-for-fixing-lockdep-mess.patch Indeed, there's more stuff :( And I've missed the export_symbol fix for the first patch. Still, is there any chance we can at least merge the first one (I don't think that many people unload framebuffers) into 3.8 with cc: stable? I'd like to get at least rid of the known deadlocks at kms takeover time, since we have quite a few unexplained such bug reports floating around ... Andrew, Dave? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/i915: fixup per-crtc locking in intel_release_load_detect_pipe 2013-01-23 16:25 [PATCH 0/2] fbcon+i915 locking fixes Daniel Vetter 2013-01-23 16:25 ` [PATCH 1/2] fb: Rework locking to fix lock ordering on takeover Daniel Vetter @ 2013-01-23 16:25 ` Daniel Vetter 2013-01-24 9:55 ` Daniel Vetter 1 sibling, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2013-01-23 16:25 UTC (permalink / raw) To: Dave Airlie Cc: Intel Graphics Development, DRI Development, LKML, linux-fbdev-devel, Daniel Vetter One of the early return cases missed the mutex unlocking. Hilarity ensued. This regression has been introduced in commit 7b24056be6db7ce907baffdd4cf142ab774ea60c Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Wed Dec 12 00:35:33 2012 +0100 drm: don't hold crtc mutexes for connector ->detect callbacks Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59750 Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 26df9e3..ece4afd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6522,6 +6522,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, drm_framebuffer_unreference(old->release_fb); } + mutex_unlock(&crtc->mutex); return; } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: fixup per-crtc locking in intel_release_load_detect_pipe 2013-01-23 16:25 ` [PATCH 2/2] drm/i915: fixup per-crtc locking in intel_release_load_detect_pipe Daniel Vetter @ 2013-01-24 9:55 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2013-01-24 9:55 UTC (permalink / raw) To: Dave Airlie Cc: Daniel Vetter, Intel Graphics Development, linux-fbdev-devel, LKML, DRI Development On Wed, Jan 23, 2013 at 05:25:09PM +0100, Daniel Vetter wrote: > One of the early return cases missed the mutex unlocking. Hilarity > ensued. > > This regression has been introduced in > > commit 7b24056be6db7ce907baffdd4cf142ab774ea60c > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Wed Dec 12 00:35:33 2012 +0100 > > drm: don't hold crtc mutexes for connector ->detect callbacks > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59750 > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Tested-by: Cancan Feng <cancan.feng@intel.com> Dave, please pick this one up for your drm-next tree since the issue only happens there due to the modeset locking rework. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-24 9:55 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-23 16:25 [PATCH 0/2] fbcon+i915 locking fixes Daniel Vetter 2013-01-23 16:25 ` [PATCH 1/2] fb: Rework locking to fix lock ordering on takeover Daniel Vetter 2013-01-23 16:38 ` Takashi Iwai 2013-01-23 16:49 ` Daniel Vetter 2013-01-23 16:25 ` [PATCH 2/2] drm/i915: fixup per-crtc locking in intel_release_load_detect_pipe Daniel Vetter 2013-01-24 9:55 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox