Re: rfkill rewrite bug

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 --]