[PATCH] rfkill: document removal of notifier chain

[PATCH] rfkill: document removal of notifier chain

[Alan Jenkins]

This unused feature was removed in 4dec9b807be757780ca3611a959ac22c28d292a7

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---

p.s. Is netdev really the right list for rfkill (as listed in MAINTAINERS)?
     I have a feeling most development happens on linux-wireless.

 Documentation/rfkill.txt |   23 ++++++++---------------
 1 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
index 4d3ee31..357ef01 100644
--- a/Documentation/rfkill.txt
+++ b/Documentation/rfkill.txt
@@ -47,10 +47,8 @@ know when they should enable or disable a wireless network device transmitter.
 This is enabled by the CONFIG_RFKILL Kconfig option.
 
 The rfkill class support makes sure userspace will be notified of all state
-changes on rfkill devices through uevents.  It provides a notification chain
-for interested parties in the kernel to also get notified of rfkill state
-changes in other drivers.  It creates several sysfs entries which can be used
-by userspace.  See section "Userspace support".
+changes on rfkill devices through uevents.  It creates several sysfs entries
+which can be used by userspace.  See section "Userspace support".
 
 The rfkill-input module provides the kernel with the ability to implement a
 basic response when the user presses a key or button (or toggles a switch)
@@ -156,9 +154,8 @@ rfkill class:
 	  transmitter state;
 	* Keeps track of the wireless transmitter state (with help from
 	  the driver);
-	* Generates userspace notifications (uevents) and a call to a
-	  notification chain (kernel) when there is a wireless transmitter
-	  state change;
+	* Generates userspace notifications (uevents) when there is a wireless
+	  transmitter state change;
 	* Connects a wireless communications driver with the common rfkill
 	  control system, which, for example, allows actions such as
 	  "switch all bluetooth devices offline" to be carried out by
@@ -206,18 +203,15 @@ Userspace input handlers (uevents) or kernel input handlers (rfkill-input):
 	  restore the transmitters to their state before the EPO, or unblock
 	  them all.
 
-Userspace uevent handler or kernel platform-specific drivers hooked to the
-rfkill notifier chain:
+Userspace uevent handler or kernel platform-specific drivers:
 
-	* Taps into the rfkill notifier chain or to KOBJ_CHANGE uevents,
-	  in order to know when a device that is registered with the rfkill
-	  class changes state;
+	* Listens to KOBJ_CHANGE uevents or the platform in order to know when
+	  a device that is registered with the rfkill class changes state;
 	* Issues feedback notifications to the user;
 	* In the rare platforms where this is required, synthesizes an input
 	  event to command all *OTHER* rfkill devices to also change their
 	  statues when a specific rfkill device changes state.
 
-
 ===============================================================================
 3: Kernel driver guidelines
 
@@ -269,8 +263,7 @@ SW_RFKILL_ALL, etc) when ALL of the folowing conditions are met:
 When in doubt, do not issue input events.  For drivers that should generate
 input events in some platforms, but not in others (e.g. b43), the best solution
 is to NEVER generate input events in the first place.  That work should be
-deferred to a platform-specific kernel module (which will know when to generate
-events through the rfkill notifier chain) or to userspace.  This avoids the
+deferred to a platform-specific kernel module or to userspace.  This avoids the
 usual maintenance problems with DMI whitelisting.
 
 
-- 
1.5.4.3

Re: [PATCH] rfkill: document removal of notifier chain

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

On Wed, 2009-04-08 at 14:37 +0100, Alan Jenkins wrote:
> This unused feature was removed in 4dec9b807be757780ca3611a959ac22c28d292a7
> 
> Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
> ---
> 
> p.s. Is netdev really the right list for rfkill (as listed in MAINTAINERS)?
>      I have a feeling most development happens on linux-wireless.

You're right, and I'm rewriting rfkill and part of that is changing the
maintainer to myself and the list to linux-wireless.

I have no problems with this patch, but note that I'm rewriting that
entire file as well so it's not really necessary in the long run -- now
that -rc1 is out I can hopefully convert all in-tree rfkill stuff and
get the patch in.

johannes


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] rfkill: document removal of notifier chain

[Alan Jenkins]

Johannes Berg wrote:
> On Wed, 2009-04-08 at 14:37 +0100, Alan Jenkins wrote:
>   
>> This unused feature was removed in 4dec9b807be757780ca3611a959ac22c28d292a7
>>
>> Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
>> ---
>>
>> p.s. Is netdev really the right list for rfkill (as listed in MAINTAINERS)?
>>      I have a feeling most development happens on linux-wireless.
>>     
>
> You're right, and I'm rewriting rfkill and part of that is changing the
> maintainer to myself and the list to linux-wireless.
>   

Good luck!  I'll try to have a look at your patch, to see how (if) it
would affect the benighted eeepc-laptop.

> I have no problems with this patch, but note that I'm rewriting that
> entire file as well so it's not really necessary in the long run -- now
> that -rc1 is out I can hopefully convert all in-tree rfkill stuff and
> get the patch in.
>   

Heh, ok.

Alan

Re: rfkill rewrite bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

On Mon, 2009-04-13 at 20:00 +0100, Alan Jenkins wrote:
> Alan Jenkins wrote:
> > Johannes Berg wrote:
> >   
> >> On Wed, 2009-04-08 at 14:37 +0100, Alan Jenkins wrote:    
> >>     
> >> You're right, and I'm rewriting rfkill and part of that is changing the
> >> maintainer to myself and the list to linux-wireless.
> >>   
> >>     
> >
> > Good luck!  I'll try to have a look at your patch, to see how (if) it
> > would affect the benighted eeepc-laptop.
> >   
> 
> I tried the latest patch (#19) from your website, it didn't work so well 
> :-).

:)
Thanks. I need to find some spare cycles to go through that patch again,
probably sometime this week.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

rfkill rewrite bug

[Alan Jenkins]

Alan Jenkins wrote:
> Johannes Berg wrote:
>   
>> On Wed, 2009-04-08 at 14:37 +0100, Alan Jenkins wrote:    
>>     
>> You're right, and I'm rewriting rfkill and part of that is changing the
>> maintainer to myself and the list to linux-wireless.
>>   
>>     
>
> Good luck!  I'll try to have a look at your patch, to see how (if) it
> would affect the benighted eeepc-laptop.
>   

I tried the latest patch (#19) from your website, it didn't work so well 
:-).

[   78.301309] BUG: unable to handle kernel NULL pointer dereference at 
(null)
[   78.301326] IP: [<c0279468>] led_trigger_event+0x31/0x40
[   78.301349] *pdpt = 0000000012375001 *pde = 0000000000000000
[   78.301362] Oops: 0000 [#1]
[   78.301369] last sysfs file: /sys/module/backlight/initstate
[   78.301378] Modules linked in: eeepc_laptop(+) rfkill aes_i586 
aes_generic af_packet i915 drm i2c_algo_bit cfbcopyarea cfbimgblt 
cfbfillrect ipv6 joydev arc4 ecb usual_tables(P) snd_hda_codec_realtek 
snd_hda_intel snd_hda_codec snd_hwdep ath5k snd_pcm_oss snd_mixer_oss 
mac80211 psmouse i2c_i801 uhci_hcd led_class serio_raw snd_pcm i2c_core 
pcspkr intel_agp ehci_hcd atl2 cfg80211 usbcore snd_timer snd_page_alloc 
agpgart video backlight output battery ac button processor evdev thermal 
fan ata_generic
[   78.301492]
[   78.301503] Pid: 2904, comm: modprobe Tainted: P           
(2.6.30-rc1-wleeepc #2) 701
[   78.301513] EIP: 0060:[<c0279468>] EFLAGS: 00210286 CPU: 0
[   78.301523] EIP is at led_trigger_event+0x31/0x40
[   78.301532] EAX: df34e618 EBX: 00000000 ECX: df34e618 EDX: 000000ff
[   78.301541] ESI: 000000ff EDI: df34e624 EBP: d2373e44 ESP: d2373e38
[   78.301551]  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
[   78.301561] Process modprobe (pid: 2904, ti=d2372000 task=d2312400 
task.ti=d2372000)
[   78.301569] Stack:
[   78.301574]  df34e600 e0017f58 df268800 d2373e4c e01692af d2373e5c 
e01695f8 00000001
[   78.301592]  d21f6800 d2373e80 e00178bc 00000000 00000200 00000010 
d22b1280 df268800
[   78.301611]  ffffffed e0019478 d2373e9c c020eff3 df268a04 ffffffda 
df268a04 ffffffed
[   78.301631] Call Trace:
[   78.301638]  [<e01692af>] ? rfkill_led_trigger_event+0x1a/0x1c [rfkill]
[   78.301663]  [<e01695f8>] ? rfkill_set_sw_state+0x53/0x69 [rfkill]
[   78.301685]  [<e00178bc>] ? eeepc_hotk_add+0x2ca/0x3ef [eeepc_laptop]
[   78.301706]  [<c020eff3>] ? acpi_device_probe+0x44/0x127
[   78.301723]  [<c02470a9>] ? driver_probe_device+0x7f/0xf2
[   78.301740]  [<c024715f>] ? __driver_attach+0x43/0x5f
[   78.301753]  [<c024675b>] ? bus_for_each_dev+0x39/0x5a
[   78.301766]  [<c0246f7c>] ? driver_attach+0x14/0x16
[   78.301778]  [<c024711c>] ? __driver_attach+0x0/0x5f
[   78.301791]  [<c0246c5f>] ? bus_add_driver+0xd7/0x1e7
[   78.301804]  [<c0247365>] ? driver_register+0x7b/0xd7
[   78.301822]  [<c02106c9>] ? acpi_bus_register_driver+0x3a/0x3c
[   78.301835]  [<e0011022>] ? eeepc_laptop_init+0x22/0x1b9 [eeepc_laptop]
[   78.301852]  [<c0101131>] ? _stext+0x49/0x10b
[   78.301865]  [<e0011000>] ? eeepc_laptop_init+0x0/0x1b9 [eeepc_laptop]
[   78.301882]  [<c012f3c2>] ? __blocking_notifier_call_chain+0x40/0x4c
[   78.301899]  [<c013a680>] ? sys_init_module+0x87/0x18b
[   78.301915]  [<c0102804>] ? sysenter_do_call+0x12/0x22
[   78.301928] Code: 56 89 d6 53 74 2f 8b 58 0c 8d 78 0c eb 1d 8d 4b c4 
8b 41 08 39 c6 89 c2 0f 46 d6 f6 41 0c 01 89 51 04 75 05 89 c8 ff 51 10 
8b 1b <8b> 03 0f 18 00 90 39 fb 75 d9 5b 5e 5f 5d c3 55 89 e5 56 8d 70
[   78.302018] EIP: [<c0279468>] led_trigger_event+0x31/0x40 SS:ESP 
0068:d2373e38
[   78.302032] CR2: 0000000000000000
[   78.302041] ---[ end trace 3e0b201a788d0ed8 ]---

Regards
Alan

Re: rfkill rewrite bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 294 bytes --]

On Mon, 2009-04-13 at 20:00 +0100, Alan Jenkins wrote:

> I tried the latest patch (#19) from your website, it didn't work so well 
> :-).

I think I fixed that now, I allowed using set_sw_state before register
and that didn't go too well with the LED system w/o any checks.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: rfkill rewrite bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 3310 bytes --]

Hi!

> Ok, I retried with v6.  It seemed to work well enough; my wireless 
> toggle key worked and either state was preserved over suspend and 
> hibernation.

Alright, cool.

> When I looked at the code earlier, I saw no obvious replacement for 
> rfkill_set_default().  So I tried disabling the wireless and rebooting 
> to see what happened.  It didn't like that :-).

Oops. I'll take a look tonight, thanks for testing!

johannes

> [    3.258061] ------------[ cut here ]------------
> [    3.258160] kernel BUG at kernel/workqueue.c:192!
> [    3.258256] invalid opcode: 0000 [#1]
> [    3.258391] last sysfs file: /sys/module/backlight/initstate
> [    3.258489] Modules linked in: video(+) output eeepc_laptop(+) 
> backlight ac battery rfkill but
> ton processor evdev thermal fan ata_generic
> [    3.259261]
> [    3.259346] Pid: 1571, comm: modprobe Not tainted (2.6.30-rc2-wleeepc 
> #13) 701
> [    3.259482] EIP: 0060:[<c0129e0a>] EFLAGS: 00010203 CPU: 0
> [    3.259590] EIP is at queue_work_on+0x1a/0x31
> [    3.259687] EAX: df089730 EBX: df00a060 ECX: df08972c EDX: 00000000
> [    3.259790] ESI: e0045f54 EDI: df284800 EBP: df361e3c ESP: df361e38
> [    3.259893]  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
> [    3.259996] Process modprobe (pid: 1571, ti=df360000 task=de853c00 
> task.ti=df360000)
> [    3.260131] Stack:
> [    3.260160]  df089600 df361e44 c0129e41 df361e4c c0129e52 df361e5c 
> e00125f7 00010000
> [    3.260160]  df09c400 df361e80 e00458b9 00000000 00000200 00000010 
> de59ae90 df284800
> [    3.260160]  ffffffed e0047474 df361e9c c02116c3 df2849fc ffffffda 
> df2849fc f
> fffffed
> [    3.260160] Call Trace:
> [    3.260160]  [<c0129e41>] ? queue_work+0xe/0x10
> [    3.260160]  [<c0129e52>] ? schedule_work+0xf/0x11
> [    3.260160]  [<e00125f7>] ? rfkill_set_sw_state+0x4c/0x69 [rfkill]
> [    3.260160]  [<e00458b9>] ? eeepc_hotk_add+0x2c7/0x3ec [eeepc_laptop]
> [    3.260160]  [<c02116c3>] ? acpi_device_probe+0x44/0x127
> [    3.260160]  [<c0249719>] ? driver_probe_device+0x7f/0xf2
> [    3.260160]  [<c02497cf>] ? __driver_attach+0x43/0x5f
> [    3.260160]  [<c0248dcb>] ? bus_for_each_dev+0x39/0x5a
> [    3.260160]  [<c02495ec>] ? driver_attach+0x14/0x16
> [    3.260160]  [<c024978c>] ? __driver_attach+0x0/0x5f
> [    3.260160]  [<c02492cf>] ? bus_add_driver+0xd7/0x1e7
> [    3.260160]  [<c02499d5>] ? driver_register+0x7b/0xd7
> [    3.260160]  [<c0212d44>] ? acpi_bus_register_driver+0x3a/0x3c
> [    3.260160]  [<e004a022>] ? eeepc_laptop_init+0x22/0x1b9 [eeepc_laptop]
> [    3.260160]  [<c0101131>] ? _stext+0x49/0x10b
> [    3.260160]  [<e004a000>] ? eeepc_laptop_init+0x0/0x1b9 [eeepc_laptop]
> [    3.260160]  [<c012f476>] ? __blocking_notifier_call_chain+0x40/0x4c
> [    3.260160]  [<c013a738>] ? sys_init_module+0x87/0x18f
> [    3.260160]  [<c0102804>] ? sysenter_do_call+0x12/0x22
> [    3.260160] Code: 83 e2 fc 8b 4a 14 89 c2 8b 01 e8 d9 ff ff ff 5d c3 
> 55 89 e5 53 89 d3 0f ba 29 00 19 c0 31 d2 85 c0 75 1a 8d 41 04 39 41 04 
> 74 04 <0f> 0b eb fe 8b 03 89 ca e8 b0 ff ff ff ba 01 00 00 00 5b 89 d0
> [    3.260160] EIP: [<c0129e0a>] queue_work_on+0x1a/0x31 SS:ESP 
> 0068:df361e38
> [    3.269670] ---[ end trace f8b6941350b5e97c ]---
> 
> Thanks
> Alan
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: rfkill rewrite bug

[Alan Jenkins]

Johannes Berg wrote:
> I think I fixed that now, I allowed using set_sw_state before register
> and that didn't go too well with the LED system w/o any checks.
>
> johannes
>   

Ok, I retried with v6.  It seemed to work well enough; my wireless 
toggle key worked and either state was preserved over suspend and 
hibernation.

When I looked at the code earlier, I saw no obvious replacement for 
rfkill_set_default().  So I tried disabling the wireless and rebooting 
to see what happened.  It didn't like that :-).

[    3.258061] ------------[ cut here ]------------
[    3.258160] kernel BUG at kernel/workqueue.c:192!
[    3.258256] invalid opcode: 0000 [#1]
[    3.258391] last sysfs file: /sys/module/backlight/initstate
[    3.258489] Modules linked in: video(+) output eeepc_laptop(+) 
backlight ac battery rfkill but
ton processor evdev thermal fan ata_generic
[    3.259261]
[    3.259346] Pid: 1571, comm: modprobe Not tainted (2.6.30-rc2-wleeepc 
#13) 701
[    3.259482] EIP: 0060:[<c0129e0a>] EFLAGS: 00010203 CPU: 0
[    3.259590] EIP is at queue_work_on+0x1a/0x31
[    3.259687] EAX: df089730 EBX: df00a060 ECX: df08972c EDX: 00000000
[    3.259790] ESI: e0045f54 EDI: df284800 EBP: df361e3c ESP: df361e38
[    3.259893]  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
[    3.259996] Process modprobe (pid: 1571, ti=df360000 task=de853c00 
task.ti=df360000)
[    3.260131] Stack:
[    3.260160]  df089600 df361e44 c0129e41 df361e4c c0129e52 df361e5c 
e00125f7 00010000
[    3.260160]  df09c400 df361e80 e00458b9 00000000 00000200 00000010 
de59ae90 df284800
[    3.260160]  ffffffed e0047474 df361e9c c02116c3 df2849fc ffffffda 
df2849fc f
fffffed
[    3.260160] Call Trace:
[    3.260160]  [<c0129e41>] ? queue_work+0xe/0x10
[    3.260160]  [<c0129e52>] ? schedule_work+0xf/0x11
[    3.260160]  [<e00125f7>] ? rfkill_set_sw_state+0x4c/0x69 [rfkill]
[    3.260160]  [<e00458b9>] ? eeepc_hotk_add+0x2c7/0x3ec [eeepc_laptop]
[    3.260160]  [<c02116c3>] ? acpi_device_probe+0x44/0x127
[    3.260160]  [<c0249719>] ? driver_probe_device+0x7f/0xf2
[    3.260160]  [<c02497cf>] ? __driver_attach+0x43/0x5f
[    3.260160]  [<c0248dcb>] ? bus_for_each_dev+0x39/0x5a
[    3.260160]  [<c02495ec>] ? driver_attach+0x14/0x16
[    3.260160]  [<c024978c>] ? __driver_attach+0x0/0x5f
[    3.260160]  [<c02492cf>] ? bus_add_driver+0xd7/0x1e7
[    3.260160]  [<c02499d5>] ? driver_register+0x7b/0xd7
[    3.260160]  [<c0212d44>] ? acpi_bus_register_driver+0x3a/0x3c
[    3.260160]  [<e004a022>] ? eeepc_laptop_init+0x22/0x1b9 [eeepc_laptop]
[    3.260160]  [<c0101131>] ? _stext+0x49/0x10b
[    3.260160]  [<e004a000>] ? eeepc_laptop_init+0x0/0x1b9 [eeepc_laptop]
[    3.260160]  [<c012f476>] ? __blocking_notifier_call_chain+0x40/0x4c
[    3.260160]  [<c013a738>] ? sys_init_module+0x87/0x18f
[    3.260160]  [<c0102804>] ? sysenter_do_call+0x12/0x22
[    3.260160] Code: 83 e2 fc 8b 4a 14 89 c2 8b 01 e8 d9 ff ff ff 5d c3 
55 89 e5 53 89 d3 0f ba 29 00 19 c0 31 d2 85 c0 75 1a 8d 41 04 39 41 04 
74 04 <0f> 0b eb fe 8b 03 89 ca e8 b0 ff ff ff ba 01 00 00 00 5b 89 d0
[    3.260160] EIP: [<c0129e0a>] queue_work_on+0x1a/0x31 SS:ESP 
0068:df361e38
[    3.269670] ---[ end trace f8b6941350b5e97c ]---

Thanks
Alan

Re: rfkill rewrite bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

On Sat, 2009-04-18 at 09:17 +0100, Alan Jenkins wrote:

> When I looked at the code earlier, I saw no obvious replacement for 
> rfkill_set_default().  So I tried disabling the wireless and rebooting 
> to see what happened.  It didn't like that :-).

Ok that wasn't too hard -- try this on top if you get a chance:

--- wireless-testing.orig/net/rfkill/core.c	2009-04-18 10:16:53.000000000 +0200
+++ wireless-testing/net/rfkill/core.c	2009-04-18 10:18:17.000000000 +0200
@@ -401,6 +401,9 @@ bool rfkill_set_hw_state(struct rfkill *
 
 	ret = __rfkill_set_hw_state(rfkill, blocked, &change);
 
+	if (!rfkill->registered)
+		return ret;
+
 	if (change)
 		schedule_work(&rfkill->uevent_work);
 
@@ -422,13 +425,17 @@ bool rfkill_set_sw_state(struct rfkill *
 					    &rfkill->blocked);
 
 	hwblock = !!test_bit(RFKILL_BLOCK_HW_BIT, &rfkill->blocked);
+	blocked = blocked || hwblock;
+
+	if (!rfkill->registered)
+		return blocked;
 
 	if (prev != blocked && !hwblock)
 		schedule_work(&rfkill->uevent_work);
 
 	rfkill_led_trigger_event(rfkill);
 
-	return blocked || hwblock;
+	return blocked;
 }
 EXPORT_SYMBOL(rfkill_set_sw_state);
 
@@ -452,6 +459,9 @@ void rfkill_set_states(struct rfkill *rf
 		hwprev = !!test_and_clear_bit(RFKILL_BLOCK_HW_BIT,
 					      &rfkill->blocked);
 
+	if (!rfkill->registered)
+		return;
+
 	if (swprev != sw || hwprev != hw)
 		schedule_work(&rfkill->uevent_work);
 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: rfkill rewrite bug

[Alan Jenkins]

Johannes Berg wrote:
> On Sat, 2009-04-18 at 09:17 +0100, Alan Jenkins wrote:
>
>   
>> When I looked at the code earlier, I saw no obvious replacement for 
>> rfkill_set_default().  So I tried disabling the wireless and rebooting 
>> to see what happened.  It didn't like that :-).
>>     
>
> Ok that wasn't too hard -- try this on top if you get a chance:
>   

Great, that fixes the crash.


1) I think we need to add a resume method to eeepc-laptop.

Without this, funny things happen when I hibernate, disable wireless in
the BIOS, and resume:

    ath5k phy0: failed to wake up the MAC chip

It's an really stupid thing to do, but it can happen.  It's bad from a
UI point of view.  E.g. in network-manager, you can see a "wlan0"
device, but it doesn't work.

The EEE rfkill is unusual in that it hotplugs the PCI device, making
eeepc-laptop something like a custom pci hotplug driver.  With your
rewrite, eeepc-laptop doesn't notice the state change on resume. 
Previously, the rfkill-core would restore the pre-hibernation state,
which would sort everything out.  I don't think anything else does this,
so we can just add a resume method to eeepc-laptop.  The resume method
would re-check the state and do the PCI hotplug dance if necessary.

If you agree, I can do the patch for this and send it to you.


2) Do you have any thoughts about an rfkill_set_default() equivalent? 
AFAICS your current patch simply removes it.

This means that when I boot linux, it doesn't respect the previous
rfkill state.  I can no longer disable the wireless in the BIOS setup
screen, and the rfkill state won't be preserved over reboots.

I don't have a strong feeling about reboots _on their own_.  But I would
be annoyed if the option in the BIOS setup screen stopped working in a
future version of linux.  Admittedly it's only a matter of principle /
nostalgia - since the eeepc-laptop was fixed to implement rfkill
properly, I've never used the BIOS option in anger.

Thanks
Alan

Re: rfkill rewrite bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2719 bytes --]

On Sat, 2009-04-18 at 10:43 +0100, Alan Jenkins wrote:

> >> When I looked at the code earlier, I saw no obvious replacement for 
> >> rfkill_set_default().  So I tried disabling the wireless and rebooting 
> >> to see what happened.  It didn't like that :-).
> >>     
> >
> > Ok that wasn't too hard -- try this on top if you get a chance:
> >   
> 
> Great, that fixes the crash.
> 
> 
> 1) I think we need to add a resume method to eeepc-laptop.
> 
> Without this, funny things happen when I hibernate, disable wireless in
> the BIOS, and resume:
> 
>     ath5k phy0: failed to wake up the MAC chip
> 
> It's an really stupid thing to do, but it can happen.  It's bad from a
> UI point of view.  E.g. in network-manager, you can see a "wlan0"
> device, but it doesn't work.
> 
> The EEE rfkill is unusual in that it hotplugs the PCI device, making
> eeepc-laptop something like a custom pci hotplug driver.  With your
> rewrite, eeepc-laptop doesn't notice the state change on resume. 
> Previously, the rfkill-core would restore the pre-hibernation state,
> which would sort everything out.  I don't think anything else does this,
> so we can just add a resume method to eeepc-laptop.  The resume method
> would re-check the state and do the PCI hotplug dance if necessary.
> 
> If you agree, I can do the patch for this and send it to you.

Sounds good to me, yeah.

I could make the rfkill core do that at resume, but I'm not really sure
it's what we want -- there are too many cases imho:
 * hard rfkill might have changed
 * soft rfkill might still be ok in hw
 * soft rfkill might need reconfiguring
etc. I think generally it's saner to let the driver sort it out -- it
can always ask for the current state by using set_hw_state() or so.

> 2) Do you have any thoughts about an rfkill_set_default() equivalent? 
> AFAICS your current patch simply removes it.
> 
> This means that when I boot linux, it doesn't respect the previous
> rfkill state.  I can no longer disable the wireless in the BIOS setup
> screen, and the rfkill state won't be preserved over reboots.
> 
> I don't have a strong feeling about reboots _on their own_.  But I would
> be annoyed if the option in the BIOS setup screen stopped working in a
> future version of linux.  Admittedly it's only a matter of principle /
> nostalgia - since the eeepc-laptop was fixed to implement rfkill
> properly, I've never used the BIOS option in anger.

That's odd, I thought I added a set_sw_state() to rfkill which would
disable that rfkill. But there's rfkill_set_global_sw_state() which
should do what you want -- can you try replacing the EEE set_sw_state
call with that?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Rfkill rewrite: eeepc-laptop resume

[Alan Jenkins]

Johannes Berg wrote:
> On Sat, 2009-04-18 at 10:43 +0100, Alan Jenkins wrote:
>   
>>
>>
>> 1) I think we need to add a resume method to eeepc-laptop.
>>
>> Without this, funny things happen when I hibernate, disable wireless in
>> the BIOS, and resume:
>>
>>     ath5k phy0: failed to wake up the MAC chip
>>
>> It's an really stupid thing to do, but it can happen.  It's bad from a
>> UI point of view.  E.g. in network-manager, you can see a "wlan0"
>> device, but it doesn't work.
>>
>> The EEE rfkill is unusual in that it hotplugs the PCI device, making
>> eeepc-laptop something like a custom pci hotplug driver.  With your
>> rewrite, eeepc-laptop doesn't notice the state change on resume. 
>> Previously, the rfkill-core would restore the pre-hibernation state,
>> which would sort everything out.  I don't think anything else does this,
>> so we can just add a resume method to eeepc-laptop.  The resume method
>> would re-check the state and do the PCI hotplug dance if necessary.
>>
>> If you agree, I can do the patch for this and send it to you.
>>     
>
> Sounds good to me, yeah.
>
> I could make the rfkill core do that at resume, but I'm not really sure
> it's what we want -- there are too many cases imho:
>  * hard rfkill might have changed
>  * soft rfkill might still be ok in hw
>  * soft rfkill might need reconfiguring
> etc. I think generally it's saner to let the driver sort it out -- it
> can always ask for the current state by using set_hw_state() or so.
>   

API nit:

 * This function tells the rfkill core that the device is capable of
 * remembering soft blocks (which it is notified of via the set_block
 * method) -- this means that the driver may ignore the return value
 * from rfkill_set_hw_state().

Doesn't this conflict with the declaration of rfkill_set_sw_state() as
__must_check?

Ta
Alan

Re: Rfkill rewrite: eeepc-laptop resume

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

On Sat, 2009-04-18 at 14:29 +0100, Alan Jenkins wrote:

> API nit:
> 
>  * This function tells the rfkill core that the device is capable of
>  * remembering soft blocks (which it is notified of via the set_block
>  * method) -- this means that the driver may ignore the return value
>  * from rfkill_set_hw_state().
> 
> Doesn't this conflict with the declaration of rfkill_set_sw_state() as
> __must_check?

Yeah, in a way it does, but I figure it's rare enough that those who
really can ignore it can write
	(void) rfkill_set_sw_state(...)

Don't really have a strong opinion, it just seemed the mistake in the
other direction would be more common.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Rfkill rewrite: eeepc-laptop resume

[Alan Jenkins]

Johannes Berg wrote:
> On Sat, 2009-04-18 at 14:29 +0100, Alan Jenkins wrote:
>
>   
>> API nit:
>>
>>  * This function tells the rfkill core that the device is capable of
>>  * remembering soft blocks (which it is notified of via the set_block
>>  * method) -- this means that the driver may ignore the return value
>>  * from rfkill_set_hw_state().
>>
>> Doesn't this conflict with the declaration of rfkill_set_sw_state() as
>> __must_check?
>>     
>
> Yeah, in a way it does, but I figure it's rare enough that those who
> really can ignore it can write
> 	(void) rfkill_set_sw_state(...)
>
> Don't really have a strong opinion, it just seemed the mistake in the
> other direction would be more common.
>   
Oops... I meant to write rfkill_set_hw_state(), I think you copied me.  Ok.

So then why is the _sw_ variant marked __must_check?  That looks like a
mistake.  I don't see what I can sensibly do with the return value. 
Unless you want EPO to veto a firmware-initiated enable?

Thanks
Alan

Re: Rfkill rewrite: eeepc-laptop resume

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]

On Sat, 2009-04-18 at 15:02 +0100, Alan Jenkins wrote:

> >>  * This function tells the rfkill core that the device is capable of
> >>  * remembering soft blocks (which it is notified of via the set_block
> >>  * method) -- this means that the driver may ignore the return value
> >>  * from rfkill_set_hw_state().
> >>
> >> Doesn't this conflict with the declaration of rfkill_set_sw_state() as
> >> __must_check?
> >>     
> >
> > Yeah, in a way it does, but I figure it's rare enough that those who
> > really can ignore it can write
> > 	(void) rfkill_set_sw_state(...)
> >
> > Don't really have a strong opinion, it just seemed the mistake in the
> > other direction would be more common.
> >   
> Oops... I meant to write rfkill_set_hw_state(), I think you copied me.  Ok.

I, uh, didn't even pay that much attention.

> So then why is the _sw_ variant marked __must_check?  That looks like a
> mistake.  I don't see what I can sensibly do with the return value. 
> Unless you want EPO to veto a firmware-initiated enable?

Good question. It gives you the hardware enable state but I guess you
know about that already. Hmm :) Yeah it seems that we should remove that
__must_check.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: rfkill rewrite bug

[Alan Jenkins]

Johannes Berg wrote:
> On Sat, 2009-04-18 at 10:43 +0100, Alan Jenkins wrote:
>
>   
>>>> When I looked at the code earlier, I saw no obvious replacement for 
>>>> rfkill_set_default().  So I tried disabling the wireless and rebooting 
>>>> to see what happened.  It didn't like that :-).
>>>>     
>>>>         
>>> Ok that wasn't too hard -- try this on top if you get a chance:
>>>   
>>>       
>> Great, that fixes the crash.
>>
>>
>> 1) I think we need to add a resume method to eeepc-laptop.
>>
>> Without this, funny things happen when I hibernate, disable wireless in
>> the BIOS, and resume:
>>
>>     ath5k phy0: failed to wake up the MAC chip
>>
>> It's an really stupid thing to do, but it can happen.  It's bad from a
>> UI point of view.  E.g. in network-manager, you can see a "wlan0"
>> device, but it doesn't work.
>>
>> The EEE rfkill is unusual in that it hotplugs the PCI device, making
>> eeepc-laptop something like a custom pci hotplug driver.  With your
>> rewrite, eeepc-laptop doesn't notice the state change on resume. 
>> Previously, the rfkill-core would restore the pre-hibernation state,
>> which would sort everything out.  I don't think anything else does this,
>> so we can just add a resume method to eeepc-laptop.  The resume method
>> would re-check the state and do the PCI hotplug dance if necessary.
>>
>> If you agree, I can do the patch for this and send it to you.
>>     
>
> Sounds good to me, yeah.
>
> I could make the rfkill core do that at resume, but I'm not really sure
> it's what we want -- there are too many cases imho:
>  * hard rfkill might have changed
>  * soft rfkill might still be ok in hw
>  * soft rfkill might need reconfiguring
> etc. I think generally it's saner to let the driver sort it out -- it
> can always ask for the current state by using set_hw_state() or so.
>
>   
>> 2) Do you have any thoughts about an rfkill_set_default() equivalent? 
>> AFAICS your current patch simply removes it.
>>
>> This means that when I boot linux, it doesn't respect the previous
>> rfkill state.  I can no longer disable the wireless in the BIOS setup
>> screen, and the rfkill state won't be preserved over reboots.
>>
>> I don't have a strong feeling about reboots _on their own_.  But I would
>> be annoyed if the option in the BIOS setup screen stopped working in a
>> future version of linux.  Admittedly it's only a matter of principle /
>> nostalgia - since the eeepc-laptop was fixed to implement rfkill
>> properly, I've never used the BIOS option in anger.
>>     
>
> That's odd, I thought I added a set_sw_state() to rfkill which would
> disable that rfkill. But there's rfkill_set_global_sw_state() which
> should do what you want -- can you try replacing the EEE set_sw_state
> call with that?
>
> johannes
>   

Well, I think the problem is clear :-P.

    if (!(rfkill_states_default_locked & BIT(rfkill->type))) {
        /* first of its kind */
        BUILD_BUG_ON(NUM_RFKILL_TYPES >
            sizeof(rfkill_states_default_locked) * 8);
        rfkill_states_default_locked |= BIT(rfkill->type);
        rfkill_global_states[rfkill->type].cur =
            rfkill_global_states[rfkill->type].def;
    }

One would expect to see a reference to rfkill->blocked in here.

Alan

Re: rfkill rewrite bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

On Sat, 2009-04-18 at 16:49 +0100, Alan Jenkins wrote:

> > That's odd, I thought I added a set_sw_state() to rfkill which would
> > disable that rfkill. But there's rfkill_set_global_sw_state() which
> > should do what you want -- can you try replacing the EEE set_sw_state
> > call with that?
> >
> > johannes
> >   
> 
> Well, I think the problem is clear :-P.
> 
>     if (!(rfkill_states_default_locked & BIT(rfkill->type))) {
>         /* first of its kind */
>         BUILD_BUG_ON(NUM_RFKILL_TYPES >
>             sizeof(rfkill_states_default_locked) * 8);
>         rfkill_states_default_locked |= BIT(rfkill->type);
>         rfkill_global_states[rfkill->type].cur =
>             rfkill_global_states[rfkill->type].def;
>     }
> 
> One would expect to see a reference to rfkill->blocked in here.

No, that's done asynchronously in rfkill_sync_work(). But actually, heh,
that means set_sw_state can _not_ work before registering.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: rfkill rewrite bug

[Alan Jenkins]

Johannes Berg wrote:
> On Sat, 2009-04-18 at 10:43 +0100, Alan Jenkins wrote:
>
>   
>>>> When I looked at the code earlier, I saw no obvious replacement for 
>>>> rfkill_set_default().  So I tried disabling the wireless and rebooting 
>>>> to see what happened.  It didn't like that :-).
>>>>     
>>>>         
>>> Ok that wasn't too hard -- try this on top if you get a chance:
>>>   
>>>       
>> Great, that fixes the crash.
>>
>>
>> 1) I think we need to add a resume method to eeepc-laptop.
>>
>> Without this, funny things happen when I hibernate, disable wireless in
>> the BIOS, and resume:
>>
>>     ath5k phy0: failed to wake up the MAC chip
>>
>> It's an really stupid thing to do, but it can happen.  It's bad from a
>> UI point of view.  E.g. in network-manager, you can see a "wlan0"
>> device, but it doesn't work.
>>
>> The EEE rfkill is unusual in that it hotplugs the PCI device, making
>> eeepc-laptop something like a custom pci hotplug driver.  With your
>> rewrite, eeepc-laptop doesn't notice the state change on resume. 
>> Previously, the rfkill-core would restore the pre-hibernation state,
>> which would sort everything out.  I don't think anything else does this,
>> so we can just add a resume method to eeepc-laptop.  The resume method
>> would re-check the state and do the PCI hotplug dance if necessary.
>>
>> If you agree, I can do the patch for this and send it to you.
>>     
>
> Sounds good to me, yeah.
>
> I could make the rfkill core do that at resume, but I'm not really sure
> it's what we want -- there are too many cases imho:
>  * hard rfkill might have changed
>  * soft rfkill might still be ok in hw
>  * soft rfkill might need reconfiguring
> etc. I think generally it's saner to let the driver sort it out -- it
> can always ask for the current state by using set_hw_state() or so.
>
>   
>> 2) Do you have any thoughts about an rfkill_set_default() equivalent? 
>> AFAICS your current patch simply removes it.
>>
>> This means that when I boot linux, it doesn't respect the previous
>> rfkill state.  I can no longer disable the wireless in the BIOS setup
>> screen, and the rfkill state won't be preserved over reboots.
>>
>> I don't have a strong feeling about reboots _on their own_.  But I would
>> be annoyed if the option in the BIOS setup screen stopped working in a
>> future version of linux.  Admittedly it's only a matter of principle /
>> nostalgia - since the eeepc-laptop was fixed to implement rfkill
>> properly, I've never used the BIOS option in anger.
>>     
>
> That's odd, I thought I added a set_sw_state() to rfkill which would
> disable that rfkill. But there's rfkill_set_global_sw_state() which
> should do what you want -- can you try replacing the EEE set_sw_state
> call with that?
>
> johannes
>   

Yes, that fixes it.  Now it works the same as the old code which used
rfkill_set_default().

Here's my resume code for eeepc-laptop.

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index c822bfa..5f594c6 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -177,6 +177,7 @@ static struct key_entry eeepc_keymap[] = {
  */
 static int eeepc_hotk_add(struct acpi_device *device);
 static int eeepc_hotk_remove(struct acpi_device *device, int type);
+static int eeepc_hotk_resume(struct acpi_device *device);
 
 static const struct acpi_device_id eeepc_device_ids[] = {
 	{EEEPC_HOTK_HID, 0},
@@ -191,6 +192,7 @@ static struct acpi_driver eeepc_hotk_driver = {
 	.ops = {
 		.add = eeepc_hotk_add,
 		.remove = eeepc_hotk_remove,
+		.resume = eeepc_hotk_resume
 	},
 };
 
@@ -495,14 +497,11 @@ static void notify_brn(void)
 		bd->props.brightness = read_brightness(bd);
 }
 
-static void eeepc_rfkill_notify(acpi_handle handle, u32 event, void *data)
+static void eeepc_rfkill_hotplug()
 {
 	struct pci_dev *dev;
 	struct pci_bus *bus = pci_find_bus(0, 1);
 
-	if (event != ACPI_NOTIFY_BUS_CHECK)
-		return;
-
 	if (!bus) {
 		printk(EEEPC_WARNING "Unable to find PCI bus 1?\n");
 		return;
@@ -530,6 +529,14 @@ static void eeepc_rfkill_notify(acpi_handle handle, u32 event, void *data)
 	}
 }
 
+static void eeepc_rfkill_notify(acpi_handle handle, u32 event, void *data)
+{
+	if (event != ACPI_NOTIFY_BUS_CHECK)
+		return;
+
+	eeepc_rfkill_hotplug();
+}
+
 static void eeepc_hotk_notify(acpi_handle handle, u32 event, void *data)
 {
 	static struct key_entry *key;
@@ -695,6 +702,22 @@ static int eeepc_hotk_remove(struct acpi_device *device, int type)
 	return 0;
 }
 
+static int eeepc_hotk_resume(struct acpi_device *device)
+{
+	if (ehotk->eeepc_wlan_rfkill) {
+		rfkill_set_sw_state(ehotk->eeepc_wlan_rfkill,
+				    get_acpi(CM_ASL_WLAN) != 1);
+
+		eeepc_rfkill_hotplug();
+	}
+
+	if (ehotk->eeepc_bluetooth_rfkill)
+		rfkill_set_sw_state(ehotk->eeepc_bluetooth_rfkill,
+				    get_acpi(CM_ASL_BLUETOOTH) != 1);
+
+	return 0;
+}
+
 /*
  * Hwmon
  */

Re: rfkill rewrite bug

[Alan Jenkins]

Johannes Berg wrote:
> On Sat, 2009-04-18 at 16:49 +0100, Alan Jenkins wrote:
>
>   
>>> That's odd, I thought I added a set_sw_state() to rfkill which would
>>> disable that rfkill. But there's rfkill_set_global_sw_state() which
>>> should do what you want -- can you try replacing the EEE set_sw_state
>>> call with that?
>>>       


> No, that's done asynchronously in rfkill_sync_work(). But actually, heh,
> that means set_sw_state can _not_ work before registering.
>   

Ah, I think I see it now.

Um, what's the use-case for calling set_sw_state() before registration? 
Is it actually needed?

I think it was doing _something_.  If the initial wireless state is
soft-blocked, but rfkill.default_state = 1 (unblocked), then without the
set_sw_state() call, the wireless would remain soft blocked.  When the
first sync_work calls rfkill_set_block(false), the "prev" value would
also be false, so it would return without calling .set_block().  And
you'd have an inconsistency, because "/sys/class/rfkill/rfkill0/state"
would say "1" (unblocked).

You'd have a similar problem the other way around, if the wireless was
initially enabled, but the user specified rfkill.default_state = 0.

So maybe you need a "first time, force sync" flag instead.  That would
also help if you had a write-only rfkill line, no?

Regards
Alan

Re: rfkill rewrite bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

On Sat, 2009-04-18 at 18:48 +0100, Alan Jenkins wrote:

> Ah, I think I see it now.
> 
> Um, what's the use-case for calling set_sw_state() before registration? 
> Is it actually needed?

Probably not. I thought you would use it to update the core's idea of
your state. But the core always forces you to its idea of the state :)

> I think it was doing _something_.  If the initial wireless state is
> soft-blocked, but rfkill.default_state = 1 (unblocked), then without the
> set_sw_state() call, the wireless would remain soft blocked.  When the
> first sync_work calls rfkill_set_block(false), the "prev" value would
> also be false, so it would return without calling .set_block().  And
> you'd have an inconsistency, because "/sys/class/rfkill/rfkill0/state"
> would say "1" (unblocked).
> 
> You'd have a similar problem the other way around, if the wireless was
> initially enabled, but the user specified rfkill.default_state = 0.
> 
> So maybe you need a "first time, force sync" flag instead.  That would
> also help if you had a write-only rfkill line, no?

Not sure what you mean -- the sync is always done on register()?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: rfkill rewrite bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]

On Sat, 2009-04-18 at 18:42 +0100, Alan Jenkins wrote:

> > That's odd, I thought I added a set_sw_state() to rfkill which would
> > disable that rfkill. But there's rfkill_set_global_sw_state() which
> > should do what you want -- can you try replacing the EEE set_sw_state
> > call with that?

> Yes, that fixes it.  Now it works the same as the old code which used
> rfkill_set_default().

Cool.

> +static int eeepc_hotk_resume(struct acpi_device *device);
>  
>  static const struct acpi_device_id eeepc_device_ids[] = {
>  	{EEEPC_HOTK_HID, 0},
> @@ -191,6 +192,7 @@ static struct acpi_driver eeepc_hotk_driver = {
>  	.ops = {
>  		.add = eeepc_hotk_add,
>  		.remove = eeepc_hotk_remove,
> +		.resume = eeepc_hotk_resume

Please add a , at the end so other people updating that in the future
don't need to change that line too.

> +static void eeepc_rfkill_hotplug()

Need (void) not ()

If you change those and give me a S-o-b I'll integrate it into my patch
with something like

S-o-b: <you> [eeepc driver parts]

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: rfkill rewrite bug

[Alan Jenkins]

Johannes Berg wrote:
> On Sat, 2009-04-18 at 18:48 +0100, Alan Jenkins wrote:
>
>   
>> Ah, I think I see it now.
>>
>> Um, what's the use-case for calling set_sw_state() before registration? 
>> Is it actually needed?
>>     
>
> Probably not. I thought you would use it to update the core's idea of
> your state. But the core always forces you to its idea of the state :)
>
>   
>> I think it was doing _something_.  If the initial wireless state is
>> soft-blocked, but rfkill.default_state = 1 (unblocked), then without the
>> set_sw_state() call, the wireless would remain soft blocked.  When the
>> first sync_work calls rfkill_set_block(false), the "prev" value would
>> also be false, so it would return without calling .set_block().  And
>> you'd have an inconsistency, because "/sys/class/rfkill/rfkill0/state"
>> would say "1" (unblocked).
>>
>> You'd have a similar problem the other way around, if the wireless was
>> initially enabled, but the user specified rfkill.default_state = 0.
>>
>> So maybe you need a "first time, force sync" flag instead.  That would
>> also help if you had a write-only rfkill line, no?
>>     
>
> Not sure what you mean -- the sync is always done on register()?
>
> johannes
>   

Sorry, I misread the code again.  I thought rfkill_set_block() returned
early if the new state was equal to the old one, but it doesn't.

Re: rfkill rewrite bug

[Alan Jenkins]

Johannes Berg wrote:
> On Sat, 2009-04-18 at 18:42 +0100, Alan Jenkins wrote:
>
>   
>>> That's odd, I thought I added a set_sw_state() to rfkill which would
>>> disable that rfkill. But there's rfkill_set_global_sw_state() which
>>> should do what you want -- can you try replacing the EEE set_sw_state
>>> call with that?
>>>       
>
>   
>> Yes, that fixes it.  Now it works the same as the old code which used
>> rfkill_set_default().
>>     
>
> Cool.
>
>   
>> +static int eeepc_hotk_resume(struct acpi_device *device);
>>  
>>  static const struct acpi_device_id eeepc_device_ids[] = {
>>  	{EEEPC_HOTK_HID, 0},
>> @@ -191,6 +192,7 @@ static struct acpi_driver eeepc_hotk_driver = {
>>  	.ops = {
>>  		.add = eeepc_hotk_add,
>>  		.remove = eeepc_hotk_remove,
>> +		.resume = eeepc_hotk_resume
>>     
>
> Please add a , at the end so other people updating that in the future
> don't need to change that line too.
>
>   
>> +static void eeepc_rfkill_hotplug()
>>     
>
> Need (void) not ()
>
> If you change those and give me a S-o-b I'll integrate it into my patch
> with something like
>
> S-o-b: <you> [eeepc driver parts]
>
>   
> johannes

Ok, revised patch is below. Don't forget the set_sw_state() ->
set_global_sw_state() change; I didn't include that.


Note that there other pending changes to the rfkill code in eeepc-laptop.

"[PATCH] eee-laptop: Register as a pci-hotplug device"
<http://thread.gmane.org/gmane.linux.kernel/791730/focus=38724>

"[PATCH] eeepc-laptop: Work around rfkill firmware bug"
<http://thread.gmane.org/gmane.linux.acpi.devel/38424>

The second is mine; sorry for not warning about it sooner.  So you/we
probably need to co-ordinate with the maintainer.  I'd CC linux-acpi,
because that's where most platform driver changes are submitted.

---
Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 0e7a946..10366b2 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -177,6 +177,7 @@ static struct key_entry eeepc_keymap[] = {
  */
 static int eeepc_hotk_add(struct acpi_device *device);
 static int eeepc_hotk_remove(struct acpi_device *device, int type);
+static int eeepc_hotk_resume(struct acpi_device *device);
 
 static const struct acpi_device_id eeepc_device_ids[] = {
 	{EEEPC_HOTK_HID, 0},
@@ -191,6 +192,7 @@ static struct acpi_driver eeepc_hotk_driver = {
 	.ops = {
 		.add = eeepc_hotk_add,
 		.remove = eeepc_hotk_remove,
+		.resume = eeepc_hotk_resume,
 	},
 };
 
@@ -495,14 +497,11 @@ static void notify_brn(void)
 		bd->props.brightness = read_brightness(bd);
 }
 
-static void eeepc_rfkill_notify(acpi_handle handle, u32 event, void *data)
+static void eeepc_rfkill_hotplug(void)
 {
 	struct pci_dev *dev;
 	struct pci_bus *bus = pci_find_bus(0, 1);
 
-	if (event != ACPI_NOTIFY_BUS_CHECK)
-		return;
-
 	if (!bus) {
 		printk(EEEPC_WARNING "Unable to find PCI bus 1?\n");
 		return;
@@ -530,6 +529,14 @@ static void eeepc_rfkill_notify(acpi_handle handle, u32 event, void *data)
 	}
 }
 
+static void eeepc_rfkill_notify(acpi_handle handle, u32 event, void *data)
+{
+	if (event != ACPI_NOTIFY_BUS_CHECK)
+		return;
+
+	eeepc_rfkill_hotplug();
+}
+
 static void eeepc_hotk_notify(acpi_handle handle, u32 event, void *data)
 {
 	static struct key_entry *key;
@@ -695,6 +702,22 @@ static int eeepc_hotk_remove(struct acpi_device *device, int type)
 	return 0;
 }
 
+static int eeepc_hotk_resume(struct acpi_device *device)
+{
+	if (ehotk->eeepc_wlan_rfkill) {
+		rfkill_set_sw_state(ehotk->eeepc_wlan_rfkill,
+				    get_acpi(CM_ASL_WLAN) != 1);
+
+		eeepc_rfkill_hotplug();
+	}
+
+	if (ehotk->eeepc_bluetooth_rfkill)
+		rfkill_set_sw_state(ehotk->eeepc_bluetooth_rfkill,
+				    get_acpi(CM_ASL_BLUETOOTH) != 1);
+
+	return 0;
+}
+
 /*
  * Hwmon
  */

Re: rfkill rewrite bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

On Mon, 2009-04-20 at 09:33 +0100, Alan Jenkins wrote:

> Ok, revised patch is below. Don't forget the set_sw_state() ->
> set_global_sw_state() change; I didn't include that.

Ok, will do. Thanks for the patch, I'll integrate it into mine.

> Note that there other pending changes to the rfkill code in eeepc-laptop.
> 
> "[PATCH] eee-laptop: Register as a pci-hotplug device"
> <http://thread.gmane.org/gmane.linux.kernel/791730/focus=38724>
> 
> "[PATCH] eeepc-laptop: Work around rfkill firmware bug"
> <http://thread.gmane.org/gmane.linux.acpi.devel/38424>
> 
> The second is mine; sorry for not warning about it sooner.  So you/we
> probably need to co-ordinate with the maintainer.  I'd CC linux-acpi,
> because that's where most platform driver changes are submitted.

Ok, thanks for the heads up. Do you think this will generate significant
conflicts?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: rfkill rewrite bug

[Alan Jenkins]

Johannes Berg wrote:
> On Mon, 2009-04-20 at 09:33 +0100, Alan Jenkins wrote:
>
>   
>> Ok, revised patch is below. Don't forget the set_sw_state() ->
>> set_global_sw_state() change; I didn't include that.
>>     
>
> Ok, will do. Thanks for the patch, I'll integrate it into mine.
>
>   
>> Note that there other pending changes to the rfkill code in eeepc-laptop.
>>
>> "[PATCH] eee-laptop: Register as a pci-hotplug device"
>> <http://thread.gmane.org/gmane.linux.kernel/791730/focus=38724>
>>
>> "[PATCH] eeepc-laptop: Work around rfkill firmware bug"
>> <http://thread.gmane.org/gmane.linux.acpi.devel/38424>
>>
>> The second is mine; sorry for not warning about it sooner.  So you/we
>> probably need to co-ordinate with the maintainer.  I'd CC linux-acpi,
>> because that's where most platform driver changes are submitted.
>>     
>
> Ok, thanks for the heads up. Do you think this will generate significant
> conflicts?
>   

I'm not quite sure what would be considered significant, but it's not
trivial.  At a high level, I don't think the new behaviours conflict
with the new rfkill semantics :-).  But the firmware bug workaround
can't be resolved completely mechanically.  And the pci-hotplug patch
touches the rfkill error path.

Re: rfkill rewrite bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

On Mon, 2009-04-20 at 10:20 +0100, Alan Jenkins wrote:

> > Ok, thanks for the heads up. Do you think this will generate significant
> > conflicts?
> >   
> 
> I'm not quite sure what would be considered significant, but it's not
> trivial.  At a high level, I don't think the new behaviours conflict
> with the new rfkill semantics :-).  But the firmware bug workaround
> can't be resolved completely mechanically.  And the pci-hotplug patch
> touches the rfkill error path.

Ok, that helps, I think I'll just resolve the conflict when it hits
linux-next. Might need to ask for your help then :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]