diff mbox

i2c: suppress lockdep warning on delete_device

Message ID 51962903.9050901@nsn.com
State Accepted
Headers show

Commit Message

Alexander Sverdlin May 17, 2013, 12:56 p.m. UTC 
i2c: suppress lockdep warning on delete_device

Since commit 846f99749ab68bbc7f75c74fec305de675b1a1bf the following lockdep
warning is thrown in case i2c device is removed (via delete_device sysfs
attribute) which contains subdevices (e.g. i2c multiplexer):

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Alan Stern May 17, 2013, 2:42 p.m. UTC | #1 
On Fri, 17 May 2013, Alexander Sverdlin wrote:

> i2c: suppress lockdep warning on delete_device
> 
> Since commit 846f99749ab68bbc7f75c74fec305de675b1a1bf the following lockdep
> warning is thrown in case i2c device is removed (via delete_device sysfs
> attribute) which contains subdevices (e.g. i2c multiplexer):

Can you explain this in a little more detail?  Exactly what does the 
delete_device attribute do, when called for device D?

That is, what does a write to /sys/bus/i2c/devices/D/delete_device do?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Sverdlin May 17, 2013, 3:02 p.m. UTC | #2 
Hi!

On 05/17/2013 04:42 PM, ext Alan Stern wrote:
>> i2c: suppress lockdep warning on delete_device
>>
>> Since commit 846f99749ab68bbc7f75c74fec305de675b1a1bf the following lockdep
>> warning is thrown in case i2c device is removed (via delete_device sysfs
>> attribute) which contains subdevices (e.g. i2c multiplexer):
>
> Can you explain this in a little more detail?  Exactly what does the
> delete_device attribute do, when called for device D?
>
> That is, what does a write to /sys/bus/i2c/devices/D/delete_device do?

Like USB with its hubs, I2C structure could be tree-like, with I2C multiplexers.
delete_device attribute removes I2C device from the subsystem, which is usually not
a problem, except the case when device is a multiplexer and sub-devices should be
removed first. Here we have the problem: holding a "s_active" lock on
"delete_device" attribute of a parent (multiplexer) we need to delete children, but
they were created with the same static attribute "delete_device".

The safety of this operation is exactly the same as in USB case... Any more clever
lockdep annotation is exactly not so easy...
Alan Stern May 17, 2013, 3:31 p.m. UTC | #3 
On Fri, 17 May 2013, Alexander Sverdlin wrote:

> Hi!
> 
> On 05/17/2013 04:42 PM, ext Alan Stern wrote:
> >> i2c: suppress lockdep warning on delete_device
> >>
> >> Since commit 846f99749ab68bbc7f75c74fec305de675b1a1bf the following lockdep
> >> warning is thrown in case i2c device is removed (via delete_device sysfs
> >> attribute) which contains subdevices (e.g. i2c multiplexer):
> >
> > Can you explain this in a little more detail?  Exactly what does the
> > delete_device attribute do, when called for device D?
> >
> > That is, what does a write to /sys/bus/i2c/devices/D/delete_device do?
> 
> Like USB with its hubs, I2C structure could be tree-like, with I2C multiplexers.
> delete_device attribute removes I2C device from the subsystem, which is usually not
> a problem, except the case when device is a multiplexer and sub-devices should be
> removed first. Here we have the problem: holding a "s_active" lock on
> "delete_device" attribute of a parent (multiplexer) we need to delete children, but
> they were created with the same static attribute "delete_device".
> 
> The safety of this operation is exactly the same as in USB case... Any more clever
> lockdep annotation is exactly not so easy...

If I understand you correctly, if D is an I2C multiplexer then writing
to /sys/bus/i2c/devices/D/delete_device will get rid of all of D's
children (and their descendants) and will also get rid of D itself --
right?

That's _not_ what the USB attributes do.  For example, if D is a USB
hub then writing 0 to /sys/bus/usb/devices/D/bConfigurationValue will
get rid of all D's descendants but will not get rid of D.

Instead of doing it this way, the attribute method should call 
device_schedule_callback().  See commit d9a9cdfb078d.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Sverdlin May 17, 2013, 4:25 p.m. UTC | #4 
Hi!

On 05/17/2013 05:31 PM, ext Alan Stern wrote:
>>>> i2c: suppress lockdep warning on delete_device
>>>>
>>>> Since commit 846f99749ab68bbc7f75c74fec305de675b1a1bf the following lockdep
>>>> warning is thrown in case i2c device is removed (via delete_device sysfs
>>>> attribute) which contains subdevices (e.g. i2c multiplexer):
>>>
>>> Can you explain this in a little more detail?  Exactly what does the
>>> delete_device attribute do, when called for device D?
>>>
>>> That is, what does a write to /sys/bus/i2c/devices/D/delete_device do?
>>
>> Like USB with its hubs, I2C structure could be tree-like, with I2C multiplexers.
>> delete_device attribute removes I2C device from the subsystem, which is usually not
>> a problem, except the case when device is a multiplexer and sub-devices should be
>> removed first. Here we have the problem: holding a "s_active" lock on
>> "delete_device" attribute of a parent (multiplexer) we need to delete children, but
>> they were created with the same static attribute "delete_device".
>>
>> The safety of this operation is exactly the same as in USB case... Any more clever
>> lockdep annotation is exactly not so easy...
>
> If I understand you correctly, if D is an I2C multiplexer then writing
> to /sys/bus/i2c/devices/D/delete_device will get rid of all of D's
> children (and their descendants) and will also get rid of D itself --
> right?
>
> That's _not_ what the USB attributes do.  For example, if D is a USB
> hub then writing 0 to /sys/bus/usb/devices/D/bConfigurationValue will
> get rid of all D's descendants but will not get rid of D.

Well, seems that what I've described above wasn't precise enough...
Actually only "bus controllers" have "delete_device" attribute. Simple devices
on the bus do not have it. User write an address of the slave device to this
attribute to delete it. This works fine for simple devices. In case of multiplexer
(which is in turn also a "bus controller") there are nested "delete_device"
attributes. So it's only possible to delete child nodes and not the controller itself,
writing to its "delete_device". So from my POV, it fits to USB concept...

> Instead of doing it this way, the attribute method should call
> device_schedule_callback().  See commit d9a9cdfb078d.
Alan Stern May 17, 2013, 5:16 p.m. UTC | #5 
On Fri, 17 May 2013, Alexander Sverdlin wrote:

> Hi!
> 
> On 05/17/2013 05:31 PM, ext Alan Stern wrote:
> >>>> i2c: suppress lockdep warning on delete_device
> >>>>
> >>>> Since commit 846f99749ab68bbc7f75c74fec305de675b1a1bf the following lockdep
> >>>> warning is thrown in case i2c device is removed (via delete_device sysfs
> >>>> attribute) which contains subdevices (e.g. i2c multiplexer):
> >>>
> >>> Can you explain this in a little more detail?  Exactly what does the
> >>> delete_device attribute do, when called for device D?
> >>>
> >>> That is, what does a write to /sys/bus/i2c/devices/D/delete_device do?
> >>
> >> Like USB with its hubs, I2C structure could be tree-like, with I2C multiplexers.
> >> delete_device attribute removes I2C device from the subsystem, which is usually not
> >> a problem, except the case when device is a multiplexer and sub-devices should be
> >> removed first. Here we have the problem: holding a "s_active" lock on
> >> "delete_device" attribute of a parent (multiplexer) we need to delete children, but
> >> they were created with the same static attribute "delete_device".
> >>
> >> The safety of this operation is exactly the same as in USB case... Any more clever
> >> lockdep annotation is exactly not so easy...
> >
> > If I understand you correctly, if D is an I2C multiplexer then writing
> > to /sys/bus/i2c/devices/D/delete_device will get rid of all of D's
> > children (and their descendants) and will also get rid of D itself --
> > right?
> >
> > That's _not_ what the USB attributes do.  For example, if D is a USB
> > hub then writing 0 to /sys/bus/usb/devices/D/bConfigurationValue will
> > get rid of all D's descendants but will not get rid of D.
> 
> Well, seems that what I've described above wasn't precise enough...

That's why I asked what "exactly" the attribute does.  :-)

> Actually only "bus controllers" have "delete_device" attribute. Simple devices
> on the bus do not have it. User write an address of the slave device to this
> attribute to delete it. This works fine for simple devices. In case of multiplexer
> (which is in turn also a "bus controller") there are nested "delete_device"
> attributes. So it's only possible to delete child nodes and not the controller itself,
> writing to its "delete_device". So from my POV, it fits to USB concept...

Okay.  If writing to the attribute doesn't delete the object which the 
attribute is attached to, then your patch is the correct approach.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang May 17, 2013, 8:47 p.m. UTC | #6 
On Fri, May 17, 2013 at 02:56:35PM +0200, Alexander Sverdlin wrote:
> i2c: suppress lockdep warning on delete_device
> 
> Since commit 846f99749ab68bbc7f75c74fec305de675b1a1bf the following lockdep
> warning is thrown in case i2c device is removed (via delete_device sysfs
> attribute) which contains subdevices (e.g. i2c multiplexer):

Applied to for-current with Alan's ack, thanks!

I couldn't apply the patch due to whitespace problems. I fixed them here
because I wanted to have the patch in my very soon pull request. But
please have a look.


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

=============================================
[ INFO: possible recursive locking detected ]
3.8.7-0-sampleversion-fct #8 Tainted: G           O
---------------------------------------------
bash/3743 is trying to acquire lock:
  (s_active#110){++++.+}, at: [<ffffffff802b3048>] sysfs_hash_and_remove+0x58/0xc8

but task is already holding lock:
  (s_active#110){++++.+}, at: [<ffffffff802b3cb8>] sysfs_write_file+0xc8/0x208

other info that might help us debug this:
  Possible unsafe locking scenario:

        CPU0
        ----
   lock(s_active#110);
   lock(s_active#110);

  *** DEADLOCK ***

  May be due to missing lock nesting notation

4 locks held by bash/3743:
  #0:  (&buffer->mutex){+.+.+.}, at: [<ffffffff802b3c3c>] sysfs_write_file+0x4c/0x208
  #1:  (s_active#110){++++.+}, at: [<ffffffff802b3cb8>] sysfs_write_file+0xc8/0x208
  #2:  (&adap->userspace_clients_lock/1){+.+.+.}, at: [<ffffffff80454a18>] i2c_sysfs_delete_device+0x90/0x238
  #3:  (&__lockdep_no_validate__){......}, at: [<ffffffff803dcc24>] device_release_driver+0x24/0x48

stack backtrace:
Call Trace:
[<ffffffff80575cc8>] dump_stack+0x8/0x34
[<ffffffff801b50fc>] __lock_acquire+0x161c/0x2110
[<ffffffff801b5c3c>] lock_acquire+0x4c/0x70
[<ffffffff802b60cc>] sysfs_addrm_finish+0x19c/0x1e0
[<ffffffff802b3048>] sysfs_hash_and_remove+0x58/0xc8
[<ffffffff802b7d8c>] sysfs_remove_group+0x64/0x148
[<ffffffff803d990c>] device_remove_attrs+0x9c/0x1a8
[<ffffffff803d9b1c>] device_del+0x104/0x1d8
[<ffffffff803d9c18>] device_unregister+0x28/0x70
[<ffffffff8045505c>] i2c_del_adapter+0x1cc/0x328
[<ffffffff8045802c>] i2c_del_mux_adapter+0x14/0x38
[<ffffffffc025c108>] pca954x_remove+0x90/0xe0 [pca954x]
[<ffffffff804542f8>] i2c_device_remove+0x80/0xe8
[<ffffffff803dca9c>] __device_release_driver+0x74/0xf8
[<ffffffff803dcc2c>] device_release_driver+0x2c/0x48
[<ffffffff803dbc14>] bus_remove_device+0x13c/0x1d8
[<ffffffff803d9b24>] device_del+0x10c/0x1d8
[<ffffffff803d9c18>] device_unregister+0x28/0x70
[<ffffffff80454b08>] i2c_sysfs_delete_device+0x180/0x238
[<ffffffff802b3cd4>] sysfs_write_file+0xe4/0x208
[<ffffffff8023ddc4>] vfs_write+0xbc/0x160
[<ffffffff8023df6c>] SyS_write+0x54/0xd8
[<ffffffff8013d424>] handle_sys64+0x44/0x64

The problem is already known for USB and PCI subsystems. The reason is that
delete_device attribute is defined statically in i2c-core.c and used for all
devices in i2c subsystem.

Discussion of original USB problem:
http://lkml.indiana.edu/hypermail/linux/kernel/1204.3/01160.html

Commit 356c05d58af05d582e634b54b40050c73609617b introduced new macro to suppress
lockdep warnings for this special case and included workaround for USB code.

LKML discussion of the workaround:
http://lkml.indiana.edu/hypermail/linux/kernel/1205.1/03634.html

As i2c case is in principle the same, the same workaround could be used here.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nsn.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---

--- linux-next.orig/drivers/i2c/i2c-core.c
+++ linux-next/drivers/i2c/i2c-core.c
@@ -892,7 +892,8 @@  i2c_sysfs_delete_device(struct device *d
  }

  static DEVICE_ATTR(new_device, S_IWUSR, NULL, i2c_sysfs_new_device);
-static DEVICE_ATTR(delete_device, S_IWUSR, NULL, i2c_sysfs_delete_device);
+static DEVICE_ATTR_IGNORE_LOCKDEP(delete_device, S_IWUSR, NULL,
+                                  i2c_sysfs_delete_device);

  static struct attribute *i2c_adapter_attrs[] = {
  	&dev_attr_name.attr,