From bf248ca1f5c7ba1e535ba4bd517a15a1ae965c69 Mon Sep 17 00:00:00 2001 From: Daniel Stone Date: Tue, 3 Nov 2015 21:42:31 +0000 Subject: drm/i915: Fix locking around GuC firmware load The GuC firmware load requires struct_mutex to create a GEM object, but this collides badly with request_firmware. Move struct_mutex locking down into the loader itself, so we don't hold it across the entire load process, including request_firmware. [ 20.451400] ====================================================== [ 20.451420] [ INFO: possible circular locking dependency detected ] [ 20.451441] 4.3.0-rc5+ #1 Tainted: G W [ 20.451457] ------------------------------------------------------- [ 20.451477] plymouthd/371 is trying to acquire lock: [ 20.451494] (&dev->struct_mutex){+.+.+.}, at: [] drm_gem_mmap+0x112/0x290 [drm] [ 20.451538] but task is already holding lock: [ 20.451557] (&mm->mmap_sem){++++++}, at: [] vm_mmap_pgoff+0x8c/0xf0 [ 20.451591] which lock already depends on the new lock. [ 20.451617] the existing dependency chain (in reverse order) is: [ 20.451640] -> #3 (&mm->mmap_sem){++++++}: [ 20.451661] [] lock_acquire+0xce/0x1c0 [ 20.451683] [] __might_fault+0x7a/0xa0 [ 20.451705] [] filldir+0x9e/0x130 [ 20.451726] [] dcache_readdir+0x186/0x230 [ 20.451748] [] iterate_dir+0x97/0x130 [ 20.451769] [] SyS_getdents+0x9a/0x130 [ 20.451790] [] entry_SYSCALL_64_fastpath+0x12/0x76 [ 20.451829] -> #2 (&sb->s_type->i_mutex_key#2){+.+.+.}: [ 20.451852] [] lock_acquire+0xce/0x1c0 [ 20.451872] [] mutex_lock_nested+0x86/0x400 [ 20.451893] [] walk_component+0x1d0/0x2a0 [ 20.451914] [] link_path_walk+0x190/0x5a0 [ 20.451935] [] path_openat+0xab/0x1260 [ 20.451955] [] do_filp_open+0x91/0x100 [ 20.451975] [] file_open_name+0xf7/0x150 [ 20.451995] [] filp_open+0x33/0x60 [ 20.452014] [] _request_firmware+0x277/0x880 [ 20.452038] [] request_firmware_work_func+0x34/0x80 [ 20.452060] [] process_one_work+0x230/0x680 [ 20.452082] [] worker_thread+0x4e/0x450 [ 20.452102] [] kthread+0x101/0x120 [ 20.452121] [] ret_from_fork+0x3f/0x70 [ 20.452140] -> #1 (umhelper_sem){++++.+}: [ 20.452159] [] lock_acquire+0xce/0x1c0 [ 20.452178] [] down_read+0x51/0xa0 [ 20.452197] [] usermodehelper_read_trylock+0x5b/0x130 [ 20.452221] [] _request_firmware+0x1d7/0x880 [ 20.452242] [] request_firmware+0x31/0x50 [ 20.452262] [] intel_guc_ucode_init+0xf4/0x400 [i915] [ 20.452305] [] i915_driver_load+0xd63/0x16e0 [i915] [ 20.452343] [] drm_dev_register+0xa9/0xc0 [drm] [ 20.452369] [] drm_get_pci_dev+0x8d/0x1e0 [drm] [ 20.452396] [] i915_pci_probe+0x34/0x50 [i915] [ 20.452421] [] local_pci_probe+0x45/0xa0 [ 20.452443] [] pci_device_probe+0xfd/0x140 [ 20.452464] [] driver_probe_device+0x224/0x480 [ 20.452486] [] __driver_attach+0x88/0x90 [ 20.452505] [] bus_for_each_dev+0x73/0xc0 [ 20.452526] [] driver_attach+0x1e/0x20 [ 20.452546] [] bus_add_driver+0x1ee/0x280 [ 20.452566] [] driver_register+0x60/0xe0 [ 20.453197] [] __pci_register_driver+0x60/0x70 [ 20.453845] [] drm_pci_init+0xe0/0x110 [drm] [ 20.454497] [] 0xffffffffa027f092 [ 20.455156] [] do_one_initcall+0xb3/0x200 [ 20.455796] [] do_init_module+0x5f/0x1e7 [ 20.456434] [] load_module+0x2126/0x27d0 [ 20.457071] [] SyS_finit_module+0xb9/0xf0 [ 20.457738] [] entry_SYSCALL_64_fastpath+0x12/0x76 [ 20.458370] -> #0 (&dev->struct_mutex){+.+.+.}: [ 20.459773] [] __lock_acquire+0x191f/0x1ba0 [ 20.460451] [] lock_acquire+0xce/0x1c0 [ 20.461074] [] drm_gem_mmap+0x138/0x290 [drm] [ 20.461693] [] mmap_region+0x3ec/0x670 [ 20.462298] [] do_mmap+0x342/0x420 [ 20.462901] [] vm_mmap_pgoff+0xb2/0xf0 [ 20.463532] [] SyS_mmap_pgoff+0x1f2/0x290 [ 20.464118] [] SyS_mmap+0x1b/0x30 [ 20.464702] [] entry_SYSCALL_64_fastpath+0x12/0x76 [ 20.465289] other info that might help us debug this: [ 20.467179] Chain exists of: &dev->struct_mutex --> &sb->s_type->i_mutex_key#2 --> &mm->mmap_sem [ 20.468928] Possible unsafe locking scenario: [ 20.470161] CPU0 CPU1 [ 20.470745] ---- ---- [ 20.471325] lock(&mm->mmap_sem); [ 20.471902] lock(&sb->s_type->i_mutex_key#2); [ 20.472538] lock(&mm->mmap_sem); [ 20.473118] lock(&dev->struct_mutex); [ 20.473704] *** DEADLOCK *** Signed-off-by: Daniel Stone Signed-off-by: Dave Airlie diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 2336af9..b4741d1 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -406,10 +406,7 @@ static int i915_load_modeset_init(struct drm_device *dev) * working irqs for e.g. gmbus and dp aux transfers. */ intel_modeset_init(dev); - /* intel_guc_ucode_init() needs the mutex to allocate GEM objects */ - mutex_lock(&dev->struct_mutex); intel_guc_ucode_init(dev); - mutex_unlock(&dev->struct_mutex); ret = i915_gem_init(dev); if (ret) @@ -452,9 +449,7 @@ cleanup_gem: i915_gem_context_fini(dev); mutex_unlock(&dev->struct_mutex); cleanup_irq: - mutex_lock(&dev->struct_mutex); intel_guc_ucode_fini(dev); - mutex_unlock(&dev->struct_mutex); drm_irq_uninstall(dev); cleanup_gem_stolen: i915_gem_cleanup_stolen(dev); @@ -1193,8 +1188,8 @@ int i915_driver_unload(struct drm_device *dev) /* Flush any outstanding unpin_work. */ flush_workqueue(dev_priv->wq); - mutex_lock(&dev->struct_mutex); intel_guc_ucode_fini(dev); + mutex_lock(&dev->struct_mutex); i915_gem_cleanup_ringbuffer(dev); i915_gem_context_fini(dev); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index a17b6a5..3541f76 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -504,7 +504,9 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found, guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted); + mutex_lock(&dev->struct_mutex); obj = i915_gem_object_create_from_data(dev, fw->data, fw->size); + mutex_unlock(&dev->struct_mutex); if (IS_ERR_OR_NULL(obj)) { err = obj ? PTR_ERR(obj) : -ENOMEM; goto fail; @@ -540,8 +542,6 @@ fail: * @dev: drm device * * Called early during driver load, but after GEM is initialised. - * The device struct_mutex must be held by the caller, as we're - * going to allocate a GEM object to hold the firmware image. * * The firmware will be transferred to the GuC's memory later, * when intel_guc_ucode_load() is called. @@ -598,9 +598,11 @@ void intel_guc_ucode_fini(struct drm_device *dev) direct_interrupts_to_host(dev_priv); i915_guc_submission_fini(dev); + mutex_lock(&dev->struct_mutex); if (guc_fw->guc_fw_obj) drm_gem_object_unreference(&guc_fw->guc_fw_obj->base); guc_fw->guc_fw_obj = NULL; + mutex_unlock(&dev->struct_mutex); guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE; } -- cgit v0.10.2