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 <[email protected]>
---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
