* 3.8-rc6: nouveau lockdep recursive lock acquisition
@ 2013-02-03 15:09 Daniel J Blueman
2013-02-04 21:30 ` [PATCH] drm/nouveau: add lockdep annotations Marcin Slusarz
0 siblings, 1 reply; 6+ messages in thread
From: Daniel J Blueman @ 2013-02-03 15:09 UTC (permalink / raw)
To: Marcin Ślusarz
Cc: nouveau, dri-devel, Linux Kernel, Dave Airlie, Ben Skeggs
>From recent additional locking in nouveau, it looks like we see
recursive lock acquisition in 3.8-rc6:
nouveau [ DEVICE][0000:01:00.0] BOOT0 : 0x0e7150a2
nouveau [ DEVICE][0000:01:00.0] Chipset: GK107 (NVE7)
nouveau [ DEVICE][0000:01:00.0] Family : NVE0
nouveau [ VBIOS][0000:01:00.0] checking PRAMIN for image...
nouveau [ VBIOS][0000:01:00.0] ... appears to be valid
nouveau [ VBIOS][0000:01:00.0] using image from PRAMIN
nouveau [ VBIOS][0000:01:00.0] BIT signature found
nouveau [ VBIOS][0000:01:00.0] version 80.07.26.04.01
nouveau [ PFB][0000:01:00.0] RAM type: GDDR5
nouveau [ PFB][0000:01:00.0] RAM size: 1024 MiB
nouveau [ PFB][0000:01:00.0] ZCOMP: 0 tags
init: gdm main process (960) killed by TERM signal
vga_switcheroo: enabled
[TTM] Zone kernel: Available graphics memory: 4038258 kiB
[TTM] Zone dma32: Available graphics memory: 2097152 kiB
[TTM] Initializing pool allocator
[TTM] Initializing DMA pool allocator
nouveau [ DRM] VRAM: 1024 MiB
nouveau [ DRM] GART: 512 MiB
nouveau [ DRM] BIT BIOS found
nouveau [ DRM] Bios version 80.07.26.04
nouveau [ DRM] TMDS table version 2.0
nouveau [ DRM] DCB version 4.0
nouveau [ DRM] DCB outp 00: 048101b6 0f230010
nouveau [ DRM] DCB outp 01: 018212d6 0f220020
nouveau [ DRM] DCB outp 02: 01021212 00020020
nouveau [ DRM] DCB outp 03: 088324c6 0f220010
nouveau [ DRM] DCB outp 04: 08032402 00020010
nouveau [ DRM] DCB outp 05: 02843862 00020010
nouveau [ DRM] DCB conn 00: 00020047
nouveau [ DRM] DCB conn 01: 02208146
nouveau [ DRM] DCB conn 02: 01104246
nouveau [ DRM] DCB conn 03: 00410361
=============================================
[ INFO: possible recursive locking detected ]
3.8.0-rc6-ninja+ #1 Not tainted
---------------------------------------------
modprobe/585 is trying to acquire lock:
(&subdev->mutex){+.+.+.}, at: [<ffffffffa016c323>]
nouveau_instobj_create_+0x43/0x90 [nouveau]
but task is already holding lock:
(&subdev->mutex){+.+.+.}, at: [<ffffffffa017672d>]
nv50_disp_data_ctor+0x5d/0xd0 [nouveau]
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&subdev->mutex);
lock(&subdev->mutex);
*** DEADLOCK ***
May be due to missing lock nesting notation
4 locks held by modprobe/585:
#0: (&__lockdep_no_validate__){......}, at: [<ffffffff813075f3>]
__driver_attach+0x53/0xb0
#1: (&__lockdep_no_validate__){......}, at: [<ffffffff81307601>]
__driver_attach+0x61/0xb0
#2: (drm_global_mutex){+.+.+.}, at: [<ffffffff812ee59c>]
drm_get_pci_dev+0xbc/0x2b0
#3: (&subdev->mutex){+.+.+.}, at: [<ffffffffa017672d>]
nv50_disp_data_ctor+0x5d/0xd0 [nouveau]
stack backtrace:
Pid: 585, comm: modprobe Not tainted 3.8.0-rc6-expert+ #1
Call Trace:
[<ffffffff8108fde2>] validate_chain.isra.33+0xd72/0x10d0
[<ffffffff8105fa08>] ? __kernel_text_address+0x58/0x80
[<ffffffff8100575d>] ? print_context_stack+0x5d/0xd0
[<ffffffff81090bc1>] __lock_acquire+0x3a1/0xb60
[<ffffffff8108d504>] ? __lock_is_held+0x54/0x80
[<ffffffff8109184a>] lock_acquire+0x5a/0x70
[<ffffffffa016c323>] ? nouveau_instobj_create_+0x43/0x90 [nouveau]
[<ffffffff81558739>] mutex_lock_nested+0x69/0x340
[<ffffffffa016c323>] ? nouveau_instobj_create_+0x43/0x90 [nouveau]
[<ffffffffa0152370>] ? nouveau_object_create_+0x60/0xa0 [nouveau]
[<ffffffffa016c323>] nouveau_instobj_create_+0x43/0x90 [nouveau]
[<ffffffffa016cf8c>] nv50_instobj_ctor+0x4c/0xf0 [nouveau]
[<ffffffffa0152163>] nouveau_object_ctor+0x33/0xc0 [nouveau]
[<ffffffffa016cd51>] nv50_instmem_alloc+0x21/0x30 [nouveau]
[<ffffffffa0150917>] nouveau_gpuobj_create_+0x247/0x2f0 [nouveau]
[<ffffffff8155b35a>] ? _raw_spin_unlock_irqrestore+0x3a/0x70
[<ffffffff810921fd>] ? trace_hardirqs_on_caller+0x10d/0x1a0
[<ffffffffa014f4bc>] nouveau_engctx_create_+0x25c/0x2a0 [nouveau]
[<ffffffffa0176791>] nv50_disp_data_ctor+0xc1/0xd0 [nouveau]
[<ffffffffa0153722>] ? nouveau_subdev_reset+0x52/0x60 [nouveau]
[<ffffffffa0152163>] nouveau_object_ctor+0x33/0xc0 [nouveau]
[<ffffffffa0152a42>] nouveau_object_new+0x112/0x240 [nouveau]
[<ffffffffa01f4b1d>] nv50_display_create+0x18d/0x860 [nouveau]
[<ffffffff8105cb5d>] ? __cancel_work_timer+0x6d/0xc0
[<ffffffffa01db8eb>] nouveau_display_create+0x3cb/0x670 [nouveau]
[<ffffffffa01cb1bf>] nouveau_drm_load+0x26f/0x590 [nouveau]
[<ffffffff81304c99>] ? device_register+0x19/0x20
[<ffffffff812efe91>] ? drm_sysfs_device_add+0x81/0xb0
[<ffffffff812ee65e>] drm_get_pci_dev+0x17e/0x2b0
[<ffffffff81245e56>] ? __pci_set_master+0x26/0x80
[<ffffffffa01cab2a>] nouveau_drm_probe+0x25a/0x2a0 [nouveau]
[<ffffffff8124a386>] local_pci_probe+0x46/0x80
[<ffffffff8124ac11>] pci_device_probe+0x101/0x110
[<ffffffff813073d6>] driver_probe_device+0x76/0x240
[<ffffffff81307643>] __driver_attach+0xa3/0xb0
[<ffffffff813075a0>] ? driver_probe_device+0x240/0x240
[<ffffffff8130564d>] bus_for_each_dev+0x4d/0x90
[<ffffffff81306f39>] driver_attach+0x19/0x20
[<ffffffff81306af0>] bus_add_driver+0x1a0/0x270
[<ffffffffa023d000>] ? 0xffffffffa023cfff
[<ffffffff81307cd2>] driver_register+0x72/0x170
[<ffffffffa023d000>] ? 0xffffffffa023cfff
[<ffffffff8124ad0f>] __pci_register_driver+0x5f/0x70
[<ffffffff812ee8a5>] drm_pci_init+0x115/0x130
[<ffffffffa023d000>] ? 0xffffffffa023cfff
[<ffffffffa023d000>] ? 0xffffffffa023cfff
[<ffffffffa023d04d>] nouveau_drm_init+0x4d/0x1000 [nouveau]
[<ffffffff810002da>] do_one_initcall+0x11a/0x170
[<ffffffff8109d044>] load_module+0xe84/0x1470
[<ffffffff81098c30>] ? in_lock_functions+0x20/0x20
[<ffffffff8122c22e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[<ffffffff8109d6e7>] sys_init_module+0xb7/0xe0
[<ffffffff8155c156>] system_call_fastpath+0x1a/0x1f
[drm] Supports vblank timestamp caching Rev 1 (10.10.2010).
[drm] No driver support for vblank timestamp query.
nouveau W[ DRM] voltage table 0x50 unknown
nouveau [ DRM] 4 available performance level(s)
nouveau [ DRM] 1: core 209MHz shader 419MHz memory 405MHz voltage 520mV
nouveau [ DRM] 2: core 390MHz shader 780MHz memory 1080MHz voltage 610mV
nouveau [ DRM] 3: core 1000MHz shader 2000MHz memory 1080MHz voltage 630mV
nouveau [ DRM] 4: core 1254MHz shader 2508MHz memory 1080MHz voltage 630mV
nouveau [ DRM] c:
nouveau [ DRM] MM: using COPY for buffer copies
nouveau 0000:01:00.0: No connectors reported connected with modes
[drm] Cannot find any crtc or sizes - going 1024x768
nouveau [ DRM] allocated 1024x768 fb: 0x80000, bo ffff88025b966800
nouveau 0000:01:00.0: fb1: nouveaufb frame buffer device
[drm] Initialized nouveau 1.1.0 20120801 for 0000:01:00.0 on minor 1
snd_hda_intel 0000:01:00.1: enabling device (0000 -> 0002)
hda-intel 0000:01:00.1: Handle VGA-switcheroo audio client
snd_hda_intel 0000:01:00.1: irq 49 for MSI/MSI-X
input: HDA NVidia HDMI/DP,pcm=8 as
/devices/pci0000:00/0000:00:01.0/0000:01:00.1/sound/card1/input11
input: HDA NVidia HDMI/DP,pcm=7 as
/devices/pci0000:00/0000:00:01.0/0000:01:00.1/sound/card1/input12
input: HDA NVidia HDMI/DP,pcm=3 as
/devices/pci0000:00/0000:00:01.0/0000:01:00.1/sound/card1/input13
hda-intel 0000:01:00.1: Disabling via VGA-switcheroo
VGA switcheroo: switched nouveau off
nouveau [ DRM] suspending fbcon...
nouveau [ DRM] suspending display...
nouveau [ DRM] unpinning framebuffer(s)...
nouveau [ DRM] evicting buffers...
nouveau [ DRM] suspending client object trees...
nouveau E[ I2C][0000:01:00.0] AUXCH(3): begin idle timeout 0xffffffff
nouveau E[ I2C][0000:01:00.0] AUXCH(2): begin idle timeout 0xffffffff
nouveau E[ I2C][0000:01:00.0] AUXCH(1): begin idle timeout 0xffffffff
applesmc: light sensor data length set to 10
nouveau E[ I2C][0000:01:00.0] AUXCH(1): begin idle timeout 0xffffffff
nouveau E[ I2C][0000:01:00.0] AUXCH(3): begin idle timeout 0xffffffff
nouveau E[ I2C][0000:01:00.0] AUXCH(2): begin idle timeout 0xffffffff
--
Daniel J Blueman
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] drm/nouveau: add lockdep annotations
2013-02-03 15:09 3.8-rc6: nouveau lockdep recursive lock acquisition Daniel J Blueman
@ 2013-02-04 21:30 ` Marcin Slusarz
2013-02-04 21:59 ` Maarten Lankhorst
0 siblings, 1 reply; 6+ messages in thread
From: Marcin Slusarz @ 2013-02-04 21:30 UTC (permalink / raw)
To: Ben Skeggs
Cc: nouveau, dri-devel, Linux Kernel, Dave Airlie, Daniel J Blueman,
Maarten Lankhorst, Peter Hurley, Arend van Spriel
1) Lockdep thinks all nouveau subdevs belong to the same class and can be
locked in arbitrary order, which is not true (at least in general case).
Tell it to distinguish subdevs by (o)class type.
2) DRM client can be locked under user client lock - tell lockdep to put DRM
client lock in a separate class.
Reported-by: Arend van Spriel <arend@broadcom.com>
Reported-by: Peter Hurley <peter@hurleysoftware.com>
Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Reported-by: Daniel J Blueman <daniel@quora.org>
Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
Cc: stable@vger.kernel.org [3.7, but needs s/const ofuncs/ofuncs/ to build]
---
Lightly tested, only on NV4B and NVC1.
---
drivers/gpu/drm/nouveau/core/core/subdev.c | 2 +-
drivers/gpu/drm/nouveau/core/include/core/object.h | 7 +++++--
drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +++
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/core/core/subdev.c b/drivers/gpu/drm/nouveau/core/core/subdev.c
index f74c30a..48f0637 100644
--- a/drivers/gpu/drm/nouveau/core/core/subdev.c
+++ b/drivers/gpu/drm/nouveau/core/core/subdev.c
@@ -99,7 +99,7 @@ nouveau_subdev_create_(struct nouveau_object *parent,
if (ret)
return ret;
- mutex_init(&subdev->mutex);
+ __mutex_init(&subdev->mutex, subname, &oclass->lock_class_key);
subdev->name = subname;
if (parent) {
diff --git a/drivers/gpu/drm/nouveau/core/include/core/object.h b/drivers/gpu/drm/nouveau/core/include/core/object.h
index 6a90267..62e68ba 100644
--- a/drivers/gpu/drm/nouveau/core/include/core/object.h
+++ b/drivers/gpu/drm/nouveau/core/include/core/object.h
@@ -50,10 +50,13 @@ int nouveau_object_fini(struct nouveau_object *, bool suspend);
extern struct nouveau_ofuncs nouveau_object_ofuncs;
+/* Don't allocate dynamically, because lockdep needs lock_class_keys to be in
+ * ".data". */
struct nouveau_oclass {
u32 handle;
- struct nouveau_ofuncs *ofuncs;
- struct nouveau_omthds *omthds;
+ struct nouveau_ofuncs * const ofuncs;
+ struct nouveau_omthds * const omthds;
+ struct lock_class_key lock_class_key;
};
#define nv_oclass(o) nv_object(o)->oclass
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index ef1ad21..bc00587 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -245,6 +245,8 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
return 0;
}
+static struct lock_class_key drm_client_lock_class_key;
+
static int
nouveau_drm_load(struct drm_device *dev, unsigned long flags)
{
@@ -256,6 +258,7 @@ nouveau_drm_load(struct drm_device *dev, unsigned long flags)
ret = nouveau_cli_create(pdev, "DRM", sizeof(*drm), (void**)&drm);
if (ret)
return ret;
+ lockdep_set_class(&drm->client.mutex, &drm_client_lock_class_key);
dev->dev_private = drm;
drm->dev = dev;
--
1.8.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/nouveau: add lockdep annotations
2013-02-04 21:30 ` [PATCH] drm/nouveau: add lockdep annotations Marcin Slusarz
@ 2013-02-04 21:59 ` Maarten Lankhorst
2013-02-04 23:24 ` Peter Hurley
2013-02-05 20:52 ` Ben Skeggs
0 siblings, 2 replies; 6+ messages in thread
From: Maarten Lankhorst @ 2013-02-04 21:59 UTC (permalink / raw)
To: Marcin Slusarz
Cc: Ben Skeggs, nouveau, dri-devel, Linux Kernel, Dave Airlie,
Daniel J Blueman, Peter Hurley, Arend van Spriel
Op 04-02-13 22:30, Marcin Slusarz schreef:
> 1) Lockdep thinks all nouveau subdevs belong to the same class and can be
> locked in arbitrary order, which is not true (at least in general case).
> Tell it to distinguish subdevs by (o)class type.
Apart from this specific case, is there any other reason why we require being able to nest 2 subdev locks?
Add a NVOBJ_FLAG_CREATE_EXCLUSIVE flag to nouveau_engctx_create and return -EBUSY if there is any existing object. Problem solved, and lockdep is still generic.
> 2) DRM client can be locked under user client lock - tell lockdep to put DRM
> client lock in a separate class.
Can you copy paste a typical lockdep splat for this? Also this should be a separate patch. :-)
> Reported-by: Arend van Spriel <arend@broadcom.com>
> Reported-by: Peter Hurley <peter@hurleysoftware.com>
> Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Reported-by: Daniel J Blueman <daniel@quora.org>
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> Cc: stable@vger.kernel.org [3.7, but needs s/const ofuncs/ofuncs/ to build]
> ---
> Lightly tested, only on NV4B and NVC1.
> ---
> drivers/gpu/drm/nouveau/core/core/subdev.c | 2 +-
> drivers/gpu/drm/nouveau/core/include/core/object.h | 7 +++++--
> drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +++
> 3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/core/core/subdev.c b/drivers/gpu/drm/nouveau/core/core/subdev.c
> index f74c30a..48f0637 100644
> --- a/drivers/gpu/drm/nouveau/core/core/subdev.c
> +++ b/drivers/gpu/drm/nouveau/core/core/subdev.c
> @@ -99,7 +99,7 @@ nouveau_subdev_create_(struct nouveau_object *parent,
> if (ret)
> return ret;
>
> - mutex_init(&subdev->mutex);
> + __mutex_init(&subdev->mutex, subname, &oclass->lock_class_key);
> subdev->name = subname;
>
> if (parent) {
> diff --git a/drivers/gpu/drm/nouveau/core/include/core/object.h b/drivers/gpu/drm/nouveau/core/include/core/object.h
> index 6a90267..62e68ba 100644
> --- a/drivers/gpu/drm/nouveau/core/include/core/object.h
> +++ b/drivers/gpu/drm/nouveau/core/include/core/object.h
> @@ -50,10 +50,13 @@ int nouveau_object_fini(struct nouveau_object *, bool suspend);
>
> extern struct nouveau_ofuncs nouveau_object_ofuncs;
>
> +/* Don't allocate dynamically, because lockdep needs lock_class_keys to be in
> + * ".data". */
> struct nouveau_oclass {
> u32 handle;
> - struct nouveau_ofuncs *ofuncs;
> - struct nouveau_omthds *omthds;
> + struct nouveau_ofuncs * const ofuncs;
> + struct nouveau_omthds * const omthds;
> + struct lock_class_key lock_class_key;
> };
>
> #define nv_oclass(o) nv_object(o)->oclass
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index ef1ad21..bc00587 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -245,6 +245,8 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> return 0;
> }
>
> +static struct lock_class_key drm_client_lock_class_key;
> +
> static int
> nouveau_drm_load(struct drm_device *dev, unsigned long flags)
> {
> @@ -256,6 +258,7 @@ nouveau_drm_load(struct drm_device *dev, unsigned long flags)
> ret = nouveau_cli_create(pdev, "DRM", sizeof(*drm), (void**)&drm);
> if (ret)
> return ret;
> + lockdep_set_class(&drm->client.mutex, &drm_client_lock_class_key);
>
> dev->dev_private = drm;
> drm->dev = dev;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/nouveau: add lockdep annotations
2013-02-04 21:59 ` Maarten Lankhorst
@ 2013-02-04 23:24 ` Peter Hurley
2013-02-05 20:52 ` Ben Skeggs
1 sibling, 0 replies; 6+ messages in thread
From: Peter Hurley @ 2013-02-04 23:24 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Marcin Slusarz, Ben Skeggs, nouveau, dri-devel, Linux Kernel,
Dave Airlie, Daniel J Blueman, Arend van Spriel
Hi Maarten
On Mon, 2013-02-04 at 22:59 +0100, Maarten Lankhorst wrote:
> Op 04-02-13 22:30, Marcin Slusarz schreef:
> > 1) Lockdep thinks all nouveau subdevs belong to the same class and can be
> > locked in arbitrary order, which is not true (at least in general case).
> > Tell it to distinguish subdevs by (o)class type.
> Apart from this specific case, is there any other reason why we require being able to nest 2 subdev locks?
>
> Add a NVOBJ_FLAG_CREATE_EXCLUSIVE flag to nouveau_engctx_create and return -EBUSY if there is any existing object. Problem solved, and lockdep is still generic.
>
> > 2) DRM client can be locked under user client lock - tell lockdep to put DRM
> > client lock in a separate class.
> Can you copy paste a typical lockdep splat for this? Also this should be a separate patch. :-)
PS - Deep call chain. Has anyone looked into max stack depth yet?
[ 5.995881] =============================================
[ 5.995886] [ INFO: possible recursive locking detected ]
[ 5.995892] 3.8.0-next-20130125+ttypatch-xeon+lockdep #20130125+ttypatch Not tainted
[ 5.995898] ---------------------------------------------
[ 5.995904] modprobe/275 is trying to acquire lock:
[ 5.995909] (&subdev->mutex){+.+.+.}, at: [<ffffffffa00d10b8>] nouveau_instobj_create_+0x48/0x90 [nouveau]
[ 5.995955]
[ 5.995955] but task is already holding lock:
[ 5.995961] (&subdev->mutex){+.+.+.}, at: [<ffffffffa00da3b5>] nv50_disp_data_ctor+0x65/0xd0 [nouveau]
[ 5.995989]
[ 5.995989] other info that might help us debug this:
[ 5.995995] Possible unsafe locking scenario:
[ 5.995995]
[ 5.996001] CPU0
[ 5.996004] ----
[ 5.996005] lock(&subdev->mutex);
[ 5.996005] lock(&subdev->mutex);
[ 5.996005]
[ 5.996005] *** DEADLOCK ***
[ 5.996005]
[ 5.996005] May be due to missing lock nesting notation
[ 5.996005]
[ 5.996005] 4 locks held by modprobe/275:
[ 5.996005] #0: (&__lockdep_no_validate__){......}, at: [<ffffffff8146fa5b>] __driver_attach+0x5b/0xb0
[ 5.996005] #1: (&__lockdep_no_validate__){......}, at: [<ffffffff8146fa69>] __driver_attach+0x69/0xb0
[ 5.996005] #2: (drm_global_mutex){+.+.+.}, at: [<ffffffffa0028d86>] drm_get_pci_dev+0xc6/0x2d0 [drm]
[ 5.996005] #3: (&subdev->mutex){+.+.+.}, at: [<ffffffffa00da3b5>] nv50_disp_data_ctor+0x65/0xd0 [nouveau]
[ 5.996005]
[ 5.996005] stack backtrace:
[ 5.996005] Pid: 275, comm: modprobe Not tainted 3.8.0-next-20130125+ttypatch-xeon+lockdep #20130125+ttypatch
[ 5.996005] Call Trace:
[ 5.996005] [<ffffffff810bd437>] __lock_acquire+0x687/0x1a70
[ 5.996005] [<ffffffff810bf70b>] ? mark_held_locks+0x9b/0x100
[ 5.996005] [<ffffffff810bf87d>] ? trace_hardirqs_on_caller+0x10d/0x1a0
[ 5.996005] [<ffffffff810bf91d>] ? trace_hardirqs_on+0xd/0x10
[ 5.996005] [<ffffffff8170a87a>] ? _raw_write_unlock_irqrestore+0x4a/0x90
[ 5.996005] [<ffffffff810bedc8>] lock_acquire+0x98/0x150
[ 5.996005] [<ffffffffa00d10b8>] ? nouveau_instobj_create_+0x48/0x90 [nouveau]
[ 5.996005] [<ffffffffa00d10b8>] ? nouveau_instobj_create_+0x48/0x90 [nouveau]
[ 5.996005] [<ffffffff81707f60>] mutex_lock_nested+0x50/0x390
[ 5.996005] [<ffffffffa00d10b8>] ? nouveau_instobj_create_+0x48/0x90 [nouveau]
[ 5.996005] [<ffffffffa00b6376>] ? nouveau_object_ref+0x46/0xd0 [nouveau]
[ 5.996005] [<ffffffffa00b64b5>] ? nouveau_object_create_+0x65/0xa0 [nouveau]
[ 5.996005] [<ffffffffa00d10b8>] nouveau_instobj_create_+0x48/0x90 [nouveau]
[ 5.996005] [<ffffffffa00d1be1>] nv50_instobj_ctor+0x51/0xf0 [nouveau]
[ 5.996005] [<ffffffffa00b62a8>] nouveau_object_ctor+0x38/0xc0 [nouveau]
[ 5.996005] [<ffffffffa00d1b36>] nv50_instmem_alloc+0x26/0x30 [nouveau]
[ 5.996005] [<ffffffffa00b49a7>] nouveau_gpuobj_create_+0x247/0x2f0 [nouveau]
[ 5.996005] [<ffffffff8170afed>] ? _raw_spin_unlock_irqrestore+0x6d/0x90
[ 5.996005] [<ffffffff810bf87d>] ? trace_hardirqs_on_caller+0x10d/0x1a0
[ 5.996005] [<ffffffffa00b34dc>] nouveau_engctx_create_+0x26c/0x2b0 [nouveau]
[ 5.996005] [<ffffffffa00da411>] nv50_disp_data_ctor+0xc1/0xd0 [nouveau]
[ 5.996005] [<ffffffffa00b62a8>] nouveau_object_ctor+0x38/0xc0 [nouveau]
[ 5.996005] [<ffffffffa00b6b82>] nouveau_object_new+0x112/0x240 [nouveau]
[ 5.996005] [<ffffffffa0155fe5>] nv50_display_create+0x1a5/0x890 [nouveau]
[ 5.996005] [<ffffffff8107837b>] ? __cancel_work_timer+0x8b/0xe0
[ 5.996005] [<ffffffffa013c80b>] nouveau_display_create+0x3cb/0x670 [nouveau]
[ 5.996005] [<ffffffffa012ba0f>] nouveau_drm_load+0x26f/0x590 [nouveau]
[ 5.996005] [<ffffffff8146caee>] ? device_register+0x1e/0x30
[ 5.996005] [<ffffffffa002a626>] ? drm_sysfs_device_add+0x86/0xb0 [drm]
[ 5.996005] [<ffffffffa0028e46>] drm_get_pci_dev+0x186/0x2d0 [drm]
[ 5.996005] [<ffffffffa012bf9a>] nouveau_drm_probe+0x26a/0x2c0 [nouveau]
[ 5.996005] [<ffffffff8170afca>] ? _raw_spin_unlock_irqrestore+0x4a/0x90
[ 5.996005] [<ffffffff81393a8b>] local_pci_probe+0x4b/0x80
[ 5.996005] [<ffffffff81393da1>] pci_device_probe+0x111/0x120
[ 5.996005] [<ffffffff8146f6eb>] driver_probe_device+0x8b/0x3a0
[ 5.996005] [<ffffffff8146faab>] __driver_attach+0xab/0xb0
[ 5.996005] [<ffffffff8146fa00>] ? driver_probe_device+0x3a0/0x3a0
[ 5.996005] [<ffffffff8146d635>] bus_for_each_dev+0x55/0x90
[ 5.996005] [<ffffffff8146f12e>] driver_attach+0x1e/0x20
[ 5.996005] [<ffffffff8146ebe1>] bus_add_driver+0x121/0x2b0
[ 5.996005] [<ffffffffa01ad000>] ? 0xffffffffa01acfff
[ 5.996005] [<ffffffff814701b7>] driver_register+0x77/0x170
[ 5.996005] [<ffffffffa01ad000>] ? 0xffffffffa01acfff
[ 5.996005] [<ffffffff81392b14>] __pci_register_driver+0x64/0x70
[ 5.996005] [<ffffffffa00290aa>] drm_pci_init+0x11a/0x130 [drm]
[ 5.996005] [<ffffffffa01ad000>] ? 0xffffffffa01acfff
[ 5.996005] [<ffffffffa01ad000>] ? 0xffffffffa01acfff
[ 5.996005] [<ffffffffa01ad04d>] nouveau_drm_init+0x4d/0x1000 [nouveau]
[ 5.996005] [<ffffffff8100215a>] do_one_initcall+0x12a/0x180
[ 5.996005] [<ffffffff810cc6a7>] load_module+0x12f7/0x1be0
[ 5.996005] [<ffffffff8137ce20>] ? ddebug_proc_open+0xd0/0xd0
[ 5.996005] [<ffffffff8136993e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 5.996005] [<ffffffff810cd067>] sys_init_module+0xd7/0x120
[ 5.996005] [<ffffffff81712f59>] system_call_fastpath+0x16/0x1b
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/nouveau: add lockdep annotations
2013-02-04 21:59 ` Maarten Lankhorst
2013-02-04 23:24 ` Peter Hurley
@ 2013-02-05 20:52 ` Ben Skeggs
2013-02-07 16:19 ` Maarten Lankhorst
1 sibling, 1 reply; 6+ messages in thread
From: Ben Skeggs @ 2013-02-05 20:52 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Marcin Slusarz, Peter Hurley, nouveau, Linux Kernel, dri-devel,
Daniel J Blueman, Ben Skeggs, Arend van Spriel
On Mon, Feb 04, 2013 at 10:59:28PM +0100, Maarten Lankhorst wrote:
> Op 04-02-13 22:30, Marcin Slusarz schreef:
> > 1) Lockdep thinks all nouveau subdevs belong to the same class and can be
> > locked in arbitrary order, which is not true (at least in general case).
> > Tell it to distinguish subdevs by (o)class type.
> Apart from this specific case, is there any other reason why we require being able to nest 2 subdev locks?
I think I tend to prefer Marcin's fix for this actually. The subdev's
are completely separate classes of objects and as interaction between
them increases (PM will be very much like this), we may very well
require holding multiple subdev mutexes at once.
Ben.
>
> Add a NVOBJ_FLAG_CREATE_EXCLUSIVE flag to nouveau_engctx_create and return -EBUSY if there is any existing object. Problem solved, and lockdep is still generic.
>
> > 2) DRM client can be locked under user client lock - tell lockdep to put DRM
> > client lock in a separate class.
> Can you copy paste a typical lockdep splat for this? Also this should be a separate patch. :-)
>
>
> > Reported-by: Arend van Spriel <arend@broadcom.com>
> > Reported-by: Peter Hurley <peter@hurleysoftware.com>
> > Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> > Reported-by: Daniel J Blueman <daniel@quora.org>
> > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > Cc: stable@vger.kernel.org [3.7, but needs s/const ofuncs/ofuncs/ to build]
> > ---
> > Lightly tested, only on NV4B and NVC1.
> > ---
> > drivers/gpu/drm/nouveau/core/core/subdev.c | 2 +-
> > drivers/gpu/drm/nouveau/core/include/core/object.h | 7 +++++--
> > drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +++
> > 3 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/core/core/subdev.c b/drivers/gpu/drm/nouveau/core/core/subdev.c
> > index f74c30a..48f0637 100644
> > --- a/drivers/gpu/drm/nouveau/core/core/subdev.c
> > +++ b/drivers/gpu/drm/nouveau/core/core/subdev.c
> > @@ -99,7 +99,7 @@ nouveau_subdev_create_(struct nouveau_object *parent,
> > if (ret)
> > return ret;
> >
> > - mutex_init(&subdev->mutex);
> > + __mutex_init(&subdev->mutex, subname, &oclass->lock_class_key);
> > subdev->name = subname;
> >
> > if (parent) {
> > diff --git a/drivers/gpu/drm/nouveau/core/include/core/object.h b/drivers/gpu/drm/nouveau/core/include/core/object.h
> > index 6a90267..62e68ba 100644
> > --- a/drivers/gpu/drm/nouveau/core/include/core/object.h
> > +++ b/drivers/gpu/drm/nouveau/core/include/core/object.h
> > @@ -50,10 +50,13 @@ int nouveau_object_fini(struct nouveau_object *, bool suspend);
> >
> > extern struct nouveau_ofuncs nouveau_object_ofuncs;
> >
> > +/* Don't allocate dynamically, because lockdep needs lock_class_keys to be in
> > + * ".data". */
> > struct nouveau_oclass {
> > u32 handle;
> > - struct nouveau_ofuncs *ofuncs;
> > - struct nouveau_omthds *omthds;
> > + struct nouveau_ofuncs * const ofuncs;
> > + struct nouveau_omthds * const omthds;
> > + struct lock_class_key lock_class_key;
> > };
> >
> > #define nv_oclass(o) nv_object(o)->oclass
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index ef1ad21..bc00587 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -245,6 +245,8 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > return 0;
> > }
> >
> > +static struct lock_class_key drm_client_lock_class_key;
> > +
> > static int
> > nouveau_drm_load(struct drm_device *dev, unsigned long flags)
> > {
> > @@ -256,6 +258,7 @@ nouveau_drm_load(struct drm_device *dev, unsigned long flags)
> > ret = nouveau_cli_create(pdev, "DRM", sizeof(*drm), (void**)&drm);
> > if (ret)
> > return ret;
> > + lockdep_set_class(&drm->client.mutex, &drm_client_lock_class_key);
> >
> > dev->dev_private = drm;
> > drm->dev = dev;
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/nouveau: add lockdep annotations
2013-02-05 20:52 ` Ben Skeggs
@ 2013-02-07 16:19 ` Maarten Lankhorst
0 siblings, 0 replies; 6+ messages in thread
From: Maarten Lankhorst @ 2013-02-07 16:19 UTC (permalink / raw)
To: Ben Skeggs
Cc: Marcin Slusarz, Peter Hurley, nouveau, Linux Kernel, dri-devel,
Daniel J Blueman, Ben Skeggs, Arend van Spriel
Hey,
Op 05-02-13 21:52, Ben Skeggs schreef:
> On Mon, Feb 04, 2013 at 10:59:28PM +0100, Maarten Lankhorst wrote:
>> Op 04-02-13 22:30, Marcin Slusarz schreef:
>>> 1) Lockdep thinks all nouveau subdevs belong to the same class and can be
>>> locked in arbitrary order, which is not true (at least in general case).
>>> Tell it to distinguish subdevs by (o)class type.
>> Apart from this specific case, is there any other reason why we require being able to nest 2 subdev locks?
> I think I tend to prefer Marcin's fix for this actually. The subdev's
> are completely separate classes of objects and as interaction between
> them increases (PM will be very much like this), we may very well
> require holding multiple subdev mutexes at once.
>
> Ben.
Depends, I think for this specific example I think my cleanup is better.
For the generic case you could use nested mutexes, which will give you a
different lockdep class when you need it. It's probably better to have those
cases where you do need to nest locking annotated:
mutex_lock_nested(&mutex, SINGLE_DEPTH_NESTING);
See also Documentation/lockdep-design.txt
~Maarten
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-07 16:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-03 15:09 3.8-rc6: nouveau lockdep recursive lock acquisition Daniel J Blueman
2013-02-04 21:30 ` [PATCH] drm/nouveau: add lockdep annotations Marcin Slusarz
2013-02-04 21:59 ` Maarten Lankhorst
2013-02-04 23:24 ` Peter Hurley
2013-02-05 20:52 ` Ben Skeggs
2013-02-07 16:19 ` Maarten Lankhorst
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).