Messages in this thread Patch in this message | | | Date | Sat, 14 Apr 2007 21:31:53 +0800 | From | Nicolas Boichat <> | Subject | [PATCH] applesmc - fix crash when activating a led trigger on the keyboard backlight - use a workqueue |
| |
Hi again,
Richard Purdie wrote: > Hi, > > On Sat, 2007-04-14 at 16:05 +0800, Nicolas Boichat wrote: > >> Bradley Hook wrote: >> >>> Slightly off-topic, but I've been experiencing a minor bug in the >>> keyboard backlight feature. >>> >>> I say it is "minor" only because the feature serves no real functional >>> purpose. You can activate a trigger called "heartbeat" that will cause >>> the keyboard light to pulse at a speed based on the CPU usage. On my >>> MBP17, after activating this trigger the machine will either lock-up >>> or core dump within about a minute (timing is not consistent). >>> >>> >> This is caused by the fact applesmc_backlight_set locks a mutex (or more >> precisely sleeps while trying to lock a mutex) while being in a softirq >> context. >> >> This might be obvious for others, but it was not for me, and there is >> absolutely no mention in the documentation of the fact it is not always >> safe to sleep in the brightness_set handler of a led_class device (it is >> safe when it is called because someone wrote to the brightness sysfs file). >> > > Its never safe for a brightness_set handler to sleep. They're designed > to be called from interrupt context and as you've noted, several > triggers do that. > > The solution if you have locks like your case is to offload the work to > a workqueue, there is simply no other way to do it. >
Ok, I used a workqueue. Andrew, please ignore the previous patch ("[PATCH] applesmc - fix crash when activating a led trigger on the keyboard backlight"), and use this one instead, thanks.
> I'll have a look to see if I can improve the documentation (patches > welcome). > > >> Also, the led-trigger code seems buggy when it comes to locking. Setting >> CONFIG_DEBUG_SPINLOCK_SLEEP causes a lot a warnings. The problem is that >> the list of triggers is locked using a rw spinlock, but the rest of the >> code seems to ignore that, and calls a lot of functions which can sleep >> (kzalloc with GFP_KERNEL, sysfs_add_file, mutex_lock, etc...). I think >> the list lock should be converted to a mutex (or maybe modified to use >> RCU). I'm not very experienced in that domain, but if you want I can >> provide a patch for this. >> > > Someone else has mentioned this within the past few days and there is a > problem with the trigger->activate and ->deactivate calls occurring > within a spinlock. Its not a simple problem to solve unfortunately and > you can't just convert to a mutex but I'm looking at it. Again, patches > welcome but its going to need careful thought. >
Ok you know your code better so maybe it's easier and better if you do it. I'll tell you if I suddenly decide to work on that, so we don't duplicate our work.
Best regards,
Nicolas
- Cannot sleep in led->brightness_set handler, as it might be called from a softirq, so we use a workqueue to change the brightness (as recommended by Richard Purdie) - Reduce wait_status timetout from 100ms to 2ms, as wait_status either takes less than 1.5 ms, or fails.
Signed-off-by: Nicolas Boichat <nicolas@boichat.ch> ---
drivers/hwmon/applesmc.c | 61 +++++++++++++++++++++++++++++++++++++--------- 1 files changed, 49 insertions(+), 12 deletions(-)
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c index 4ec38ef..ea0a004 100644 --- a/drivers/hwmon/applesmc.c +++ b/drivers/hwmon/applesmc.c @@ -38,6 +38,7 @@ #include <asm/io.h> #include <linux/leds.h> #include <linux/hwmon.h> +#include <linux/workqueue.h> /* data port used by Apple SMC */ #define APPLESMC_DATA_PORT 0x300 @@ -116,7 +117,7 @@ struct dmi_match_data { int temperature_set; }; -static int debug = 0; +static const int debug = 0; static struct platform_device *pdev; static s16 rest_x; static s16 rest_y; @@ -141,8 +142,10 @@ static struct mutex applesmc_lock; */ static unsigned int key_at_index; +static struct workqueue_struct *applesmc_led_wq; + /* - * __wait_status - Wait up to 100ms for the status port to get a certain value + * __wait_status - Wait up to 2ms for the status port to get a certain value * (masked with 0x0f), returning zero if the value is obtained. Callers must * hold applesmc_lock. */ @@ -152,9 +155,14 @@ static int __wait_status(u8 val) val = val & APPLESMC_STATUS_MASK; - for (i = 0; i < 10000; i++) { - if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val) + for (i = 0; i < 200; i++) { + if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val) { + if (debug) + printk(KERN_DEBUG + "Waited %d us for status %x\n", + i*10, val); return 0; + } udelay(10); } @@ -721,17 +729,33 @@ static ssize_t applesmc_calibrate_store(struct device *dev, return count; } -static void applesmc_backlight_set(struct led_classdev *led_cdev, - enum led_brightness value) +/* Store the next backlight value to be written by the work */ +static unsigned int backlight_value; + +static void applesmc_backlight_set(struct work_struct *work) { u8 buffer[2]; mutex_lock(&applesmc_lock); - buffer[0] = value; + buffer[0] = backlight_value; buffer[1] = 0x00; applesmc_write_key(BACKLIGHT_KEY, buffer, 2); mutex_unlock(&applesmc_lock); } +DECLARE_WORK(backlight_work, &applesmc_backlight_set); + +static void applesmc_brightness_set(struct led_classdev *led_cdev, + enum led_brightness value) +{ + int ret; + + backlight_value = value; + ret = queue_work(applesmc_led_wq, &backlight_work); + + if (debug && (!ret)) { + printk(KERN_DEBUG "applesmc: work was already on the queue.\n"); + } +} static ssize_t applesmc_key_count_show(struct device *dev, struct device_attribute *attr, char *sysfsbuf) @@ -887,7 +911,7 @@ static ssize_t applesmc_key_at_index_store(struct device *dev, static struct led_classdev applesmc_backlight = { .name = "smc:kbd_backlight", .default_trigger = "nand-disk", - .brightness_set = applesmc_backlight_set, + .brightness_set = applesmc_brightness_set, }; static DEVICE_ATTR(position, 0444, applesmc_position_show, NULL); @@ -1234,25 +1258,35 @@ static int __init applesmc_init(void) if (ret) goto out_accelerometer; + /* Create the workqueue */ + applesmc_led_wq = create_singlethread_workqueue("applesmc-led"); + if (!applesmc_led_wq) { + ret = -ENOMEM; + goto out_light_sysfs; + } + /* register as a led device */ ret = led_classdev_register(&pdev->dev, &applesmc_backlight); if (ret < 0) - goto out_light_sysfs; + goto out_light_wq; } hwmon_class_dev = hwmon_device_register(&pdev->dev); if (IS_ERR(hwmon_class_dev)) { ret = PTR_ERR(hwmon_class_dev); - goto out_light; + goto out_light_ledclass; } printk(KERN_INFO "applesmc: driver successfully loaded.\n"); return 0; -out_light: +out_light_ledclass: if (applesmc_light) led_classdev_unregister(&applesmc_backlight); +out_light_wq: + if (applesmc_light) + destroy_workqueue(applesmc_led_wq); out_light_sysfs: if (applesmc_light) sysfs_remove_file(&pdev->dev.kobj, &dev_attr_light.attr); @@ -1280,8 +1314,11 @@ out: static void __exit applesmc_exit(void) { hwmon_device_unregister(hwmon_class_dev); - if (applesmc_light) + if (applesmc_light) { led_classdev_unregister(&applesmc_backlight); + destroy_workqueue(applesmc_led_wq); + sysfs_remove_file(&pdev->dev.kobj, &dev_attr_light.attr); + } if (applesmc_accelerometer) applesmc_release_accelerometer(); sysfs_remove_group(&pdev->dev.kobj, &temperature_attributes_group);
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |