From patchwork Fri Sep 25 11:57:13 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lucas Stach X-Patchwork-Id: 7263561 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 6D1DDBF036 for ; Fri, 25 Sep 2015 11:58:24 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D486020AE8 for ; Fri, 25 Sep 2015 11:58:22 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 4637A20AC0 for ; Fri, 25 Sep 2015 11:58:21 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B8A456F08D; Fri, 25 Sep 2015 04:58:07 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from metis.ext.pengutronix.de (metis.ext.4.pengutronix.de [92.198.50.35]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8C0046F08C for ; Fri, 25 Sep 2015 04:58:04 -0700 (PDT) Received: from weser.hi.4.pengutronix.de ([10.1.0.109] helo=weser.pengutronix.de.) by metis.ext.pengutronix.de with esmtp (Exim 4.80) (envelope-from ) id 1ZfRdZ-0004Em-RR; Fri, 25 Sep 2015 13:58:01 +0200 From: Lucas Stach To: Russell King , Christian Gmeiner Subject: [PATCH 01/48] staging: etnaviv: avoid holding struct_mutex over dma_alloc_coherent() Date: Fri, 25 Sep 2015 13:57:13 +0200 Message-Id: <1443182280-15868-2-git-send-email-l.stach@pengutronix.de> X-Mailer: git-send-email 2.1.4 In-Reply-To: <1443182280-15868-1-git-send-email-l.stach@pengutronix.de> References: <20150916080435.GA21084@n2100.arm.linux.org.uk> <1443182280-15868-1-git-send-email-l.stach@pengutronix.de> X-SA-Exim-Connect-IP: 10.1.0.109 X-SA-Exim-Mail-From: l.stach@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: dri-devel@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Russell King Holding the DRM struct_mutex over a call to dma_alloc_coherent() is asking for trouble; the DRM struct_mutex is held inside several other system-wide locks, and when CMA is enabled, this causes the CMA lock to be nested inside struct_mutex. In conjunction with other system locks, this eventually causes a AB-BA lock ordering bug, which becomes most apparent when using CPU hotplug: [ INFO: possible circular locking dependency detected ] 3.19.0-rc6+ #1497 Not tainted ------------------------------------------------------- bash/2154 is trying to acquire lock: (console_lock){+.+.+.}, at: [] console_cpu_notify+0x28/0x34 but task is already holding lock: (cpu_hotplug.lock#2){+.+.+.}, at: [] cpu_hotplug_begin+0x64/0xb8 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #5 (cpu_hotplug.lock#2){+.+.+.}: [] mutex_lock_nested+0x5c/0x3d8 [] lru_add_drain_all+0x1c/0x18c [] migrate_prep+0x10/0x18 [] alloc_contig_range+0xd0/0x2cc [] cma_alloc+0xe0/0x1ac [] dma_alloc_from_contiguous+0x3c/0x44 [] __alloc_from_contiguous+0x3c/0xe8 [] atomic_pool_init+0x6c/0x15c [] do_one_initcall+0x88/0x1d8 [] kernel_init_freeable+0x110/0x1dc [] kernel_init+0x10/0xec [] ret_from_fork+0x14/0x24 -> #4 (lock){+.+...}: [] mutex_lock_nested+0x5c/0x3d8 [] lru_add_drain_all+0x1c/0x18c [] migrate_prep+0x10/0x18 [] alloc_contig_range+0xd0/0x2cc [] cma_alloc+0xe0/0x1ac [] dma_alloc_from_contiguous+0x3c/0x44 [] __alloc_from_contiguous+0x3c/0xe8 [] atomic_pool_init+0x6c/0x15c [] do_one_initcall+0x88/0x1d8 [] kernel_init_freeable+0x110/0x1dc [] kernel_init+0x10/0xec [] ret_from_fork+0x14/0x24 -> #3 (cma_mutex){+.+.+.}: [] mutex_lock_nested+0x5c/0x3d8 [] cma_alloc+0xd0/0x1ac [] dma_alloc_from_contiguous+0x3c/0x44 [] __alloc_from_contiguous+0x3c/0xe8 [] __dma_alloc+0x15c/0x28c [] arm_dma_alloc+0xa0/0xa8 [] etnaviv_iommu_domain_init+0x54/0x138 [] etnaviv_iommu_domain_alloc+0x4c/0xd8 [] etnaviv_gpu_init+0x380/0x620 [] etnaviv_load+0xc0/0x128 [] drm_dev_register+0xac/0x10c [] drm_platform_init+0x48/0xd4 [] etnaviv_bind+0x18/0x20 [] try_to_bring_up_master+0x140/0x17c [] component_master_add_with_match+0x84/0xe0 [] etnaviv_pdev_probe+0xb4/0x104 [] platform_drv_probe+0x50/0xac [] driver_probe_device+0x114/0x234 [] __driver_attach+0x9c/0xa0 [] bus_for_each_dev+0x5c/0x90 [] driver_attach+0x24/0x28 [] bus_add_driver+0xe0/0x1d8 [] driver_register+0x80/0xfc [] __platform_driver_register+0x50/0x64 [] etnaviv_init+0x2c/0x4c [] do_one_initcall+0x88/0x1d8 [] kernel_init_freeable+0x110/0x1dc [] kernel_init+0x10/0xec [] ret_from_fork+0x14/0x24 -> #2 (&dev->struct_mutex){+.+.+.}: [] mutex_lock_nested+0x5c/0x3d8 [] drm_gem_mmap+0x3c/0xd4 [] drm_gem_cma_mmap+0x14/0x2c [] mmap_region+0x3d0/0x6a4 [] do_mmap_pgoff+0x2e4/0x374 [] vm_mmap_pgoff+0x6c/0x9c [] SyS_mmap_pgoff+0x94/0xb8 [] ret_fast_syscall+0x0/0x4c -> #1 (&mm->mmap_sem){++++++}: [] might_fault+0x64/0x98 [] con_set_unimap+0x160/0x25c [] vt_ioctl+0x126c/0x1328 [] tty_ioctl+0x498/0xc5c [] do_vfs_ioctl+0x84/0x66c [] SyS_ioctl+0x3c/0x60 [] ret_fast_syscall+0x0/0x4c -> #0 (console_lock){+.+.+.}: [] lock_acquire+0xb0/0x124 [] console_lock+0x44/0x6c [] console_cpu_notify+0x28/0x34 [] notifier_call_chain+0x4c/0x8c [] __raw_notifier_call_chain+0x1c/0x24 [] __cpu_notify+0x34/0x50 [] cpu_notify+0x18/0x1c [] cpu_notify_nofail+0x10/0x1c [] _cpu_down+0x100/0x248 [] cpu_down+0x2c/0x48 [] cpu_subsys_offline+0x14/0x18 [] device_offline+0x90/0xc0 [] online_store+0x4c/0x74 [] dev_attr_store+0x20/0x2c [] sysfs_kf_write+0x54/0x58 [] kernfs_fop_write+0xfc/0x1ac [] vfs_write+0xac/0x1b4 [] SyS_write+0x44/0x90 [] ret_fast_syscall+0x0/0x4c other info that might help us debug this: Chain exists of: console_lock --> lock --> cpu_hotplug.lock#2 Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(cpu_hotplug.lock#2); lock(lock); lock(cpu_hotplug.lock#2); lock(console_lock); *** DEADLOCK *** 8 locks held by bash/2154: #0: (sb_writers#5){.+.+.+}, at: [] vfs_write+0x18c/0x1b4 #1: (&of->mutex){+.+.+.}, at: [] kernfs_fop_write+0x88/0x1ac #2: (s_active#40){.+.+.+}, at: [] kernfs_fop_write+0x90/0x1ac #3: (device_hotplug_lock){+.+.+.}, at: [] lock_device_hotplug_sysfs+0x14/0x54 #4: (&dev->mutex){......}, at: [] device_offline+0x44/0xc0 #5: (cpu_add_remove_lock){+.+.+.}, at: [] cpu_maps_update_begin+0x18/0x20 #6: (cpu_hotplug.lock){++++++}, at: [] cpu_hotplug_begin+0x0/0xb8 #7: (cpu_hotplug.lock#2){+.+.+.}, at: [] cpu_hotplug_begin+0x64/0xb8 stack backtrace: CPU: 0 PID: 2154 Comm: bash Not tainted 3.19.0-rc6+ #1497 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) Backtrace: [] (dump_backtrace) from [] (show_stack+0x18/0x1c) [] (show_stack) from [] (dump_stack+0x7c/0x98) [] (dump_stack) from [] (print_circular_bug+0x28c/0x2d8) [] (print_circular_bug) from [] (__lock_acquire+0x1acc/0x1bb0) [] (__lock_acquire) from [] (lock_acquire+0xb0/0x124) [] (lock_acquire) from [] (console_lock+0x44/0x6c) [] (console_lock) from [] (console_cpu_notify+0x28/0x34) [] (console_cpu_notify) from [] (notifier_call_chain+0x4c/0x8c) [] (notifier_call_chain) from [] (__raw_notifier_call_chain+0x1c/0x24) [] (__raw_notifier_call_chain) from [] (__cpu_notify+0x34/0x50) [] (__cpu_notify) from [] (cpu_notify+0x18/0x1c) [] (cpu_notify) from [] (cpu_notify_nofail+0x10/0x1c) [] (cpu_notify_nofail) from [] (_cpu_down+0x100/0x248) [] (_cpu_down) from [] (cpu_down+0x2c/0x48) [] (cpu_down) from [] (cpu_subsys_offline+0x14/0x18) [] (cpu_subsys_offline) from [] (device_offline+0x90/0xc0) [] (device_offline) from [] (online_store+0x4c/0x74) [] (online_store) from [] (dev_attr_store+0x20/0x2c) [] (dev_attr_store) from [] (sysfs_kf_write+0x54/0x58) [] (sysfs_kf_write) from [] (kernfs_fop_write+0xfc/0x1ac) [] (kernfs_fop_write) from [] (vfs_write+0xac/0x1b4) [] (vfs_write) from [] (SyS_write+0x44/0x90) [] (SyS_write) from [] (ret_fast_syscall+0x0/0x4c) The locking ordering for each of the chain backtraces are: 5: cpu_hotplug.lock (in lru_add_drain_all, get_online_cpus) lock (in lru_add_drain_all) cma_mutex (in cma_alloc) 4: lock (in lru_add_drain_all), cma_mutex (in cma_alloc) 3: cma_mutex (in cma_alloc) drm dev->struct_mutex (in etnaviv_load) 2: drm dev->struct_mutex (in drm_gem_mmap) mm->mmap_sem (in vm_mmap_pgoff) 1: mm->mmap_sem (in might_fault) console_lock (in con_set_unimap, console_lock) 0: console_lock (in console_cpu_notify, console_lock) cpu_hotplug.lock (in _cpu_down, cpu_hotplug_begin) Hence the dependency chain of: cpu_hotplug.lock -> console_lock -> mmap_sem -> struct_mutex -> cma_mutex -> cpu_hotplug.lock *deadlock* The operation which etnadrm needs to lock is not the allocations, but the addition of the etnaviv_obj to the inactive list (to prevent the list becoming corrupted.) Move this to a separate operation which is performed once all the setup of the object is complete, and move the locking such that the allocation and setup is unlocked. This is overall more efficient, as we permit multiple expensive operations to occur in parallel (memory allocation) while only locking what we need. Signed-off-by: Russell King --- drivers/staging/etnaviv/etnaviv_drv.c | 6 -- drivers/staging/etnaviv/etnaviv_gem.c | 94 +++++++++++++++++++---------- drivers/staging/etnaviv/etnaviv_gem.h | 1 + drivers/staging/etnaviv/etnaviv_gem_prime.c | 4 ++ drivers/staging/etnaviv/etnaviv_gpu.c | 2 + 5 files changed, 69 insertions(+), 38 deletions(-) diff --git a/drivers/staging/etnaviv/etnaviv_drv.c b/drivers/staging/etnaviv/etnaviv_drv.c index 0dedbbeb1fa5..1ba480aed4fd 100644 --- a/drivers/staging/etnaviv/etnaviv_drv.c +++ b/drivers/staging/etnaviv/etnaviv_drv.c @@ -84,9 +84,7 @@ static int etnaviv_unload(struct drm_device *dev) flush_workqueue(priv->wq); destroy_workqueue(priv->wq); - mutex_lock(&dev->struct_mutex); component_unbind_all(dev->dev, dev); - mutex_unlock(&dev->struct_mutex); dev->dev_private = NULL; @@ -138,16 +136,12 @@ static int etnaviv_load(struct drm_device *dev, unsigned long flags) platform_set_drvdata(pdev, dev); - mutex_lock(&dev->struct_mutex); - err = component_bind_all(dev->dev, dev); if (err < 0) return err; load_gpu(dev); - mutex_unlock(&dev->struct_mutex); - return 0; } diff --git a/drivers/staging/etnaviv/etnaviv_gem.c b/drivers/staging/etnaviv/etnaviv_gem.c index 09800b0dec35..22407846320b 100644 --- a/drivers/staging/etnaviv/etnaviv_gem.c +++ b/drivers/staging/etnaviv/etnaviv_gem.c @@ -566,37 +566,26 @@ void etnaviv_gem_free_object(struct drm_gem_object *obj) kfree(etnaviv_obj); } -/* convenience method to construct a GEM buffer object, and userspace handle */ -int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file, - uint32_t size, uint32_t flags, uint32_t *handle) +int etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj) { - struct drm_gem_object *obj; + struct etnaviv_drm_private *priv = dev->dev_private; + struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); int ret; - ret = mutex_lock_interruptible(&dev->struct_mutex); + ret = mutex_lock_killable(&dev->struct_mutex); if (ret) return ret; - obj = etnaviv_gem_new(dev, size, flags); - + list_add_tail(&etnaviv_obj->mm_list, &priv->inactive_list); mutex_unlock(&dev->struct_mutex); - if (IS_ERR(obj)) - return PTR_ERR(obj); - - ret = drm_gem_handle_create(file, obj, handle); - - /* drop reference from allocate - handle holds it now */ - drm_gem_object_unreference_unlocked(obj); - - return ret; + return 0; } static int etnaviv_gem_new_impl(struct drm_device *dev, uint32_t size, uint32_t flags, struct drm_gem_object **obj) { - struct etnaviv_drm_private *priv = dev->dev_private; struct etnaviv_gem_object *etnaviv_obj; unsigned sz = sizeof(*etnaviv_obj); bool valid = true; @@ -651,14 +640,12 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, return 0; } -struct drm_gem_object *etnaviv_gem_new(struct drm_device *dev, +static struct drm_gem_object *__etnaviv_gem_new(struct drm_device *dev, uint32_t size, uint32_t flags) { struct drm_gem_object *obj = NULL; int ret; - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); - size = PAGE_ALIGN(size); ret = etnaviv_gem_new_impl(dev, size, flags, &obj); @@ -681,11 +668,55 @@ struct drm_gem_object *etnaviv_gem_new(struct drm_device *dev, fail: if (obj) - drm_gem_object_unreference(obj); + drm_gem_object_unreference_unlocked(obj); return ERR_PTR(ret); } +/* convenience method to construct a GEM buffer object, and userspace handle */ +int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file, + uint32_t size, uint32_t flags, uint32_t *handle) +{ + struct drm_gem_object *obj; + int ret; + + obj = __etnaviv_gem_new(dev, size, flags); + if (IS_ERR(obj)) + return PTR_ERR(obj); + + ret = etnaviv_gem_obj_add(dev, obj); + if (ret < 0) { + drm_gem_object_unreference_unlocked(obj); + return ret; + } + + ret = drm_gem_handle_create(file, obj, handle); + + /* drop reference from allocate - handle holds it now */ + drm_gem_object_unreference_unlocked(obj); + + return ret; +} + +struct drm_gem_object *etnaviv_gem_new(struct drm_device *dev, + uint32_t size, uint32_t flags) +{ + struct drm_gem_object *obj; + int ret; + + obj = __etnaviv_gem_new(dev, size, flags); + if (IS_ERR(obj)) + return obj; + + ret = etnaviv_gem_obj_add(dev, obj); + if (ret < 0) { + drm_gem_object_unreference_unlocked(obj); + return ERR_PTR(ret); + } + + return obj; +} + int etnaviv_gem_new_private(struct drm_device *dev, size_t size, uint32_t flags, struct etnaviv_gem_object **res) { @@ -879,22 +910,21 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file, struct etnaviv_gem_object *etnaviv_obj; int ret; - ret = mutex_lock_interruptible(&dev->struct_mutex); + ret = etnaviv_gem_new_private(dev, size, ETNA_BO_CACHED, &etnaviv_obj); if (ret) return ret; - ret = etnaviv_gem_new_private(dev, size, ETNA_BO_CACHED, &etnaviv_obj); - if (ret == 0) { - etnaviv_obj->ops = &etnaviv_gem_userptr_ops; - etnaviv_obj->userptr.ptr = ptr; - etnaviv_obj->userptr.task = current; - etnaviv_obj->userptr.ro = !(flags & ETNA_USERPTR_WRITE); - get_task_struct(current); - } - mutex_unlock(&dev->struct_mutex); + etnaviv_obj->ops = &etnaviv_gem_userptr_ops; + etnaviv_obj->userptr.ptr = ptr; + etnaviv_obj->userptr.task = current; + etnaviv_obj->userptr.ro = !(flags & ETNA_USERPTR_WRITE); + get_task_struct(current); - if (ret) + ret = etnaviv_gem_obj_add(dev, &etnaviv_obj->base); + if (ret) { + drm_gem_object_unreference_unlocked(&etnaviv_obj->base); return ret; + } ret = drm_gem_handle_create(file, &etnaviv_obj->base, handle); diff --git a/drivers/staging/etnaviv/etnaviv_gem.h b/drivers/staging/etnaviv/etnaviv_gem.h index cc5e307465f6..c801b9a161e6 100644 --- a/drivers/staging/etnaviv/etnaviv_gem.h +++ b/drivers/staging/etnaviv/etnaviv_gem.h @@ -133,6 +133,7 @@ etnaviv_gem_get_vram_mapping(struct etnaviv_gem_object *obj, struct etnaviv_iommu *mmu); int etnaviv_gem_new_private(struct drm_device *dev, size_t size, uint32_t flags, struct etnaviv_gem_object **res); +int etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj); struct page **etnaviv_gem_get_pages(struct etnaviv_gem_object *obj); void etnaviv_gem_put_pages(struct etnaviv_gem_object *obj); diff --git a/drivers/staging/etnaviv/etnaviv_gem_prime.c b/drivers/staging/etnaviv/etnaviv_gem_prime.c index aad5a96f9fba..cca569be98bf 100644 --- a/drivers/staging/etnaviv/etnaviv_gem_prime.c +++ b/drivers/staging/etnaviv/etnaviv_gem_prime.c @@ -108,6 +108,10 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev, if (ret) goto fail; + ret = etnaviv_gem_obj_add(dev, &etnaviv_obj->base); + if (ret) + goto fail; + return &etnaviv_obj->base; fail: diff --git a/drivers/staging/etnaviv/etnaviv_gpu.c b/drivers/staging/etnaviv/etnaviv_gpu.c index 9c06bc6c245d..dc02c69512ff 100644 --- a/drivers/staging/etnaviv/etnaviv_gpu.c +++ b/drivers/staging/etnaviv/etnaviv_gpu.c @@ -529,7 +529,9 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu) } /* Now program the hardware */ + mutex_lock(&gpu->drm->struct_mutex); etnaviv_gpu_hw_init(gpu); + mutex_unlock(&gpu->drm->struct_mutex); pm_runtime_mark_last_busy(gpu->dev); pm_runtime_put_autosuspend(gpu->dev);