* [PATCH] sysfs: Disable lockdep for driver bind/unbind files
@ 2018-12-18 20:14 ` Daniel Vetter
0 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2018-12-18 20:14 UTC (permalink / raw)
To: DRI Development
Cc: Arend van Spriel, Heikki Krogerus, Geert Uytterhoeven,
Greg Kroah-Hartman, Intel Graphics Development,
Rafael J. Wysocki, LKML, Vivek Gautam, Daniel Vetter,
Joe Perches, Daniel Vetter, Andy Shevchenko, Bartosz Golaszewski
This is the much more correct fix for my earlier attempt at:
https://lkml.org/lkml/2018/12/10/118
Short recap:
- There's not actually a locking issue, it's just lockdep being a bit
too eager to complain about a possible deadlock.
- Contrary to what I claimed the real problem is recursion on
kn->count. Greg pointed me at sysfs_break_active_protection(), used
by the scsi subsystem to allow a sysfs file to unbind itself. That
would be a real deadlock, which isn't what's happening here. Also,
breaking the active protection means we'd need to manually handle
all the lifetime fun.
- With Rafael we discussed the task_work approach, which kinda works,
but has two downsides: It's a functional change for a lockdep
annotation issue, and it won't work for the bind file (which needs
to get the errno from the driver load function back to userspace).
- Greg also asked why this never showed up: To hit this you need to
unregister a 2nd driver from the unload code of your first driver. I
guess only gpus do that. The bug has always been there, but only
with a recent patch series did we add more locks so that lockdep
built a chain from unbinding the snd-hda driver to the
acpi_video_unregister call.
Full lockdep splat:
[12301.898799] ============================================
[12301.898805] WARNING: possible recursive locking detected
[12301.898811] 4.20.0-rc7+ #84 Not tainted
[12301.898815] --------------------------------------------
[12301.898821] bash/5297 is trying to acquire lock:
[12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
[12301.898841] but task is already holding lock:
[12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
[12301.898856] other info that might help us debug this:
[12301.898862] Possible unsafe locking scenario:
[12301.898867] CPU0
[12301.898870] ----
[12301.898874] lock(kn->count#39);
[12301.898879] lock(kn->count#39);
[12301.898883] *** DEADLOCK ***
[12301.898891] May be due to missing lock nesting notation
[12301.898899] 5 locks held by bash/5297:
[12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
[12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
[12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
[12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
[12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
[12301.898960] stack backtrace:
[12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
[12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
[12301.898982] Call Trace:
[12301.898989] dump_stack+0x67/0x9b
[12301.898997] __lock_acquire+0x6ad/0x1410
[12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
[12301.899010] ? find_held_lock+0x2d/0x90
[12301.899017] ? mutex_spin_on_owner+0xe4/0x150
[12301.899023] ? find_held_lock+0x2d/0x90
[12301.899030] ? lock_acquire+0x90/0x180
[12301.899036] lock_acquire+0x90/0x180
[12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
[12301.899049] __kernfs_remove+0x296/0x310
[12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
[12301.899060] ? kernfs_name_hash+0xd/0x80
[12301.899066] ? kernfs_find_ns+0x6c/0x100
[12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
[12301.899080] bus_remove_driver+0x92/0xa0
[12301.899085] acpi_video_unregister+0x24/0x40
[12301.899127] i915_driver_unload+0x42/0x130 [i915]
[12301.899160] i915_pci_remove+0x19/0x30 [i915]
[12301.899169] pci_device_remove+0x36/0xb0
[12301.899176] device_release_driver_internal+0x185/0x240
[12301.899183] unbind_store+0xaf/0x180
[12301.899189] kernfs_fop_write+0x104/0x190
[12301.899195] __vfs_write+0x31/0x180
[12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
[12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
[12301.899216] ? __sb_start_write+0x13c/0x1a0
[12301.899221] ? vfs_write+0x17f/0x1b0
[12301.899227] vfs_write+0xb9/0x1b0
[12301.899233] ksys_write+0x50/0xc0
[12301.899239] do_syscall_64+0x4b/0x180
[12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[12301.899253] RIP: 0033:0x7f452ac7f7a4
[12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
[12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
[12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
[12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
[12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
[12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
Locking around I've noticed that usb and i2c already handle similar
recursion problems, where a sysfs file can unbind the same type of
sysfs somewhere else in the hierarchy. Relevant commits are:
commit 356c05d58af05d582e634b54b40050c73609617b
Author: Alan Stern <stern@rowland.harvard.edu>
Date: Mon May 14 13:30:03 2012 -0400
sysfs: get rid of some lockdep false positives
commit e9b526fe704812364bca07edd15eadeba163ebfb
Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
Date: Fri May 17 14:56:35 2013 +0200
i2c: suppress lockdep warning on delete_device
Implement the same trick for driver bind/unbind.
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Arend van Spriel <aspriel@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/base/bus.c | 4 ++--
include/linux/device.h | 4 ++++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 8bfd27ec73d6..5d2411b848cd 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -195,7 +195,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
bus_put(bus);
return err;
}
-static DRIVER_ATTR_WO(unbind);
+static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
/*
* Manually attach a device to a driver.
@@ -231,7 +231,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
bus_put(bus);
return err;
}
-static DRIVER_ATTR_WO(bind);
+static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
{
diff --git a/include/linux/device.h b/include/linux/device.h
index 1b25c7a43f4c..d2cb1a6c5d95 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -326,6 +326,10 @@ struct driver_attribute {
struct driver_attribute driver_attr_##_name = __ATTR_RO(_name)
#define DRIVER_ATTR_WO(_name) \
struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
+#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
+ struct driver_attribute driver_attr_##_name = \
+ __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
+
extern int __must_check driver_create_file(struct device_driver *driver,
const struct driver_attribute *attr);
--
2.20.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 29+ messages in thread* ✗ Fi.CI.CHECKPATCH: warning for sysfs: Disable lockdep for driver bind/unbind files
2018-12-18 20:14 ` Daniel Vetter
(?)
@ 2018-12-18 20:23 ` Patchwork
-1 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2018-12-18 20:23 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: sysfs: Disable lockdep for driver bind/unbind files
URL : https://patchwork.freedesktop.org/series/54238/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
53cd70987b66 sysfs: Disable lockdep for driver bind/unbind files
-:106: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 356c05d58af0 ("sysfs: get rid of some lockdep false positives")'
#106:
commit 356c05d58af05d582e634b54b40050c73609617b
-:112: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit e9b526fe7048 ("i2c: suppress lockdep warning on delete_device")'
#112:
commit e9b526fe704812364bca07edd15eadeba163ebfb
-:141: WARNING:SYMBOLIC_PERMS: Symbolic permissions 'S_IWUSR' are not preferred. Consider using octal permissions '0200'.
#141: FILE: drivers/base/bus.c:198:
+static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
-:150: WARNING:SYMBOLIC_PERMS: Symbolic permissions 'S_IWUSR' are not preferred. Consider using octal permissions '0200'.
#150: FILE: drivers/base/bus.c:234:
+static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
-:168: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'
total: 2 errors, 3 warnings, 0 checks, 26 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread* ✓ Fi.CI.BAT: success for sysfs: Disable lockdep for driver bind/unbind files
2018-12-18 20:14 ` Daniel Vetter
(?)
(?)
@ 2018-12-18 21:12 ` Patchwork
-1 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2018-12-18 21:12 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: sysfs: Disable lockdep for driver bind/unbind files
URL : https://patchwork.freedesktop.org/series/54238/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5331 -> Patchwork_11118
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/54238/revisions/1/mbox/
Known issues
------------
Here are the changes found in Patchwork_11118 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@amdgpu/amd_basic@userptr:
- fi-kbl-8809g: PASS -> DMESG-WARN [fdo#108965]
* igt@i915_selftest@live_hangcheck:
- fi-icl-u2: PASS -> INCOMPLETE [fdo#108315]
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
- fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362]
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
- fi-blb-e6850: PASS -> INCOMPLETE [fdo#107718]
#### Possible fixes ####
* igt@i915_module_load@reload-with-fault-injection:
- fi-byt-clapper: WARN [fdo#108688] -> PASS
* igt@i915_selftest@live_hangcheck:
- fi-skl-guc: DMESG-FAIL [fdo#108593] -> PASS
- fi-kbl-7560u: INCOMPLETE [fdo#108044] -> PASS
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7500u: FAIL [fdo#108767] -> PASS
* igt@kms_flip@basic-flip-vs-dpms:
- fi-skl-6700hq: DMESG-WARN [fdo#105998] -> PASS
* igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
- fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS
* igt@pm_rpm@module-reload:
- fi-byt-clapper: FAIL [fdo#108675] -> PASS
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#105998]: https://bugs.freedesktop.org/show_bug.cgi?id=105998
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#108044]: https://bugs.freedesktop.org/show_bug.cgi?id=108044
[fdo#108315]: https://bugs.freedesktop.org/show_bug.cgi?id=108315
[fdo#108593]: https://bugs.freedesktop.org/show_bug.cgi?id=108593
[fdo#108675]: https://bugs.freedesktop.org/show_bug.cgi?id=108675
[fdo#108688]: https://bugs.freedesktop.org/show_bug.cgi?id=108688
[fdo#108767]: https://bugs.freedesktop.org/show_bug.cgi?id=108767
[fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
Participating hosts (50 -> 43)
------------------------------
Missing (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-glk-j4005
Build changes
-------------
* Linux: CI_DRM_5331 -> Patchwork_11118
CI_DRM_5331: 9373de80d37b523811cea7cfbf4de7b996096bcd @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4749: 270da20849db4d170db09673c6b67712c90ec9fe @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_11118: 53cd70987b66d34317e8791e77b0b1360bcb11d7 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
53cd70987b66 sysfs: Disable lockdep for driver bind/unbind files
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11118/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-18 20:14 ` Daniel Vetter
` (2 preceding siblings ...)
(?)
@ 2018-12-18 21:40 ` Rafael J. Wysocki
-1 siblings, 0 replies; 29+ messages in thread
From: Rafael J. Wysocki @ 2018-12-18 21:40 UTC (permalink / raw)
To: Daniel Vetter
Cc: dri-devel, intel-gfx, Linux Kernel Mailing List,
Rafael J. Wysocki, Greg Kroah-Hartman, ramalingam.c, aspriel,
Andy Shevchenko, Geert Uytterhoeven, brgl, Heikki Krogerus,
Vivek Gautam, Joe Perches, Daniel Vetter
On Tue, Dec 18, 2018 at 9:14 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> This is the much more correct fix for my earlier attempt at:
>
> https://lkml.org/lkml/2018/12/10/118
>
> Short recap:
>
> - There's not actually a locking issue, it's just lockdep being a bit
> too eager to complain about a possible deadlock.
>
> - Contrary to what I claimed the real problem is recursion on
> kn->count. Greg pointed me at sysfs_break_active_protection(), used
> by the scsi subsystem to allow a sysfs file to unbind itself. That
> would be a real deadlock, which isn't what's happening here. Also,
> breaking the active protection means we'd need to manually handle
> all the lifetime fun.
>
> - With Rafael we discussed the task_work approach, which kinda works,
> but has two downsides: It's a functional change for a lockdep
> annotation issue, and it won't work for the bind file (which needs
> to get the errno from the driver load function back to userspace).
>
> - Greg also asked why this never showed up: To hit this you need to
> unregister a 2nd driver from the unload code of your first driver. I
> guess only gpus do that. The bug has always been there, but only
> with a recent patch series did we add more locks so that lockdep
> built a chain from unbinding the snd-hda driver to the
> acpi_video_unregister call.
>
> Full lockdep splat:
>
> [12301.898799] ============================================
> [12301.898805] WARNING: possible recursive locking detected
> [12301.898811] 4.20.0-rc7+ #84 Not tainted
> [12301.898815] --------------------------------------------
> [12301.898821] bash/5297 is trying to acquire lock:
> [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> [12301.898841] but task is already holding lock:
> [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> [12301.898856] other info that might help us debug this:
> [12301.898862] Possible unsafe locking scenario:
> [12301.898867] CPU0
> [12301.898870] ----
> [12301.898874] lock(kn->count#39);
> [12301.898879] lock(kn->count#39);
> [12301.898883] *** DEADLOCK ***
> [12301.898891] May be due to missing lock nesting notation
> [12301.898899] 5 locks held by bash/5297:
> [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> [12301.898960] stack backtrace:
> [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> [12301.898982] Call Trace:
> [12301.898989] dump_stack+0x67/0x9b
> [12301.898997] __lock_acquire+0x6ad/0x1410
> [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899010] ? find_held_lock+0x2d/0x90
> [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> [12301.899023] ? find_held_lock+0x2d/0x90
> [12301.899030] ? lock_acquire+0x90/0x180
> [12301.899036] lock_acquire+0x90/0x180
> [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899049] __kernfs_remove+0x296/0x310
> [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899060] ? kernfs_name_hash+0xd/0x80
> [12301.899066] ? kernfs_find_ns+0x6c/0x100
> [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899080] bus_remove_driver+0x92/0xa0
> [12301.899085] acpi_video_unregister+0x24/0x40
> [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> [12301.899169] pci_device_remove+0x36/0xb0
> [12301.899176] device_release_driver_internal+0x185/0x240
> [12301.899183] unbind_store+0xaf/0x180
> [12301.899189] kernfs_fop_write+0x104/0x190
> [12301.899195] __vfs_write+0x31/0x180
> [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> [12301.899216] ? __sb_start_write+0x13c/0x1a0
> [12301.899221] ? vfs_write+0x17f/0x1b0
> [12301.899227] vfs_write+0xb9/0x1b0
> [12301.899233] ksys_write+0x50/0xc0
> [12301.899239] do_syscall_64+0x4b/0x180
> [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [12301.899253] RIP: 0033:0x7f452ac7f7a4
> [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
>
> Locking around I've noticed that usb and i2c already handle similar
"Looking" I suppose? ;-)
Apart from this LGTM, so
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> recursion problems, where a sysfs file can unbind the same type of
> sysfs somewhere else in the hierarchy. Relevant commits are:
>
> commit 356c05d58af05d582e634b54b40050c73609617b
> Author: Alan Stern <stern@rowland.harvard.edu>
> Date: Mon May 14 13:30:03 2012 -0400
>
> sysfs: get rid of some lockdep false positives
>
> commit e9b526fe704812364bca07edd15eadeba163ebfb
> Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> Date: Fri May 17 14:56:35 2013 +0200
>
> i2c: suppress lockdep warning on delete_device
>
> Implement the same trick for driver bind/unbind.
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Cc: Arend van Spriel <aspriel@gmail.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/base/bus.c | 4 ++--
> include/linux/device.h | 4 ++++
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 8bfd27ec73d6..5d2411b848cd 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -195,7 +195,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> bus_put(bus);
> return err;
> }
> -static DRIVER_ATTR_WO(unbind);
> +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
>
> /*
> * Manually attach a device to a driver.
> @@ -231,7 +231,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> bus_put(bus);
> return err;
> }
> -static DRIVER_ATTR_WO(bind);
> +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
>
> static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1b25c7a43f4c..d2cb1a6c5d95 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -326,6 +326,10 @@ struct driver_attribute {
> struct driver_attribute driver_attr_##_name = __ATTR_RO(_name)
> #define DRIVER_ATTR_WO(_name) \
> struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> + struct driver_attribute driver_attr_##_name = \
> + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
> +
>
> extern int __must_check driver_create_file(struct device_driver *driver,
> const struct driver_attribute *attr);
> --
> 2.20.0.rc1
>
^ permalink raw reply [flat|nested] 29+ messages in thread* ✓ Fi.CI.IGT: success for sysfs: Disable lockdep for driver bind/unbind files
2018-12-18 20:14 ` Daniel Vetter
` (3 preceding siblings ...)
(?)
@ 2018-12-19 3:30 ` Patchwork
-1 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2018-12-19 3:30 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: sysfs: Disable lockdep for driver bind/unbind files
URL : https://patchwork.freedesktop.org/series/54238/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5331_full -> Patchwork_11118_full
====================================================
Summary
-------
**WARNING**
Minor unknown changes coming with Patchwork_11118_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_11118_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_11118_full:
### IGT changes ###
#### Warnings ####
* igt@perf_pmu@rc6:
- shard-kbl: PASS -> SKIP
* igt@pm_rc6_residency@rc6-accuracy:
- shard-snb: PASS -> SKIP
Known issues
------------
Here are the changes found in Patchwork_11118_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_schedule@pi-ringfull-render:
- shard-skl: NOTRUN -> FAIL [fdo#103158]
* igt@kms_busy@extended-modeset-hang-newfb-render-a:
- shard-iclb: NOTRUN -> DMESG-WARN [fdo#107956]
* igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c:
- shard-skl: NOTRUN -> DMESG-WARN [fdo#107956] +1
* igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
- shard-glk: NOTRUN -> DMESG-WARN [fdo#107956]
* igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
- shard-iclb: NOTRUN -> FAIL [fdo#107725]
* igt@kms_color@pipe-c-legacy-gamma:
- shard-apl: PASS -> FAIL [fdo#104782]
* igt@kms_cursor_crc@cursor-128x128-random:
- shard-apl: PASS -> FAIL [fdo#103232] +4
* igt@kms_cursor_crc@cursor-64x64-sliding:
- shard-iclb: NOTRUN -> FAIL [fdo#103232] +2
* igt@kms_cursor_crc@cursor-64x64-suspend:
- shard-skl: NOTRUN -> INCOMPLETE [fdo#104108]
* igt@kms_flip@plain-flip-fb-recreate-interruptible:
- shard-skl: PASS -> FAIL [fdo#100368]
* igt@kms_frontbuffer_tracking@fbcpsr-stridechange:
- shard-iclb: NOTRUN -> FAIL [fdo#105683]
* igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
- shard-skl: NOTRUN -> FAIL [fdo#108145]
* igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
- shard-apl: PASS -> FAIL [fdo#103166] +2
- shard-iclb: PASS -> FAIL [fdo#103166] +1
* igt@kms_sysfs_edid_timing:
- shard-iclb: PASS -> FAIL [fdo#100047]
- shard-skl: NOTRUN -> FAIL [fdo#100047]
* igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
- shard-kbl: PASS -> INCOMPLETE [fdo#103665]
* igt@perf@blocking:
- shard-hsw: PASS -> FAIL [fdo#102252]
* igt@pm_rpm@debugfs-forcewake-user:
- shard-iclb: PASS -> INCOMPLETE [fdo#107713] / [fdo#108840]
* igt@pm_rpm@modeset-lpsp-stress-no-wait:
- shard-iclb: PASS -> DMESG-WARN [fdo#108654]
* igt@pm_rpm@universal-planes:
- shard-skl: PASS -> INCOMPLETE [fdo#107807]
- shard-iclb: PASS -> DMESG-WARN [fdo#108654] / [fdo#108756]
#### Possible fixes ####
* igt@gem_ppgtt@blt-vs-render-ctx0:
- shard-skl: TIMEOUT [fdo#108039] -> PASS
* igt@gem_workarounds@suspend-resume-context:
- shard-iclb: INCOMPLETE [fdo#107713] -> PASS
* igt@kms_cursor_crc@cursor-128x128-dpms:
- shard-apl: FAIL [fdo#103232] -> PASS
* igt@kms_cursor_crc@cursor-256x256-suspend:
- shard-skl: INCOMPLETE [fdo#104108] / [fdo#107773] -> PASS
* igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
- shard-skl: FAIL [fdo#102670] / [fdo#106081] -> PASS
* igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-untiled:
- shard-iclb: WARN [fdo#108336] -> PASS +2
* igt@kms_flip@flip-vs-expired-vblank-interruptible:
- shard-skl: FAIL [fdo#105363] -> PASS
* igt@kms_flip@modeset-vs-vblank-race-interruptible:
- shard-glk: FAIL [fdo#103060] -> PASS
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
- shard-iclb: FAIL [fdo#103167] -> PASS +6
* igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-pwrite:
- shard-iclb: DMESG-FAIL [fdo#107724] -> PASS +4
* igt@kms_rotation_crc@multiplane-rotation-cropping-top:
- shard-kbl: DMESG-WARN [fdo#105604] -> PASS
- shard-glk: DMESG-FAIL [fdo#105763] / [fdo#106538] -> PASS
* igt@kms_rotation_crc@primary-rotation-180:
- shard-iclb: DMESG-WARN [fdo#107724] / [fdo#108336] -> PASS +10
* igt@pm_backlight@basic-brightness:
- shard-iclb: DMESG-WARN [fdo#107724] -> PASS +22
* igt@pm_rpm@legacy-planes:
- shard-iclb: DMESG-WARN [fdo#108654] -> PASS
* igt@pm_rpm@modeset-non-lpsp-stress-no-wait:
- shard-iclb: INCOMPLETE [fdo#108840] -> SKIP
* igt@pm_rpm@modeset-pc8-residency-stress:
- shard-skl: INCOMPLETE [fdo#107807] -> SKIP
#### Warnings ####
* igt@kms_ccs@pipe-a-crc-primary-basic:
- shard-iclb: DMESG-WARN [fdo#107724] / [fdo#108336] -> FAIL [fdo#107725]
* igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
- shard-iclb: DMESG-WARN [fdo#107724] / [fdo#108336] -> FAIL [fdo#103166] +1
[fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
[fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
[fdo#102252]: https://bugs.freedesktop.org/show_bug.cgi?id=102252
[fdo#102670]: https://bugs.freedesktop.org/show_bug.cgi?id=102670
[fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
[fdo#103158]: https://bugs.freedesktop.org/show_bug.cgi?id=103158
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
[fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#105604]: https://bugs.freedesktop.org/show_bug.cgi?id=105604
[fdo#105683]: https://bugs.freedesktop.org/show_bug.cgi?id=105683
[fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
[fdo#106081]: https://bugs.freedesktop.org/show_bug.cgi?id=106081
[fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
[fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725
[fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
[fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
[fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
[fdo#108039]: https://bugs.freedesktop.org/show_bug.cgi?id=108039
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108336]: https://bugs.freedesktop.org/show_bug.cgi?id=108336
[fdo#108654]: https://bugs.freedesktop.org/show_bug.cgi?id=108654
[fdo#108756]: https://bugs.freedesktop.org/show_bug.cgi?id=108756
[fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
Participating hosts (7 -> 7)
------------------------------
No changes in participating hosts
Build changes
-------------
* Linux: CI_DRM_5331 -> Patchwork_11118
CI_DRM_5331: 9373de80d37b523811cea7cfbf4de7b996096bcd @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4749: 270da20849db4d170db09673c6b67712c90ec9fe @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_11118: 53cd70987b66d34317e8791e77b0b1360bcb11d7 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11118/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-18 20:14 ` Daniel Vetter
` (4 preceding siblings ...)
(?)
@ 2018-12-19 7:01 ` Greg Kroah-Hartman
2018-12-19 9:24 ` Daniel Vetter
2018-12-19 9:24 ` Rafael J. Wysocki
-1 siblings, 2 replies; 29+ messages in thread
From: Greg Kroah-Hartman @ 2018-12-19 7:01 UTC (permalink / raw)
To: Daniel Vetter
Cc: DRI Development, Intel Graphics Development, LKML,
Rafael J. Wysocki, Ramalingam C, Arend van Spriel,
Andy Shevchenko, Geert Uytterhoeven, Bartosz Golaszewski,
Heikki Krogerus, Vivek Gautam, Joe Perches, Daniel Vetter
On Tue, Dec 18, 2018 at 09:14:43PM +0100, Daniel Vetter wrote:
> This is the much more correct fix for my earlier attempt at:
>
> https://lkml.org/lkml/2018/12/10/118
>
> Short recap:
>
> - There's not actually a locking issue, it's just lockdep being a bit
> too eager to complain about a possible deadlock.
>
> - Contrary to what I claimed the real problem is recursion on
> kn->count. Greg pointed me at sysfs_break_active_protection(), used
> by the scsi subsystem to allow a sysfs file to unbind itself. That
> would be a real deadlock, which isn't what's happening here. Also,
> breaking the active protection means we'd need to manually handle
> all the lifetime fun.
>
> - With Rafael we discussed the task_work approach, which kinda works,
> but has two downsides: It's a functional change for a lockdep
> annotation issue, and it won't work for the bind file (which needs
> to get the errno from the driver load function back to userspace).
>
> - Greg also asked why this never showed up: To hit this you need to
> unregister a 2nd driver from the unload code of your first driver. I
> guess only gpus do that. The bug has always been there, but only
> with a recent patch series did we add more locks so that lockdep
> built a chain from unbinding the snd-hda driver to the
> acpi_video_unregister call.
>
> Full lockdep splat:
>
> [12301.898799] ============================================
> [12301.898805] WARNING: possible recursive locking detected
> [12301.898811] 4.20.0-rc7+ #84 Not tainted
> [12301.898815] --------------------------------------------
> [12301.898821] bash/5297 is trying to acquire lock:
> [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> [12301.898841] but task is already holding lock:
> [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> [12301.898856] other info that might help us debug this:
> [12301.898862] Possible unsafe locking scenario:
> [12301.898867] CPU0
> [12301.898870] ----
> [12301.898874] lock(kn->count#39);
> [12301.898879] lock(kn->count#39);
> [12301.898883] *** DEADLOCK ***
> [12301.898891] May be due to missing lock nesting notation
> [12301.898899] 5 locks held by bash/5297:
> [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> [12301.898960] stack backtrace:
> [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> [12301.898982] Call Trace:
> [12301.898989] dump_stack+0x67/0x9b
> [12301.898997] __lock_acquire+0x6ad/0x1410
> [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899010] ? find_held_lock+0x2d/0x90
> [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> [12301.899023] ? find_held_lock+0x2d/0x90
> [12301.899030] ? lock_acquire+0x90/0x180
> [12301.899036] lock_acquire+0x90/0x180
> [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899049] __kernfs_remove+0x296/0x310
> [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899060] ? kernfs_name_hash+0xd/0x80
> [12301.899066] ? kernfs_find_ns+0x6c/0x100
> [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899080] bus_remove_driver+0x92/0xa0
> [12301.899085] acpi_video_unregister+0x24/0x40
> [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> [12301.899169] pci_device_remove+0x36/0xb0
> [12301.899176] device_release_driver_internal+0x185/0x240
> [12301.899183] unbind_store+0xaf/0x180
> [12301.899189] kernfs_fop_write+0x104/0x190
> [12301.899195] __vfs_write+0x31/0x180
> [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> [12301.899216] ? __sb_start_write+0x13c/0x1a0
> [12301.899221] ? vfs_write+0x17f/0x1b0
> [12301.899227] vfs_write+0xb9/0x1b0
> [12301.899233] ksys_write+0x50/0xc0
> [12301.899239] do_syscall_64+0x4b/0x180
> [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [12301.899253] RIP: 0033:0x7f452ac7f7a4
> [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
>
> Locking around I've noticed that usb and i2c already handle similar
> recursion problems, where a sysfs file can unbind the same type of
> sysfs somewhere else in the hierarchy. Relevant commits are:
>
> commit 356c05d58af05d582e634b54b40050c73609617b
> Author: Alan Stern <stern@rowland.harvard.edu>
> Date: Mon May 14 13:30:03 2012 -0400
>
> sysfs: get rid of some lockdep false positives
>
> commit e9b526fe704812364bca07edd15eadeba163ebfb
> Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> Date: Fri May 17 14:56:35 2013 +0200
>
> i2c: suppress lockdep warning on delete_device
>
> Implement the same trick for driver bind/unbind.
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Cc: Arend van Spriel <aspriel@gmail.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/base/bus.c | 4 ++--
> include/linux/device.h | 4 ++++
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 8bfd27ec73d6..5d2411b848cd 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -195,7 +195,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> bus_put(bus);
> return err;
> }
> -static DRIVER_ATTR_WO(unbind);
> +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
>
> /*
> * Manually attach a device to a driver.
> @@ -231,7 +231,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> bus_put(bus);
> return err;
> }
> -static DRIVER_ATTR_WO(bind);
> +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
>
> static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1b25c7a43f4c..d2cb1a6c5d95 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -326,6 +326,10 @@ struct driver_attribute {
> struct driver_attribute driver_attr_##_name = __ATTR_RO(_name)
> #define DRIVER_ATTR_WO(_name) \
> struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> + struct driver_attribute driver_attr_##_name = \
> + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
> +
I don't want to give driver writers the ability to shoot themselves in
the foot, can you just put this in the bus.c file itself so that no one
else will abuse this and "open code" the unbind/bind attributes instead
of using a #define for it?
Also I've been trying to get rid of the "specify the mode" macros, so
that everyone uses the RO, WO, and RW versions, so adding a generic one
here is not going to help with that effort :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-19 7:01 ` [PATCH] " Greg Kroah-Hartman
@ 2018-12-19 9:24 ` Daniel Vetter
2018-12-19 9:24 ` Rafael J. Wysocki
1 sibling, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2018-12-19 9:24 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: DRI Development, Intel Graphics Development, LKML,
Rafael J. Wysocki, Ramalingam C, Arend van Spriel,
Andy Shevchenko, Geert Uytterhoeven, Bartosz Golaszewski,
Heikki Krogerus, Vivek Gautam, Joe Perches, Daniel Vetter
On Wed, Dec 19, 2018 at 8:01 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Dec 18, 2018 at 09:14:43PM +0100, Daniel Vetter wrote:
> > This is the much more correct fix for my earlier attempt at:
> >
> > https://lkml.org/lkml/2018/12/10/118
> >
> > Short recap:
> >
> > - There's not actually a locking issue, it's just lockdep being a bit
> > too eager to complain about a possible deadlock.
> >
> > - Contrary to what I claimed the real problem is recursion on
> > kn->count. Greg pointed me at sysfs_break_active_protection(), used
> > by the scsi subsystem to allow a sysfs file to unbind itself. That
> > would be a real deadlock, which isn't what's happening here. Also,
> > breaking the active protection means we'd need to manually handle
> > all the lifetime fun.
> >
> > - With Rafael we discussed the task_work approach, which kinda works,
> > but has two downsides: It's a functional change for a lockdep
> > annotation issue, and it won't work for the bind file (which needs
> > to get the errno from the driver load function back to userspace).
> >
> > - Greg also asked why this never showed up: To hit this you need to
> > unregister a 2nd driver from the unload code of your first driver. I
> > guess only gpus do that. The bug has always been there, but only
> > with a recent patch series did we add more locks so that lockdep
> > built a chain from unbinding the snd-hda driver to the
> > acpi_video_unregister call.
> >
> > Full lockdep splat:
> >
> > [12301.898799] ============================================
> > [12301.898805] WARNING: possible recursive locking detected
> > [12301.898811] 4.20.0-rc7+ #84 Not tainted
> > [12301.898815] --------------------------------------------
> > [12301.898821] bash/5297 is trying to acquire lock:
> > [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.898841] but task is already holding lock:
> > [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > [12301.898856] other info that might help us debug this:
> > [12301.898862] Possible unsafe locking scenario:
> > [12301.898867] CPU0
> > [12301.898870] ----
> > [12301.898874] lock(kn->count#39);
> > [12301.898879] lock(kn->count#39);
> > [12301.898883] *** DEADLOCK ***
> > [12301.898891] May be due to missing lock nesting notation
> > [12301.898899] 5 locks held by bash/5297:
> > [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> > [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> > [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> > [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> > [12301.898960] stack backtrace:
> > [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> > [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> > [12301.898982] Call Trace:
> > [12301.898989] dump_stack+0x67/0x9b
> > [12301.898997] __lock_acquire+0x6ad/0x1410
> > [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899010] ? find_held_lock+0x2d/0x90
> > [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> > [12301.899023] ? find_held_lock+0x2d/0x90
> > [12301.899030] ? lock_acquire+0x90/0x180
> > [12301.899036] lock_acquire+0x90/0x180
> > [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899049] __kernfs_remove+0x296/0x310
> > [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899060] ? kernfs_name_hash+0xd/0x80
> > [12301.899066] ? kernfs_find_ns+0x6c/0x100
> > [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899080] bus_remove_driver+0x92/0xa0
> > [12301.899085] acpi_video_unregister+0x24/0x40
> > [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> > [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> > [12301.899169] pci_device_remove+0x36/0xb0
> > [12301.899176] device_release_driver_internal+0x185/0x240
> > [12301.899183] unbind_store+0xaf/0x180
> > [12301.899189] kernfs_fop_write+0x104/0x190
> > [12301.899195] __vfs_write+0x31/0x180
> > [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> > [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> > [12301.899216] ? __sb_start_write+0x13c/0x1a0
> > [12301.899221] ? vfs_write+0x17f/0x1b0
> > [12301.899227] vfs_write+0xb9/0x1b0
> > [12301.899233] ksys_write+0x50/0xc0
> > [12301.899239] do_syscall_64+0x4b/0x180
> > [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [12301.899253] RIP: 0033:0x7f452ac7f7a4
> > [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> > [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> > [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> > [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> > [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> > [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
> >
> > Locking around I've noticed that usb and i2c already handle similar
> > recursion problems, where a sysfs file can unbind the same type of
> > sysfs somewhere else in the hierarchy. Relevant commits are:
> >
> > commit 356c05d58af05d582e634b54b40050c73609617b
> > Author: Alan Stern <stern@rowland.harvard.edu>
> > Date: Mon May 14 13:30:03 2012 -0400
> >
> > sysfs: get rid of some lockdep false positives
> >
> > commit e9b526fe704812364bca07edd15eadeba163ebfb
> > Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> > Date: Fri May 17 14:56:35 2013 +0200
> >
> > i2c: suppress lockdep warning on delete_device
> >
> > Implement the same trick for driver bind/unbind.
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Cc: Arend van Spriel <aspriel@gmail.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> > Cc: Joe Perches <joe@perches.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > drivers/base/bus.c | 4 ++--
> > include/linux/device.h | 4 ++++
> > 2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index 8bfd27ec73d6..5d2411b848cd 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -195,7 +195,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> > bus_put(bus);
> > return err;
> > }
> > -static DRIVER_ATTR_WO(unbind);
> > +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
> >
> > /*
> > * Manually attach a device to a driver.
> > @@ -231,7 +231,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> > bus_put(bus);
> > return err;
> > }
> > -static DRIVER_ATTR_WO(bind);
> > +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
> >
> > static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> > {
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 1b25c7a43f4c..d2cb1a6c5d95 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -326,6 +326,10 @@ struct driver_attribute {
> > struct driver_attribute driver_attr_##_name = __ATTR_RO(_name)
> > #define DRIVER_ATTR_WO(_name) \
> > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> > +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> > + struct driver_attribute driver_attr_##_name = \
> > + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
> > +
>
> I don't want to give driver writers the ability to shoot themselves in
> the foot, can you just put this in the bus.c file itself so that no one
> else will abuse this and "open code" the unbind/bind attributes instead
> of using a #define for it?
So part of the reasons I've put these here is that I think we can do a
lot better. Instead of ignoring lockdep I think we can implement
nested lockdep annotations. It's going to be a bit a pain to wire that
through from sysfs->kernfs, but for all the (now 3) uses of
_IGNORE_LOCKDEP it should work if the add/remove_files functions use
mutex_lock_nested. I think then there's not really a problem with
exposing this. Aside from it's already exposed, but only for devices,
as DEVICE_ATTR_IGNORE_LOCKDEP.
> Also I've been trying to get rid of the "specify the mode" macros, so
> that everyone uses the RO, WO, and RW versions, so adding a generic one
> here is not going to help with that effort :)
Didn't know that, I went with consistency with
DEVICE_ATTR_IGNORE_LOCKDEP. Since I haven't really started typing the
follow-up series yet, I can use that one to convert over to
_ATTR_NESTED_RO/RW/WO.
Anyway, just wanted to explain why I picked these, happy to bikeshed
to taste if you insist.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
@ 2018-12-19 9:24 ` Daniel Vetter
0 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2018-12-19 9:24 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Arend van Spriel, Heikki Krogerus, Geert Uytterhoeven,
Rafael J. Wysocki, Intel Graphics Development, LKML,
DRI Development, Vivek Gautam, Joe Perches, Daniel Vetter,
Andy Shevchenko, Bartosz Golaszewski
On Wed, Dec 19, 2018 at 8:01 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Dec 18, 2018 at 09:14:43PM +0100, Daniel Vetter wrote:
> > This is the much more correct fix for my earlier attempt at:
> >
> > https://lkml.org/lkml/2018/12/10/118
> >
> > Short recap:
> >
> > - There's not actually a locking issue, it's just lockdep being a bit
> > too eager to complain about a possible deadlock.
> >
> > - Contrary to what I claimed the real problem is recursion on
> > kn->count. Greg pointed me at sysfs_break_active_protection(), used
> > by the scsi subsystem to allow a sysfs file to unbind itself. That
> > would be a real deadlock, which isn't what's happening here. Also,
> > breaking the active protection means we'd need to manually handle
> > all the lifetime fun.
> >
> > - With Rafael we discussed the task_work approach, which kinda works,
> > but has two downsides: It's a functional change for a lockdep
> > annotation issue, and it won't work for the bind file (which needs
> > to get the errno from the driver load function back to userspace).
> >
> > - Greg also asked why this never showed up: To hit this you need to
> > unregister a 2nd driver from the unload code of your first driver. I
> > guess only gpus do that. The bug has always been there, but only
> > with a recent patch series did we add more locks so that lockdep
> > built a chain from unbinding the snd-hda driver to the
> > acpi_video_unregister call.
> >
> > Full lockdep splat:
> >
> > [12301.898799] ============================================
> > [12301.898805] WARNING: possible recursive locking detected
> > [12301.898811] 4.20.0-rc7+ #84 Not tainted
> > [12301.898815] --------------------------------------------
> > [12301.898821] bash/5297 is trying to acquire lock:
> > [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.898841] but task is already holding lock:
> > [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > [12301.898856] other info that might help us debug this:
> > [12301.898862] Possible unsafe locking scenario:
> > [12301.898867] CPU0
> > [12301.898870] ----
> > [12301.898874] lock(kn->count#39);
> > [12301.898879] lock(kn->count#39);
> > [12301.898883] *** DEADLOCK ***
> > [12301.898891] May be due to missing lock nesting notation
> > [12301.898899] 5 locks held by bash/5297:
> > [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> > [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> > [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> > [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> > [12301.898960] stack backtrace:
> > [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> > [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> > [12301.898982] Call Trace:
> > [12301.898989] dump_stack+0x67/0x9b
> > [12301.898997] __lock_acquire+0x6ad/0x1410
> > [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899010] ? find_held_lock+0x2d/0x90
> > [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> > [12301.899023] ? find_held_lock+0x2d/0x90
> > [12301.899030] ? lock_acquire+0x90/0x180
> > [12301.899036] lock_acquire+0x90/0x180
> > [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899049] __kernfs_remove+0x296/0x310
> > [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899060] ? kernfs_name_hash+0xd/0x80
> > [12301.899066] ? kernfs_find_ns+0x6c/0x100
> > [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899080] bus_remove_driver+0x92/0xa0
> > [12301.899085] acpi_video_unregister+0x24/0x40
> > [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> > [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> > [12301.899169] pci_device_remove+0x36/0xb0
> > [12301.899176] device_release_driver_internal+0x185/0x240
> > [12301.899183] unbind_store+0xaf/0x180
> > [12301.899189] kernfs_fop_write+0x104/0x190
> > [12301.899195] __vfs_write+0x31/0x180
> > [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> > [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> > [12301.899216] ? __sb_start_write+0x13c/0x1a0
> > [12301.899221] ? vfs_write+0x17f/0x1b0
> > [12301.899227] vfs_write+0xb9/0x1b0
> > [12301.899233] ksys_write+0x50/0xc0
> > [12301.899239] do_syscall_64+0x4b/0x180
> > [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [12301.899253] RIP: 0033:0x7f452ac7f7a4
> > [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> > [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> > [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> > [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> > [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> > [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
> >
> > Locking around I've noticed that usb and i2c already handle similar
> > recursion problems, where a sysfs file can unbind the same type of
> > sysfs somewhere else in the hierarchy. Relevant commits are:
> >
> > commit 356c05d58af05d582e634b54b40050c73609617b
> > Author: Alan Stern <stern@rowland.harvard.edu>
> > Date: Mon May 14 13:30:03 2012 -0400
> >
> > sysfs: get rid of some lockdep false positives
> >
> > commit e9b526fe704812364bca07edd15eadeba163ebfb
> > Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> > Date: Fri May 17 14:56:35 2013 +0200
> >
> > i2c: suppress lockdep warning on delete_device
> >
> > Implement the same trick for driver bind/unbind.
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Cc: Arend van Spriel <aspriel@gmail.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> > Cc: Joe Perches <joe@perches.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > drivers/base/bus.c | 4 ++--
> > include/linux/device.h | 4 ++++
> > 2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index 8bfd27ec73d6..5d2411b848cd 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -195,7 +195,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> > bus_put(bus);
> > return err;
> > }
> > -static DRIVER_ATTR_WO(unbind);
> > +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
> >
> > /*
> > * Manually attach a device to a driver.
> > @@ -231,7 +231,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> > bus_put(bus);
> > return err;
> > }
> > -static DRIVER_ATTR_WO(bind);
> > +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
> >
> > static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> > {
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 1b25c7a43f4c..d2cb1a6c5d95 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -326,6 +326,10 @@ struct driver_attribute {
> > struct driver_attribute driver_attr_##_name = __ATTR_RO(_name)
> > #define DRIVER_ATTR_WO(_name) \
> > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> > +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> > + struct driver_attribute driver_attr_##_name = \
> > + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
> > +
>
> I don't want to give driver writers the ability to shoot themselves in
> the foot, can you just put this in the bus.c file itself so that no one
> else will abuse this and "open code" the unbind/bind attributes instead
> of using a #define for it?
So part of the reasons I've put these here is that I think we can do a
lot better. Instead of ignoring lockdep I think we can implement
nested lockdep annotations. It's going to be a bit a pain to wire that
through from sysfs->kernfs, but for all the (now 3) uses of
_IGNORE_LOCKDEP it should work if the add/remove_files functions use
mutex_lock_nested. I think then there's not really a problem with
exposing this. Aside from it's already exposed, but only for devices,
as DEVICE_ATTR_IGNORE_LOCKDEP.
> Also I've been trying to get rid of the "specify the mode" macros, so
> that everyone uses the RO, WO, and RW versions, so adding a generic one
> here is not going to help with that effort :)
Didn't know that, I went with consistency with
DEVICE_ATTR_IGNORE_LOCKDEP. Since I haven't really started typing the
follow-up series yet, I can use that one to convert over to
_ATTR_NESTED_RO/RW/WO.
Anyway, just wanted to explain why I picked these, happy to bikeshed
to taste if you insist.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-19 9:24 ` Daniel Vetter
(?)
@ 2018-12-19 9:30 ` Rafael J. Wysocki
-1 siblings, 0 replies; 29+ messages in thread
From: Rafael J. Wysocki @ 2018-12-19 9:30 UTC (permalink / raw)
To: Daniel Vetter
Cc: Greg Kroah-Hartman, dri-devel, intel-gfx,
Linux Kernel Mailing List, Rafael J. Wysocki, ramalingam.c,
aspriel, Andy Shevchenko, Geert Uytterhoeven, brgl,
Heikki Krogerus, Vivek Gautam, Joe Perches, Daniel Vetter
On Wed, Dec 19, 2018 at 10:24 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> On Wed, Dec 19, 2018 at 8:01 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Dec 18, 2018 at 09:14:43PM +0100, Daniel Vetter wrote:
> > > This is the much more correct fix for my earlier attempt at:
> > >
> > > https://lkml.org/lkml/2018/12/10/118
> > >
> > > Short recap:
> > >
> > > - There's not actually a locking issue, it's just lockdep being a bit
> > > too eager to complain about a possible deadlock.
> > >
> > > - Contrary to what I claimed the real problem is recursion on
> > > kn->count. Greg pointed me at sysfs_break_active_protection(), used
> > > by the scsi subsystem to allow a sysfs file to unbind itself. That
> > > would be a real deadlock, which isn't what's happening here. Also,
> > > breaking the active protection means we'd need to manually handle
> > > all the lifetime fun.
> > >
> > > - With Rafael we discussed the task_work approach, which kinda works,
> > > but has two downsides: It's a functional change for a lockdep
> > > annotation issue, and it won't work for the bind file (which needs
> > > to get the errno from the driver load function back to userspace).
> > >
> > > - Greg also asked why this never showed up: To hit this you need to
> > > unregister a 2nd driver from the unload code of your first driver. I
> > > guess only gpus do that. The bug has always been there, but only
> > > with a recent patch series did we add more locks so that lockdep
> > > built a chain from unbinding the snd-hda driver to the
> > > acpi_video_unregister call.
> > >
> > > Full lockdep splat:
> > >
> > > [12301.898799] ============================================
> > > [12301.898805] WARNING: possible recursive locking detected
> > > [12301.898811] 4.20.0-rc7+ #84 Not tainted
> > > [12301.898815] --------------------------------------------
> > > [12301.898821] bash/5297 is trying to acquire lock:
> > > [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.898841] but task is already holding lock:
> > > [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > > [12301.898856] other info that might help us debug this:
> > > [12301.898862] Possible unsafe locking scenario:
> > > [12301.898867] CPU0
> > > [12301.898870] ----
> > > [12301.898874] lock(kn->count#39);
> > > [12301.898879] lock(kn->count#39);
> > > [12301.898883] *** DEADLOCK ***
> > > [12301.898891] May be due to missing lock nesting notation
> > > [12301.898899] 5 locks held by bash/5297:
> > > [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> > > [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> > > [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > > [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> > > [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> > > [12301.898960] stack backtrace:
> > > [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> > > [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> > > [12301.898982] Call Trace:
> > > [12301.898989] dump_stack+0x67/0x9b
> > > [12301.898997] __lock_acquire+0x6ad/0x1410
> > > [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.899010] ? find_held_lock+0x2d/0x90
> > > [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> > > [12301.899023] ? find_held_lock+0x2d/0x90
> > > [12301.899030] ? lock_acquire+0x90/0x180
> > > [12301.899036] lock_acquire+0x90/0x180
> > > [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.899049] __kernfs_remove+0x296/0x310
> > > [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.899060] ? kernfs_name_hash+0xd/0x80
> > > [12301.899066] ? kernfs_find_ns+0x6c/0x100
> > > [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.899080] bus_remove_driver+0x92/0xa0
> > > [12301.899085] acpi_video_unregister+0x24/0x40
> > > [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> > > [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> > > [12301.899169] pci_device_remove+0x36/0xb0
> > > [12301.899176] device_release_driver_internal+0x185/0x240
> > > [12301.899183] unbind_store+0xaf/0x180
> > > [12301.899189] kernfs_fop_write+0x104/0x190
> > > [12301.899195] __vfs_write+0x31/0x180
> > > [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> > > [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> > > [12301.899216] ? __sb_start_write+0x13c/0x1a0
> > > [12301.899221] ? vfs_write+0x17f/0x1b0
> > > [12301.899227] vfs_write+0xb9/0x1b0
> > > [12301.899233] ksys_write+0x50/0xc0
> > > [12301.899239] do_syscall_64+0x4b/0x180
> > > [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > > [12301.899253] RIP: 0033:0x7f452ac7f7a4
> > > [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> > > [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > > [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> > > [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> > > [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> > > [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> > > [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
> > >
> > > Locking around I've noticed that usb and i2c already handle similar
> > > recursion problems, where a sysfs file can unbind the same type of
> > > sysfs somewhere else in the hierarchy. Relevant commits are:
> > >
> > > commit 356c05d58af05d582e634b54b40050c73609617b
> > > Author: Alan Stern <stern@rowland.harvard.edu>
> > > Date: Mon May 14 13:30:03 2012 -0400
> > >
> > > sysfs: get rid of some lockdep false positives
> > >
> > > commit e9b526fe704812364bca07edd15eadeba163ebfb
> > > Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> > > Date: Fri May 17 14:56:35 2013 +0200
> > >
> > > i2c: suppress lockdep warning on delete_device
> > >
> > > Implement the same trick for driver bind/unbind.
> > >
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Ramalingam C <ramalingam.c@intel.com>
> > > Cc: Arend van Spriel <aspriel@gmail.com>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> > > Cc: Joe Perches <joe@perches.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > > drivers/base/bus.c | 4 ++--
> > > include/linux/device.h | 4 ++++
> > > 2 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > > index 8bfd27ec73d6..5d2411b848cd 100644
> > > --- a/drivers/base/bus.c
> > > +++ b/drivers/base/bus.c
> > > @@ -195,7 +195,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> > > bus_put(bus);
> > > return err;
> > > }
> > > -static DRIVER_ATTR_WO(unbind);
> > > +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
> > >
> > > /*
> > > * Manually attach a device to a driver.
> > > @@ -231,7 +231,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> > > bus_put(bus);
> > > return err;
> > > }
> > > -static DRIVER_ATTR_WO(bind);
> > > +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
> > >
> > > static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> > > {
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 1b25c7a43f4c..d2cb1a6c5d95 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -326,6 +326,10 @@ struct driver_attribute {
> > > struct driver_attribute driver_attr_##_name = __ATTR_RO(_name)
> > > #define DRIVER_ATTR_WO(_name) \
> > > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> > > +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> > > + struct driver_attribute driver_attr_##_name = \
> > > + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
> > > +
> >
> > I don't want to give driver writers the ability to shoot themselves in
> > the foot, can you just put this in the bus.c file itself so that no one
> > else will abuse this and "open code" the unbind/bind attributes instead
> > of using a #define for it?
>
> So part of the reasons I've put these here is that I think we can do a
> lot better. Instead of ignoring lockdep I think we can implement
> nested lockdep annotations. It's going to be a bit a pain to wire that
> through from sysfs->kernfs, but for all the (now 3) uses of
> _IGNORE_LOCKDEP it should work if the add/remove_files functions use
> mutex_lock_nested.
Agreed.
>I think then there's not really a problem with
> exposing this. Aside from it's already exposed, but only for devices,
> as DEVICE_ATTR_IGNORE_LOCKDEP.
Right.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-19 7:01 ` [PATCH] " Greg Kroah-Hartman
@ 2018-12-19 9:24 ` Rafael J. Wysocki
2018-12-19 9:24 ` Rafael J. Wysocki
1 sibling, 0 replies; 29+ messages in thread
From: Rafael J. Wysocki @ 2018-12-19 9:24 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Daniel Vetter, dri-devel, intel-gfx, Linux Kernel Mailing List,
Rafael J. Wysocki, ramalingam.c, aspriel, Andy Shevchenko,
Geert Uytterhoeven, brgl, Heikki Krogerus, Vivek Gautam,
Joe Perches, Daniel Vetter
On Wed, Dec 19, 2018 at 8:01 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Dec 18, 2018 at 09:14:43PM +0100, Daniel Vetter wrote:
> > This is the much more correct fix for my earlier attempt at:
> >
> > https://lkml.org/lkml/2018/12/10/118
> >
> > Short recap:
> >
> > - There's not actually a locking issue, it's just lockdep being a bit
> > too eager to complain about a possible deadlock.
> >
> > - Contrary to what I claimed the real problem is recursion on
> > kn->count. Greg pointed me at sysfs_break_active_protection(), used
> > by the scsi subsystem to allow a sysfs file to unbind itself. That
> > would be a real deadlock, which isn't what's happening here. Also,
> > breaking the active protection means we'd need to manually handle
> > all the lifetime fun.
> >
> > - With Rafael we discussed the task_work approach, which kinda works,
> > but has two downsides: It's a functional change for a lockdep
> > annotation issue, and it won't work for the bind file (which needs
> > to get the errno from the driver load function back to userspace).
> >
> > - Greg also asked why this never showed up: To hit this you need to
> > unregister a 2nd driver from the unload code of your first driver. I
> > guess only gpus do that. The bug has always been there, but only
> > with a recent patch series did we add more locks so that lockdep
> > built a chain from unbinding the snd-hda driver to the
> > acpi_video_unregister call.
> >
> > Full lockdep splat:
> >
> > [12301.898799] ============================================
> > [12301.898805] WARNING: possible recursive locking detected
> > [12301.898811] 4.20.0-rc7+ #84 Not tainted
> > [12301.898815] --------------------------------------------
> > [12301.898821] bash/5297 is trying to acquire lock:
> > [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.898841] but task is already holding lock:
> > [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > [12301.898856] other info that might help us debug this:
> > [12301.898862] Possible unsafe locking scenario:
> > [12301.898867] CPU0
> > [12301.898870] ----
> > [12301.898874] lock(kn->count#39);
> > [12301.898879] lock(kn->count#39);
> > [12301.898883] *** DEADLOCK ***
> > [12301.898891] May be due to missing lock nesting notation
> > [12301.898899] 5 locks held by bash/5297:
> > [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> > [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> > [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> > [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> > [12301.898960] stack backtrace:
> > [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> > [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> > [12301.898982] Call Trace:
> > [12301.898989] dump_stack+0x67/0x9b
> > [12301.898997] __lock_acquire+0x6ad/0x1410
> > [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899010] ? find_held_lock+0x2d/0x90
> > [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> > [12301.899023] ? find_held_lock+0x2d/0x90
> > [12301.899030] ? lock_acquire+0x90/0x180
> > [12301.899036] lock_acquire+0x90/0x180
> > [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899049] __kernfs_remove+0x296/0x310
> > [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899060] ? kernfs_name_hash+0xd/0x80
> > [12301.899066] ? kernfs_find_ns+0x6c/0x100
> > [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899080] bus_remove_driver+0x92/0xa0
> > [12301.899085] acpi_video_unregister+0x24/0x40
> > [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> > [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> > [12301.899169] pci_device_remove+0x36/0xb0
> > [12301.899176] device_release_driver_internal+0x185/0x240
> > [12301.899183] unbind_store+0xaf/0x180
> > [12301.899189] kernfs_fop_write+0x104/0x190
> > [12301.899195] __vfs_write+0x31/0x180
> > [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> > [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> > [12301.899216] ? __sb_start_write+0x13c/0x1a0
> > [12301.899221] ? vfs_write+0x17f/0x1b0
> > [12301.899227] vfs_write+0xb9/0x1b0
> > [12301.899233] ksys_write+0x50/0xc0
> > [12301.899239] do_syscall_64+0x4b/0x180
> > [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [12301.899253] RIP: 0033:0x7f452ac7f7a4
> > [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> > [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> > [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> > [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> > [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> > [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
> >
> > Locking around I've noticed that usb and i2c already handle similar
> > recursion problems, where a sysfs file can unbind the same type of
> > sysfs somewhere else in the hierarchy. Relevant commits are:
> >
> > commit 356c05d58af05d582e634b54b40050c73609617b
> > Author: Alan Stern <stern@rowland.harvard.edu>
> > Date: Mon May 14 13:30:03 2012 -0400
> >
> > sysfs: get rid of some lockdep false positives
> >
> > commit e9b526fe704812364bca07edd15eadeba163ebfb
> > Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> > Date: Fri May 17 14:56:35 2013 +0200
> >
> > i2c: suppress lockdep warning on delete_device
> >
> > Implement the same trick for driver bind/unbind.
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Cc: Arend van Spriel <aspriel@gmail.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> > Cc: Joe Perches <joe@perches.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > drivers/base/bus.c | 4 ++--
> > include/linux/device.h | 4 ++++
> > 2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index 8bfd27ec73d6..5d2411b848cd 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -195,7 +195,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> > bus_put(bus);
> > return err;
> > }
> > -static DRIVER_ATTR_WO(unbind);
> > +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
> >
> > /*
> > * Manually attach a device to a driver.
> > @@ -231,7 +231,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> > bus_put(bus);
> > return err;
> > }
> > -static DRIVER_ATTR_WO(bind);
> > +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
> >
> > static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> > {
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 1b25c7a43f4c..d2cb1a6c5d95 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -326,6 +326,10 @@ struct driver_attribute {
> > struct driver_attribute driver_attr_##_name = __ATTR_RO(_name)
> > #define DRIVER_ATTR_WO(_name) \
> > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> > +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> > + struct driver_attribute driver_attr_##_name = \
> > + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
> > +
>
> I don't want to give driver writers the ability to shoot themselves in
> the foot, can you just put this in the bus.c file itself so that no one
> else will abuse this and "open code" the unbind/bind attributes instead
> of using a #define for it?
Good point, but any attribute starting a driver unbind via sysfs will
need this, won't it?
> Also I've been trying to get rid of the "specify the mode" macros, so
> that everyone uses the RO, WO, and RW versions, so adding a generic one
> here is not going to help with that effort :)
But if that goes into bus.c directly, the mode arg doesn't hurt IMO
and one macro would be sufficient to cover both attrs.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
@ 2018-12-19 9:24 ` Rafael J. Wysocki
0 siblings, 0 replies; 29+ messages in thread
From: Rafael J. Wysocki @ 2018-12-19 9:24 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: aspriel, Heikki Krogerus, Geert Uytterhoeven, Rafael J. Wysocki,
intel-gfx, Linux Kernel Mailing List, dri-devel, Vivek Gautam,
Daniel Vetter, Joe Perches, Daniel Vetter, Andy Shevchenko, brgl
On Wed, Dec 19, 2018 at 8:01 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Dec 18, 2018 at 09:14:43PM +0100, Daniel Vetter wrote:
> > This is the much more correct fix for my earlier attempt at:
> >
> > https://lkml.org/lkml/2018/12/10/118
> >
> > Short recap:
> >
> > - There's not actually a locking issue, it's just lockdep being a bit
> > too eager to complain about a possible deadlock.
> >
> > - Contrary to what I claimed the real problem is recursion on
> > kn->count. Greg pointed me at sysfs_break_active_protection(), used
> > by the scsi subsystem to allow a sysfs file to unbind itself. That
> > would be a real deadlock, which isn't what's happening here. Also,
> > breaking the active protection means we'd need to manually handle
> > all the lifetime fun.
> >
> > - With Rafael we discussed the task_work approach, which kinda works,
> > but has two downsides: It's a functional change for a lockdep
> > annotation issue, and it won't work for the bind file (which needs
> > to get the errno from the driver load function back to userspace).
> >
> > - Greg also asked why this never showed up: To hit this you need to
> > unregister a 2nd driver from the unload code of your first driver. I
> > guess only gpus do that. The bug has always been there, but only
> > with a recent patch series did we add more locks so that lockdep
> > built a chain from unbinding the snd-hda driver to the
> > acpi_video_unregister call.
> >
> > Full lockdep splat:
> >
> > [12301.898799] ============================================
> > [12301.898805] WARNING: possible recursive locking detected
> > [12301.898811] 4.20.0-rc7+ #84 Not tainted
> > [12301.898815] --------------------------------------------
> > [12301.898821] bash/5297 is trying to acquire lock:
> > [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.898841] but task is already holding lock:
> > [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > [12301.898856] other info that might help us debug this:
> > [12301.898862] Possible unsafe locking scenario:
> > [12301.898867] CPU0
> > [12301.898870] ----
> > [12301.898874] lock(kn->count#39);
> > [12301.898879] lock(kn->count#39);
> > [12301.898883] *** DEADLOCK ***
> > [12301.898891] May be due to missing lock nesting notation
> > [12301.898899] 5 locks held by bash/5297:
> > [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> > [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> > [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> > [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> > [12301.898960] stack backtrace:
> > [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> > [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> > [12301.898982] Call Trace:
> > [12301.898989] dump_stack+0x67/0x9b
> > [12301.898997] __lock_acquire+0x6ad/0x1410
> > [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899010] ? find_held_lock+0x2d/0x90
> > [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> > [12301.899023] ? find_held_lock+0x2d/0x90
> > [12301.899030] ? lock_acquire+0x90/0x180
> > [12301.899036] lock_acquire+0x90/0x180
> > [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899049] __kernfs_remove+0x296/0x310
> > [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899060] ? kernfs_name_hash+0xd/0x80
> > [12301.899066] ? kernfs_find_ns+0x6c/0x100
> > [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899080] bus_remove_driver+0x92/0xa0
> > [12301.899085] acpi_video_unregister+0x24/0x40
> > [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> > [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> > [12301.899169] pci_device_remove+0x36/0xb0
> > [12301.899176] device_release_driver_internal+0x185/0x240
> > [12301.899183] unbind_store+0xaf/0x180
> > [12301.899189] kernfs_fop_write+0x104/0x190
> > [12301.899195] __vfs_write+0x31/0x180
> > [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> > [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> > [12301.899216] ? __sb_start_write+0x13c/0x1a0
> > [12301.899221] ? vfs_write+0x17f/0x1b0
> > [12301.899227] vfs_write+0xb9/0x1b0
> > [12301.899233] ksys_write+0x50/0xc0
> > [12301.899239] do_syscall_64+0x4b/0x180
> > [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [12301.899253] RIP: 0033:0x7f452ac7f7a4
> > [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> > [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> > [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> > [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> > [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> > [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
> >
> > Locking around I've noticed that usb and i2c already handle similar
> > recursion problems, where a sysfs file can unbind the same type of
> > sysfs somewhere else in the hierarchy. Relevant commits are:
> >
> > commit 356c05d58af05d582e634b54b40050c73609617b
> > Author: Alan Stern <stern@rowland.harvard.edu>
> > Date: Mon May 14 13:30:03 2012 -0400
> >
> > sysfs: get rid of some lockdep false positives
> >
> > commit e9b526fe704812364bca07edd15eadeba163ebfb
> > Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> > Date: Fri May 17 14:56:35 2013 +0200
> >
> > i2c: suppress lockdep warning on delete_device
> >
> > Implement the same trick for driver bind/unbind.
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Cc: Arend van Spriel <aspriel@gmail.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> > Cc: Joe Perches <joe@perches.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > drivers/base/bus.c | 4 ++--
> > include/linux/device.h | 4 ++++
> > 2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index 8bfd27ec73d6..5d2411b848cd 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -195,7 +195,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> > bus_put(bus);
> > return err;
> > }
> > -static DRIVER_ATTR_WO(unbind);
> > +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
> >
> > /*
> > * Manually attach a device to a driver.
> > @@ -231,7 +231,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> > bus_put(bus);
> > return err;
> > }
> > -static DRIVER_ATTR_WO(bind);
> > +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
> >
> > static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> > {
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 1b25c7a43f4c..d2cb1a6c5d95 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -326,6 +326,10 @@ struct driver_attribute {
> > struct driver_attribute driver_attr_##_name = __ATTR_RO(_name)
> > #define DRIVER_ATTR_WO(_name) \
> > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> > +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> > + struct driver_attribute driver_attr_##_name = \
> > + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
> > +
>
> I don't want to give driver writers the ability to shoot themselves in
> the foot, can you just put this in the bus.c file itself so that no one
> else will abuse this and "open code" the unbind/bind attributes instead
> of using a #define for it?
Good point, but any attribute starting a driver unbind via sysfs will
need this, won't it?
> Also I've been trying to get rid of the "specify the mode" macros, so
> that everyone uses the RO, WO, and RW versions, so adding a generic one
> here is not going to help with that effort :)
But if that goes into bus.c directly, the mode arg doesn't hurt IMO
and one macro would be sufficient to cover both attrs.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-19 9:24 ` Rafael J. Wysocki
@ 2018-12-19 9:44 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 29+ messages in thread
From: Greg Kroah-Hartman @ 2018-12-19 9:44 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Daniel Vetter, dri-devel, intel-gfx, Linux Kernel Mailing List,
ramalingam.c, aspriel, Andy Shevchenko, Geert Uytterhoeven, brgl,
Heikki Krogerus, Vivek Gautam, Joe Perches, Daniel Vetter
On Wed, Dec 19, 2018 at 10:24:51AM +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 19, 2018 at 8:01 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Dec 18, 2018 at 09:14:43PM +0100, Daniel Vetter wrote:
> > > This is the much more correct fix for my earlier attempt at:
> > >
> > > https://lkml.org/lkml/2018/12/10/118
> > >
> > > Short recap:
> > >
> > > - There's not actually a locking issue, it's just lockdep being a bit
> > > too eager to complain about a possible deadlock.
> > >
> > > - Contrary to what I claimed the real problem is recursion on
> > > kn->count. Greg pointed me at sysfs_break_active_protection(), used
> > > by the scsi subsystem to allow a sysfs file to unbind itself. That
> > > would be a real deadlock, which isn't what's happening here. Also,
> > > breaking the active protection means we'd need to manually handle
> > > all the lifetime fun.
> > >
> > > - With Rafael we discussed the task_work approach, which kinda works,
> > > but has two downsides: It's a functional change for a lockdep
> > > annotation issue, and it won't work for the bind file (which needs
> > > to get the errno from the driver load function back to userspace).
> > >
> > > - Greg also asked why this never showed up: To hit this you need to
> > > unregister a 2nd driver from the unload code of your first driver. I
> > > guess only gpus do that. The bug has always been there, but only
> > > with a recent patch series did we add more locks so that lockdep
> > > built a chain from unbinding the snd-hda driver to the
> > > acpi_video_unregister call.
> > >
> > > Full lockdep splat:
> > >
> > > [12301.898799] ============================================
> > > [12301.898805] WARNING: possible recursive locking detected
> > > [12301.898811] 4.20.0-rc7+ #84 Not tainted
> > > [12301.898815] --------------------------------------------
> > > [12301.898821] bash/5297 is trying to acquire lock:
> > > [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.898841] but task is already holding lock:
> > > [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > > [12301.898856] other info that might help us debug this:
> > > [12301.898862] Possible unsafe locking scenario:
> > > [12301.898867] CPU0
> > > [12301.898870] ----
> > > [12301.898874] lock(kn->count#39);
> > > [12301.898879] lock(kn->count#39);
> > > [12301.898883] *** DEADLOCK ***
> > > [12301.898891] May be due to missing lock nesting notation
> > > [12301.898899] 5 locks held by bash/5297:
> > > [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> > > [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> > > [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > > [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> > > [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> > > [12301.898960] stack backtrace:
> > > [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> > > [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> > > [12301.898982] Call Trace:
> > > [12301.898989] dump_stack+0x67/0x9b
> > > [12301.898997] __lock_acquire+0x6ad/0x1410
> > > [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.899010] ? find_held_lock+0x2d/0x90
> > > [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> > > [12301.899023] ? find_held_lock+0x2d/0x90
> > > [12301.899030] ? lock_acquire+0x90/0x180
> > > [12301.899036] lock_acquire+0x90/0x180
> > > [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.899049] __kernfs_remove+0x296/0x310
> > > [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.899060] ? kernfs_name_hash+0xd/0x80
> > > [12301.899066] ? kernfs_find_ns+0x6c/0x100
> > > [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.899080] bus_remove_driver+0x92/0xa0
> > > [12301.899085] acpi_video_unregister+0x24/0x40
> > > [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> > > [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> > > [12301.899169] pci_device_remove+0x36/0xb0
> > > [12301.899176] device_release_driver_internal+0x185/0x240
> > > [12301.899183] unbind_store+0xaf/0x180
> > > [12301.899189] kernfs_fop_write+0x104/0x190
> > > [12301.899195] __vfs_write+0x31/0x180
> > > [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> > > [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> > > [12301.899216] ? __sb_start_write+0x13c/0x1a0
> > > [12301.899221] ? vfs_write+0x17f/0x1b0
> > > [12301.899227] vfs_write+0xb9/0x1b0
> > > [12301.899233] ksys_write+0x50/0xc0
> > > [12301.899239] do_syscall_64+0x4b/0x180
> > > [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > > [12301.899253] RIP: 0033:0x7f452ac7f7a4
> > > [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> > > [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > > [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> > > [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> > > [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> > > [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> > > [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
> > >
> > > Locking around I've noticed that usb and i2c already handle similar
> > > recursion problems, where a sysfs file can unbind the same type of
> > > sysfs somewhere else in the hierarchy. Relevant commits are:
> > >
> > > commit 356c05d58af05d582e634b54b40050c73609617b
> > > Author: Alan Stern <stern@rowland.harvard.edu>
> > > Date: Mon May 14 13:30:03 2012 -0400
> > >
> > > sysfs: get rid of some lockdep false positives
> > >
> > > commit e9b526fe704812364bca07edd15eadeba163ebfb
> > > Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> > > Date: Fri May 17 14:56:35 2013 +0200
> > >
> > > i2c: suppress lockdep warning on delete_device
> > >
> > > Implement the same trick for driver bind/unbind.
> > >
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Ramalingam C <ramalingam.c@intel.com>
> > > Cc: Arend van Spriel <aspriel@gmail.com>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> > > Cc: Joe Perches <joe@perches.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > > drivers/base/bus.c | 4 ++--
> > > include/linux/device.h | 4 ++++
> > > 2 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > > index 8bfd27ec73d6..5d2411b848cd 100644
> > > --- a/drivers/base/bus.c
> > > +++ b/drivers/base/bus.c
> > > @@ -195,7 +195,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> > > bus_put(bus);
> > > return err;
> > > }
> > > -static DRIVER_ATTR_WO(unbind);
> > > +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
> > >
> > > /*
> > > * Manually attach a device to a driver.
> > > @@ -231,7 +231,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> > > bus_put(bus);
> > > return err;
> > > }
> > > -static DRIVER_ATTR_WO(bind);
> > > +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
> > >
> > > static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> > > {
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 1b25c7a43f4c..d2cb1a6c5d95 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -326,6 +326,10 @@ struct driver_attribute {
> > > struct driver_attribute driver_attr_##_name = __ATTR_RO(_name)
> > > #define DRIVER_ATTR_WO(_name) \
> > > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> > > +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> > > + struct driver_attribute driver_attr_##_name = \
> > > + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
> > > +
> >
> > I don't want to give driver writers the ability to shoot themselves in
> > the foot, can you just put this in the bus.c file itself so that no one
> > else will abuse this and "open code" the unbind/bind attributes instead
> > of using a #define for it?
>
> Good point, but any attribute starting a driver unbind via sysfs will
> need this, won't it?
And how many of those are there? :)
> > Also I've been trying to get rid of the "specify the mode" macros, so
> > that everyone uses the RO, WO, and RW versions, so adding a generic one
> > here is not going to help with that effort :)
>
> But if that goes into bus.c directly, the mode arg doesn't hurt IMO
> and one macro would be sufficient to cover both attrs.
One macro in bus.c is fine, just don't put it in device.h.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
@ 2018-12-19 9:44 ` Greg Kroah-Hartman
0 siblings, 0 replies; 29+ messages in thread
From: Greg Kroah-Hartman @ 2018-12-19 9:44 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: aspriel, Heikki Krogerus, Geert Uytterhoeven, Daniel Vetter,
intel-gfx, Linux Kernel Mailing List, dri-devel, Vivek Gautam,
Joe Perches, Daniel Vetter, Andy Shevchenko, brgl
On Wed, Dec 19, 2018 at 10:24:51AM +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 19, 2018 at 8:01 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Dec 18, 2018 at 09:14:43PM +0100, Daniel Vetter wrote:
> > > This is the much more correct fix for my earlier attempt at:
> > >
> > > https://lkml.org/lkml/2018/12/10/118
> > >
> > > Short recap:
> > >
> > > - There's not actually a locking issue, it's just lockdep being a bit
> > > too eager to complain about a possible deadlock.
> > >
> > > - Contrary to what I claimed the real problem is recursion on
> > > kn->count. Greg pointed me at sysfs_break_active_protection(), used
> > > by the scsi subsystem to allow a sysfs file to unbind itself. That
> > > would be a real deadlock, which isn't what's happening here. Also,
> > > breaking the active protection means we'd need to manually handle
> > > all the lifetime fun.
> > >
> > > - With Rafael we discussed the task_work approach, which kinda works,
> > > but has two downsides: It's a functional change for a lockdep
> > > annotation issue, and it won't work for the bind file (which needs
> > > to get the errno from the driver load function back to userspace).
> > >
> > > - Greg also asked why this never showed up: To hit this you need to
> > > unregister a 2nd driver from the unload code of your first driver. I
> > > guess only gpus do that. The bug has always been there, but only
> > > with a recent patch series did we add more locks so that lockdep
> > > built a chain from unbinding the snd-hda driver to the
> > > acpi_video_unregister call.
> > >
> > > Full lockdep splat:
> > >
> > > [12301.898799] ============================================
> > > [12301.898805] WARNING: possible recursive locking detected
> > > [12301.898811] 4.20.0-rc7+ #84 Not tainted
> > > [12301.898815] --------------------------------------------
> > > [12301.898821] bash/5297 is trying to acquire lock:
> > > [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.898841] but task is already holding lock:
> > > [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > > [12301.898856] other info that might help us debug this:
> > > [12301.898862] Possible unsafe locking scenario:
> > > [12301.898867] CPU0
> > > [12301.898870] ----
> > > [12301.898874] lock(kn->count#39);
> > > [12301.898879] lock(kn->count#39);
> > > [12301.898883] *** DEADLOCK ***
> > > [12301.898891] May be due to missing lock nesting notation
> > > [12301.898899] 5 locks held by bash/5297:
> > > [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> > > [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> > > [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > > [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> > > [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> > > [12301.898960] stack backtrace:
> > > [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> > > [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> > > [12301.898982] Call Trace:
> > > [12301.898989] dump_stack+0x67/0x9b
> > > [12301.898997] __lock_acquire+0x6ad/0x1410
> > > [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.899010] ? find_held_lock+0x2d/0x90
> > > [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> > > [12301.899023] ? find_held_lock+0x2d/0x90
> > > [12301.899030] ? lock_acquire+0x90/0x180
> > > [12301.899036] lock_acquire+0x90/0x180
> > > [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.899049] __kernfs_remove+0x296/0x310
> > > [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.899060] ? kernfs_name_hash+0xd/0x80
> > > [12301.899066] ? kernfs_find_ns+0x6c/0x100
> > > [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> > > [12301.899080] bus_remove_driver+0x92/0xa0
> > > [12301.899085] acpi_video_unregister+0x24/0x40
> > > [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> > > [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> > > [12301.899169] pci_device_remove+0x36/0xb0
> > > [12301.899176] device_release_driver_internal+0x185/0x240
> > > [12301.899183] unbind_store+0xaf/0x180
> > > [12301.899189] kernfs_fop_write+0x104/0x190
> > > [12301.899195] __vfs_write+0x31/0x180
> > > [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> > > [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> > > [12301.899216] ? __sb_start_write+0x13c/0x1a0
> > > [12301.899221] ? vfs_write+0x17f/0x1b0
> > > [12301.899227] vfs_write+0xb9/0x1b0
> > > [12301.899233] ksys_write+0x50/0xc0
> > > [12301.899239] do_syscall_64+0x4b/0x180
> > > [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > > [12301.899253] RIP: 0033:0x7f452ac7f7a4
> > > [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> > > [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > > [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> > > [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> > > [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> > > [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> > > [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
> > >
> > > Locking around I've noticed that usb and i2c already handle similar
> > > recursion problems, where a sysfs file can unbind the same type of
> > > sysfs somewhere else in the hierarchy. Relevant commits are:
> > >
> > > commit 356c05d58af05d582e634b54b40050c73609617b
> > > Author: Alan Stern <stern@rowland.harvard.edu>
> > > Date: Mon May 14 13:30:03 2012 -0400
> > >
> > > sysfs: get rid of some lockdep false positives
> > >
> > > commit e9b526fe704812364bca07edd15eadeba163ebfb
> > > Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> > > Date: Fri May 17 14:56:35 2013 +0200
> > >
> > > i2c: suppress lockdep warning on delete_device
> > >
> > > Implement the same trick for driver bind/unbind.
> > >
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Ramalingam C <ramalingam.c@intel.com>
> > > Cc: Arend van Spriel <aspriel@gmail.com>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> > > Cc: Joe Perches <joe@perches.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > > drivers/base/bus.c | 4 ++--
> > > include/linux/device.h | 4 ++++
> > > 2 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > > index 8bfd27ec73d6..5d2411b848cd 100644
> > > --- a/drivers/base/bus.c
> > > +++ b/drivers/base/bus.c
> > > @@ -195,7 +195,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> > > bus_put(bus);
> > > return err;
> > > }
> > > -static DRIVER_ATTR_WO(unbind);
> > > +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
> > >
> > > /*
> > > * Manually attach a device to a driver.
> > > @@ -231,7 +231,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> > > bus_put(bus);
> > > return err;
> > > }
> > > -static DRIVER_ATTR_WO(bind);
> > > +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
> > >
> > > static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> > > {
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 1b25c7a43f4c..d2cb1a6c5d95 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -326,6 +326,10 @@ struct driver_attribute {
> > > struct driver_attribute driver_attr_##_name = __ATTR_RO(_name)
> > > #define DRIVER_ATTR_WO(_name) \
> > > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> > > +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> > > + struct driver_attribute driver_attr_##_name = \
> > > + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
> > > +
> >
> > I don't want to give driver writers the ability to shoot themselves in
> > the foot, can you just put this in the bus.c file itself so that no one
> > else will abuse this and "open code" the unbind/bind attributes instead
> > of using a #define for it?
>
> Good point, but any attribute starting a driver unbind via sysfs will
> need this, won't it?
And how many of those are there? :)
> > Also I've been trying to get rid of the "specify the mode" macros, so
> > that everyone uses the RO, WO, and RW versions, so adding a generic one
> > here is not going to help with that effort :)
>
> But if that goes into bus.c directly, the mode arg doesn't hurt IMO
> and one macro would be sufficient to cover both attrs.
One macro in bus.c is fine, just don't put it in device.h.
thanks,
greg k-h
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-18 20:14 ` Daniel Vetter
@ 2018-12-19 12:39 ` Daniel Vetter
-1 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2018-12-19 12:39 UTC (permalink / raw)
To: DRI Development
Cc: Intel Graphics Development, LKML, Daniel Vetter,
Rafael J. Wysocki, Greg Kroah-Hartman, Ramalingam C,
Arend van Spriel, Andy Shevchenko, Geert Uytterhoeven,
Bartosz Golaszewski, Heikki Krogerus, Vivek Gautam, Joe Perches,
Daniel Vetter
This is the much more correct fix for my earlier attempt at:
https://lkml.org/lkml/2018/12/10/118
Short recap:
- There's not actually a locking issue, it's just lockdep being a bit
too eager to complain about a possible deadlock.
- Contrary to what I claimed the real problem is recursion on
kn->count. Greg pointed me at sysfs_break_active_protection(), used
by the scsi subsystem to allow a sysfs file to unbind itself. That
would be a real deadlock, which isn't what's happening here. Also,
breaking the active protection means we'd need to manually handle
all the lifetime fun.
- With Rafael we discussed the task_work approach, which kinda works,
but has two downsides: It's a functional change for a lockdep
annotation issue, and it won't work for the bind file (which needs
to get the errno from the driver load function back to userspace).
- Greg also asked why this never showed up: To hit this you need to
unregister a 2nd driver from the unload code of your first driver. I
guess only gpus do that. The bug has always been there, but only
with a recent patch series did we add more locks so that lockdep
built a chain from unbinding the snd-hda driver to the
acpi_video_unregister call.
Full lockdep splat:
[12301.898799] ============================================
[12301.898805] WARNING: possible recursive locking detected
[12301.898811] 4.20.0-rc7+ #84 Not tainted
[12301.898815] --------------------------------------------
[12301.898821] bash/5297 is trying to acquire lock:
[12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
[12301.898841] but task is already holding lock:
[12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
[12301.898856] other info that might help us debug this:
[12301.898862] Possible unsafe locking scenario:
[12301.898867] CPU0
[12301.898870] ----
[12301.898874] lock(kn->count#39);
[12301.898879] lock(kn->count#39);
[12301.898883] *** DEADLOCK ***
[12301.898891] May be due to missing lock nesting notation
[12301.898899] 5 locks held by bash/5297:
[12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
[12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
[12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
[12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
[12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
[12301.898960] stack backtrace:
[12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
[12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
[12301.898982] Call Trace:
[12301.898989] dump_stack+0x67/0x9b
[12301.898997] __lock_acquire+0x6ad/0x1410
[12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
[12301.899010] ? find_held_lock+0x2d/0x90
[12301.899017] ? mutex_spin_on_owner+0xe4/0x150
[12301.899023] ? find_held_lock+0x2d/0x90
[12301.899030] ? lock_acquire+0x90/0x180
[12301.899036] lock_acquire+0x90/0x180
[12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
[12301.899049] __kernfs_remove+0x296/0x310
[12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
[12301.899060] ? kernfs_name_hash+0xd/0x80
[12301.899066] ? kernfs_find_ns+0x6c/0x100
[12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
[12301.899080] bus_remove_driver+0x92/0xa0
[12301.899085] acpi_video_unregister+0x24/0x40
[12301.899127] i915_driver_unload+0x42/0x130 [i915]
[12301.899160] i915_pci_remove+0x19/0x30 [i915]
[12301.899169] pci_device_remove+0x36/0xb0
[12301.899176] device_release_driver_internal+0x185/0x240
[12301.899183] unbind_store+0xaf/0x180
[12301.899189] kernfs_fop_write+0x104/0x190
[12301.899195] __vfs_write+0x31/0x180
[12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
[12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
[12301.899216] ? __sb_start_write+0x13c/0x1a0
[12301.899221] ? vfs_write+0x17f/0x1b0
[12301.899227] vfs_write+0xb9/0x1b0
[12301.899233] ksys_write+0x50/0xc0
[12301.899239] do_syscall_64+0x4b/0x180
[12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[12301.899253] RIP: 0033:0x7f452ac7f7a4
[12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
[12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
[12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
[12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
[12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
[12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
Locking around I've noticed that usb and i2c already handle similar
recursion problems, where a sysfs file can unbind the same type of
sysfs somewhere else in the hierarchy. Relevant commits are:
commit 356c05d58af05d582e634b54b40050c73609617b
Author: Alan Stern <stern@rowland.harvard.edu>
Date: Mon May 14 13:30:03 2012 -0400
sysfs: get rid of some lockdep false positives
commit e9b526fe704812364bca07edd15eadeba163ebfb
Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
Date: Fri May 17 14:56:35 2013 +0200
i2c: suppress lockdep warning on delete_device
Implement the same trick for driver bind/unbind.
v2: Put the macro into bus.c (Greg).
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Arend van Spriel <aspriel@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/base/bus.c | 7 +++++--
include/linux/device.h | 1 +
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 8bfd27ec73d6..585e2e1c9c8f 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -31,6 +31,9 @@ static struct kset *system_kset;
#define to_drv_attr(_attr) container_of(_attr, struct driver_attribute, attr)
+#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
+ struct driver_attribute driver_attr_##_name = \
+ __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
static int __must_check bus_rescan_devices_helper(struct device *dev,
void *data);
@@ -195,7 +198,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
bus_put(bus);
return err;
}
-static DRIVER_ATTR_WO(unbind);
+static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
/*
* Manually attach a device to a driver.
@@ -231,7 +234,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
bus_put(bus);
return err;
}
-static DRIVER_ATTR_WO(bind);
+static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
{
diff --git a/include/linux/device.h b/include/linux/device.h
index 1b25c7a43f4c..41424d6e27c7 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -327,6 +327,7 @@ struct driver_attribute {
#define DRIVER_ATTR_WO(_name) \
struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
+
extern int __must_check driver_create_file(struct device_driver *driver,
const struct driver_attribute *attr);
extern void driver_remove_file(struct device_driver *driver,
--
2.20.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH] sysfs: Disable lockdep for driver bind/unbind files
@ 2018-12-19 12:39 ` Daniel Vetter
0 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2018-12-19 12:39 UTC (permalink / raw)
To: DRI Development
Cc: Arend van Spriel, Heikki Krogerus, Geert Uytterhoeven,
Greg Kroah-Hartman, Intel Graphics Development,
Rafael J. Wysocki, LKML, Vivek Gautam, Daniel Vetter,
Joe Perches, Daniel Vetter, Andy Shevchenko, Bartosz Golaszewski
This is the much more correct fix for my earlier attempt at:
https://lkml.org/lkml/2018/12/10/118
Short recap:
- There's not actually a locking issue, it's just lockdep being a bit
too eager to complain about a possible deadlock.
- Contrary to what I claimed the real problem is recursion on
kn->count. Greg pointed me at sysfs_break_active_protection(), used
by the scsi subsystem to allow a sysfs file to unbind itself. That
would be a real deadlock, which isn't what's happening here. Also,
breaking the active protection means we'd need to manually handle
all the lifetime fun.
- With Rafael we discussed the task_work approach, which kinda works,
but has two downsides: It's a functional change for a lockdep
annotation issue, and it won't work for the bind file (which needs
to get the errno from the driver load function back to userspace).
- Greg also asked why this never showed up: To hit this you need to
unregister a 2nd driver from the unload code of your first driver. I
guess only gpus do that. The bug has always been there, but only
with a recent patch series did we add more locks so that lockdep
built a chain from unbinding the snd-hda driver to the
acpi_video_unregister call.
Full lockdep splat:
[12301.898799] ============================================
[12301.898805] WARNING: possible recursive locking detected
[12301.898811] 4.20.0-rc7+ #84 Not tainted
[12301.898815] --------------------------------------------
[12301.898821] bash/5297 is trying to acquire lock:
[12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
[12301.898841] but task is already holding lock:
[12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
[12301.898856] other info that might help us debug this:
[12301.898862] Possible unsafe locking scenario:
[12301.898867] CPU0
[12301.898870] ----
[12301.898874] lock(kn->count#39);
[12301.898879] lock(kn->count#39);
[12301.898883] *** DEADLOCK ***
[12301.898891] May be due to missing lock nesting notation
[12301.898899] 5 locks held by bash/5297:
[12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
[12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
[12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
[12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
[12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
[12301.898960] stack backtrace:
[12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
[12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
[12301.898982] Call Trace:
[12301.898989] dump_stack+0x67/0x9b
[12301.898997] __lock_acquire+0x6ad/0x1410
[12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
[12301.899010] ? find_held_lock+0x2d/0x90
[12301.899017] ? mutex_spin_on_owner+0xe4/0x150
[12301.899023] ? find_held_lock+0x2d/0x90
[12301.899030] ? lock_acquire+0x90/0x180
[12301.899036] lock_acquire+0x90/0x180
[12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
[12301.899049] __kernfs_remove+0x296/0x310
[12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
[12301.899060] ? kernfs_name_hash+0xd/0x80
[12301.899066] ? kernfs_find_ns+0x6c/0x100
[12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
[12301.899080] bus_remove_driver+0x92/0xa0
[12301.899085] acpi_video_unregister+0x24/0x40
[12301.899127] i915_driver_unload+0x42/0x130 [i915]
[12301.899160] i915_pci_remove+0x19/0x30 [i915]
[12301.899169] pci_device_remove+0x36/0xb0
[12301.899176] device_release_driver_internal+0x185/0x240
[12301.899183] unbind_store+0xaf/0x180
[12301.899189] kernfs_fop_write+0x104/0x190
[12301.899195] __vfs_write+0x31/0x180
[12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
[12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
[12301.899216] ? __sb_start_write+0x13c/0x1a0
[12301.899221] ? vfs_write+0x17f/0x1b0
[12301.899227] vfs_write+0xb9/0x1b0
[12301.899233] ksys_write+0x50/0xc0
[12301.899239] do_syscall_64+0x4b/0x180
[12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[12301.899253] RIP: 0033:0x7f452ac7f7a4
[12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
[12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
[12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
[12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
[12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
[12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
Locking around I've noticed that usb and i2c already handle similar
recursion problems, where a sysfs file can unbind the same type of
sysfs somewhere else in the hierarchy. Relevant commits are:
commit 356c05d58af05d582e634b54b40050c73609617b
Author: Alan Stern <stern@rowland.harvard.edu>
Date: Mon May 14 13:30:03 2012 -0400
sysfs: get rid of some lockdep false positives
commit e9b526fe704812364bca07edd15eadeba163ebfb
Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
Date: Fri May 17 14:56:35 2013 +0200
i2c: suppress lockdep warning on delete_device
Implement the same trick for driver bind/unbind.
v2: Put the macro into bus.c (Greg).
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Arend van Spriel <aspriel@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/base/bus.c | 7 +++++--
include/linux/device.h | 1 +
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 8bfd27ec73d6..585e2e1c9c8f 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -31,6 +31,9 @@ static struct kset *system_kset;
#define to_drv_attr(_attr) container_of(_attr, struct driver_attribute, attr)
+#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
+ struct driver_attribute driver_attr_##_name = \
+ __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
static int __must_check bus_rescan_devices_helper(struct device *dev,
void *data);
@@ -195,7 +198,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
bus_put(bus);
return err;
}
-static DRIVER_ATTR_WO(unbind);
+static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
/*
* Manually attach a device to a driver.
@@ -231,7 +234,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
bus_put(bus);
return err;
}
-static DRIVER_ATTR_WO(bind);
+static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
{
diff --git a/include/linux/device.h b/include/linux/device.h
index 1b25c7a43f4c..41424d6e27c7 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -327,6 +327,7 @@ struct driver_attribute {
#define DRIVER_ATTR_WO(_name) \
struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
+
extern int __must_check driver_create_file(struct device_driver *driver,
const struct driver_attribute *attr);
extern void driver_remove_file(struct device_driver *driver,
--
2.20.0.rc1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-19 12:39 ` Daniel Vetter
(?)
@ 2018-12-19 12:49 ` Greg Kroah-Hartman
2018-12-19 13:18 ` Daniel Vetter
-1 siblings, 1 reply; 29+ messages in thread
From: Greg Kroah-Hartman @ 2018-12-19 12:49 UTC (permalink / raw)
To: Daniel Vetter
Cc: DRI Development, Intel Graphics Development, LKML,
Rafael J. Wysocki, Ramalingam C, Arend van Spriel,
Andy Shevchenko, Geert Uytterhoeven, Bartosz Golaszewski,
Heikki Krogerus, Vivek Gautam, Joe Perches, Daniel Vetter
On Wed, Dec 19, 2018 at 01:39:09PM +0100, Daniel Vetter wrote:
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -327,6 +327,7 @@ struct driver_attribute {
> #define DRIVER_ATTR_WO(_name) \
> struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
>
> +
> extern int __must_check driver_create_file(struct device_driver *driver,
> const struct driver_attribute *attr);
> extern void driver_remove_file(struct device_driver *driver,
> --
I'll edit away this last chunk when I apply the patch :)
thanks for making the changes.
greg k-h
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-19 12:49 ` Greg Kroah-Hartman
@ 2018-12-19 13:18 ` Daniel Vetter
0 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2018-12-19 13:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Daniel Vetter, DRI Development, Intel Graphics Development, LKML,
Rafael J. Wysocki, Ramalingam C, Arend van Spriel,
Andy Shevchenko, Geert Uytterhoeven, Bartosz Golaszewski,
Heikki Krogerus, Vivek Gautam, Joe Perches, Daniel Vetter
On Wed, Dec 19, 2018 at 01:49:30PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Dec 19, 2018 at 01:39:09PM +0100, Daniel Vetter wrote:
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -327,6 +327,7 @@ struct driver_attribute {
> > #define DRIVER_ATTR_WO(_name) \
> > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> >
> > +
> > extern int __must_check driver_create_file(struct device_driver *driver,
> > const struct driver_attribute *attr);
> > extern void driver_remove_file(struct device_driver *driver,
> > --
>
> I'll edit away this last chunk when I apply the patch :)
Oops sry. I also forgot to do the s/Locking/Looking/ in the commit message
:-/
>
> thanks for making the changes.
>
> greg k-h
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
@ 2018-12-19 13:18 ` Daniel Vetter
0 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2018-12-19 13:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Arend van Spriel, Heikki Krogerus, Geert Uytterhoeven,
Rafael J. Wysocki, Intel Graphics Development, LKML,
DRI Development, Vivek Gautam, Daniel Vetter, Joe Perches,
Daniel Vetter, Andy Shevchenko, Bartosz Golaszewski
On Wed, Dec 19, 2018 at 01:49:30PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Dec 19, 2018 at 01:39:09PM +0100, Daniel Vetter wrote:
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -327,6 +327,7 @@ struct driver_attribute {
> > #define DRIVER_ATTR_WO(_name) \
> > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> >
> > +
> > extern int __must_check driver_create_file(struct device_driver *driver,
> > const struct driver_attribute *attr);
> > extern void driver_remove_file(struct device_driver *driver,
> > --
>
> I'll edit away this last chunk when I apply the patch :)
Oops sry. I also forgot to do the s/Locking/Looking/ in the commit message
:-/
>
> thanks for making the changes.
>
> greg k-h
Thanks, Daniel
--
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-19 13:18 ` Daniel Vetter
(?)
@ 2018-12-19 13:26 ` Daniel Vetter
2018-12-19 15:00 ` Greg Kroah-Hartman
-1 siblings, 1 reply; 29+ messages in thread
From: Daniel Vetter @ 2018-12-19 13:26 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Daniel Vetter, DRI Development, Intel Graphics Development, LKML,
Rafael J. Wysocki, Ramalingam C, Arend van Spriel,
Andy Shevchenko, Geert Uytterhoeven, Bartosz Golaszewski,
Heikki Krogerus, Vivek Gautam, Joe Perches, Daniel Vetter
On Wed, Dec 19, 2018 at 02:18:25PM +0100, Daniel Vetter wrote:
> On Wed, Dec 19, 2018 at 01:49:30PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Dec 19, 2018 at 01:39:09PM +0100, Daniel Vetter wrote:
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -327,6 +327,7 @@ struct driver_attribute {
> > > #define DRIVER_ATTR_WO(_name) \
> > > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> > >
> > > +
> > > extern int __must_check driver_create_file(struct device_driver *driver,
> > > const struct driver_attribute *attr);
> > > extern void driver_remove_file(struct device_driver *driver,
> > > --
> >
> > I'll edit away this last chunk when I apply the patch :)
>
> Oops sry. I also forgot to do the s/Locking/Looking/ in the commit message
> :-/
And I also forgot Rafael's r-b :-(
-Daniel
> >
> > thanks for making the changes.
> >
> > greg k-h
>
> Thanks, Daniel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-19 13:26 ` Daniel Vetter
@ 2018-12-19 15:00 ` Greg Kroah-Hartman
0 siblings, 0 replies; 29+ messages in thread
From: Greg Kroah-Hartman @ 2018-12-19 15:00 UTC (permalink / raw)
To: DRI Development, Intel Graphics Development, LKML,
Rafael J. Wysocki, Ramalingam C, Arend van Spriel,
Andy Shevchenko, Geert Uytterhoeven, Bartosz Golaszewski,
Heikki Krogerus, Vivek Gautam, Joe Perches, Daniel Vetter
On Wed, Dec 19, 2018 at 02:26:22PM +0100, Daniel Vetter wrote:
> On Wed, Dec 19, 2018 at 02:18:25PM +0100, Daniel Vetter wrote:
> > On Wed, Dec 19, 2018 at 01:49:30PM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Dec 19, 2018 at 01:39:09PM +0100, Daniel Vetter wrote:
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -327,6 +327,7 @@ struct driver_attribute {
> > > > #define DRIVER_ATTR_WO(_name) \
> > > > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> > > >
> > > > +
> > > > extern int __must_check driver_create_file(struct device_driver *driver,
> > > > const struct driver_attribute *attr);
> > > > extern void driver_remove_file(struct device_driver *driver,
> > > > --
> > >
> > > I'll edit away this last chunk when I apply the patch :)
> >
> > Oops sry. I also forgot to do the s/Locking/Looking/ in the commit message
> > :-/
>
> And I also forgot Rafael's r-b :-(
Ok, rebuilt with both of these things now fixed up, thanks.
greg k-h
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
@ 2018-12-19 15:00 ` Greg Kroah-Hartman
0 siblings, 0 replies; 29+ messages in thread
From: Greg Kroah-Hartman @ 2018-12-19 15:00 UTC (permalink / raw)
To: DRI Development, Intel Graphics Development, LKML,
Rafael J. Wysocki, Ramalingam C, Arend van Spriel,
Andy Shevchenko, Geert Uytterhoeven, Bartosz Golaszewski,
Heikki Krogerus, Vivek Gautam, Joe Perches, Daniel Vetter
On Wed, Dec 19, 2018 at 02:26:22PM +0100, Daniel Vetter wrote:
> On Wed, Dec 19, 2018 at 02:18:25PM +0100, Daniel Vetter wrote:
> > On Wed, Dec 19, 2018 at 01:49:30PM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Dec 19, 2018 at 01:39:09PM +0100, Daniel Vetter wrote:
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -327,6 +327,7 @@ struct driver_attribute {
> > > > #define DRIVER_ATTR_WO(_name) \
> > > > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> > > >
> > > > +
> > > > extern int __must_check driver_create_file(struct device_driver *driver,
> > > > const struct driver_attribute *attr);
> > > > extern void driver_remove_file(struct device_driver *driver,
> > > > --
> > >
> > > I'll edit away this last chunk when I apply the patch :)
> >
> > Oops sry. I also forgot to do the s/Locking/Looking/ in the commit message
> > :-/
>
> And I also forgot Rafael's r-b :-(
Ok, rebuilt with both of these things now fixed up, thanks.
greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-19 12:39 ` Daniel Vetter
@ 2018-12-19 14:21 ` Rafael J. Wysocki
-1 siblings, 0 replies; 29+ messages in thread
From: Rafael J. Wysocki @ 2018-12-19 14:21 UTC (permalink / raw)
To: Daniel Vetter
Cc: dri-devel, intel-gfx, Linux Kernel Mailing List,
Rafael J. Wysocki, Greg Kroah-Hartman, ramalingam.c, aspriel,
Andy Shevchenko, Geert Uytterhoeven, brgl, Heikki Krogerus,
Vivek Gautam, Joe Perches, Daniel Vetter
On Wed, Dec 19, 2018 at 1:39 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> This is the much more correct fix for my earlier attempt at:
>
> https://lkml.org/lkml/2018/12/10/118
>
> Short recap:
>
> - There's not actually a locking issue, it's just lockdep being a bit
> too eager to complain about a possible deadlock.
>
> - Contrary to what I claimed the real problem is recursion on
> kn->count. Greg pointed me at sysfs_break_active_protection(), used
> by the scsi subsystem to allow a sysfs file to unbind itself. That
> would be a real deadlock, which isn't what's happening here. Also,
> breaking the active protection means we'd need to manually handle
> all the lifetime fun.
>
> - With Rafael we discussed the task_work approach, which kinda works,
> but has two downsides: It's a functional change for a lockdep
> annotation issue, and it won't work for the bind file (which needs
> to get the errno from the driver load function back to userspace).
>
> - Greg also asked why this never showed up: To hit this you need to
> unregister a 2nd driver from the unload code of your first driver. I
> guess only gpus do that. The bug has always been there, but only
> with a recent patch series did we add more locks so that lockdep
> built a chain from unbinding the snd-hda driver to the
> acpi_video_unregister call.
>
> Full lockdep splat:
>
> [12301.898799] ============================================
> [12301.898805] WARNING: possible recursive locking detected
> [12301.898811] 4.20.0-rc7+ #84 Not tainted
> [12301.898815] --------------------------------------------
> [12301.898821] bash/5297 is trying to acquire lock:
> [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> [12301.898841] but task is already holding lock:
> [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> [12301.898856] other info that might help us debug this:
> [12301.898862] Possible unsafe locking scenario:
> [12301.898867] CPU0
> [12301.898870] ----
> [12301.898874] lock(kn->count#39);
> [12301.898879] lock(kn->count#39);
> [12301.898883] *** DEADLOCK ***
> [12301.898891] May be due to missing lock nesting notation
> [12301.898899] 5 locks held by bash/5297:
> [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> [12301.898960] stack backtrace:
> [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> [12301.898982] Call Trace:
> [12301.898989] dump_stack+0x67/0x9b
> [12301.898997] __lock_acquire+0x6ad/0x1410
> [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899010] ? find_held_lock+0x2d/0x90
> [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> [12301.899023] ? find_held_lock+0x2d/0x90
> [12301.899030] ? lock_acquire+0x90/0x180
> [12301.899036] lock_acquire+0x90/0x180
> [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899049] __kernfs_remove+0x296/0x310
> [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899060] ? kernfs_name_hash+0xd/0x80
> [12301.899066] ? kernfs_find_ns+0x6c/0x100
> [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899080] bus_remove_driver+0x92/0xa0
> [12301.899085] acpi_video_unregister+0x24/0x40
> [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> [12301.899169] pci_device_remove+0x36/0xb0
> [12301.899176] device_release_driver_internal+0x185/0x240
> [12301.899183] unbind_store+0xaf/0x180
> [12301.899189] kernfs_fop_write+0x104/0x190
> [12301.899195] __vfs_write+0x31/0x180
> [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> [12301.899216] ? __sb_start_write+0x13c/0x1a0
> [12301.899221] ? vfs_write+0x17f/0x1b0
> [12301.899227] vfs_write+0xb9/0x1b0
> [12301.899233] ksys_write+0x50/0xc0
> [12301.899239] do_syscall_64+0x4b/0x180
> [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [12301.899253] RIP: 0033:0x7f452ac7f7a4
> [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
>
> Locking around I've noticed that usb and i2c already handle similar
> recursion problems, where a sysfs file can unbind the same type of
> sysfs somewhere else in the hierarchy. Relevant commits are:
>
> commit 356c05d58af05d582e634b54b40050c73609617b
> Author: Alan Stern <stern@rowland.harvard.edu>
> Date: Mon May 14 13:30:03 2012 -0400
>
> sysfs: get rid of some lockdep false positives
>
> commit e9b526fe704812364bca07edd15eadeba163ebfb
> Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> Date: Fri May 17 14:56:35 2013 +0200
>
> i2c: suppress lockdep warning on delete_device
>
> Implement the same trick for driver bind/unbind.
>
> v2: Put the macro into bus.c (Greg).
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
It would have been fine to retain the R-by:
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Cc: Arend van Spriel <aspriel@gmail.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/base/bus.c | 7 +++++--
> include/linux/device.h | 1 +
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 8bfd27ec73d6..585e2e1c9c8f 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -31,6 +31,9 @@ static struct kset *system_kset;
>
> #define to_drv_attr(_attr) container_of(_attr, struct driver_attribute, attr)
>
> +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> + struct driver_attribute driver_attr_##_name = \
> + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
>
> static int __must_check bus_rescan_devices_helper(struct device *dev,
> void *data);
> @@ -195,7 +198,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> bus_put(bus);
> return err;
> }
> -static DRIVER_ATTR_WO(unbind);
> +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
>
> /*
> * Manually attach a device to a driver.
> @@ -231,7 +234,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> bus_put(bus);
> return err;
> }
> -static DRIVER_ATTR_WO(bind);
> +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
>
> static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1b25c7a43f4c..41424d6e27c7 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -327,6 +327,7 @@ struct driver_attribute {
> #define DRIVER_ATTR_WO(_name) \
> struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
>
> +
> extern int __must_check driver_create_file(struct device_driver *driver,
> const struct driver_attribute *attr);
> extern void driver_remove_file(struct device_driver *driver,
> --
> 2.20.0.rc1
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
@ 2018-12-19 14:21 ` Rafael J. Wysocki
0 siblings, 0 replies; 29+ messages in thread
From: Rafael J. Wysocki @ 2018-12-19 14:21 UTC (permalink / raw)
To: Daniel Vetter
Cc: aspriel, Heikki Krogerus, Geert Uytterhoeven, Rafael J. Wysocki,
intel-gfx, Linux Kernel Mailing List, dri-devel, Vivek Gautam,
Greg Kroah-Hartman, Joe Perches, Daniel Vetter, Andy Shevchenko,
brgl
On Wed, Dec 19, 2018 at 1:39 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> This is the much more correct fix for my earlier attempt at:
>
> https://lkml.org/lkml/2018/12/10/118
>
> Short recap:
>
> - There's not actually a locking issue, it's just lockdep being a bit
> too eager to complain about a possible deadlock.
>
> - Contrary to what I claimed the real problem is recursion on
> kn->count. Greg pointed me at sysfs_break_active_protection(), used
> by the scsi subsystem to allow a sysfs file to unbind itself. That
> would be a real deadlock, which isn't what's happening here. Also,
> breaking the active protection means we'd need to manually handle
> all the lifetime fun.
>
> - With Rafael we discussed the task_work approach, which kinda works,
> but has two downsides: It's a functional change for a lockdep
> annotation issue, and it won't work for the bind file (which needs
> to get the errno from the driver load function back to userspace).
>
> - Greg also asked why this never showed up: To hit this you need to
> unregister a 2nd driver from the unload code of your first driver. I
> guess only gpus do that. The bug has always been there, but only
> with a recent patch series did we add more locks so that lockdep
> built a chain from unbinding the snd-hda driver to the
> acpi_video_unregister call.
>
> Full lockdep splat:
>
> [12301.898799] ============================================
> [12301.898805] WARNING: possible recursive locking detected
> [12301.898811] 4.20.0-rc7+ #84 Not tainted
> [12301.898815] --------------------------------------------
> [12301.898821] bash/5297 is trying to acquire lock:
> [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> [12301.898841] but task is already holding lock:
> [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> [12301.898856] other info that might help us debug this:
> [12301.898862] Possible unsafe locking scenario:
> [12301.898867] CPU0
> [12301.898870] ----
> [12301.898874] lock(kn->count#39);
> [12301.898879] lock(kn->count#39);
> [12301.898883] *** DEADLOCK ***
> [12301.898891] May be due to missing lock nesting notation
> [12301.898899] 5 locks held by bash/5297:
> [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> [12301.898960] stack backtrace:
> [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> [12301.898982] Call Trace:
> [12301.898989] dump_stack+0x67/0x9b
> [12301.898997] __lock_acquire+0x6ad/0x1410
> [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899010] ? find_held_lock+0x2d/0x90
> [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> [12301.899023] ? find_held_lock+0x2d/0x90
> [12301.899030] ? lock_acquire+0x90/0x180
> [12301.899036] lock_acquire+0x90/0x180
> [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899049] __kernfs_remove+0x296/0x310
> [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899060] ? kernfs_name_hash+0xd/0x80
> [12301.899066] ? kernfs_find_ns+0x6c/0x100
> [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> [12301.899080] bus_remove_driver+0x92/0xa0
> [12301.899085] acpi_video_unregister+0x24/0x40
> [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> [12301.899169] pci_device_remove+0x36/0xb0
> [12301.899176] device_release_driver_internal+0x185/0x240
> [12301.899183] unbind_store+0xaf/0x180
> [12301.899189] kernfs_fop_write+0x104/0x190
> [12301.899195] __vfs_write+0x31/0x180
> [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> [12301.899216] ? __sb_start_write+0x13c/0x1a0
> [12301.899221] ? vfs_write+0x17f/0x1b0
> [12301.899227] vfs_write+0xb9/0x1b0
> [12301.899233] ksys_write+0x50/0xc0
> [12301.899239] do_syscall_64+0x4b/0x180
> [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [12301.899253] RIP: 0033:0x7f452ac7f7a4
> [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
>
> Locking around I've noticed that usb and i2c already handle similar
> recursion problems, where a sysfs file can unbind the same type of
> sysfs somewhere else in the hierarchy. Relevant commits are:
>
> commit 356c05d58af05d582e634b54b40050c73609617b
> Author: Alan Stern <stern@rowland.harvard.edu>
> Date: Mon May 14 13:30:03 2012 -0400
>
> sysfs: get rid of some lockdep false positives
>
> commit e9b526fe704812364bca07edd15eadeba163ebfb
> Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> Date: Fri May 17 14:56:35 2013 +0200
>
> i2c: suppress lockdep warning on delete_device
>
> Implement the same trick for driver bind/unbind.
>
> v2: Put the macro into bus.c (Greg).
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
It would have been fine to retain the R-by:
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Cc: Arend van Spriel <aspriel@gmail.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/base/bus.c | 7 +++++--
> include/linux/device.h | 1 +
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 8bfd27ec73d6..585e2e1c9c8f 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -31,6 +31,9 @@ static struct kset *system_kset;
>
> #define to_drv_attr(_attr) container_of(_attr, struct driver_attribute, attr)
>
> +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> + struct driver_attribute driver_attr_##_name = \
> + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
>
> static int __must_check bus_rescan_devices_helper(struct device *dev,
> void *data);
> @@ -195,7 +198,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> bus_put(bus);
> return err;
> }
> -static DRIVER_ATTR_WO(unbind);
> +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
>
> /*
> * Manually attach a device to a driver.
> @@ -231,7 +234,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> bus_put(bus);
> return err;
> }
> -static DRIVER_ATTR_WO(bind);
> +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
>
> static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1b25c7a43f4c..41424d6e27c7 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -327,6 +327,7 @@ struct driver_attribute {
> #define DRIVER_ATTR_WO(_name) \
> struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
>
> +
> extern int __must_check driver_create_file(struct device_driver *driver,
> const struct driver_attribute *attr);
> extern void driver_remove_file(struct device_driver *driver,
> --
> 2.20.0.rc1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
2018-12-19 14:21 ` Rafael J. Wysocki
@ 2018-12-19 15:23 ` Daniel Vetter
-1 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2018-12-19 15:23 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Daniel Vetter, dri-devel, intel-gfx, Linux Kernel Mailing List,
Greg Kroah-Hartman, ramalingam.c, aspriel, Andy Shevchenko,
Geert Uytterhoeven, brgl, Heikki Krogerus, Vivek Gautam,
Joe Perches, Daniel Vetter
On Wed, Dec 19, 2018 at 03:21:56PM +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 19, 2018 at 1:39 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > This is the much more correct fix for my earlier attempt at:
> >
> > https://lkml.org/lkml/2018/12/10/118
> >
> > Short recap:
> >
> > - There's not actually a locking issue, it's just lockdep being a bit
> > too eager to complain about a possible deadlock.
> >
> > - Contrary to what I claimed the real problem is recursion on
> > kn->count. Greg pointed me at sysfs_break_active_protection(), used
> > by the scsi subsystem to allow a sysfs file to unbind itself. That
> > would be a real deadlock, which isn't what's happening here. Also,
> > breaking the active protection means we'd need to manually handle
> > all the lifetime fun.
> >
> > - With Rafael we discussed the task_work approach, which kinda works,
> > but has two downsides: It's a functional change for a lockdep
> > annotation issue, and it won't work for the bind file (which needs
> > to get the errno from the driver load function back to userspace).
> >
> > - Greg also asked why this never showed up: To hit this you need to
> > unregister a 2nd driver from the unload code of your first driver. I
> > guess only gpus do that. The bug has always been there, but only
> > with a recent patch series did we add more locks so that lockdep
> > built a chain from unbinding the snd-hda driver to the
> > acpi_video_unregister call.
> >
> > Full lockdep splat:
> >
> > [12301.898799] ============================================
> > [12301.898805] WARNING: possible recursive locking detected
> > [12301.898811] 4.20.0-rc7+ #84 Not tainted
> > [12301.898815] --------------------------------------------
> > [12301.898821] bash/5297 is trying to acquire lock:
> > [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.898841] but task is already holding lock:
> > [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > [12301.898856] other info that might help us debug this:
> > [12301.898862] Possible unsafe locking scenario:
> > [12301.898867] CPU0
> > [12301.898870] ----
> > [12301.898874] lock(kn->count#39);
> > [12301.898879] lock(kn->count#39);
> > [12301.898883] *** DEADLOCK ***
> > [12301.898891] May be due to missing lock nesting notation
> > [12301.898899] 5 locks held by bash/5297:
> > [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> > [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> > [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> > [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> > [12301.898960] stack backtrace:
> > [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> > [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> > [12301.898982] Call Trace:
> > [12301.898989] dump_stack+0x67/0x9b
> > [12301.898997] __lock_acquire+0x6ad/0x1410
> > [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899010] ? find_held_lock+0x2d/0x90
> > [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> > [12301.899023] ? find_held_lock+0x2d/0x90
> > [12301.899030] ? lock_acquire+0x90/0x180
> > [12301.899036] lock_acquire+0x90/0x180
> > [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899049] __kernfs_remove+0x296/0x310
> > [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899060] ? kernfs_name_hash+0xd/0x80
> > [12301.899066] ? kernfs_find_ns+0x6c/0x100
> > [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899080] bus_remove_driver+0x92/0xa0
> > [12301.899085] acpi_video_unregister+0x24/0x40
> > [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> > [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> > [12301.899169] pci_device_remove+0x36/0xb0
> > [12301.899176] device_release_driver_internal+0x185/0x240
> > [12301.899183] unbind_store+0xaf/0x180
> > [12301.899189] kernfs_fop_write+0x104/0x190
> > [12301.899195] __vfs_write+0x31/0x180
> > [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> > [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> > [12301.899216] ? __sb_start_write+0x13c/0x1a0
> > [12301.899221] ? vfs_write+0x17f/0x1b0
> > [12301.899227] vfs_write+0xb9/0x1b0
> > [12301.899233] ksys_write+0x50/0xc0
> > [12301.899239] do_syscall_64+0x4b/0x180
> > [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [12301.899253] RIP: 0033:0x7f452ac7f7a4
> > [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> > [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> > [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> > [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> > [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> > [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
> >
> > Locking around I've noticed that usb and i2c already handle similar
> > recursion problems, where a sysfs file can unbind the same type of
> > sysfs somewhere else in the hierarchy. Relevant commits are:
> >
> > commit 356c05d58af05d582e634b54b40050c73609617b
> > Author: Alan Stern <stern@rowland.harvard.edu>
> > Date: Mon May 14 13:30:03 2012 -0400
> >
> > sysfs: get rid of some lockdep false positives
> >
> > commit e9b526fe704812364bca07edd15eadeba163ebfb
> > Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> > Date: Fri May 17 14:56:35 2013 +0200
> >
> > i2c: suppress lockdep warning on delete_device
> >
> > Implement the same trick for driver bind/unbind.
> >
> > v2: Put the macro into bus.c (Greg).
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>
> It would have been fine to retain the R-by:
>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Yeah sorry I rushed it, but Greg fixed it up for me now.
Thanks for reviewing, much appreciated.
-Daniel
>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Cc: Arend van Spriel <aspriel@gmail.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> > Cc: Joe Perches <joe@perches.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > drivers/base/bus.c | 7 +++++--
> > include/linux/device.h | 1 +
> > 2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index 8bfd27ec73d6..585e2e1c9c8f 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -31,6 +31,9 @@ static struct kset *system_kset;
> >
> > #define to_drv_attr(_attr) container_of(_attr, struct driver_attribute, attr)
> >
> > +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> > + struct driver_attribute driver_attr_##_name = \
> > + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
> >
> > static int __must_check bus_rescan_devices_helper(struct device *dev,
> > void *data);
> > @@ -195,7 +198,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> > bus_put(bus);
> > return err;
> > }
> > -static DRIVER_ATTR_WO(unbind);
> > +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
> >
> > /*
> > * Manually attach a device to a driver.
> > @@ -231,7 +234,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> > bus_put(bus);
> > return err;
> > }
> > -static DRIVER_ATTR_WO(bind);
> > +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
> >
> > static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> > {
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 1b25c7a43f4c..41424d6e27c7 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -327,6 +327,7 @@ struct driver_attribute {
> > #define DRIVER_ATTR_WO(_name) \
> > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> >
> > +
> > extern int __must_check driver_create_file(struct device_driver *driver,
> > const struct driver_attribute *attr);
> > extern void driver_remove_file(struct device_driver *driver,
> > --
> > 2.20.0.rc1
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] sysfs: Disable lockdep for driver bind/unbind files
@ 2018-12-19 15:23 ` Daniel Vetter
0 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2018-12-19 15:23 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: aspriel, Heikki Krogerus, Geert Uytterhoeven, Greg Kroah-Hartman,
intel-gfx, Linux Kernel Mailing List, dri-devel, Vivek Gautam,
Daniel Vetter, Joe Perches, Daniel Vetter, Andy Shevchenko, brgl
On Wed, Dec 19, 2018 at 03:21:56PM +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 19, 2018 at 1:39 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > This is the much more correct fix for my earlier attempt at:
> >
> > https://lkml.org/lkml/2018/12/10/118
> >
> > Short recap:
> >
> > - There's not actually a locking issue, it's just lockdep being a bit
> > too eager to complain about a possible deadlock.
> >
> > - Contrary to what I claimed the real problem is recursion on
> > kn->count. Greg pointed me at sysfs_break_active_protection(), used
> > by the scsi subsystem to allow a sysfs file to unbind itself. That
> > would be a real deadlock, which isn't what's happening here. Also,
> > breaking the active protection means we'd need to manually handle
> > all the lifetime fun.
> >
> > - With Rafael we discussed the task_work approach, which kinda works,
> > but has two downsides: It's a functional change for a lockdep
> > annotation issue, and it won't work for the bind file (which needs
> > to get the errno from the driver load function back to userspace).
> >
> > - Greg also asked why this never showed up: To hit this you need to
> > unregister a 2nd driver from the unload code of your first driver. I
> > guess only gpus do that. The bug has always been there, but only
> > with a recent patch series did we add more locks so that lockdep
> > built a chain from unbinding the snd-hda driver to the
> > acpi_video_unregister call.
> >
> > Full lockdep splat:
> >
> > [12301.898799] ============================================
> > [12301.898805] WARNING: possible recursive locking detected
> > [12301.898811] 4.20.0-rc7+ #84 Not tainted
> > [12301.898815] --------------------------------------------
> > [12301.898821] bash/5297 is trying to acquire lock:
> > [12301.898826] 00000000f61c6093 (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.898841] but task is already holding lock:
> > [12301.898847] 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > [12301.898856] other info that might help us debug this:
> > [12301.898862] Possible unsafe locking scenario:
> > [12301.898867] CPU0
> > [12301.898870] ----
> > [12301.898874] lock(kn->count#39);
> > [12301.898879] lock(kn->count#39);
> > [12301.898883] *** DEADLOCK ***
> > [12301.898891] May be due to missing lock nesting notation
> > [12301.898899] 5 locks held by bash/5297:
> > [12301.898903] #0: 00000000cd800e54 (sb_writers#4){.+.+}, at: vfs_write+0x17f/0x1b0
> > [12301.898915] #1: 000000000465e7c2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xd3/0x190
> > [12301.898925] #2: 000000005f634021 (kn->count#39){++++}, at: kernfs_fop_write+0xdc/0x190
> > [12301.898936] #3: 00000000414ef7ac (&dev->mutex){....}, at: device_release_driver_internal+0x34/0x240
> > [12301.898950] #4: 000000003218fbdf (register_count_mutex){+.+.}, at: acpi_video_unregister+0xe/0x40
> > [12301.898960] stack backtrace:
> > [12301.898968] CPU: 1 PID: 5297 Comm: bash Not tainted 4.20.0-rc7+ #84
> > [12301.898974] Hardware name: Hewlett-Packard HP EliteBook 8460p/161C, BIOS 68SCF Ver. F.01 03/11/2011
> > [12301.898982] Call Trace:
> > [12301.898989] dump_stack+0x67/0x9b
> > [12301.898997] __lock_acquire+0x6ad/0x1410
> > [12301.899003] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899010] ? find_held_lock+0x2d/0x90
> > [12301.899017] ? mutex_spin_on_owner+0xe4/0x150
> > [12301.899023] ? find_held_lock+0x2d/0x90
> > [12301.899030] ? lock_acquire+0x90/0x180
> > [12301.899036] lock_acquire+0x90/0x180
> > [12301.899042] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899049] __kernfs_remove+0x296/0x310
> > [12301.899055] ? kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899060] ? kernfs_name_hash+0xd/0x80
> > [12301.899066] ? kernfs_find_ns+0x6c/0x100
> > [12301.899073] kernfs_remove_by_name_ns+0x3b/0x80
> > [12301.899080] bus_remove_driver+0x92/0xa0
> > [12301.899085] acpi_video_unregister+0x24/0x40
> > [12301.899127] i915_driver_unload+0x42/0x130 [i915]
> > [12301.899160] i915_pci_remove+0x19/0x30 [i915]
> > [12301.899169] pci_device_remove+0x36/0xb0
> > [12301.899176] device_release_driver_internal+0x185/0x240
> > [12301.899183] unbind_store+0xaf/0x180
> > [12301.899189] kernfs_fop_write+0x104/0x190
> > [12301.899195] __vfs_write+0x31/0x180
> > [12301.899203] ? rcu_read_lock_sched_held+0x6f/0x80
> > [12301.899209] ? rcu_sync_lockdep_assert+0x29/0x50
> > [12301.899216] ? __sb_start_write+0x13c/0x1a0
> > [12301.899221] ? vfs_write+0x17f/0x1b0
> > [12301.899227] vfs_write+0xb9/0x1b0
> > [12301.899233] ksys_write+0x50/0xc0
> > [12301.899239] do_syscall_64+0x4b/0x180
> > [12301.899247] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [12301.899253] RIP: 0033:0x7f452ac7f7a4
> > [12301.899259] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 aa f0 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> > [12301.899273] RSP: 002b:00007ffceafa6918 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > [12301.899282] RAX: ffffffffffffffda RBX: 000000000000000d RCX: 00007f452ac7f7a4
> > [12301.899288] RDX: 000000000000000d RSI: 00005612a1abf7c0 RDI: 0000000000000001
> > [12301.899295] RBP: 00005612a1abf7c0 R08: 000000000000000a R09: 00005612a1c46730
> > [12301.899301] R10: 000000000000000a R11: 0000000000000246 R12: 000000000000000d
> > [12301.899308] R13: 0000000000000001 R14: 00007f452af4a740 R15: 000000000000000d
> >
> > Locking around I've noticed that usb and i2c already handle similar
> > recursion problems, where a sysfs file can unbind the same type of
> > sysfs somewhere else in the hierarchy. Relevant commits are:
> >
> > commit 356c05d58af05d582e634b54b40050c73609617b
> > Author: Alan Stern <stern@rowland.harvard.edu>
> > Date: Mon May 14 13:30:03 2012 -0400
> >
> > sysfs: get rid of some lockdep false positives
> >
> > commit e9b526fe704812364bca07edd15eadeba163ebfb
> > Author: Alexander Sverdlin <alexander.sverdlin@nsn.com>
> > Date: Fri May 17 14:56:35 2013 +0200
> >
> > i2c: suppress lockdep warning on delete_device
> >
> > Implement the same trick for driver bind/unbind.
> >
> > v2: Put the macro into bus.c (Greg).
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>
> It would have been fine to retain the R-by:
>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Yeah sorry I rushed it, but Greg fixed it up for me now.
Thanks for reviewing, much appreciated.
-Daniel
>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Cc: Arend van Spriel <aspriel@gmail.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> > Cc: Joe Perches <joe@perches.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > drivers/base/bus.c | 7 +++++--
> > include/linux/device.h | 1 +
> > 2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index 8bfd27ec73d6..585e2e1c9c8f 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -31,6 +31,9 @@ static struct kset *system_kset;
> >
> > #define to_drv_attr(_attr) container_of(_attr, struct driver_attribute, attr)
> >
> > +#define DRIVER_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
> > + struct driver_attribute driver_attr_##_name = \
> > + __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
> >
> > static int __must_check bus_rescan_devices_helper(struct device *dev,
> > void *data);
> > @@ -195,7 +198,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> > bus_put(bus);
> > return err;
> > }
> > -static DRIVER_ATTR_WO(unbind);
> > +static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
> >
> > /*
> > * Manually attach a device to a driver.
> > @@ -231,7 +234,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
> > bus_put(bus);
> > return err;
> > }
> > -static DRIVER_ATTR_WO(bind);
> > +static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
> >
> > static ssize_t show_drivers_autoprobe(struct bus_type *bus, char *buf)
> > {
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 1b25c7a43f4c..41424d6e27c7 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -327,6 +327,7 @@ struct driver_attribute {
> > #define DRIVER_ATTR_WO(_name) \
> > struct driver_attribute driver_attr_##_name = __ATTR_WO(_name)
> >
> > +
> > extern int __must_check driver_create_file(struct device_driver *driver,
> > const struct driver_attribute *attr);
> > extern void driver_remove_file(struct device_driver *driver,
> > --
> > 2.20.0.rc1
> >
--
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for sysfs: Disable lockdep for driver bind/unbind files (rev2)
2018-12-18 20:14 ` Daniel Vetter
` (6 preceding siblings ...)
(?)
@ 2018-12-19 13:07 ` Patchwork
-1 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2018-12-19 13:07 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: sysfs: Disable lockdep for driver bind/unbind files (rev2)
URL : https://patchwork.freedesktop.org/series/54238/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
edb688c07371 sysfs: Disable lockdep for driver bind/unbind files
-:106: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 356c05d58af0 ("sysfs: get rid of some lockdep false positives")'
#106:
commit 356c05d58af05d582e634b54b40050c73609617b
-:112: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit e9b526fe7048 ("i2c: suppress lockdep warning on delete_device")'
#112:
commit e9b526fe704812364bca07edd15eadeba163ebfb
-:153: WARNING:SYMBOLIC_PERMS: Symbolic permissions 'S_IWUSR' are not preferred. Consider using octal permissions '0200'.
#153: FILE: drivers/base/bus.c:201:
+static DRIVER_ATTR_IGNORE_LOCKDEP(unbind, S_IWUSR, NULL, unbind_store);
-:162: WARNING:SYMBOLIC_PERMS: Symbolic permissions 'S_IWUSR' are not preferred. Consider using octal permissions '0200'.
#162: FILE: drivers/base/bus.c:237:
+static DRIVER_ATTR_IGNORE_LOCKDEP(bind, S_IWUSR, NULL, bind_store);
-:174: CHECK:LINE_SPACING: Please don't use multiple blank lines
#174: FILE: include/linux/device.h:330:
+
-:177: WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <daniel.vetter@ffwll.ch>'
total: 2 errors, 3 warnings, 1 checks, 32 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread* ✓ Fi.CI.BAT: success for sysfs: Disable lockdep for driver bind/unbind files (rev2)
2018-12-18 20:14 ` Daniel Vetter
` (7 preceding siblings ...)
(?)
@ 2018-12-19 13:26 ` Patchwork
-1 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2018-12-19 13:26 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: sysfs: Disable lockdep for driver bind/unbind files (rev2)
URL : https://patchwork.freedesktop.org/series/54238/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5332 -> Patchwork_11124
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/54238/revisions/2/mbox/
Known issues
------------
Here are the changes found in Patchwork_11124 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@amdgpu/amd_basic@cs-compute:
- fi-kbl-8809g: NOTRUN -> FAIL [fdo#108094]
* igt@amdgpu/amd_prime@amd-to-i915:
- fi-kbl-8809g: NOTRUN -> FAIL [fdo#107341]
* igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
- fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362]
* {igt@runner@aborted}:
- fi-icl-y: NOTRUN -> FAIL [fdo#108070]
#### Possible fixes ####
* igt@amdgpu/amd_basic@userptr:
- fi-kbl-8809g: DMESG-WARN [fdo#108965] -> PASS
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7500u: FAIL [fdo#108767] -> PASS
* igt@kms_pipe_crc_basic@read-crc-pipe-a:
- fi-byt-clapper: FAIL [fdo#107362] -> PASS
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#107341]: https://bugs.freedesktop.org/show_bug.cgi?id=107341
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#108070]: https://bugs.freedesktop.org/show_bug.cgi?id=108070
[fdo#108094]: https://bugs.freedesktop.org/show_bug.cgi?id=108094
[fdo#108767]: https://bugs.freedesktop.org/show_bug.cgi?id=108767
[fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
Participating hosts (50 -> 45)
------------------------------
Additional (1): fi-icl-y
Missing (6): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus
Build changes
-------------
* Linux: CI_DRM_5332 -> Patchwork_11124
CI_DRM_5332: 29cd50b134a44bab74bfc8b275d24a32e140196c @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4750: f05c8c2739dce89185349703062784a7745cab14 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_11124: edb688c07371ed8a1fa3c60c635ec106b0ced850 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
edb688c07371 sysfs: Disable lockdep for driver bind/unbind files
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11124/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread* ✓ Fi.CI.IGT: success for sysfs: Disable lockdep for driver bind/unbind files (rev2)
2018-12-18 20:14 ` Daniel Vetter
` (8 preceding siblings ...)
(?)
@ 2018-12-19 14:44 ` Patchwork
-1 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2018-12-19 14:44 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: sysfs: Disable lockdep for driver bind/unbind files (rev2)
URL : https://patchwork.freedesktop.org/series/54238/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5332_full -> Patchwork_11124_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_11124_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_suspend@shrink:
- shard-skl: NOTRUN -> DMESG-WARN [fdo#108784]
* igt@kms_busy@extended-modeset-hang-newfb-render-a:
- shard-skl: NOTRUN -> DMESG-WARN [fdo#107956] +3
* igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
- shard-hsw: PASS -> DMESG-WARN [fdo#107956]
* igt@kms_busy@extended-modeset-hang-oldfb-with-reset-render-a:
- shard-iclb: PASS -> DMESG-WARN [fdo#107724] +19
* igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
- shard-glk: PASS -> FAIL [fdo#108145]
* igt@kms_color@pipe-b-ctm-negative:
- shard-kbl: PASS -> DMESG-WARN [fdo#103558] / [fdo#105602] +10
* igt@kms_color@pipe-b-degamma:
- shard-apl: PASS -> FAIL [fdo#104782]
* igt@kms_color@pipe-c-ctm-max:
- shard-apl: PASS -> FAIL [fdo#108147]
* igt@kms_cursor_crc@cursor-128x42-random:
- shard-glk: PASS -> FAIL [fdo#103232] +3
* igt@kms_cursor_crc@cursor-256x85-random:
- shard-apl: PASS -> FAIL [fdo#103232] +4
- shard-skl: NOTRUN -> FAIL [fdo#103232]
* igt@kms_cursor_legacy@cursora-vs-flipa-legacy:
- shard-apl: PASS -> DMESG-WARN [fdo#103558] / [fdo#105602]
* igt@kms_draw_crc@draw-method-xrgb2101010-render-untiled:
- shard-iclb: PASS -> WARN [fdo#108336] +5
* igt@kms_flip@flip-vs-dpms-interruptible:
- shard-iclb: PASS -> DMESG-WARN [fdo#107724] / [fdo#109093]
* igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-mmap-gtt:
- shard-iclb: PASS -> DMESG-FAIL [fdo#107724] +5
* igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-blt:
- shard-iclb: PASS -> DMESG-FAIL [fdo#107720] / [fdo#107724] +1
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-render:
- shard-apl: PASS -> FAIL [fdo#103167] +1
* igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
- shard-glk: PASS -> FAIL [fdo#103167] +4
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-blt:
- shard-iclb: PASS -> FAIL [fdo#103167]
* igt@kms_frontbuffer_tracking@fbcpsr-stridechange:
- shard-skl: NOTRUN -> FAIL [fdo#105683]
- shard-iclb: PASS -> FAIL [fdo#105683]
* igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-fullscreen:
- shard-iclb: PASS -> DMESG-WARN [fdo#107724] / [fdo#108336] +8
* igt@kms_frontbuffer_tracking@psr-suspend:
- shard-iclb: PASS -> INCOMPLETE [fdo#106978] / [fdo#107713]
* igt@kms_plane@pixel-format-pipe-a-planes:
- shard-iclb: NOTRUN -> FAIL [fdo#103166]
* igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
- shard-skl: PASS -> FAIL [fdo#107815]
* igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
- shard-skl: NOTRUN -> FAIL [fdo#108145] +1
* igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
- shard-iclb: PASS -> FAIL [fdo#103166]
* igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
- shard-skl: NOTRUN -> FAIL [fdo#103166] / [fdo#107815]
* igt@kms_setmode@basic:
- shard-hsw: PASS -> FAIL [fdo#99912]
* igt@pm_rpm@gem-idle:
- shard-skl: PASS -> INCOMPLETE [fdo#107807]
* igt@pm_rpm@legacy-planes:
- shard-skl: PASS -> INCOMPLETE [fdo#105959] / [fdo#107807]
#### Possible fixes ####
* igt@gem_workarounds@suspend-resume-context:
- shard-iclb: INCOMPLETE [fdo#107713] -> PASS
* igt@kms_available_modes_crc@available_mode_test_crc:
- shard-apl: FAIL [fdo#106641] -> PASS
* igt@kms_chv_cursor_fail@pipe-b-64x64-right-edge:
- shard-iclb: DMESG-WARN [fdo#107724] / [fdo#108336] -> PASS +2
* igt@kms_cursor_crc@cursor-128x42-sliding:
- shard-glk: FAIL [fdo#103232] -> PASS +1
* igt@kms_cursor_crc@cursor-64x64-sliding:
- shard-apl: FAIL [fdo#103232] -> PASS +3
* igt@kms_cursor_legacy@cursor-vs-flip-atomic-transitions:
- shard-iclb: FAIL [fdo#103355] -> PASS
* igt@kms_draw_crc@draw-method-xrgb2101010-blt-ytiled:
- shard-iclb: WARN [fdo#108336] -> PASS +2
* igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
- shard-glk: FAIL [fdo#105363] -> PASS
* igt@kms_flip@modeset-vs-vblank-race-interruptible:
- shard-glk: FAIL [fdo#103060] -> PASS
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-onoff:
- shard-apl: FAIL [fdo#103167] -> PASS +1
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
- shard-iclb: DMESG-FAIL [fdo#107724] -> PASS +2
* igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-move:
- shard-glk: FAIL [fdo#103167] -> PASS +5
* igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-mmap-cpu:
- shard-iclb: FAIL [fdo#103167] -> PASS +3
* igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
- shard-skl: INCOMPLETE [fdo#104108] / [fdo#107773] -> PASS +1
* igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
- shard-iclb: DMESG-FAIL [fdo#103166] / [fdo#107724] -> PASS
* igt@kms_plane@plane-position-covered-pipe-b-planes:
- shard-iclb: FAIL [fdo#103166] -> PASS
* igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb:
- shard-glk: FAIL [fdo#108145] -> PASS +2
* igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
- shard-glk: FAIL [fdo#103166] -> PASS
* igt@kms_plane_multiple@atomic-pipe-c-tiling-y:
- shard-apl: FAIL [fdo#103166] -> PASS +1
* igt@kms_psr@sprite_mmap_gtt:
- shard-iclb: DMESG-WARN [fdo#107724] -> PASS +10
* igt@kms_rotation_crc@multiplane-rotation-cropping-top:
- shard-glk: DMESG-WARN [fdo#105763] / [fdo#106538] -> PASS
* igt@kms_setmode@basic:
- shard-glk: FAIL [fdo#99912] -> PASS
* igt@pm_rpm@modeset-lpsp-stress-no-wait:
- shard-iclb: DMESG-WARN [fdo#108654] -> PASS
#### Warnings ####
* igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
- shard-iclb: FAIL [fdo#107725] -> DMESG-WARN [fdo#107724] / [fdo#108336]
* igt@kms_ccs@pipe-c-crc-primary-basic:
- shard-iclb: DMESG-WARN [fdo#107724] / [fdo#108336] -> FAIL [fdo#107725]
* igt@kms_cursor_crc@cursor-128x128-onscreen:
- shard-apl: FAIL [fdo#103232] -> DMESG-FAIL [fdo#103232] / [fdo#103558] / [fdo#105602]
* igt@kms_cursor_crc@cursor-64x21-onscreen:
- shard-iclb: FAIL [fdo#103232] -> DMESG-WARN [fdo#107724] / [fdo#108336] +1
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-mmap-cpu:
- shard-iclb: DMESG-FAIL [fdo#107724] -> FAIL [fdo#103167]
* igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
- shard-iclb: DMESG-WARN [fdo#107724] / [fdo#108336] -> FAIL [fdo#108948]
[fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
[fdo#103355]: https://bugs.freedesktop.org/show_bug.cgi?id=103355
[fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
[fdo#105683]: https://bugs.freedesktop.org/show_bug.cgi?id=105683
[fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
[fdo#105959]: https://bugs.freedesktop.org/show_bug.cgi?id=105959
[fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
[fdo#106641]: https://bugs.freedesktop.org/show_bug.cgi?id=106641
[fdo#106978]: https://bugs.freedesktop.org/show_bug.cgi?id=106978
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#107720]: https://bugs.freedesktop.org/show_bug.cgi?id=107720
[fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
[fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725
[fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
[fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
[fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
[fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
[fdo#108336]: https://bugs.freedesktop.org/show_bug.cgi?id=108336
[fdo#108654]: https://bugs.freedesktop.org/show_bug.cgi?id=108654
[fdo#108784]: https://bugs.freedesktop.org/show_bug.cgi?id=108784
[fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
[fdo#109093]: https://bugs.freedesktop.org/show_bug.cgi?id=109093
[fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
Participating hosts (7 -> 7)
------------------------------
No changes in participating hosts
Build changes
-------------
* Linux: CI_DRM_5332 -> Patchwork_11124
CI_DRM_5332: 29cd50b134a44bab74bfc8b275d24a32e140196c @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4750: f05c8c2739dce89185349703062784a7745cab14 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_11124: edb688c07371ed8a1fa3c60c635ec106b0ced850 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11124/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread