Re: Rfkill rewrite

Re: Rfkill rewrite

[Alan Jenkins]

On 4/18/09, Johannes Berg <johannes@sipsolutions.net> 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_hw_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_hw_state(...)
>
> Don't really have a strong opinion, it just seemed the mistake in the
> other direction would be more common.

It seems my GCC has a stronger definition of "must" than you do :-).  I can't get rid of the warning by casting it to void.  So I'm not sure __must_check is really appropriate here.

drivers/platform/x86/hp-wmi.c: In function ‘hp_wmi_bios_setup’:
drivers/platform/x86/hp-wmi.c:467: warning: ignoring return value of ‘rfkill_set_hw_state’, declared with attribute warn_unused_result

$ grep -n rfkill_set_hw_state drivers/platform/x86/hp-wmi.c
467:            (void) rfkill_set_hw_state(wifi_rfkill,

$ gcc -v
Using built-in specs.
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --enable-languages=c,c++,fortran,objc,obj-c++,treelang --prefix=/usr --enable-shared --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --enable-nls --with-gxx-include-dir=/usr/include/c++/4.2 --program-suffix=-4.2 --enable-clocale=gnu --enable-libstdcxx-debug --enable-objc-gc --enable-mpfr --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.2.4 (Ubuntu 4.2.4-1ubuntu3)


Thanks
Alan

Re: Rfkill rewrite

[Johannes Berg]

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

On Sat, 2009-07-18 at 16:46 +0100, Alan Jenkins wrote:

> It seems my GCC has a stronger definition of "must" than you do :-).
> I can't get rid of the warning by casting it to void.  So I'm not sure
> __must_check is really appropriate here.

Heh, my gcc agrees with you :)
I'm happy to remove it -- want to send a patch?

johannes

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

[PATCH] rfkill: remove too-strict __must_check

[Alan Jenkins]

Some drivers don't need the return value of rfkill_set_hw_state(),
so it should not be marked as __must_check.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---
 include/linux/rfkill.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index f3d5812..1020290 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -238,7 +238,7 @@ void rfkill_destroy(struct rfkill *rfkill);
  * should be blocked) so that drivers need not keep track of the soft
  * block state -- which they might not be able to.
  */
-bool __must_check rfkill_set_hw_state(struct rfkill *rfkill, bool blocked);
+bool rfkill_set_hw_state(struct rfkill *rfkill, bool blocked);
 
 /**
  * rfkill_set_sw_state - Set the internal rfkill software block state
-- 
1.6.3.2

Re: [PATCH] rfkill: remove too-strict __must_check

[Johannes Berg]

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

On Sat, 2009-07-18 at 19:20 +0100, Alan Jenkins wrote:
> Some drivers don't need the return value of rfkill_set_hw_state(),
> so it should not be marked as __must_check.
> 
> Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

Acked-by: Johannes Berg <johannes@sipsolutions.net>

Thanks.

> ---
>  include/linux/rfkill.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
> index f3d5812..1020290 100644
> --- a/include/linux/rfkill.h
> +++ b/include/linux/rfkill.h
> @@ -238,7 +238,7 @@ void rfkill_destroy(struct rfkill *rfkill);
>   * should be blocked) so that drivers need not keep track of the soft
>   * block state -- which they might not be able to.
>   */
> -bool __must_check rfkill_set_hw_state(struct rfkill *rfkill, bool blocked);
> +bool rfkill_set_hw_state(struct rfkill *rfkill, bool blocked);
>  
>  /**
>   * rfkill_set_sw_state - Set the internal rfkill software block state

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

Re: rfkill rewrite

[Alan Jenkins]

On 4/18/09, Johannes Berg <johannes@sipsolutions.net> 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.

I just realized or remembered something non-obvious.  This means _all_
rfkill drivers need a resume handler.  They don't at the moment, so
this would need to be fixed and documented.  In which case, it's
probably simpler for the core to do it.

You need this to handle hibernation.  If the rfkill state persists
across hibernation (which mine does, even in S5), you can always cause
the state to change, by pressing the wireless toggle key _while the
hibernation image is being written to disk_.  At that stage, all
devices are active, so that s2disk can interact with the user and
write the image wherever it chooses.  The kernel will not "remember"
the state change on resume, since it happened after the kernel image
was snapshotted.

If the rfkill state does _not_ persist over hibernation, then clearly
the state can change on resume and you will again need a resume
handler,

On my EeePC, this is just more dramatic because it can de-power a PCI
device without notifying the driver.  But the new rfkill design will
require all devices to have a resume method, because there is no
get_state() callback.  Otherwise, reading  of
/sys/class/rfkill/rfkill*/state after resume from hibernation may
return an incorrect result.  I don't think we should allow that to
happen.

Regards
Alan

Re: rfkill rewrite

[Johannes Berg]

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

Hi,

Sorry for the long wait.

> > 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.
> 
> I just realized or remembered something non-obvious.  This means _all_
> rfkill drivers need a resume handler.  They don't at the moment, so
> this would need to be fixed and documented.  In which case, it's
> probably simpler for the core to do it.

Only those that have a hard-rfkill line, which I think isn't all. But
yes. However, how would the core handle it?

> You need this to handle hibernation.  If the rfkill state persists
> across hibernation (which mine does, even in S5), you can always cause
> the state to change, by pressing the wireless toggle key _while the
> hibernation image is being written to disk_.  At that stage, all
> devices are active, so that s2disk can interact with the user and
> write the image wherever it chooses.  The kernel will not "remember"
> the state change on resume, since it happened after the kernel image
> was snapshotted.

Good scenario, yes, it's definitely possible.

> If the rfkill state does _not_ persist over hibernation, then clearly
> the state can change on resume and you will again need a resume
> handler,
> 
> On my EeePC, this is just more dramatic because it can de-power a PCI
> device without notifying the driver.  But the new rfkill design will
> require all devices to have a resume method, because there is no
> get_state() callback.  Otherwise, reading  of
> /sys/class/rfkill/rfkill*/state after resume from hibernation may
> return an incorrect result.  I don't think we should allow that to
> happen.

So we would need to add a query_resume() method like query that is
called at resume time, and needs to call rfkill_set{,_hw,_sw}_state as
appopriate. Thoughts? We can enforce that much easier by making it a
required method.

johannes

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

Re: rfkill rewrite

[Alan Jenkins]

On 5/5/09, Johannes Berg <johannes@sipsolutions.net> wrote:
> Hi,
>
> Sorry for the long wait.
>
>> > 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.
>>
>> I just realized or remembered something non-obvious.  This means _all_
>> rfkill drivers need a resume handler.  They don't at the moment, so
>> this would need to be fixed and documented.  In which case, it's
>> probably simpler for the core to do it.
>
> Only those that have a hard-rfkill line, which I think isn't all.

My scenario quoted below affects soft-rfkill only.

> But yes. However, how would the core handle it?

I was thinking of calling set_block() on resume to restore the
pre-suspend state.  That's what the old rfkill does, and why
eeepc-laptop got away without a resume handler.

>> You need this to handle hibernation.  If the rfkill state persists
>> across hibernation (which mine does, even in S5), you can always cause
>> the state to change, by pressing the wireless toggle key _while the
>> hibernation image is being written to disk_.  At that stage, all
>> devices are active, so that s2disk can interact with the user and
>> write the image wherever it chooses.  The kernel will not "remember"
>> the state change on resume, since it happened after the kernel image
>> was snapshotted.
>
> Good scenario, yes, it's definitely possible.
>
>> If the rfkill state does _not_ persist over hibernation, then clearly
>> the state can change on resume and you will again need a resume
>> handler,
>>
>> On my EeePC, this is just more dramatic because it can de-power a PCI
>> device without notifying the driver.  But the new rfkill design will
>> require all devices to have a resume method, because there is no
>> get_state() callback.  Otherwise, reading  of
>> /sys/class/rfkill/rfkill*/state after resume from hibernation may
>> return an incorrect result.  I don't think we should allow that to
>> happen.
>
> So we would need to add a query_resume() method like query that is
> called at resume time, and needs to call rfkill_set{,_hw,_sw}_state as
> appopriate. Thoughts? We can enforce that much easier by making it a
> required method.

That would certainly follow your existing model.  (BTW the kerneldoc
for "query" is out of sync in v8; it says "return true for blocked"
even though the prototype returns void :-).

It's debatable as to what behaviour is more user-friendly for my nasty
corner-case.  query_resume() would allow the radio to always come back
in the same state as it was in the moment the laptop went to sleep, if
the hardware supports it.  But the _global_ rfkill state will then be
out of sync, and that means the next time you press the wireless
toggle key it does nothing, which is disconcerting.  Plus, it means
the corner-case behaviour varies depending on whether the soft-rfkill
line state survives over hibernation - which may even vary on the same
machine (S5 versus S4? whether you remove the laptop's battery? weird
firmware bugs?).

All I'm trying to do is selfishly preserve eeepc-laptop across this
re-write, for which purpose it seems easier to have set_block() called
on resume :-).  I don't know if that makes rfkill more awkward in
wireless drivers (as opposed to platform drivers), or drivers which
have a hardware rfkill line.

Alan

Re: rfkill rewrite

[Johannes Berg]

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

> >> I just realized or remembered something non-obvious.  This means _all_
> >> rfkill drivers need a resume handler.  They don't at the moment, so
> >> this would need to be fixed and documented.  In which case, it's
> >> probably simpler for the core to do it.
> >
> > Only those that have a hard-rfkill line, which I think isn't all.
> 
> My scenario quoted below affects soft-rfkill only.

Ah, you're thinking of that.

> > But yes. However, how would the core handle it?
> 
> I was thinking of calling set_block() on resume to restore the
> pre-suspend state.  That's what the old rfkill does, and why
> eeepc-laptop got away without a resume handler.

That makes sense.

> > So we would need to add a query_resume() method like query that is
> > called at resume time, and needs to call rfkill_set{,_hw,_sw}_state as
> > appopriate. Thoughts? We can enforce that much easier by making it a
> > required method.
> 
> That would certainly follow your existing model.  (BTW the kerneldoc
> for "query" is out of sync in v8; it says "return true for blocked"
> even though the prototype returns void :-).

Yeah, I noticed already and fixed it yesterday but didn't post v9 yet
(will also amend v9 with the outcome of this discussion)

> It's debatable as to what behaviour is more user-friendly for my nasty
> corner-case.  query_resume() would allow the radio to always come back
> in the same state as it was in the moment the laptop went to sleep, if
> the hardware supports it.  But the _global_ rfkill state will then be
> out of sync, and that means the next time you press the wireless
> toggle key it does nothing, which is disconcerting.  Plus, it means
> the corner-case behaviour varies depending on whether the soft-rfkill
> line state survives over hibernation - which may even vary on the same
> machine (S5 versus S4? whether you remove the laptop's battery? weird
> firmware bugs?).
> 
> All I'm trying to do is selfishly preserve eeepc-laptop across this
> re-write, for which purpose it seems easier to have set_block() called
> on resume :-).  I don't know if that makes rfkill more awkward in
> wireless drivers (as opposed to platform drivers), or drivers which
> have a hardware rfkill line.

No, should be fine. I can add that and remove the resume code from
eeepc.

johannes

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