From 7a6f7133c573f237f2ed51652a947bc02aeeda4d Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Mon, 23 Nov 2015 10:32:34 +0100 Subject: drm/armada: use unlocked gem unreferencing For drm_gem_object_unreference callers are required to hold dev->struct_mutex, which these paths don't. Enforcing this requirement has become a bit more strict with commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663 Author: Daniel Vetter Date: Thu Oct 15 09:36:25 2015 +0200 drm/gem: Check locking in drm_gem_object_unreference Signed-off-by: Daniel Vetter Signed-off-by: Russell King --- drivers/gpu/drm/armada/armada_crtc.c | 2 +- drivers/gpu/drm/armada/armada_gem.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers') diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index cebcab560626..7ea35bee7cd5 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -972,7 +972,7 @@ static void armada_drm_crtc_destroy(struct drm_crtc *crtc) struct armada_private *priv = crtc->dev->dev_private; if (dcrtc->cursor_obj) - drm_gem_object_unreference(&dcrtc->cursor_obj->obj); + drm_gem_object_unreference_unlocked(&dcrtc->cursor_obj->obj); priv->dcrtc[dcrtc->num] = NULL; drm_crtc_cleanup(&dcrtc->crtc); diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 60a688ef81c7..aaf88641bfc5 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -352,13 +352,13 @@ int armada_gem_mmap_ioctl(struct drm_device *dev, void *data, return -ENOENT; if (!dobj->obj.filp) { - drm_gem_object_unreference(&dobj->obj); + drm_gem_object_unreference_unlocked(&dobj->obj); return -EINVAL; } addr = vm_mmap(dobj->obj.filp, 0, args->size, PROT_READ | PROT_WRITE, MAP_SHARED, args->offset); - drm_gem_object_unreference(&dobj->obj); + drm_gem_object_unreference_unlocked(&dobj->obj); if (IS_ERR_VALUE(addr)) return addr; -- cgit From 8d6185b55ceadea4fe3d7b451843786c4cae564e Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Mon, 23 Nov 2015 10:32:43 +0100 Subject: drm/armada: plug leak in dumb_map_offset We need to drop the gem bo reference if it's an imported one. Signed-off-by: Daniel Vetter Signed-off-by: Russell King --- drivers/gpu/drm/armada/armada_gem.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index aaf88641bfc5..2a3ef7938f30 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -285,7 +285,7 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, /* Don't allow imported objects to be mapped */ if (obj->obj.import_attach) { ret = -EINVAL; - goto err_unlock; + goto err_unref; } ret = drm_gem_create_mmap_offset(&obj->obj); @@ -294,6 +294,7 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, DRM_DEBUG_DRIVER("handle %#x offset %llx\n", handle, *offset); } + err_unref: drm_gem_object_unreference(&obj->obj); err_unlock: mutex_unlock(&dev->struct_mutex); -- cgit From 39146d6f866888beefaecba3115576c10e1033aa Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Mon, 23 Nov 2015 10:32:44 +0100 Subject: drm/armada: don't grab dev->struct_mutex for in mmap offset ioctl Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares). v2: Split out the leak fix in dump_map_offset into a separate patch as requested by Russell. Also align labels the same way as before to stick with local coding style. Signed-off-by: Daniel Vetter Signed-off-by: Russell King --- drivers/gpu/drm/armada/armada_gem.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) (limited to 'drivers') diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 2a3ef7938f30..e3a86b96af2a 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -274,12 +274,10 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, struct armada_gem_object *obj; int ret = 0; - mutex_lock(&dev->struct_mutex); obj = armada_gem_object_lookup(dev, file, handle); if (!obj) { DRM_ERROR("failed to lookup gem object\n"); - ret = -EINVAL; - goto err_unlock; + return -EINVAL; } /* Don't allow imported objects to be mapped */ @@ -295,9 +293,7 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, } err_unref: - drm_gem_object_unreference(&obj->obj); - err_unlock: - mutex_unlock(&dev->struct_mutex); + drm_gem_object_unreference_unlocked(&obj->obj); return ret; } -- cgit From 4bd3fd443aa4bc3cede05de201e9cf3277059477 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Mon, 23 Nov 2015 10:32:45 +0100 Subject: drm/armada: drop struct_mutex from cursor paths The kms state itself is already protected by the modeset locks acquired by the drm core. The only thing left is gem bo state, and since the cursor code expects small objects which are statically mapped at create time and then invariant over the lifetime of the gem bo there's nothing to protect. See armada_gem_dumb_create -> armada_gem_linear_back which assigns obj->addr which is the only thing used by the cursor code. Only tricky bit is to switch to the _unlocked unreference function. Signed-off-by: Daniel Vetter Signed-off-by: Russell King --- drivers/gpu/drm/armada/armada_crtc.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) (limited to 'drivers') diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index 7ea35bee7cd5..8ebfe565bf05 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -928,11 +928,10 @@ static int armada_drm_crtc_cursor_set(struct drm_crtc *crtc, } } - mutex_lock(&dev->struct_mutex); if (dcrtc->cursor_obj) { dcrtc->cursor_obj->update = NULL; dcrtc->cursor_obj->update_data = NULL; - drm_gem_object_unreference(&dcrtc->cursor_obj->obj); + drm_gem_object_unreference_unlocked(&dcrtc->cursor_obj->obj); } dcrtc->cursor_obj = obj; dcrtc->cursor_w = w; @@ -942,14 +941,12 @@ static int armada_drm_crtc_cursor_set(struct drm_crtc *crtc, obj->update_data = dcrtc; obj->update = cursor_update; } - mutex_unlock(&dev->struct_mutex); return ret; } static int armada_drm_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) { - struct drm_device *dev = crtc->dev; struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc); int ret; @@ -957,11 +954,9 @@ static int armada_drm_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) if (!dcrtc->variant->has_spu_adv_reg) return -EFAULT; - mutex_lock(&dev->struct_mutex); dcrtc->cursor_x = x; dcrtc->cursor_y = y; ret = armada_drm_crtc_cursor_update(dcrtc, false); - mutex_unlock(&dev->struct_mutex); return ret; } -- cgit From 0b8ebeacf5ef43a467c7ec5400ccc1ffc3fbdfba Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Tue, 24 Nov 2015 10:00:36 +0100 Subject: drm/armada: use a private mutex to protect priv->linear Reusing the Big DRM Lock just leaks, and the few things left that dev->struct_mutex protected are very well contained - it's just the linear drm_mm manager. With this armada is completely struct_mutex free! v2: Convert things properly and also take the lock in armada_gem_free_object, and remove the stale comment (Russell). Signed-off-by: Daniel Vetter Signed-off-by: Russell King --- drivers/gpu/drm/armada/armada_debugfs.c | 4 ++-- drivers/gpu/drm/armada/armada_drm.h | 3 ++- drivers/gpu/drm/armada/armada_drv.c | 1 + drivers/gpu/drm/armada/armada_gem.c | 14 +++++++++----- 4 files changed, 14 insertions(+), 8 deletions(-) (limited to 'drivers') diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c index 471e45627f1e..d4f7ab0a30d4 100644 --- a/drivers/gpu/drm/armada/armada_debugfs.c +++ b/drivers/gpu/drm/armada/armada_debugfs.c @@ -21,9 +21,9 @@ static int armada_debugfs_gem_linear_show(struct seq_file *m, void *data) struct armada_private *priv = dev->dev_private; int ret; - mutex_lock(&dev->struct_mutex); + mutex_lock(&priv->linear_lock); ret = drm_mm_dump_table(m, &priv->linear); - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&priv->linear_lock); return ret; } diff --git a/drivers/gpu/drm/armada/armada_drm.h b/drivers/gpu/drm/armada/armada_drm.h index 4df6f2af2b21..3b2bb6128d40 100644 --- a/drivers/gpu/drm/armada/armada_drm.h +++ b/drivers/gpu/drm/armada/armada_drm.h @@ -57,7 +57,8 @@ struct armada_private { DECLARE_KFIFO(fb_unref, struct drm_framebuffer *, 8); struct drm_fb_helper *fbdev; struct armada_crtc *dcrtc[2]; - struct drm_mm linear; + struct drm_mm linear; /* protected by linear_lock */ + struct mutex linear_lock; struct drm_property *csc_yuv_prop; struct drm_property *csc_rgb_prop; struct drm_property *colorkey_prop; diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c index 3f1396e673dd..f7f47cb71db4 100644 --- a/drivers/gpu/drm/armada/armada_drv.c +++ b/drivers/gpu/drm/armada/armada_drv.c @@ -101,6 +101,7 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags) dev->mode_config.preferred_depth = 24; dev->mode_config.funcs = &armada_drm_mode_config_funcs; drm_mm_init(&priv->linear, mem->start, resource_size(mem)); + mutex_init(&priv->linear_lock); ret = component_bind_all(dev->dev, dev); if (ret) diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index e3a86b96af2a..6e731db31aa4 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -46,22 +46,26 @@ static size_t roundup_gem_size(size_t size) return roundup(size, PAGE_SIZE); } -/* dev->struct_mutex is held here */ void armada_gem_free_object(struct drm_gem_object *obj) { struct armada_gem_object *dobj = drm_to_armada_gem(obj); + struct armada_private *priv = obj->dev->dev_private; DRM_DEBUG_DRIVER("release obj %p\n", dobj); drm_gem_free_mmap_offset(&dobj->obj); + might_lock(&priv->linear_lock); + if (dobj->page) { /* page backed memory */ unsigned int order = get_order(dobj->obj.size); __free_pages(dobj->page, order); } else if (dobj->linear) { /* linear backed memory */ + mutex_lock(&priv->linear_lock); drm_mm_remove_node(dobj->linear); + mutex_unlock(&priv->linear_lock); kfree(dobj->linear); if (dobj->addr) iounmap(dobj->addr); @@ -144,10 +148,10 @@ armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj) if (!node) return -ENOSPC; - mutex_lock(&dev->struct_mutex); + mutex_lock(&priv->linear_lock); ret = drm_mm_insert_node(&priv->linear, node, size, align, DRM_MM_SEARCH_DEFAULT); - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&priv->linear_lock); if (ret) { kfree(node); return ret; @@ -158,9 +162,9 @@ armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj) /* Ensure that the memory we're returning is cleared. */ ptr = ioremap_wc(obj->linear->start, size); if (!ptr) { - mutex_lock(&dev->struct_mutex); + mutex_lock(&priv->linear_lock); drm_mm_remove_node(obj->linear); - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&priv->linear_lock); kfree(obj->linear); obj->linear = NULL; return -ENOMEM; -- cgit