|
From: Nicolas B. <ni...@bo...> - 2007-03-20 07:02:52
|
Hello, Bob Copeland wrote: > On 3/14/07, Nicolas Boichat <ni...@bo...> wrote: >> Hello, >> >> I developed, a while ago, a driver the Apple System Management >> Controller, which provides an accelerometer (Apple Sudden Motion >> Sensor), light sensors, temperature sensors, keyboard backlight control >> and fan control on Intel-based Apple's computers (MacBook Pro, MacBook, >> MacMini). > > Hi Nicolas, > > I tried out an earlier version of this patch several months ago just to > play > around with the joystick part of the accelerometer driver on my MacBook, > and > found that it was backwards in the y-direction compared to what Neverball > seemed to want (of course, NB has no way to invert the joystick). I think > I just did something like this in my own copy: > > + y = -y; > input_report_abs(applesmc_idev, ABS_X, x - rest_x); > input_report_abs(applesmc_idev, ABS_Y, y - rest_y); > > I don't claim you necessarily want to change it, but thought I'd pass it > along. I tried neverball on my Macbook Pro 1st generation (Core Duo, not Core 2 Duo), and the x axis in inverted, not the y axis. Could you confirm which axis is inverted on your Macbook? Also, have you tried the modified hdaps-gl, available here: http://mactel-linux.svn.sourceforge.net/viewvc/mactel-linux/trunk/tools/hdaps-gl/ ? Is it working correctly? Thanks, Best regards, Nicolas |
|
From: Nicolas B. <ni...@bo...> - 2007-04-14 08:06:18
|
Hi,
I got this bug report a while ago:
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).
So, with the patch below, in case the mutex is locked for another
operation, the "brightness_set" called by the led trigger is simply
ignored. I don't think it is the behaviour we want, and I think it would
be a good idea to try again a little while afterwards. Richard, would
you like me to provide a patch for this? It would imply adding a
parameter to brightness_set indicating whether it's safe to sleep or
not, make it return an int, and modify the triggers code to retry if the
return value indicates an error.
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.
Best regards,
Nicolas
Cannot sleep in led->brightness_set handler if it is called from a softirq.
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 <ni...@bo...>
---
drivers/hwmon/applesmc.c | 25 ++++++++++++++++++++-----
1 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 4ec38ef..c93c290 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -142,7 +142,7 @@ static struct mutex applesmc_lock;
static unsigned int key_at_index;
/*
- * __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 +152,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);
}
@@ -725,8 +730,18 @@ static void applesmc_backlight_set(struct led_classdev *led_cdev,
enum led_brightness value)
{
u8 buffer[2];
-
- mutex_lock(&applesmc_lock);
+
+ if (in_interrupt()) {
+ /* Cannot sleep, as we are called from a timer. */
+ if (!mutex_trylock(&applesmc_lock)) {
+ printk(KERN_ERR "applesmc: Could not set the backlight,"
+ " mutex is locked.\n");
+ return;
+ }
+ } else {
+ mutex_lock(&applesmc_lock);
+ }
+
buffer[0] = value;
buffer[1] = 0x00;
applesmc_write_key(BACKLIGHT_KEY, buffer, 2);
|
|
From: Richard P. <rp...@rp...> - 2007-04-14 08:46:02
|
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. 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. Cheers, Richard |
|
From: Nicolas B. <ni...@bo...> - 2007-04-14 13:32:44
|
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 <ni...@bo...>
---
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);
|
|
From: Bradley H. <bd...@gm...> - 2007-03-20 07:16:10
|
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). I'm not sure if this is a part of the driver you wrote, but if it is then I thought you might like to know. If not, maybe the person who did write this module will see this message. ~Bradley On 3/20/07, Nicolas Boichat <ni...@bo...> wrote: > Hello, > > Bob Copeland wrote: > > On 3/14/07, Nicolas Boichat <ni...@bo...> wrote: > >> Hello, > >> > >> I developed, a while ago, a driver the Apple System Management > >> Controller, which provides an accelerometer (Apple Sudden Motion > >> Sensor), light sensors, temperature sensors, keyboard backlight control > >> and fan control on Intel-based Apple's computers (MacBook Pro, MacBook, > >> MacMini). > > > > Hi Nicolas, > > > > I tried out an earlier version of this patch several months ago just to > > play > > around with the joystick part of the accelerometer driver on my MacBook, > > and > > found that it was backwards in the y-direction compared to what Neverball > > seemed to want (of course, NB has no way to invert the joystick). I think > > I just did something like this in my own copy: > > > > + y = -y; > > input_report_abs(applesmc_idev, ABS_X, x - rest_x); > > input_report_abs(applesmc_idev, ABS_Y, y - rest_y); > > > > I don't claim you necessarily want to change it, but thought I'd pass it > > along. > > I tried neverball on my Macbook Pro 1st generation (Core Duo, not Core 2 > Duo), and the x axis in inverted, not the y axis. > > Could you confirm which axis is inverted on your Macbook? > > Also, have you tried the modified hdaps-gl, available here: > http://mactel-linux.svn.sourceforge.net/viewvc/mactel-linux/trunk/tools/hdaps-gl/ > ? Is it working correctly? > > Thanks, > > Best regards, > > Nicolas > > ------------------------------------------------------------------------- > Take Surveys. Earn Cash. Influence the Future of IT > Join SourceForge.net's Techsay panel and you'll get the chance to share your > opinions on IT & business topics through brief surveys-and earn cash > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > _______________________________________________ > Mactel-linux-devel mailing list > Mac...@li... > https://lists.sourceforge.net/lists/listinfo/mactel-linux-devel > |
|
From: Nicolas B. <ni...@bo...> - 2007-03-29 16:25:28
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Thanks for reporting this bug. I can reproduce it, I'll report it upstream as soon as I can... Best regards, Nicolas 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). > > I'm not sure if this is a part of the driver you wrote, but if it is > then I thought you might like to know. If not, maybe the person who > did write this module will see this message. > > ~Bradley > > On 3/20/07, Nicolas Boichat <ni...@bo...> wrote: >> Hello, >> >> Bob Copeland wrote: >>> On 3/14/07, Nicolas Boichat <ni...@bo...> wrote: >>>> Hello, >>>> >>>> I developed, a while ago, a driver the Apple System Management >>>> Controller, which provides an accelerometer (Apple Sudden Motion >>>> Sensor), light sensors, temperature sensors, keyboard backlight control >>>> and fan control on Intel-based Apple's computers (MacBook Pro, MacBook, >>>> MacMini). >>> Hi Nicolas, >>> >>> I tried out an earlier version of this patch several months ago just to >>> play >>> around with the joystick part of the accelerometer driver on my MacBook, >>> and >>> found that it was backwards in the y-direction compared to what Neverball >>> seemed to want (of course, NB has no way to invert the joystick). I think >>> I just did something like this in my own copy: >>> >>> + y = -y; >>> input_report_abs(applesmc_idev, ABS_X, x - rest_x); >>> input_report_abs(applesmc_idev, ABS_Y, y - rest_y); >>> >>> I don't claim you necessarily want to change it, but thought I'd pass it >>> along. >> I tried neverball on my Macbook Pro 1st generation (Core Duo, not Core 2 >> Duo), and the x axis in inverted, not the y axis. >> >> Could you confirm which axis is inverted on your Macbook? >> >> Also, have you tried the modified hdaps-gl, available here: >> http://mactel-linux.svn.sourceforge.net/viewvc/mactel-linux/trunk/tools/hdaps-gl/ >> ? Is it working correctly? >> >> Thanks, >> >> Best regards, >> >> Nicolas >> >> ------------------------------------------------------------------------- >> Take Surveys. Earn Cash. Influence the Future of IT >> Join SourceForge.net's Techsay panel and you'll get the chance to share your >> opinions on IT & business topics through brief surveys-and earn cash >> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV >> _______________________________________________ >> Mactel-linux-devel mailing list >> Mac...@li... >> https://lists.sourceforge.net/lists/listinfo/mactel-linux-devel >> > > ------------------------------------------------------------------------- > Take Surveys. Earn Cash. Influence the Future of IT > Join SourceForge.net's Techsay panel and you'll get the chance to share your > opinions on IT & business topics through brief surveys-and earn cash > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > _______________________________________________ > Mactel-linux-devel mailing list > Mac...@li... > https://lists.sourceforge.net/lists/listinfo/mactel-linux-devel -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.3 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFGC+hc01ajQnpJXgERAsIAAJ4zucrmmevwR8wEr9O9nTQpKm/CFACfeg/K JK21dl0zeJlb0JtP+zP9Am4= =vXLv -----END PGP SIGNATURE----- |
|
From: Bob C. <me...@bo...> - 2007-03-20 15:14:22
|
> I tried neverball on my Macbook Pro 1st generation (Core Duo, not Core 2 > Duo), and the x axis in inverted, not the y axis. > > Could you confirm which axis is inverted on your Macbook? Indeed, my memory is hazy and it may well have been the x-axis. I can't find my modified copy. I'll check it out again tonight along with your hdaps-gl and let you know how it goes... -Bob |
|
From: Bob C. <me...@bo...> - 2007-03-21 04:07:49
|
On Tue, Mar 20, 2007 at 03:02:14PM +0800, Nicolas Boichat wrote: > I tried neverball on my Macbook Pro 1st generation (Core Duo, not Core 2 > Duo), and the x axis in inverted, not the y axis. > > Could you confirm which axis is inverted on your Macbook? > > Also, have you tried the modified hdaps-gl, available here: > http://mactel-linux.svn.sourceforge.net/viewvc/mactel-linux/trunk/tools/hdaps-gl/ > ? Is it working correctly? Ok I tried it out again and it is in fact the x-axis that is inverted within neverball. The hdaps-gl works fine (Macbook Core Duo here). Thanks for the driver! -- Bob Copeland %% www.bobcopeland.com |