diff mbox series

sysfs: Disable lockdep for driver bind/unbind files

Message ID 20181218201443.4950-1-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show
Series sysfs: Disable lockdep for driver bind/unbind files | expand

Commit Message

Daniel Vetter Dec. 18, 2018, 8:14 p.m. UTC
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(-)

Comments

Rafael J. Wysocki Dec. 18, 2018, 9:40 p.m. UTC | #1
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
>
Greg KH Dec. 19, 2018, 7:01 a.m. UTC | #2
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
Daniel Vetter Dec. 19, 2018, 9:24 a.m. UTC | #3
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
Rafael J. Wysocki Dec. 19, 2018, 9:24 a.m. UTC | #4
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.
Rafael J. Wysocki Dec. 19, 2018, 9:30 a.m. UTC | #5
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.
Greg KH Dec. 19, 2018, 9:44 a.m. UTC | #6
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
diff mbox series

Patch

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);