2.6.21-mm2: HDAPS? BUG: at kernel/mutex.c:311

2.6.21-mm2: HDAPS? BUG: at kernel/mutex.c:311

[Matt Mackall]

This just hit:

[    7.856000] usbcore: registered new interface driver usbhid
[    7.860000] BUG: at kernel/mutex.c:311 __mutex_trylock_slowpath()
[    7.868000]  [<c0104584>] show_trace_log_lvl+0x1a/0x30
[    7.872000]  [<c0105196>] show_trace+0x12/0x14
[    7.876000]  [<c0105222>] dump_stack+0x15/0x17
[    7.880000]  [<c03cd4c8>] mutex_trylock+0x56/0x15a
[    7.888000]  [<c03172c5>] hdaps_mousedev_poll+0x10/0xcb
[    7.892000]  [<c011f8c5>] run_timer_softirq+0x10e/0x16f
[    7.896000]  [<c011cabc>] __do_softirq+0x5d/0xc0
[    7.900000]  [<c0105c88>] do_softirq+0x6e/0xf0
[    7.904000]  [<c011ca22>] irq_exit+0x3e/0x7b
[    7.912000]  [<c0105da7>] do_IRQ+0x9d/0xb2
[    7.916000]  [<c01040fa>] common_interrupt+0x2e/0x34
[    7.920000]  [<c0118e5f>] printk+0x1b/0x1d
[    7.924000]  [<c02f2f96>] usb_register_driver+0xa0/0xe5
[    7.928000]  [<c0525018>] hid_init+0x28/0x51
[    7.932000]  [<c050e6c4>] kernel_init+0xbc/0x23e
[    7.940000]  [<c010420f>] kernel_thread_helper+0x7/0x10
[    7.944000]  =======================
[    7.948000] drivers/hid/usbhid/hid-core.c: v2.6:USB HID core driver

Looks like it's triggered by the HDAPS driver.

-- 
Mathematics is the supreme nostalgia of our time.

Re: 2.6.21-mm2: HDAPS? BUG: at kernel/mutex.c:311

[Andrew Morton]

On Fri, 11 May 2007 19:21:15 -0500
Matt Mackall <mpm@selenic.com> wrote:

> This just hit:
> 
> [    7.856000] usbcore: registered new interface driver usbhid
> [    7.860000] BUG: at kernel/mutex.c:311 __mutex_trylock_slowpath()
> [    7.868000]  [<c0104584>] show_trace_log_lvl+0x1a/0x30
> [    7.872000]  [<c0105196>] show_trace+0x12/0x14
> [    7.876000]  [<c0105222>] dump_stack+0x15/0x17
> [    7.880000]  [<c03cd4c8>] mutex_trylock+0x56/0x15a
> [    7.888000]  [<c03172c5>] hdaps_mousedev_poll+0x10/0xcb
> [    7.892000]  [<c011f8c5>] run_timer_softirq+0x10e/0x16f
> [    7.896000]  [<c011cabc>] __do_softirq+0x5d/0xc0
> [    7.900000]  [<c0105c88>] do_softirq+0x6e/0xf0
> [    7.904000]  [<c011ca22>] irq_exit+0x3e/0x7b
> [    7.912000]  [<c0105da7>] do_IRQ+0x9d/0xb2
> [    7.916000]  [<c01040fa>] common_interrupt+0x2e/0x34
> [    7.920000]  [<c0118e5f>] printk+0x1b/0x1d
> [    7.924000]  [<c02f2f96>] usb_register_driver+0xa0/0xe5
> [    7.928000]  [<c0525018>] hid_init+0x28/0x51
> [    7.932000]  [<c050e6c4>] kernel_init+0xbc/0x23e
> [    7.940000]  [<c010420f>] kernel_thread_helper+0x7/0x10
> [    7.944000]  =======================
> [    7.948000] drivers/hid/usbhid/hid-core.c: v2.6:USB HID core driver
> 
> Looks like it's triggered by the HDAPS driver.
> 

It's complaining about a mutex_trylock() being run from irq context.

And indeed that's buggy - the non-debug version of spin_lock_mutex() is not
irq-safe.

I'd say that's pretty dumb of the mutex interface, really.  Doing a
mutex_trylock() should be OK from all contexts.

This is caused by a recent semaphore->mutex conversion and it's in mainline
now.

Ho hum.  I suppose a suitable workaround would be to convert hdaps_mtx back
into a semaphore.  ug.

Re: 2.6.21-mm2: HDAPS? BUG: at kernel/mutex.c:311

[Andrew Morton]

On Fri, 11 May 2007 17:53:35 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> And indeed that's buggy - the non-debug version of spin_lock_mutex() is not
> irq-safe.
> 
> I'd say that's pretty dumb of the mutex interface, really.  Doing a
> mutex_trylock() should be OK from all contexts.

We can fix this in a low-impact fashion by making mutex_trylock() do a
spin_trylock() on mutex->wait_lock, no?

Re: 2.6.21-mm2: HDAPS? BUG: at kernel/mutex.c:311

[Dmitry Torokhov]

On Friday 11 May 2007 20:53, Andrew Morton wrote:
> Ho hum.  I suppose a suitable workaround would be to convert hdaps_mtx back
> into a semaphore.  ug.

Actually I was looking for victimes^Wvolunteers to test the patch below.
It gets rid of _trylock business.

-- 
Dmitry

HWMON: hdaps - convert to use input-polldev

Switch to using input-polldev skeleton instead of implementing
polling loop by itself.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/hwmon/Kconfig |    1 
 drivers/hwmon/hdaps.c |   55 +++++++++++++++++++++-----------------------------
 2 files changed, 25 insertions(+), 31 deletions(-)

Index: work/drivers/hwmon/Kconfig
===================================================================
--- work.orig/drivers/hwmon/Kconfig
+++ work/drivers/hwmon/Kconfig
@@ -602,6 +602,7 @@ config SENSORS_W83627EHF
 config SENSORS_HDAPS
 	tristate "IBM Hard Drive Active Protection System (hdaps)"
 	depends on INPUT && X86
+	select INPUT_POLLDEV
 	default n
 	help
 	  This driver provides support for the IBM Hard Drive Active Protection
Index: work/drivers/hwmon/hdaps.c
===================================================================
--- work.orig/drivers/hwmon/hdaps.c
+++ work/drivers/hwmon/hdaps.c
@@ -28,7 +28,7 @@
 
 #include <linux/delay.h>
 #include <linux/platform_device.h>
-#include <linux/input.h>
+#include <linux/input-polldev.h>
 #include <linux/kernel.h>
 #include <linux/mutex.h>
 #include <linux/module.h>
@@ -61,13 +61,12 @@
 #define INIT_TIMEOUT_MSECS	4000	/* wait up to 4s for device init ... */
 #define INIT_WAIT_MSECS		200	/* ... in 200ms increments */
 
-#define HDAPS_POLL_PERIOD	(HZ/20)	/* poll for input every 1/20s */
+#define HDAPS_POLL_INTERVAL	50	/* poll for input every 1/20s (50 ms)*/
 #define HDAPS_INPUT_FUZZ	4	/* input event threshold */
 #define HDAPS_INPUT_FLAT	4
 
-static struct timer_list hdaps_timer;
 static struct platform_device *pdev;
-static struct input_dev *hdaps_idev;
+static struct input_polled_dev *hdaps_idev;
 static unsigned int hdaps_invert;
 static u8 km_activity;
 static int rest_x;
@@ -323,24 +322,19 @@ static void hdaps_calibrate(void)
 	__hdaps_read_pair(HDAPS_PORT_XPOS, HDAPS_PORT_YPOS, &rest_x, &rest_y);
 }
 
-static void hdaps_mousedev_poll(unsigned long unused)
+static void hdaps_mousedev_poll(struct input_polled_dev *dev)
 {
+	struct input_dev *input_dev = dev->input;
 	int x, y;
 
-	/* Cannot sleep.  Try nonblockingly.  If we fail, try again later. */
-	if (mutex_trylock(&hdaps_mtx)) {
-		mod_timer(&hdaps_timer,jiffies + HDAPS_POLL_PERIOD);
-		return;
-	}
+	mutex_lock(&hdaps_mtx);
 
 	if (__hdaps_read_pair(HDAPS_PORT_XPOS, HDAPS_PORT_YPOS, &x, &y))
 		goto out;
 
-	input_report_abs(hdaps_idev, ABS_X, x - rest_x);
-	input_report_abs(hdaps_idev, ABS_Y, y - rest_y);
-	input_sync(hdaps_idev);
-
-	mod_timer(&hdaps_timer, jiffies + HDAPS_POLL_PERIOD);
+	input_report_abs(input_dev, ABS_X, x - rest_x);
+	input_report_abs(input_dev, ABS_Y, y - rest_y);
+	input_sync(input_dev);
 
 out:
 	mutex_unlock(&hdaps_mtx);
@@ -536,6 +530,7 @@ static struct dmi_system_id __initdata h
 
 static int __init hdaps_init(void)
 {
+	struct input_dev *idev;
 	int ret;
 
 	if (!dmi_check_system(hdaps_whitelist)) {
@@ -563,39 +558,37 @@ static int __init hdaps_init(void)
 	if (ret)
 		goto out_device;
 
-	hdaps_idev = input_allocate_device();
+	hdaps_idev = input_allocate_polled_device();
 	if (!hdaps_idev) {
 		ret = -ENOMEM;
 		goto out_group;
 	}
 
+	hdaps_idev->poll = hdaps_mousedev_poll;
+	hdaps_idev->poll_interval = HDAPS_POLL_INTERVAL;
+
 	/* initial calibrate for the input device */
 	hdaps_calibrate();
 
 	/* initialize the input class */
-	hdaps_idev->name = "hdaps";
-	hdaps_idev->dev.parent = &pdev->dev;
-	hdaps_idev->evbit[0] = BIT(EV_ABS);
-	input_set_abs_params(hdaps_idev, ABS_X,
+	idev = hdaps_idev->input;
+	idev->name = "hdaps";
+	idev->dev.parent = &pdev->dev;
+	idev->evbit[0] = BIT(EV_ABS);
+	input_set_abs_params(idev, ABS_X,
 			-256, 256, HDAPS_INPUT_FUZZ, HDAPS_INPUT_FLAT);
-	input_set_abs_params(hdaps_idev, ABS_Y,
+	input_set_abs_params(idev, ABS_Y,
 			-256, 256, HDAPS_INPUT_FUZZ, HDAPS_INPUT_FLAT);
 
-	ret = input_register_device(hdaps_idev);
+	ret = input_register_polled_device(hdaps_idev);
 	if (ret)
 		goto out_idev;
 
-	/* start up our timer for the input device */
-	init_timer(&hdaps_timer);
-	hdaps_timer.function = hdaps_mousedev_poll;
-	hdaps_timer.expires = jiffies + HDAPS_POLL_PERIOD;
-	add_timer(&hdaps_timer);
-
 	printk(KERN_INFO "hdaps: driver successfully loaded.\n");
 	return 0;
 
 out_idev:
-	input_free_device(hdaps_idev);
+	input_free_polled_device(hdaps_idev);
 out_group:
 	sysfs_remove_group(&pdev->dev.kobj, &hdaps_attribute_group);
 out_device:
@@ -611,8 +604,8 @@ out:
 
 static void __exit hdaps_exit(void)
 {
-	del_timer_sync(&hdaps_timer);
-	input_unregister_device(hdaps_idev);
+	input_unregister_polled_device(hdaps_idev);
+	input_free_polled_device(hdaps_idev);
 	sysfs_remove_group(&pdev->dev.kobj, &hdaps_attribute_group);
 	platform_device_unregister(pdev);
 	platform_driver_unregister(&hdaps_driver);

Re: 2.6.21-mm2: HDAPS? BUG: at kernel/mutex.c:311

[Satyam Sharma]

Hi Dmitry,

On 5/12/07, Dmitry Torokhov <dtor@insightbb.com> wrote:
> On Friday 11 May 2007 20:53, Andrew Morton wrote:
> > Ho hum. I suppose a suitable workaround would be to convert hdaps_mtx back
> > into a semaphore. ug.
>
> Actually I was looking for victimes^Wvolunteers to test the patch below.
> It gets rid of _trylock business.

Ah! You just beat me here, and your patch is definitely better.

I was wondering why this driver wanted to use a mutex (previously the
semaphore) to synchronize between process and interrupt context in the
first place. Most of the code in here uses synchronous delays so never
sleeps anyway, but then unfortunately it does a weird
repeated-waiting-hardware-status-register-check thingy in its .probe()
which meant a straightforward mutex -> spinlock wasn't possible.

So then made a patch pushing off the poll to keventd workqueue, when
I saw your mail that does exactly the same, but wrapped about in the
generic input-polldev infrastructure! It's barely 12 days old in mainline --
no wonder I didn't know about it. Seems to be good-looking code!

Thanks,
Satyam

Re: 2.6.21-mm2: HDAPS? BUG: at kernel/mutex.c:311

[Dmitry Torokhov]

Hi Satyam,

On Saturday 12 May 2007 01:45, Satyam Sharma wrote:
> Seems to be good-looking code! 

Thanks. Do you have the hardware? Were you able to test the patch? 

-- 
Dmitry

Re: 2.6.21-mm2: HDAPS? BUG: at kernel/mutex.c:311

[Satyam Sharma]

On 5/14/07, Dmitry Torokhov <dtor@insightbb.com> wrote:
> Hi Satyam,
>
> On Saturday 12 May 2007 01:45, Satyam Sharma wrote:
> > Seems to be good-looking code!
>
> Thanks. Do you have the hardware? Were you able to test the patch?

Oh, sorry, no. I was actually referring to the input-polldev code. Let's
hope Matt can test and ack this.

Thanks,
Satyam

Re: 2.6.21-mm2: HDAPS? BUG: at kernel/mutex.c:311

[Cédric Augonnet]

2007/5/14, Satyam Sharma <satyam.sharma@gmail.com>:
> On 5/14/07, Dmitry Torokhov <dtor@insightbb.com> wrote:
> > Hi Satyam,
> >
> > On Saturday 12 May 2007 01:45, Satyam Sharma wrote:
> > > Seems to be good-looking code!
> >
> > Thanks. Do you have the hardware? Were you able to test the patch?
>
> Oh, sorry, no. I was actually referring to the input-polldev code. Let's
> hope Matt can test and ack this.
>
> Thanks,
> Satyam
> -

Hi all,

Since i still had that BUG on my T43 running a 2.6.22-rc2-mm1 kernel,
I tried that patch. It sounds like this has solved the bug since I
can't find any trace of that issue anymore once the patch is applied.

Thanks for the patch,
Regards,
Cédric