[PATCH 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs

[PATCH 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs

[Chris Wilson]

The fb_helper->connector_count is modified when a new connector is
constructed following a hotplug event (e.g. DP-MST). This causes trouble
for drm_setup_crtcs() and friends that assume that fb_helper is
constant:

[ 1250.872997] BUG: KASAN: slab-out-of-bounds in drm_setup_crtcs+0x320/0xf80 at addr ffff88074cdd2608
[ 1250.873020] Write of size 40 by task kworker/u8:3/480
[ 1250.873039] CPU: 2 PID: 480 Comm: kworker/u8:3 Tainted: G     U          4.9.0-rc6+ #285
[ 1250.873043] Hardware name:                  /NUC6i3SYB, BIOS SYSKLi35.86A.0024.2015.1027.2142 10/27/2015
[ 1250.873050] Workqueue: events_unbound async_run_entry_fn
[ 1250.873056]  ffff88070f9d78f0 ffffffff814b72aa ffff88074e40c5c0 ffff88074cdd2608
[ 1250.873067]  ffff88070f9d7918 ffffffff8124ff3c ffff88070f9d79b0 ffff88074cdd2600
[ 1250.873079]  ffff88074e40c5c0 ffff88070f9d79a0 ffffffff812501e4 0000000000000005
[ 1250.873090] Call Trace:
[ 1250.873099]  [<ffffffff814b72aa>] dump_stack+0x67/0x9d
[ 1250.873106]  [<ffffffff8124ff3c>] kasan_object_err+0x1c/0x70
[ 1250.873113]  [<ffffffff812501e4>] kasan_report_error+0x204/0x4f0
[ 1250.873120]  [<ffffffff81698df0>] ? drm_dev_printk+0x140/0x140
[ 1250.873127]  [<ffffffff81250ac3>] kasan_report+0x53/0x60
[ 1250.873134]  [<ffffffff81688b40>] ? drm_setup_crtcs+0x320/0xf80
[ 1250.873142]  [<ffffffff8124f18e>] check_memory_region+0x13e/0x1a0
[ 1250.873147]  [<ffffffff8124f5f3>] memset+0x23/0x40
[ 1250.873154]  [<ffffffff81688b40>] drm_setup_crtcs+0x320/0xf80
[ 1250.873161]  [<ffffffff810be7c5>] ? wake_up_q+0x45/0x80
[ 1250.873169]  [<ffffffff81b0c180>] ? mutex_lock_nested+0x5a0/0x5a0
[ 1250.873176]  [<ffffffff8168a0e6>] drm_fb_helper_initial_config+0x206/0x7a0
[ 1250.873183]  [<ffffffff81689ee0>] ? drm_fb_helper_set_par+0x90/0x90
[ 1250.873303]  [<ffffffffa0b68690>] ? intel_fbdev_fini+0x140/0x140 [i915]
[ 1250.873387]  [<ffffffffa0b686b2>] intel_fbdev_initial_config+0x22/0x40 [i915]
[ 1250.873391]  [<ffffffff810b50ff>] async_run_entry_fn+0x7f/0x270
[ 1250.873394]  [<ffffffff810a64b0>] process_one_work+0x3d0/0x960
[ 1250.873398]  [<ffffffff810a641d>] ? process_one_work+0x33d/0x960
[ 1250.873401]  [<ffffffff810a60e0>] ? max_active_store+0xf0/0xf0
[ 1250.873406]  [<ffffffff810f6f9d>] ? do_raw_spin_lock+0x10d/0x1a0
[ 1250.873413]  [<ffffffff810a767d>] worker_thread+0x8d/0x840
[ 1250.873419]  [<ffffffff810a75f0>] ? create_worker+0x2e0/0x2e0
[ 1250.873426]  [<ffffffff810b0454>] kthread+0x194/0x1c0
[ 1250.873432]  [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
[ 1250.873438]  [<ffffffff810f095d>] ? trace_hardirqs_on+0xd/0x10
[ 1250.873446]  [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
[ 1250.873453]  [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
[ 1250.873457]  [<ffffffff81b12277>] ret_from_fork+0x27/0x40
[ 1250.873460] Object at ffff88074cdd2608, in cache kmalloc-32 size: 32

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98826
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_fb_helper.c | 73 ++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 14547817566d..d13c85682891 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -97,6 +97,10 @@ static LIST_HEAD(kernel_fb_helper_list);
  * mmap page writes.
  */
 
+#define drm_fb_helper_for_each_connector(fbh, i__) \
+	for (({lockdep_assert_held(&(fbh)->dev->mode_config.mutex); 1;}), \
+	     i__ = 0; i__ < (fbh)->connector_count; i__++)
+
 /**
  * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
  * 					       emulation helper
@@ -130,7 +134,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 	mutex_unlock(&dev->mode_config.mutex);
 	return 0;
 fail:
-	for (i = 0; i < fb_helper->connector_count; i++) {
+	drm_fb_helper_for_each_connector(fb_helper, i) {
 		struct drm_fb_helper_connector *fb_helper_connector =
 			fb_helper->connector_info[i];
 
@@ -565,7 +569,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 			continue;
 
 		/* Walk the connectors & encoders on this fb turning them on/off */
-		for (j = 0; j < fb_helper->connector_count; j++) {
+		drm_fb_helper_for_each_connector(fb_helper, j) {
 			connector = fb_helper->connector_info[j]->connector;
 			connector->funcs->dpms(connector, dpms_mode);
 			drm_object_property_set_value(&connector->base,
@@ -1469,7 +1473,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	int ret = 0;
 	int crtc_count = 0;
 	int i;
-	struct fb_info *info;
 	struct drm_fb_helper_surface_size sizes;
 	int gamma_size = 0;
 
@@ -1485,7 +1488,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 		sizes.surface_depth = sizes.surface_bpp = preferred_bpp;
 
 	/* first up get a count of crtcs now in use and new min/maxes width/heights */
-	for (i = 0; i < fb_helper->connector_count; i++) {
+	drm_fb_helper_for_each_connector(fb_helper, i) {
 		struct drm_fb_helper_connector *fb_helper_conn = fb_helper->connector_info[i];
 		struct drm_cmdline_mode *cmdline_mode;
 
@@ -1572,8 +1575,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	if (ret < 0)
 		return ret;
 
-	info = fb_helper->fbdev;
-
 	/*
 	 * Set the fb pointer - usually drm_setup_crtcs does this for hotplug
 	 * events, but at init time drm_setup_crtcs needs to be called before
@@ -1585,20 +1586,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 		if (fb_helper->crtc_info[i].mode_set.num_connectors)
 			fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
 
-
-	info->var.pixclock = 0;
-	if (register_framebuffer(info) < 0)
-		return -EINVAL;
-
-	dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
-			info->node, info->fix.id);
-
-	if (list_empty(&kernel_fb_helper_list)) {
-		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
-	}
-
-	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
-
 	return 0;
 }
 
@@ -1730,7 +1717,7 @@ static int drm_fb_helper_probe_connector_modes(struct drm_fb_helper *fb_helper,
 	int count = 0;
 	int i;
 
-	for (i = 0; i < fb_helper->connector_count; i++) {
+	drm_fb_helper_for_each_connector(fb_helper, i) {
 		connector = fb_helper->connector_info[i]->connector;
 		count += connector->funcs->fill_modes(connector, maxX, maxY);
 	}
@@ -1830,7 +1817,7 @@ static void drm_enable_connectors(struct drm_fb_helper *fb_helper,
 	struct drm_connector *connector;
 	int i = 0;
 
-	for (i = 0; i < fb_helper->connector_count; i++) {
+	drm_fb_helper_for_each_connector(fb_helper, i) {
 		connector = fb_helper->connector_info[i]->connector;
 		enabled[i] = drm_connector_enabled(connector, true);
 		DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
@@ -1841,7 +1828,7 @@ static void drm_enable_connectors(struct drm_fb_helper *fb_helper,
 	if (any_enabled)
 		return;
 
-	for (i = 0; i < fb_helper->connector_count; i++) {
+	drm_fb_helper_for_each_connector(fb_helper, i) {
 		connector = fb_helper->connector_info[i]->connector;
 		enabled[i] = drm_connector_enabled(connector, false);
 	}
@@ -1862,7 +1849,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
 		return false;
 
 	count = 0;
-	for (i = 0; i < fb_helper->connector_count; i++) {
+	drm_fb_helper_for_each_connector(fb_helper, i) {
 		if (enabled[i])
 			count++;
 	}
@@ -1873,7 +1860,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
 
 	/* check the command line or if nothing common pick 1024x768 */
 	can_clone = true;
-	for (i = 0; i < fb_helper->connector_count; i++) {
+	drm_fb_helper_for_each_connector(fb_helper, i) {
 		if (!enabled[i])
 			continue;
 		fb_helper_conn = fb_helper->connector_info[i];
@@ -1899,8 +1886,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
 	can_clone = true;
 	dmt_mode = drm_mode_find_dmt(fb_helper->dev, 1024, 768, 60, false);
 
-	for (i = 0; i < fb_helper->connector_count; i++) {
-
+	drm_fb_helper_for_each_connector(fb_helper, i) {
 		if (!enabled[i])
 			continue;
 
@@ -1931,7 +1917,7 @@ static int drm_get_tile_offsets(struct drm_fb_helper *fb_helper,
 	int i;
 	int hoffset = 0, voffset = 0;
 
-	for (i = 0; i < fb_helper->connector_count; i++) {
+	drm_fb_helper_for_each_connector(fb_helper, i) {
 		fb_helper_conn = fb_helper->connector_info[i];
 		if (!fb_helper_conn->connector->has_tile)
 			continue;
@@ -1964,7 +1950,7 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
 	int tile_pass = 0;
 	mask = (1 << fb_helper->connector_count) - 1;
 retry:
-	for (i = 0; i < fb_helper->connector_count; i++) {
+	drm_fb_helper_for_each_connector(fb_helper, i) {
 		fb_helper_conn = fb_helper->connector_info[i];
 
 		if (conn_configured & (1 << i))
@@ -2127,6 +2113,9 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 	width = dev->mode_config.max_width;
 	height = dev->mode_config.max_height;
 
+	/* prevent concurrent modification of connector_count by hotplug */
+	lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
+
 	crtcs = kcalloc(fb_helper->connector_count,
 			sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
 	modes = kcalloc(fb_helper->connector_count,
@@ -2140,7 +2129,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 		goto out;
 	}
 
-
 	drm_enable_connectors(fb_helper, enabled);
 
 	if (!(fb_helper->funcs->initial_config &&
@@ -2169,7 +2157,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 		drm_fb_helper_modeset_release(fb_helper,
 					      &fb_helper->crtc_info[i].mode_set);
 
-	for (i = 0; i < fb_helper->connector_count; i++) {
+	drm_fb_helper_for_each_connector(fb_helper, i) {
 		struct drm_display_mode *mode = modes[i];
 		struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
 		struct drm_fb_offset *offset = &offsets[i];
@@ -2246,7 +2234,9 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 {
 	struct drm_device *dev = fb_helper->dev;
+	struct fb_info *info;
 	int count = 0;
+	int ret;
 
 	if (!drm_fbdev_emulation)
 		return 0;
@@ -2255,7 +2245,6 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 	count = drm_fb_helper_probe_connector_modes(fb_helper,
 						    dev->mode_config.max_width,
 						    dev->mode_config.max_height);
-	mutex_unlock(&dev->mode_config.mutex);
 	/*
 	 * we shouldn't end up with no modes here.
 	 */
@@ -2263,8 +2252,26 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 		dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
 
 	drm_setup_crtcs(fb_helper);
+	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
+	mutex_unlock(&dev->mode_config.mutex);
+	if (ret)
+		return ret;
 
-	return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
+	info = fb_helper->fbdev;
+	info->var.pixclock = 0;
+	ret = register_framebuffer(info);
+	if (ret < 0)
+		return ret;
+
+	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
+		 info->node, info->fix.id);
+
+	if (list_empty(&kernel_fb_helper_list))
+		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
+
+	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
+
+	return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_initial_config);
 
-- 
2.10.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm: Pull together probe + setup for drm_fb_helper

[Chris Wilson]

drm_fb_helper_probe_connector_modes() is always called before
drm_setup_crtcs(), so just move the call into drm_setup_crtcs for a
small bit of code compaction.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_fb_helper.c | 37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d13c85682891..a19afc7eccde 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2098,20 +2098,19 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 	return best_score;
 }
 
-static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
+static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
+			    u32 width, u32 height)
 {
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_fb_helper_crtc **crtcs;
 	struct drm_display_mode **modes;
 	struct drm_fb_offset *offsets;
 	bool *enabled;
-	int width, height;
 	int i;
 
 	DRM_DEBUG_KMS("\n");
-
-	width = dev->mode_config.max_width;
-	height = dev->mode_config.max_height;
+	if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
+		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
 
 	/* prevent concurrent modification of connector_count by hotplug */
 	lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
@@ -2235,23 +2234,15 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 {
 	struct drm_device *dev = fb_helper->dev;
 	struct fb_info *info;
-	int count = 0;
 	int ret;
 
 	if (!drm_fbdev_emulation)
 		return 0;
 
 	mutex_lock(&dev->mode_config.mutex);
-	count = drm_fb_helper_probe_connector_modes(fb_helper,
-						    dev->mode_config.max_width,
-						    dev->mode_config.max_height);
-	/*
-	 * we shouldn't end up with no modes here.
-	 */
-	if (count == 0)
-		dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
-
-	drm_setup_crtcs(fb_helper);
+	drm_setup_crtcs(fb_helper,
+			dev->mode_config.max_width,
+			dev->mode_config.max_height);
 	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
 	mutex_unlock(&dev->mode_config.mutex);
 	if (ret)
@@ -2299,28 +2290,22 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
 int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
-	u32 max_width, max_height;
 
 	if (!drm_fbdev_emulation)
 		return 0;
 
-	mutex_lock(&fb_helper->dev->mode_config.mutex);
+	mutex_lock(&dev->mode_config.mutex);
 	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
 		fb_helper->delayed_hotplug = true;
-		mutex_unlock(&fb_helper->dev->mode_config.mutex);
+		mutex_unlock(&dev->mode_config.mutex);
 		return 0;
 	}
 	DRM_DEBUG_KMS("\n");
 
-	max_width = fb_helper->fb->width;
-	max_height = fb_helper->fb->height;
+	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
 
-	drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height);
-	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+	mutex_unlock(&dev->mode_config.mutex);
 
-	drm_modeset_lock_all(dev);
-	drm_setup_crtcs(fb_helper);
-	drm_modeset_unlock_all(dev);
 	drm_fb_helper_set_par(fb_helper->fbdev);
 
 	return 0;
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm: Protect fb_helper list manipulation with a mutex

[Chris Wilson]

Though we only walk the kernel_fb_helper_list inside a panic (or single
thread debugging), we still need to protect the list manipulation on
creating/removing a framebuffer device in order to prevent list
corruption.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_fb_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a19afc7eccde..2ac2f462d37b 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -49,6 +49,7 @@ MODULE_PARM_DESC(fbdev_emulation,
 		 "Enable legacy fbdev emulation [default=true]");
 
 static LIST_HEAD(kernel_fb_helper_list);
+static DEFINE_MUTEX(kernel_fb_helper_lock);
 
 /**
  * DOC: fbdev helpers
@@ -855,12 +856,14 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 	if (!drm_fbdev_emulation)
 		return;
 
+	mutex_lock(&kernel_fb_helper_lock);
 	if (!list_empty(&fb_helper->kernel_fb_list)) {
 		list_del(&fb_helper->kernel_fb_list);
 		if (list_empty(&kernel_fb_helper_list)) {
 			unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 		}
 	}
+	mutex_unlock(&kernel_fb_helper_lock);
 
 	drm_fb_helper_crtc_free(fb_helper);
 
@@ -2257,10 +2260,12 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
 		 info->node, info->fix.id);
 
+	mutex_lock(&kernel_fb_helper_lock);
 	if (list_empty(&kernel_fb_helper_list))
 		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 
 	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
+	mutex_unlock(&kernel_fb_helper_lock);
 
 	return 0;
 }
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs

[Jani Nikula]

On Wed, 23 Nov 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> +#define drm_fb_helper_for_each_connector(fbh, i__) \
> +	for (({lockdep_assert_held(&(fbh)->dev->mode_config.mutex); 1;}), \
> +	     i__ = 0; i__ < (fbh)->connector_count; i__++)

No comments on the substance, but just curious, why is that "1;"
required there? Or is it?

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs
URL   : https://patchwork.freedesktop.org/series/15823/
State : success

== Summary ==

Series 15823v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/15823/revisions/1/mbox/


fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

01896e61d9cc0cad08e19990cd095cdf679f6142 drm-intel-nightly: 2016y-11m-23d-14h-45m-53s UTC integration manifest
82c26ed drm: Protect fb_helper list manipulation with a mutex
7b135e9 drm: Pull together probe + setup for drm_fb_helper
8f48c48 drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3092/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs

[Daniel Vetter]

On Wed, Nov 23, 2016 at 02:04:15PM +0000, Chris Wilson wrote:
> The fb_helper->connector_count is modified when a new connector is
> constructed following a hotplug event (e.g. DP-MST). This causes trouble
> for drm_setup_crtcs() and friends that assume that fb_helper is
> constant:
> 
> [ 1250.872997] BUG: KASAN: slab-out-of-bounds in drm_setup_crtcs+0x320/0xf80 at addr ffff88074cdd2608
> [ 1250.873020] Write of size 40 by task kworker/u8:3/480
> [ 1250.873039] CPU: 2 PID: 480 Comm: kworker/u8:3 Tainted: G     U          4.9.0-rc6+ #285
> [ 1250.873043] Hardware name:                  /NUC6i3SYB, BIOS SYSKLi35.86A.0024.2015.1027.2142 10/27/2015
> [ 1250.873050] Workqueue: events_unbound async_run_entry_fn
> [ 1250.873056]  ffff88070f9d78f0 ffffffff814b72aa ffff88074e40c5c0 ffff88074cdd2608
> [ 1250.873067]  ffff88070f9d7918 ffffffff8124ff3c ffff88070f9d79b0 ffff88074cdd2600
> [ 1250.873079]  ffff88074e40c5c0 ffff88070f9d79a0 ffffffff812501e4 0000000000000005
> [ 1250.873090] Call Trace:
> [ 1250.873099]  [<ffffffff814b72aa>] dump_stack+0x67/0x9d
> [ 1250.873106]  [<ffffffff8124ff3c>] kasan_object_err+0x1c/0x70
> [ 1250.873113]  [<ffffffff812501e4>] kasan_report_error+0x204/0x4f0
> [ 1250.873120]  [<ffffffff81698df0>] ? drm_dev_printk+0x140/0x140
> [ 1250.873127]  [<ffffffff81250ac3>] kasan_report+0x53/0x60
> [ 1250.873134]  [<ffffffff81688b40>] ? drm_setup_crtcs+0x320/0xf80
> [ 1250.873142]  [<ffffffff8124f18e>] check_memory_region+0x13e/0x1a0
> [ 1250.873147]  [<ffffffff8124f5f3>] memset+0x23/0x40
> [ 1250.873154]  [<ffffffff81688b40>] drm_setup_crtcs+0x320/0xf80
> [ 1250.873161]  [<ffffffff810be7c5>] ? wake_up_q+0x45/0x80
> [ 1250.873169]  [<ffffffff81b0c180>] ? mutex_lock_nested+0x5a0/0x5a0
> [ 1250.873176]  [<ffffffff8168a0e6>] drm_fb_helper_initial_config+0x206/0x7a0
> [ 1250.873183]  [<ffffffff81689ee0>] ? drm_fb_helper_set_par+0x90/0x90
> [ 1250.873303]  [<ffffffffa0b68690>] ? intel_fbdev_fini+0x140/0x140 [i915]
> [ 1250.873387]  [<ffffffffa0b686b2>] intel_fbdev_initial_config+0x22/0x40 [i915]
> [ 1250.873391]  [<ffffffff810b50ff>] async_run_entry_fn+0x7f/0x270
> [ 1250.873394]  [<ffffffff810a64b0>] process_one_work+0x3d0/0x960
> [ 1250.873398]  [<ffffffff810a641d>] ? process_one_work+0x33d/0x960
> [ 1250.873401]  [<ffffffff810a60e0>] ? max_active_store+0xf0/0xf0
> [ 1250.873406]  [<ffffffff810f6f9d>] ? do_raw_spin_lock+0x10d/0x1a0
> [ 1250.873413]  [<ffffffff810a767d>] worker_thread+0x8d/0x840
> [ 1250.873419]  [<ffffffff810a75f0>] ? create_worker+0x2e0/0x2e0
> [ 1250.873426]  [<ffffffff810b0454>] kthread+0x194/0x1c0
> [ 1250.873432]  [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
> [ 1250.873438]  [<ffffffff810f095d>] ? trace_hardirqs_on+0xd/0x10
> [ 1250.873446]  [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
> [ 1250.873453]  [<ffffffff810b02c0>] ? kthread_park+0x60/0x60
> [ 1250.873457]  [<ffffffff81b12277>] ret_from_fork+0x27/0x40
> [ 1250.873460] Object at ffff88074cdd2608, in cache kmalloc-32 size: 32
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98826
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 73 ++++++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 14547817566d..d13c85682891 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -97,6 +97,10 @@ static LIST_HEAD(kernel_fb_helper_list);
>   * mmap page writes.
>   */
>  
> +#define drm_fb_helper_for_each_connector(fbh, i__) \
> +	for (({lockdep_assert_held(&(fbh)->dev->mode_config.mutex); 1;}), \
> +	     i__ = 0; i__ < (fbh)->connector_count; i__++)
> +

+1 on Jani's question.

>  /**
>   * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
>   * 					       emulation helper
> @@ -130,7 +134,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
>  	mutex_unlock(&dev->mode_config.mutex);
>  	return 0;
>  fail:
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> +	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		struct drm_fb_helper_connector *fb_helper_connector =
>  			fb_helper->connector_info[i];
>  
> @@ -565,7 +569,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
>  			continue;
>  
>  		/* Walk the connectors & encoders on this fb turning them on/off */
> -		for (j = 0; j < fb_helper->connector_count; j++) {
> +		drm_fb_helper_for_each_connector(fb_helper, j) {
>  			connector = fb_helper->connector_info[j]->connector;
>  			connector->funcs->dpms(connector, dpms_mode);
>  			drm_object_property_set_value(&connector->base,
> @@ -1469,7 +1473,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  	int ret = 0;
>  	int crtc_count = 0;
>  	int i;
> -	struct fb_info *info;
>  	struct drm_fb_helper_surface_size sizes;
>  	int gamma_size = 0;
>  
> @@ -1485,7 +1488,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  		sizes.surface_depth = sizes.surface_bpp = preferred_bpp;
>  
>  	/* first up get a count of crtcs now in use and new min/maxes width/heights */
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> +	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		struct drm_fb_helper_connector *fb_helper_conn = fb_helper->connector_info[i];
>  		struct drm_cmdline_mode *cmdline_mode;
>  
> @@ -1572,8 +1575,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  	if (ret < 0)
>  		return ret;
>  
> -	info = fb_helper->fbdev;
> -
>  	/*
>  	 * Set the fb pointer - usually drm_setup_crtcs does this for hotplug
>  	 * events, but at init time drm_setup_crtcs needs to be called before
> @@ -1585,20 +1586,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  		if (fb_helper->crtc_info[i].mode_set.num_connectors)
>  			fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
>  
> -
> -	info->var.pixclock = 0;
> -	if (register_framebuffer(info) < 0)
> -		return -EINVAL;
> -
> -	dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
> -			info->node, info->fix.id);
> -
> -	if (list_empty(&kernel_fb_helper_list)) {
> -		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> -	}
> -
> -	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> -

Why move this hunk? kernel_fb_helper_list seems entirely unprotected, so
shouldn't matter really where we touch it ;-)

Otherwise lgtm.
-Daniel

>  	return 0;
>  }
>  
> @@ -1730,7 +1717,7 @@ static int drm_fb_helper_probe_connector_modes(struct drm_fb_helper *fb_helper,
>  	int count = 0;
>  	int i;
>  
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> +	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		connector = fb_helper->connector_info[i]->connector;
>  		count += connector->funcs->fill_modes(connector, maxX, maxY);
>  	}
> @@ -1830,7 +1817,7 @@ static void drm_enable_connectors(struct drm_fb_helper *fb_helper,
>  	struct drm_connector *connector;
>  	int i = 0;
>  
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> +	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		connector = fb_helper->connector_info[i]->connector;
>  		enabled[i] = drm_connector_enabled(connector, true);
>  		DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
> @@ -1841,7 +1828,7 @@ static void drm_enable_connectors(struct drm_fb_helper *fb_helper,
>  	if (any_enabled)
>  		return;
>  
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> +	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		connector = fb_helper->connector_info[i]->connector;
>  		enabled[i] = drm_connector_enabled(connector, false);
>  	}
> @@ -1862,7 +1849,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
>  		return false;
>  
>  	count = 0;
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> +	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		if (enabled[i])
>  			count++;
>  	}
> @@ -1873,7 +1860,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
>  
>  	/* check the command line or if nothing common pick 1024x768 */
>  	can_clone = true;
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> +	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		if (!enabled[i])
>  			continue;
>  		fb_helper_conn = fb_helper->connector_info[i];
> @@ -1899,8 +1886,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
>  	can_clone = true;
>  	dmt_mode = drm_mode_find_dmt(fb_helper->dev, 1024, 768, 60, false);
>  
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> -
> +	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		if (!enabled[i])
>  			continue;
>  
> @@ -1931,7 +1917,7 @@ static int drm_get_tile_offsets(struct drm_fb_helper *fb_helper,
>  	int i;
>  	int hoffset = 0, voffset = 0;
>  
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> +	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		fb_helper_conn = fb_helper->connector_info[i];
>  		if (!fb_helper_conn->connector->has_tile)
>  			continue;
> @@ -1964,7 +1950,7 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
>  	int tile_pass = 0;
>  	mask = (1 << fb_helper->connector_count) - 1;
>  retry:
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> +	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		fb_helper_conn = fb_helper->connector_info[i];
>  
>  		if (conn_configured & (1 << i))
> @@ -2127,6 +2113,9 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>  	width = dev->mode_config.max_width;
>  	height = dev->mode_config.max_height;
>  
> +	/* prevent concurrent modification of connector_count by hotplug */
> +	lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
> +
>  	crtcs = kcalloc(fb_helper->connector_count,
>  			sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
>  	modes = kcalloc(fb_helper->connector_count,
> @@ -2140,7 +2129,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>  		goto out;
>  	}
>  
> -
>  	drm_enable_connectors(fb_helper, enabled);
>  
>  	if (!(fb_helper->funcs->initial_config &&
> @@ -2169,7 +2157,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>  		drm_fb_helper_modeset_release(fb_helper,
>  					      &fb_helper->crtc_info[i].mode_set);
>  
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> +	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		struct drm_display_mode *mode = modes[i];
>  		struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
>  		struct drm_fb_offset *offset = &offsets[i];
> @@ -2246,7 +2234,9 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>  int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>  {
>  	struct drm_device *dev = fb_helper->dev;
> +	struct fb_info *info;
>  	int count = 0;
> +	int ret;
>  
>  	if (!drm_fbdev_emulation)
>  		return 0;
> @@ -2255,7 +2245,6 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>  	count = drm_fb_helper_probe_connector_modes(fb_helper,
>  						    dev->mode_config.max_width,
>  						    dev->mode_config.max_height);
> -	mutex_unlock(&dev->mode_config.mutex);
>  	/*
>  	 * we shouldn't end up with no modes here.
>  	 */
> @@ -2263,8 +2252,26 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>  		dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
>  
>  	drm_setup_crtcs(fb_helper);
> +	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> +	mutex_unlock(&dev->mode_config.mutex);
> +	if (ret)
> +		return ret;
>  
> -	return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> +	info = fb_helper->fbdev;
> +	info->var.pixclock = 0;
> +	ret = register_framebuffer(info);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> +		 info->node, info->fix.id);
> +
> +	if (list_empty(&kernel_fb_helper_list))
> +		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> +
> +	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_initial_config);
>  
> -- 
> 2.10.2
> 
> _______________________________________________
> 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

Re: [PATCH 2/3] drm: Pull together probe + setup for drm_fb_helper

[Daniel Vetter]

On Wed, Nov 23, 2016 at 02:04:16PM +0000, Chris Wilson wrote:
> drm_fb_helper_probe_connector_modes() is always called before
> drm_setup_crtcs(), so just move the call into drm_setup_crtcs for a
> small bit of code compaction.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

This drops drm_modeset_lock_all() around one call of drm_setup_crtcs and
just replaces it with dev->mode_config.mutex. But drm_setup_crtcs only
looks at probe state and not at modeset state, so that's all fine.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 37 +++++++++++--------------------------
>  1 file changed, 11 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index d13c85682891..a19afc7eccde 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2098,20 +2098,19 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>  	return best_score;
>  }
>  
> -static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
> +static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
> +			    u32 width, u32 height)
>  {
>  	struct drm_device *dev = fb_helper->dev;
>  	struct drm_fb_helper_crtc **crtcs;
>  	struct drm_display_mode **modes;
>  	struct drm_fb_offset *offsets;
>  	bool *enabled;
> -	int width, height;
>  	int i;
>  
>  	DRM_DEBUG_KMS("\n");
> -
> -	width = dev->mode_config.max_width;
> -	height = dev->mode_config.max_height;
> +	if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
> +		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
>  
>  	/* prevent concurrent modification of connector_count by hotplug */
>  	lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
> @@ -2235,23 +2234,15 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>  {
>  	struct drm_device *dev = fb_helper->dev;
>  	struct fb_info *info;
> -	int count = 0;
>  	int ret;
>  
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
>  	mutex_lock(&dev->mode_config.mutex);
> -	count = drm_fb_helper_probe_connector_modes(fb_helper,
> -						    dev->mode_config.max_width,
> -						    dev->mode_config.max_height);
> -	/*
> -	 * we shouldn't end up with no modes here.
> -	 */
> -	if (count == 0)
> -		dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n");
> -
> -	drm_setup_crtcs(fb_helper);
> +	drm_setup_crtcs(fb_helper,
> +			dev->mode_config.max_width,
> +			dev->mode_config.max_height);
>  	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
>  	mutex_unlock(&dev->mode_config.mutex);
>  	if (ret)
> @@ -2299,28 +2290,22 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
>  int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  {
>  	struct drm_device *dev = fb_helper->dev;
> -	u32 max_width, max_height;
>  
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
> -	mutex_lock(&fb_helper->dev->mode_config.mutex);
> +	mutex_lock(&dev->mode_config.mutex);
>  	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
>  		fb_helper->delayed_hotplug = true;
> -		mutex_unlock(&fb_helper->dev->mode_config.mutex);
> +		mutex_unlock(&dev->mode_config.mutex);
>  		return 0;
>  	}
>  	DRM_DEBUG_KMS("\n");
>  
> -	max_width = fb_helper->fb->width;
> -	max_height = fb_helper->fb->height;
> +	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
>  
> -	drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height);
> -	mutex_unlock(&fb_helper->dev->mode_config.mutex);
> +	mutex_unlock(&dev->mode_config.mutex);
>  
> -	drm_modeset_lock_all(dev);
> -	drm_setup_crtcs(fb_helper);
> -	drm_modeset_unlock_all(dev);
>  	drm_fb_helper_set_par(fb_helper->fbdev);
>  
>  	return 0;
> -- 
> 2.10.2
> 
> _______________________________________________
> 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

Re: [PATCH 3/3] drm: Protect fb_helper list manipulation with a mutex

[Daniel Vetter]

On Wed, Nov 23, 2016 at 02:04:17PM +0000, Chris Wilson wrote:
> Though we only walk the kernel_fb_helper_list inside a panic (or single
> thread debugging), we still need to protect the list manipulation on
> creating/removing a framebuffer device in order to prevent list
> corruption.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I guess this explains the moved hunk in patch 1. Still feels misplaced,
but with or without moving that around:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index a19afc7eccde..2ac2f462d37b 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -49,6 +49,7 @@ MODULE_PARM_DESC(fbdev_emulation,
>  		 "Enable legacy fbdev emulation [default=true]");
>  
>  static LIST_HEAD(kernel_fb_helper_list);
> +static DEFINE_MUTEX(kernel_fb_helper_lock);
>  
>  /**
>   * DOC: fbdev helpers
> @@ -855,12 +856,14 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>  	if (!drm_fbdev_emulation)
>  		return;
>  
> +	mutex_lock(&kernel_fb_helper_lock);
>  	if (!list_empty(&fb_helper->kernel_fb_list)) {
>  		list_del(&fb_helper->kernel_fb_list);
>  		if (list_empty(&kernel_fb_helper_list)) {
>  			unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
>  		}
>  	}
> +	mutex_unlock(&kernel_fb_helper_lock);
>  
>  	drm_fb_helper_crtc_free(fb_helper);
>  
> @@ -2257,10 +2260,12 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>  	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
>  		 info->node, info->fix.id);
>  
> +	mutex_lock(&kernel_fb_helper_lock);
>  	if (list_empty(&kernel_fb_helper_list))
>  		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
>  
>  	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> +	mutex_unlock(&kernel_fb_helper_lock);
>  
>  	return 0;
>  }
> -- 
> 2.10.2
> 
> _______________________________________________
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: Protect fb_helper list manipulation with a mutex

[Chris Wilson]

On Mon, Nov 28, 2016 at 08:55:58AM +0100, Daniel Vetter wrote:
> On Wed, Nov 23, 2016 at 02:04:17PM +0000, Chris Wilson wrote:
> > Though we only walk the kernel_fb_helper_list inside a panic (or single
> > thread debugging), we still need to protect the list manipulation on
> > creating/removing a framebuffer device in order to prevent list
> > corruption.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> I guess this explains the moved hunk in patch 1. Still feels misplaced,
> but with or without moving that around:

No, that had to be moved to pull the register_framebuffer out from
underneath the lock (as it causes a lock recursion into the fbdev trying
to do a modeset).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: Protect fb_helper list manipulation with a mutex

[Daniel Vetter]

On Mon, Nov 28, 2016 at 08:38:21AM +0000, Chris Wilson wrote:
> On Mon, Nov 28, 2016 at 08:55:58AM +0100, Daniel Vetter wrote:
> > On Wed, Nov 23, 2016 at 02:04:17PM +0000, Chris Wilson wrote:
> > > Though we only walk the kernel_fb_helper_list inside a panic (or single
> > > thread debugging), we still need to protect the list manipulation on
> > > creating/removing a framebuffer device in order to prevent list
> > > corruption.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > I guess this explains the moved hunk in patch 1. Still feels misplaced,
> > but with or without moving that around:
> 
> No, that had to be moved to pull the register_framebuffer out from
> underneath the lock (as it causes a lock recursion into the fbdev trying
> to do a modeset).

Ah right, I missed that. Can you pls add that the commit message and
address Jani's question/comment when resending? Then I can pluck these 3
up.

Thanks, Daniel
-- 
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

Re: [PATCH 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs

[Chris Wilson]

On Mon, Nov 28, 2016 at 08:26:07AM +0100, Daniel Vetter wrote:
> On Wed, Nov 23, 2016 at 02:04:15PM +0000, Chris Wilson wrote:
> > +#define drm_fb_helper_for_each_connector(fbh, i__) \
> > +	for (({lockdep_assert_held(&(fbh)->dev->mode_config.mutex); 1;}), \
> > +	     i__ = 0; i__ < (fbh)->connector_count; i__++)
> > +
> 
> +1 on Jani's question.

I'm missing the question. Found it under pw

" No comments on the substance, but just curious, why is that "1;"
required there? Or is it?"

Hmm, the 1; itself isn't required. I was just uncomfortable when
thinking that ({}) evaluated to the result of the last statement, and
didn't want to contemplate what if that last statement was not valid in
that context. Appears gcc is quite happy since it is discarded and not
used as a rhs.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel