eeepc-laptop fixes

PATCH In review Alan Jenkins 3 participants 7 messages Aug 2, 2009 – Aug 19, 2009 ACPI

Share & tools

Alan Jenkins

author

2009-08-02 15:54 UTC #0 lore raw

cover eeepc-laptop fixes

I promised a specific race fix, which is in the second patch. While I was there, I found some other things I could fix as well.

Regards
Alan

eeepc-laptop: use a mutex to serialize pci hotplug (resume vs. notify)
eeepc-laptop: fix pci hotplug race on load and unload
eeepc-laptop: fix ordering of init and exit functions
eeepc-laptop: make input device a child of the platform device
eeepc-laptop: remove redundant rfkill_set_sw_state in resume handler
eeepc-laptop: check the 3G rfkill state on resume
eeepc-laptop: correct the description of the hibernation abort bug
eeepc-laptop: switch to dev_pm_ops

Corentin Chary

2009-08-02 17:18 UTC #1 lore raw

Quoted 6 lines · Alan Jenkins

I promised a specific race fix, which is in the second patch. While I was there, I found some other things I could fix as well.
Regards
Alan

Quoted 9 lines

eeepc-laptop: use a mutex to serialize pci hotplug (resume vs. notify)
eeepc-laptop: fix pci hotplug race on load and unload
eeepc-laptop: fix ordering of init and exit functions
eeepc-laptop: make input device a child of the platform device
eeepc-laptop: remove redundant rfkill_set_sw_state in resume handler
eeepc-laptop: check the 3G rfkill state on resume
eeepc-laptop: correct the description of the hibernation abort bug
eeepc-laptop: switch to dev_pm_ops

Thanks,
Applied in acpi4asus eeepc tree:
http://git.iksaif.net/?p=acpi4asus.git;a=shortlog;h=refs/heads/eeepc-laptop
With two other patch on top of yours (wimax and __init). I think I'll send them for 2.6.32 because there is a lot of change.

Signature · 6 lines

Corentin Chary
http://xf.iksaif.net - http://uffs.org
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Jenkins

2009-08-18 15:44 UTC #2 lore raw

Quoted 5 lines · Corentin Chary

Thanks,
Applied in acpi4asus eeepc tree:
http://git.iksaif.net/?p=acpi4asus.git;a=shortlog;h=refs/heads/eeepc-laptop
With two other patch on top of yours (wimax and __init). I think I'll send them for 2.6.32 because there is a lot of change.

That was the idea. Thanks! I am suspicious of your __init and ABI-doc commits, I will have a closer look at them :-).

Unfortunately I can't pull your tree

$ git fetch git://git.iksaif.net/acpi4asus.git eeepc-laptop
git.iksaif.net[0: 88.191.73.63]: errno=Connection timed out
fatal: unable to connect a socket (Connection timed out)

Regards
Alan

ps. I hope to send two more patches for 2.6.32, to make rfkill-hotplug work on the 900A (pending tester(s) from debian-eeepc), and fix an rfkill memory leak on unload.

Alan Jenkins

2009-08-19 11:35 UTC #3 lore raw

Quoted 9 lines · Alan Jenkins

Quoted 5 lines · Corentin Chary
Thanks,
Applied in acpi4asus eeepc tree:
http://git.iksaif.net/?p=acpi4asus.git;a=shortlog;h=refs/heads/eeepc-laptop
With two other patch on top of yours (wimax and __init). I think I'll send them for 2.6.32 because there is a lot of change.
That was the idea. Thanks! I am suspicious of your __init and ABI-doc commits, I will have a closer look at them :-).

Ok, the ABI-doc looks good. There's just one typo: "Control he card reader".

However I do think it's suspicious to have an __init function, eeepc_hotk_add(), as an "add" method on the ACPI device. This isn't generally acceptable because hotplug can call the "add" method after the driver is loaded. There is a special registration function for *platform* devices, platform_driver_probe(), which allows init code to be marked __init if hotplug can be ruled out. But there is no equivalent for ACPI.

If we add the __init annotations, we open a codepath which would lead to a crash. We don't expect the firmware to ever trigger the crash, but is it worth the risk just to save a few kilobytes?

Alan

Corentin Chary

2009-08-19 14:28 UTC #4 lore raw

Hi,

Quoted 22 lines · Alan Jenkins

Quoted 9 lines · Alan Jenkins
Quoted 5 lines · Corentin Chary
Thanks,
Applied in acpi4asus eeepc tree:
http://git.iksaif.net/?p=acpi4asus.git;a=shortlog;h=refs/heads/eeepc-laptop
With two other patch on top of yours (wimax and __init). I think I'll send them for 2.6.32 because there is a lot of change.
That was the idea. Thanks! I am suspicious of your __init and ABI-doc commits, I will have a closer look at them :-).
Ok, the ABI-doc looks good. There's just one typo: "Control he card reader".
However I do think it's suspicious to have an __init function, eeepc_hotk_add(), as an "add" method on the ACPI device. This isn't generally acceptable because hotplug can call the "add" method after the driver is loaded. There is a special registration function for *platform* devices, platform_driver_probe(), which allows init code to be marked __init if hotplug can be ruled out. But there is no equivalent for ACPI.

Quoted 6 lines

If we add the __init annotations, we open a codepath which would lead to a crash. We don't expect the firmware to ever trigger the crash, but is it worth the risk just to save a few kilobytes?
Alan

Hi,
Thanks for the review.
There is more documentation patchs in acpi4asus and asus-laptop branches as well, maybe you can review them too ?
About the __init patch, I think I'll revert it if this can lead to a crash.

Signature · 2 lines

Corentin Chary
http://xf.iksaif.net - http://uffs.org

Alan Jenkins

2009-08-19 16:51 UTC #5 lore raw

Quoted 4 lines · Corentin Chary

Hi,
Thanks for the review.
There is more documentation patchs in acpi4asus and asus-laptop branches as well, maybe you can review them too ?

<homer>Mmm, documentation

Generic sysfs ABI docs:

I assume you will CC Richard Purdie on the generic sysfs docs which include his address.

The versions/dates don't look right - the backlight and lcd interfaces were introduced a long time before 2.6.29.

I know the backlight device is well used. IMO it should go under "stable" instead of "testing". I don't know whether the other two deserve similar treatment. They're certainly simple enough that we can expect them to remain stable.

The backlight doc could also use "Users: HAL".

It would be nice to update Documentation/leds-class.txt at the same time, since it still says the maximum brightness value is fixed at 255.

Review

Quoted 2 lines

(quoted diff) +2 -0

+               You can change triggers in a similar manner to the way an IO scheduler
+               is chosen . Trigger specific parameters can appear in

There's a stray space here, before the full stop on the second line.

The backlight and led docs are well overdue, I think these will be very welcome.

acpi4asus / asus-laptop:

I wouldn't call /sys/devices/platform/asus-laptop/infos a sysfs abi :-P. I don't think it should be documented as such (but feel free to mention it in Documentation/laptops/asus-laptop.txt.

"Mixing types, expressing multiple lines of data, and doing fancy formatting of data is heavily frowned upon. Doing these things may get you publically humiliated and your code rewritten without notice."

I don't see any typos in the ABI. Other than that, no comment.

Quoted 7 lines

+ This driver provides support for extra features of ACPI-compatible ASUS laptops.
+ It may also support some MEDION, JVC or VICTOR laptops (such as MEDION 9675 or
+ VICTOR XP7210 for example). It makes all the extra buttons generate standard
+ ACPI events that go through /proc/acpi/events, and (on some models) adds support
+ for changing the display brightness and output, switching the LCD backlight on
+ and off, and most importantly, allows you to blink those fancy LEDs intended for
+ reporting mail and wireless status.

You mention ACPI events, perhaps you should mention input events here as well.

Quoted 1 line

+ This driver superseed the old asus_acpi driver.

*supercedes

Quoted 1 line

+ Kernel 2.6.X sources, configured for you computer, with ACPI support.

s/you/your/

Quoted 3 lines

+ - GPS enable and disable
+ - video output switching
+ - Ambient Light Sensor on and off

s/video/Video

Review

Quoted 6 lines

(quoted diff) +6 -0

+  That's all, now, all the events generated by the hotkeys of your laptop
+  should be reported in your /proc/acpi/event entry. You can check it, if you
+  are root, by doing a "cat /proc/acpi/event" and pressing those hotkeys, you
+  should see them appear. You won't be able to open /proc/acpi/event if it is
+  already opened by another program, so check if you haven't a daemon running
+  (like acpid), before reporting a bug :)

Hmm, most people do run acpid so this is not too helpful :). I would mention acpi_listen.

Review

Quoted 2 lines

(quoted diff) +2 -0

+  You can modify LEDs be echoing values to /sys/class/leds/asus:*/brightness :
+    echo 1 >  /sys/class/leds/asus:mail/brightness

asus::mail. There should be two colons, I think.

Quoted 1 line

+ will switch the mail LED on ...

Just one full stop please.

Review

Quoted 2 lines

(quoted diff) +2 -0

+  You can as well know if they are on/off by reading their content and use
+  kernel triggers like ide-disk or heartbeat.

s/as well/also/

Review

Quoted 7 lines

(quoted diff) +7 -0

+            wapf: WAPF defines the behavior of the Fn+Fx wlan key
+                  The significance of values is yet to be found, but
+                  most of the time:
+                  - 0x0 will do nothing
+                  - 0x1 will allow to control the device with Fn+Fx key.
+                  - 0x4 will send an ACPI event (0x88) while pressing the Fn+Fx key
+                  - 0x5 like 0x1 or 0x4

It would be good to know what the default value is.

Regards
Alan

Corentin Chary

2009-08-19 18:55 UTC #6 lore raw

Quoted 103 lines · Alan Jenkins

Quoted 4 lines · Corentin Chary
Hi,
Thanks for the review.
There is more documentation patchs in acpi4asus and asus-laptop branches as well, maybe you can review them too ?
<homer>Mmm, documentation
Generic sysfs ABI docs:
I assume you will CC Richard Purdie on the generic sysfs docs which include his address.
The versions/dates don't look right - the backlight and lcd interfaces were introduced a long time before 2.6.29.
I know the backlight device is well used. IMO it should go under "stable" instead of "testing". I don't know whether the other two deserve similar treatment. They're certainly simple enough that we can expect them to remain stable.
The backlight doc could also use "Users: HAL".
It would be nice to update Documentation/leds-class.txt at the same time, since it still says the maximum brightness value is fixed at 255.
Quoted 2 lines
+ You can change triggers in a similar manner to the way an IO scheduler
+ is chosen . Trigger specific parameters can appear in
There's a stray space here, before the full stop on the second line.
The backlight and led docs are well overdue, I think these will be very welcome.
acpi4asus / asus-laptop:
I wouldn't call /sys/devices/platform/asus-laptop/infos a sysfs abi :-P. I don't think it should be documented as such (but feel free to mention it in Documentation/laptops/asus-laptop.txt.
"Mixing types, expressing multiple lines of data, and doing fancy formatting of data is heavily frowned upon. Doing these things may get you publically humiliated and your code rewritten without notice."
I don't see any typos in the ABI. Other than that, no comment.
Quoted 7 lines
+ This driver provides support for extra features of ACPI-compatible ASUS laptops.
+ It may also support some MEDION, JVC or VICTOR laptops (such as MEDION 9675 or
+ VICTOR XP7210 for example). It makes all the extra buttons generate standard
+ ACPI events that go through /proc/acpi/events, and (on some models) adds support
+ for changing the display brightness and output, switching the LCD backlight on
+ and off, and most importantly, allows you to blink those fancy LEDs intended for
+ reporting mail and wireless status.
You mention ACPI events, perhaps you should mention input events here as well.
Quoted 1 line
+ This driver superseed the old asus_acpi driver.
*supercedes
Quoted 1 line
+ Kernel 2.6.X sources, configured for you computer, with ACPI support.
s/you/your/
Quoted 3 lines
+ - GPS enable and disable
+ - video output switching
+ - Ambient Light Sensor on and off
s/video/Video
Quoted 6 lines
+ That's all, now, all the events generated by the hotkeys of your laptop
+ should be reported in your /proc/acpi/event entry. You can check it, if you
+ are root, by doing a "cat /proc/acpi/event" and pressing those hotkeys, you
+ should see them appear. You won't be able to open /proc/acpi/event if it is
+ already opened by another program, so check if you haven't a daemon running
+ (like acpid), before reporting a bug :)
Hmm, most people do run acpid so this is not too helpful :). I would mention acpi_listen.
Quoted 2 lines
+ You can modify LEDs be echoing values to /sys/class/leds/asus:*/brightness :
+ echo 1 > /sys/class/leds/asus:mail/brightness
asus::mail. There should be two colons, I think.
Quoted 1 line
+ will switch the mail LED on ...
Just one full stop please.
Quoted 2 lines
+ You can as well know if they are on/off by reading their content and use
+ kernel triggers like ide-disk or heartbeat.
s/as well/also/
Quoted 7 lines
+ wapf: WAPF defines the behavior of the Fn+Fx wlan key
+ The significance of values is yet to be found, but
+ most of the time:
+ - 0x0 will do nothing
+ - 0x1 will allow to control the device with Fn+Fx key.
+ - 0x4 will send an ACPI event (0x88) while pressing the Fn+Fx key
+ - 0x5 like 0x1 or 0x4

Quoted 5 lines

It would be good to know what the default value is.
Regards
Alan

Wow, thanks for the extensive review, I'll fix that when I'm back (next week).

Signature · 6 lines

Corentin Chary
http://xf.iksaif.net - http://uffs.org
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html