lockdep input layer warnings.

lockdep input layer warnings.

[Dave Jones]

One of our Fedora-devel users picked up on this this morning
in an 18rc1 based kernel.

		Dave


 Synaptics Touchpad, model: 1, fw: 5.9, id: 0x2c6ab1, caps: 0x884793/0x0
 serio: Synaptics pass-through port at isa0060/serio1/input0
 input: SynPS/2 Synaptics TouchPad as /class/input/input1
 PM: Adding info for serio:serio2
 
 =============================================
 [ INFO: possible recursive locking detected ]
 ---------------------------------------------
 kseriod/111 is trying to acquire lock:
  (&ps2dev->cmd_mutex/1){--..}, at: [<c05937fd>] ps2_command+0x6a/0x2bd
 
 but task is already holding lock:
  (&ps2dev->cmd_mutex/1){--..}, at: [<c05937fd>] ps2_command+0x6a/0x2bd
 
 other info that might help us debug this:
 4 locks held by kseriod/111:
  #0:  (serio_mutex){--..}, at: [<c060d6bb>] mutex_lock+0x21/0x24
  #1:  (&serio->drv_mutex){--..}, at: [<c060d6bb>] mutex_lock+0x21/0x24
  #2:  (psmouse_mutex){--..}, at: [<c060d6bb>] mutex_lock+0x21/0x24
  #3:  (&ps2dev->cmd_mutex/1){--..}, at: [<c05937fd>] ps2_command+0x6a/0x2bd
 
 stack backtrace:
  [<c0405167>] show_trace_log_lvl+0x54/0xfd
  [<c040571e>] show_trace+0xd/0x10
  [<c040583d>] dump_stack+0x19/0x1b
  [<c043bdb2>] __lock_acquire+0x76a/0x98d
  [<c043c546>] lock_acquire+0x4b/0x6d
  [<c060d793>] mutex_lock_nested+0xd5/0x251
  [<c05937fd>] ps2_command+0x6a/0x2bd
  [<c0598f41>] psmouse_sliced_command+0x1c/0x5a
  [<c059c45a>] synaptics_pt_write+0x1e/0x44
  [<c05936fb>] ps2_sendbyte+0x3e/0xd6
  [<c0593879>] ps2_command+0xe6/0x2bd
  [<c0598b3e>] psmouse_probe+0x1d/0x68
  [<c0599ad0>] psmouse_connect+0xe8/0x20f
  [<c05910d9>] serio_connect_driver+0x1e/0x2e
  [<c05910ff>] serio_driver_probe+0x16/0x18
  [<c0550076>] driver_probe_device+0x45/0x92
  [<c05500cb>] __device_attach+0x8/0xa
  [<c054fa0c>] bus_for_each_drv+0x3a/0x65
  [<c0550126>] device_attach+0x59/0x6e
  [<c054f74a>] bus_attach_device+0x16/0x2b
  [<c054eb03>] device_add+0x1f4/0x307
  [<c0591b9d>] serio_thread+0xfd/0x27c
  [<c04365dc>] kthread+0xc3/0xef
  [<c0402005>] kernel_thread_helper+0x5/0xb

-- 
http://www.codemonkey.org.uk

Re: lockdep input layer warnings.

[Dmitry Torokhov]

On 7/6/06, Dave Jones <davej@redhat.com> wrote:
> One of our Fedora-devel users picked up on this this morning
> in an 18rc1 based kernel.
>
>                Dave
>
>
>  Synaptics Touchpad, model: 1, fw: 5.9, id: 0x2c6ab1, caps: 0x884793/0x0
>  serio: Synaptics pass-through port at isa0060/serio1/input0
>  input: SynPS/2 Synaptics TouchPad as /class/input/input1
>  PM: Adding info for serio:serio2
>
>  =============================================
>  [ INFO: possible recursive locking detected ]
>  ---------------------------------------------

False alarm, there was a lockdep annotating patch for it in -mm.

-- 
Dmitry

Re: lockdep input layer warnings.

[Arjan van de Ven]

On Thu, 2006-07-06 at 14:37 -0400, Dmitry Torokhov wrote:
> On 7/6/06, Dave Jones <davej@redhat.com> wrote:
> > One of our Fedora-devel users picked up on this this morning
> > in an 18rc1 based kernel.
> >
> >                Dave
> >
> >
> >  Synaptics Touchpad, model: 1, fw: 5.9, id: 0x2c6ab1, caps: 0x884793/0x0
> >  serio: Synaptics pass-through port at isa0060/serio1/input0
> >  input: SynPS/2 Synaptics TouchPad as /class/input/input1
> >  PM: Adding info for serio:serio2
> >
> >  =============================================
> >  [ INFO: possible recursive locking detected ]
> >  ---------------------------------------------
> 
> False alarm, there was a lockdep annotating patch for it in -mm.
not so sure; that patch is supposed to be in -rc1 already; investigating

Re: lockdep input layer warnings.

[Dmitry Torokhov]

On 7/6/06, Arjan van de Ven <arjan@infradead.org> wrote:
> On Thu, 2006-07-06 at 14:37 -0400, Dmitry Torokhov wrote:
> > On 7/6/06, Dave Jones <davej@redhat.com> wrote:
> > > One of our Fedora-devel users picked up on this this morning
> > > in an 18rc1 based kernel.
> > >
> > >                Dave
> > >
> > >
> > >  Synaptics Touchpad, model: 1, fw: 5.9, id: 0x2c6ab1, caps: 0x884793/0x0
> > >  serio: Synaptics pass-through port at isa0060/serio1/input0
> > >  input: SynPS/2 Synaptics TouchPad as /class/input/input1
> > >  PM: Adding info for serio:serio2
> > >
> > >  =============================================
> > >  [ INFO: possible recursive locking detected ]
> > >  ---------------------------------------------
> >
> > False alarm, there was a lockdep annotating patch for it in -mm.
> not so sure; that patch is supposed to be in -rc1 already; investigating
>

Well, you are right, the patch is in -rc1 and I see mutex_lock_nested
in the backtrace but for some reason it is still not happy. Again,
this is with pass-through Synaptics port and we first taking mutex of
the child device and then (going through pass-through port) trying to
take mutex of the parent.

-- 
Dmitry

Re: lockdep input layer warnings.

[Arjan van de Ven]

On Thu, 2006-07-06 at 16:29 -0400, Dmitry Torokhov wrote:
> On 7/6/06, Arjan van de Ven <arjan@infradead.org> wrote:
> > On Thu, 2006-07-06 at 14:37 -0400, Dmitry Torokhov wrote:
> > > On 7/6/06, Dave Jones <davej@redhat.com> wrote:
> > > > One of our Fedora-devel users picked up on this this morning
> > > > in an 18rc1 based kernel.
> > > >
> > > >                Dave
> > > >
> > > >
> > > >  Synaptics Touchpad, model: 1, fw: 5.9, id: 0x2c6ab1, caps: 0x884793/0x0
> > > >  serio: Synaptics pass-through port at isa0060/serio1/input0
> > > >  input: SynPS/2 Synaptics TouchPad as /class/input/input1
> > > >  PM: Adding info for serio:serio2
> > > >
> > > >  =============================================
> > > >  [ INFO: possible recursive locking detected ]
> > > >  ---------------------------------------------
> > >
> > > False alarm, there was a lockdep annotating patch for it in -mm.
> > not so sure; that patch is supposed to be in -rc1 already; investigating
> >
> 
> Well, you are right, the patch is in -rc1 and I see mutex_lock_nested
> in the backtrace but for some reason it is still not happy. Again,
> this is with pass-through Synaptics port and we first taking mutex of
> the child device and then (going through pass-through port) trying to
> take mutex of the parent.

Ok it seems more drastic measures are needed; and a split of the
cmd_mutex class on a per driver basis. The easiest way to do that is to
inline the lock initialization (patch below) but to be honest I think
the patch is a bit ugly; I considered inlining the entire function
instead, any opinions on that?

Index: linux-2.6.18-rc1/drivers/input/serio/libps2.c
===================================================================
--- linux-2.6.18-rc1.orig/drivers/input/serio/libps2.c
+++ linux-2.6.18-rc1/drivers/input/serio/libps2.c
@@ -27,7 +27,7 @@ MODULE_AUTHOR("Dmitry Torokhov <dtor@mai
 MODULE_DESCRIPTION("PS/2 driver library");
 MODULE_LICENSE("GPL");
 
-EXPORT_SYMBOL(ps2_init);
+EXPORT_SYMBOL(__ps2_init);
 EXPORT_SYMBOL(ps2_sendbyte);
 EXPORT_SYMBOL(ps2_drain);
 EXPORT_SYMBOL(ps2_command);
@@ -177,7 +177,7 @@ int ps2_command(struct ps2dev *ps2dev, u
 		return -1;
 	}
 
-	mutex_lock_nested(&ps2dev->cmd_mutex, SINGLE_DEPTH_NESTING);
+	mutex_lock(&ps2dev->cmd_mutex);
 
 	serio_pause_rx(ps2dev->serio);
 	ps2dev->flags = command == PS2_CMD_GETID ? PS2_FLAG_WAITID : 0;
@@ -279,7 +279,7 @@ int ps2_schedule_command(struct ps2dev *
  * ps2_init() initializes ps2dev structure
  */
 
-void ps2_init(struct ps2dev *ps2dev, struct serio *serio)
+void __ps2_init(struct ps2dev *ps2dev, struct serio *serio)
 {
 	mutex_init(&ps2dev->cmd_mutex);
 	init_waitqueue_head(&ps2dev->wait);
Index: linux-2.6.18-rc1/include/linux/libps2.h
===================================================================
--- linux-2.6.18-rc1.orig/include/linux/libps2.h
+++ linux-2.6.18-rc1/include/linux/libps2.h
@@ -39,7 +39,12 @@ struct ps2dev {
 	unsigned char nak;
 };
 
-void ps2_init(struct ps2dev *ps2dev, struct serio *serio);
+void __ps2_init(struct ps2dev *ps2dev, struct serio *serio);
+static inline void ps2_init(struct ps2dev *ps2dev, struct serio *serio)
+{
+	__ps2_init(ps2dev, serio);
+	mutex_init(&ps2dev->cmd_mutex);
+}
 int ps2_sendbyte(struct ps2dev *ps2dev, unsigned char byte, int timeout);
 void ps2_drain(struct ps2dev *ps2dev, int maxbytes, int timeout);
 int ps2_command(struct ps2dev *ps2dev, unsigned char *param, int command);

Re: lockdep input layer warnings.

[Dmitry Torokhov]

On 7/10/06, Arjan van de Ven <arjan@infradead.org> wrote:
> On Thu, 2006-07-06 at 16:29 -0400, Dmitry Torokhov wrote:
> >
> > Well, you are right, the patch is in -rc1 and I see mutex_lock_nested
> > in the backtrace but for some reason it is still not happy. Again,
> > this is with pass-through Synaptics port and we first taking mutex of
> > the child device and then (going through pass-through port) trying to
> > take mutex of the parent.
>
> Ok it seems more drastic measures are needed; and a split of the
> cmd_mutex class on a per driver basis. The easiest way to do that is to
> inline the lock initialization (patch below) but to be honest I think
> the patch is a bit ugly; I considered inlining the entire function
> instead, any opinions on that?
>

It is ugly. Maybe we could have something like mutex_init_nolockdep()
to annotate that lockdep is confused and make it ignore such locks?

Of course there is a chance that lockdep is correct but I do not think so.

-- 
Dmitry

Re: lockdep input layer warnings.

[Arjan van de Ven]

On Mon, 2006-07-10 at 11:49 -0400, Dmitry Torokhov wrote:
> On 7/10/06, Arjan van de Ven <arjan@infradead.org> wrote:
> > On Thu, 2006-07-06 at 16:29 -0400, Dmitry Torokhov wrote:
> > >
> > > Well, you are right, the patch is in -rc1 and I see mutex_lock_nested
> > > in the backtrace but for some reason it is still not happy. Again,
> > > this is with pass-through Synaptics port and we first taking mutex of
> > > the child device and then (going through pass-through port) trying to
> > > take mutex of the parent.
> >
> > Ok it seems more drastic measures are needed; and a split of the
> > cmd_mutex class on a per driver basis. The easiest way to do that is to
> > inline the lock initialization (patch below) but to be honest I think
> > the patch is a bit ugly; I considered inlining the entire function
> > instead, any opinions on that?
> >
> 
> It is ugly. Maybe we could have something like mutex_init_nolockdep()
> to annotate that lockdep is confused and make it ignore such locks?

nope but there is a function to make it unique, we could put that in the
wrapper instead of mutex_init if that makes it less ugly..

Re: lockdep input layer warnings.

[Dmitry Torokhov]

On 7/10/06, Arjan van de Ven <arjan@infradead.org> wrote:
> On Mon, 2006-07-10 at 11:49 -0400, Dmitry Torokhov wrote:
> > On 7/10/06, Arjan van de Ven <arjan@infradead.org> wrote:
> > > On Thu, 2006-07-06 at 16:29 -0400, Dmitry Torokhov wrote:
> > > >
> > > > Well, you are right, the patch is in -rc1 and I see mutex_lock_nested
> > > > in the backtrace but for some reason it is still not happy. Again,
> > > > this is with pass-through Synaptics port and we first taking mutex of
> > > > the child device and then (going through pass-through port) trying to
> > > > take mutex of the parent.
> > >
> > > Ok it seems more drastic measures are needed; and a split of the
> > > cmd_mutex class on a per driver basis. The easiest way to do that is to
> > > inline the lock initialization (patch below) but to be honest I think
> > > the patch is a bit ugly; I considered inlining the entire function
> > > instead, any opinions on that?
> > >
> >
> > It is ugly. Maybe we could have something like mutex_init_nolockdep()
> > to annotate that lockdep is confused and make it ignore such locks?
>
> nope but there is a function to make it unique, we could put that in the
> wrapper instead of mutex_init if that makes it less ugly..
>

What function is that? BTW, I dont think that inlining will work
because it is truly recursive scanario. The only driver in question in
the backtrace provided is psmouse; there is only one call to ps2_init
there.

-- 
Dmitry