This patchset contains rfkill changes that have a chance of being good
enough to be considered for inclusion in wireless-testing. Their target
is the next merge window (2.6.28).
Please comment/ask for changes, or ACK as appropriate.
Henrique de Moraes Holschuh (8):
rfkill: detect bogus double-registering (v2)
rfkill: add default global states (v2)
rfkill: add __must_check annotations
rfkill: introduce RFKILL_STATE_MAX
rfkill: add WARN_ON and BUG_ON paranoia
rfkill: use the new WARN()
rfkill: rename rfkill_mutex to rfkill_global_mutex
rfkill: add support for wake-on-wireless-packet
I have removed from the stack the changes dealing with rfkill-input
global operations and "master_switch_mode" for refactoring. They will
be back at a later patchset.
For some reason, I got this idea that the new Intel iwl5k cards are
capable of WoWL. The last patch (rfkill: add support for
wake-on-wireless-packet) was added because of that (possibly incorrect)
idea. Should we keep it, or just drop it? Is it of any use or just
some useless rfkill feature nobody will need?
Thanks.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Saturday 02 August 2008, Henrique de Moraes Holschuh wrote:
> Switch use of WARN_ON(1) to the new WARN() macro. Do this on a separate
> patch to make rfkill backports easier on trees like wireless-compat.
I rather see it merged into the other patch, backporting shouldn't really
be kept in mind for development for the next kernel.
The WARN() macro can easily be backported, and it doesn't make a real difference
if this patch is provided seperately or merged with the pevious one.
Both patches will end up in the same backport package anyway ;)
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
> ---
> net/rfkill/rfkill.c | 28 +++++++++++++++++++++-------
> 1 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index d5f95cb..88368c4 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -204,7 +204,9 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
> * RFKILL_STATE_HARD_BLOCKED */
> break;
> default:
> - WARN_ON(1);
> + WARN(1, KERN_WARNING
> + "rfkill: illegal state %d passed as parameter "
> + "to rfkill_toggle_radio\n", state);
> return -EINVAL;
> }
>
> @@ -240,7 +242,9 @@ static void __rfkill_switch_all(const enum rfkill_type type,
> struct rfkill *rfkill;
>
> if (unlikely(state >= RFKILL_STATE_MAX || type >= RFKILL_TYPE_MAX)) {
> - WARN_ON(1);
> + WARN(1, KERN_WARNING
> + "rfkill: illegal state %d or type %d passed as "
> + "parameter to __rfkill_switch_all\n", state, type);
> return;
> }
>
> @@ -341,7 +345,9 @@ int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state)
>
> BUG_ON(!rfkill);
> if (unlikely(state >= RFKILL_STATE_MAX)) {
> - WARN_ON(1);
> + WARN(1, KERN_WARNING
> + "rfkill: illegal state %d passed as parameter "
> + "to rfkill_force_state\n", state);
> return -EINVAL;
> }
>
> @@ -600,7 +606,9 @@ static int rfkill_check_duplicity(const struct rfkill *rfkill)
>
> list_for_each_entry(p, &rfkill_list, node) {
> if (p == rfkill) {
> - WARN_ON(1);
> + WARN(1, KERN_WARNING
> + "rfkill: illegal attempt to register "
> + "an already registered rfkill struct\n");
> return -EEXIST;
> }
> set_bit(p->type, seen);
> @@ -671,7 +679,9 @@ struct rfkill * __must_check rfkill_allocate(struct device *parent,
> struct device *dev;
>
> if (type >= RFKILL_TYPE_MAX) {
> - WARN_ON(1);
> + WARN(1, KERN_WARNING
> + "rfkill: illegal type %d passed as parameter "
> + "to rfkill_allocate\n", type);
> return NULL;
> }
>
> @@ -751,7 +761,9 @@ int __must_check rfkill_register(struct rfkill *rfkill)
> !rfkill->toggle_radio ||
> rfkill->type >= RFKILL_TYPE_MAX ||
> rfkill->state >= RFKILL_STATE_MAX)) {
> - WARN_ON(1);
> + WARN(1, KERN_WARNING
> + "rfkill: attempt to register a "
> + "badly initialized rfkill struct\n");
> return -EINVAL;
> }
>
> @@ -827,7 +839,9 @@ int rfkill_set_default(enum rfkill_type type, enum rfkill_state state)
> if (type >= RFKILL_TYPE_MAX ||
> (state != RFKILL_STATE_SOFT_BLOCKED &&
> state != RFKILL_STATE_UNBLOCKED)) {
> - WARN_ON(1);
> + WARN(1, KERN_WARNING
> + "rfkill: illegal state %d or type %d passed as "
> + "parameter to rfkill_set_default\n", state, type);
> return -EINVAL;
> }
>
On Sunday 03 August 2008, Henrique de Moraes Holschuh wrote:
> On Sun, 03 Aug 2008, Johannes Berg wrote:
> > if (WARN_ON(!nb))
> > return -EINVAL;
>
> I could use the notation above instead of:
> if (foo) {
> WARN_ON(1);
> return -ERROR;
> }
>
> Ivo, which one you prefer? The if() with the condition and WARN on the
> branch, or the if(WARN_ON(condition)) ?
Well my preference is either:
if (WARN_ON(!nb))
return -EINVAL;
or
if (foo) {
WARN();
return -EINVAL;
}
Doesn't really matter which of those 2. But like I said in the other patch,
WARN_ON(1) sounds just ugly. ;)
> > BUG() never returns. Same for all the other places you pointed out.
>
> Yes. And I used BUG() on the notify chain calls, because the primitives in
> the kernel code are not doing proper error checking anyway (for speed, I
> suppose)... they just OOPS.
Ok, if for the notify chain BUG() is standard you can use that there. But the
others are preferably WARN(). :)
Ivo
On Mon, 2008-08-04 at 19:30 -0300, Henrique de Moraes Holschuh wrote:
> On Mon, 04 Aug 2008, Dan Williams wrote:
> > On Sat, 2008-08-02 at 16:27 -0300, Henrique de Moraes Holschuh wrote:
> > > On Sat, 02 Aug 2008, Johannes Berg wrote:
> > > > On Sat, 2008-08-02 at 15:11 -0300, Henrique de Moraes Holschuh wrote:
> > > > > Currently, rfkill would stand in the way of properly supporting wireless
> > > > > devices that are capable of waking the system up from sleep or hibernation
> > > > > when they receive a special wireless message.
> > > > >
> > > > > Since rfkill attempts to soft-block any transmitters during class suspend,
> > > >
> > > > why does it interfere with suspend anyway?
> > >
> > > The class makes sure that all transmitters are blocked on suspend. You'd
> > > have to ask Ivo for the reason, but AFAIK, it is for both safety and to help
> > > conserve power.
> >
> > rfkill shouldn't be touching stuff during suspend.
>
> rfkill shouldn't *need* to touch stuff during suspend. But for that to be
> true, all drivers using rfkill need to properly suspend in the first place.
>
> And that's more than just drivers/net/wireless.
>
> > In the OLPC libertas case, the radio may remain _ON_ during suspend,
> > because the OLPC machines are expected to suspend/resume many times per
> > second, and the radio must continue to participate in the mesh during
> > that time. The only case where the radio gets blocked is when the user
> > requests it or when regulations require it.
>
> Yes. And the fact that rfkill stood in the way of doing that is a bug.
> However, even my first try of a patch would already allow libertas to
> declare it doesn't want rfkill to bother it on suspend.
>
> It is very clear some drivers don't want rfkill to block radios on suspend.
> Really. So far, libertas and iwl* are on my list of "don't want" or "don't
> need". From what I can see, PCI-based rt2xxx is also "don't need". And I
> can assume everything in drivers/misc is "don't need" without too much risk
> of being wrong.
>
> But the OPPOSITE is not clear at all to me. I don't know whether the other
> users of rfkill need a radio block on suspend or not. Unless someone can
> look over *all* in-tree users of linux/rfkill.h and state that none of them
> need it because all of them DO shutdown their devices on suspend, I will
> have to ask the maintainers of every single one about it before I ask a
> patch to be merged. I already looked, and I don't know enough to have a
> definitive answer by myself.
Using rfkill to enforce suspend power policy at a kernel-level is just
wrong. That's a policy decision for gnome-power-manager or
kde-power-manager or whatever. At the very least, it should be an
option in sysfs to turn this behavior on or off.
Dan
> > Suspend != block, and tying suspend and rfkill together really is a
> > policy decision. Thus, I don't agree that rfkill should block radios on
> > suspend.
>
> If some drivers out there are relying on it to, we need to know that before
> we remove it completely.
>
On Mon, 2008-08-04 at 20:35 -0300, Henrique de Moraes Holschuh wrote:
> On Mon, 04 Aug 2008, Dan Williams wrote:
> > > But the OPPOSITE is not clear at all to me. I don't know whether the other
> > > users of rfkill need a radio block on suspend or not. Unless someone can
> > > look over *all* in-tree users of linux/rfkill.h and state that none of them
> > > need it because all of them DO shutdown their devices on suspend, I will
> > > have to ask the maintainers of every single one about it before I ask a
> > > patch to be merged. I already looked, and I don't know enough to have a
> > > definitive answer by myself.
> >
> > Using rfkill to enforce suspend power policy at a kernel-level is just
> > wrong. That's a policy decision for gnome-power-manager or
> > kde-power-manager or whatever. At the very least, it should be an
> > option in sysfs to turn this behavior on or off.
>
> There is no way I am adding an interface for userspace to decide how a
> driver+rfkill stack should go in order to properly suspend a device. The
> kernel is to get it right by itself. It already knows whether the device
> was blocked or not before the suspend. And, when it is suppored by the
> device, the device driver already knows if it is part of a non-stop mesh
> (libertas), or has to have WoWL enabled, etc.
>
> And it is already damn clear that what we currently have (rfkill always
> blocks on suspend) is not the correct way to go about it. WHAT I want to
> know now is whether there are any drivers out there which need the current
> behaviour.
Ah! I seem to have misunderstood you. If some drivers _do_ need the
current block-on-suspend behavior, I feel like that should be an
internal driver decision that rfkill shouldn't need to be aware of.
Drivers know how to suspend themselves; we shouldn't expect rfkill to
know how certain hardware needs to suspend.
Dan
On Mon, 04 Aug 2008, Dan Williams wrote:
> > But the OPPOSITE is not clear at all to me. I don't know whether the other
> > users of rfkill need a radio block on suspend or not. Unless someone can
> > look over *all* in-tree users of linux/rfkill.h and state that none of them
> > need it because all of them DO shutdown their devices on suspend, I will
> > have to ask the maintainers of every single one about it before I ask a
> > patch to be merged. I already looked, and I don't know enough to have a
> > definitive answer by myself.
>
> Using rfkill to enforce suspend power policy at a kernel-level is just
> wrong. That's a policy decision for gnome-power-manager or
> kde-power-manager or whatever. At the very least, it should be an
> option in sysfs to turn this behavior on or off.
There is no way I am adding an interface for userspace to decide how a
driver+rfkill stack should go in order to properly suspend a device. The
kernel is to get it right by itself. It already knows whether the device
was blocked or not before the suspend. And, when it is suppored by the
device, the device driver already knows if it is part of a non-stop mesh
(libertas), or has to have WoWL enabled, etc.
And it is already damn clear that what we currently have (rfkill always
blocks on suspend) is not the correct way to go about it. WHAT I want to
know now is whether there are any drivers out there which need the current
behaviour.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tue, Aug 05, 2008 at 09:03:29AM -0400, Dan Williams wrote:
> On Mon, 2008-08-04 at 20:35 -0300, Henrique de Moraes Holschuh wrote:
> > And it is already damn clear that what we currently have (rfkill always
> > blocks on suspend) is not the correct way to go about it. WHAT I want to
> > know now is whether there are any drivers out there which need the current
> > behaviour.
>
> Ah! I seem to have misunderstood you. If some drivers _do_ need the
> current block-on-suspend behavior, I feel like that should be an
> internal driver decision that rfkill shouldn't need to be aware of.
> Drivers know how to suspend themselves; we shouldn't expect rfkill to
> know how certain hardware needs to suspend.
I agree with Dan. Blocking and suspending should be separate operations.
John
--
John W. Linville
[email protected]
On Mon, 04 Aug 2008, Tomas Winkler wrote:
> > Err, no. If it involves energy emission, if rfkill forbade it [which is to
> > be taken as the user forbade it], the driver must not, EVER, cause it to
> > happen. There are no exceptions.
>
> I didn't mean those kind of events :) iwlwifi driver has proper FCC
> certification.
One thing has nothing to do with the other :) transmitter blocked == energy
emission forbidden. I say it like that so that people don't think it is
okay to just block data transmission and leave a carrier still being
transmitted, or whatever.
Remember, rfkill is *generic*, it goes well beyond WiFi.
> >> > Sure. I was wondering about drivers that *don't* have it, if any, out of
> >> > the potential set of drivers that should be using rfkill (it is not a matter
> >> > of those who are using rfkill right now).
> >>
> >> I think we are aligned in general.
> >
> > I may still have it as optional, but I will switch the default behaviour
> > around. It will likely be useful for someone, and it is less than 10 LOC.
>
> I make an effort to remove it's a mess.
Mess? How so?
Anyway, I have been looking at everything that includes linux/rfkill.h. I
cannot trust every current in-tree user of rfkill to be shutting down the
devices on suspend by just a quick look at their drivers, I just don't know
enough about drivers/net/usb/hso or arm/mach-pxa/tosa. I can't even be sure
about rt2xxx USB by just looking at the code...
So I will ask their maintainers first, instead of trying to guess. Lets see
what answers I get.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
Switch use of WARN_ON(1) to the new WARN() macro. Do this on a separate
patch to make rfkill backports easier on trees like wireless-compat.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
---
net/rfkill/rfkill.c | 28 +++++++++++++++++++++-------
1 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index d5f95cb..88368c4 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -204,7 +204,9 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
* RFKILL_STATE_HARD_BLOCKED */
break;
default:
- WARN_ON(1);
+ WARN(1, KERN_WARNING
+ "rfkill: illegal state %d passed as parameter "
+ "to rfkill_toggle_radio\n", state);
return -EINVAL;
}
@@ -240,7 +242,9 @@ static void __rfkill_switch_all(const enum rfkill_type type,
struct rfkill *rfkill;
if (unlikely(state >= RFKILL_STATE_MAX || type >= RFKILL_TYPE_MAX)) {
- WARN_ON(1);
+ WARN(1, KERN_WARNING
+ "rfkill: illegal state %d or type %d passed as "
+ "parameter to __rfkill_switch_all\n", state, type);
return;
}
@@ -341,7 +345,9 @@ int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state)
BUG_ON(!rfkill);
if (unlikely(state >= RFKILL_STATE_MAX)) {
- WARN_ON(1);
+ WARN(1, KERN_WARNING
+ "rfkill: illegal state %d passed as parameter "
+ "to rfkill_force_state\n", state);
return -EINVAL;
}
@@ -600,7 +606,9 @@ static int rfkill_check_duplicity(const struct rfkill *rfkill)
list_for_each_entry(p, &rfkill_list, node) {
if (p == rfkill) {
- WARN_ON(1);
+ WARN(1, KERN_WARNING
+ "rfkill: illegal attempt to register "
+ "an already registered rfkill struct\n");
return -EEXIST;
}
set_bit(p->type, seen);
@@ -671,7 +679,9 @@ struct rfkill * __must_check rfkill_allocate(struct device *parent,
struct device *dev;
if (type >= RFKILL_TYPE_MAX) {
- WARN_ON(1);
+ WARN(1, KERN_WARNING
+ "rfkill: illegal type %d passed as parameter "
+ "to rfkill_allocate\n", type);
return NULL;
}
@@ -751,7 +761,9 @@ int __must_check rfkill_register(struct rfkill *rfkill)
!rfkill->toggle_radio ||
rfkill->type >= RFKILL_TYPE_MAX ||
rfkill->state >= RFKILL_STATE_MAX)) {
- WARN_ON(1);
+ WARN(1, KERN_WARNING
+ "rfkill: attempt to register a "
+ "badly initialized rfkill struct\n");
return -EINVAL;
}
@@ -827,7 +839,9 @@ int rfkill_set_default(enum rfkill_type type, enum rfkill_state state)
if (type >= RFKILL_TYPE_MAX ||
(state != RFKILL_STATE_SOFT_BLOCKED &&
state != RFKILL_STATE_UNBLOCKED)) {
- WARN_ON(1);
+ WARN(1, KERN_WARNING
+ "rfkill: illegal state %d or type %d passed as "
+ "parameter to rfkill_set_default\n", state, type);
return -EINVAL;
}
--
1.5.6.3
On Saturday 02 August 2008, Henrique de Moraes Holschuh wrote:
> Detect and abort with -EEXIST if rfkill_register is called twice on the
> same rfkill struct. And WARN_ON(it) for good measure.
>
> While at it, flag when we are adding the first switch of a type, we will
> need that information later.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
> Cc: Johannes Berg <[email protected]>
Acked-by: Ivo van Doorn <[email protected]>
> ---
> net/rfkill/rfkill.c | 29 ++++++++++++++++++++++++++++-
> 1 files changed, 28 insertions(+), 1 deletions(-)
>
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index 2ec6312..297bfd5 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -533,17 +533,44 @@ static struct class rfkill_class = {
> .dev_uevent = rfkill_dev_uevent,
> };
>
> +static int rfkill_check_duplicity(const struct rfkill *rfkill)
> +{
> + struct rfkill *p;
> + unsigned long seen[BITS_TO_LONGS(RFKILL_TYPE_MAX)];
> +
> + memset(seen, 0, sizeof(seen));
> +
> + list_for_each_entry(p, &rfkill_list, node) {
> + if (p == rfkill) {
> + WARN_ON(1);
> + return -EEXIST;
> + }
> + set_bit(p->type, seen);
> + }
> +
> + /* 0: first switch of its kind */
> + return test_bit(rfkill->type, seen);
> +}
> +
> static int rfkill_add_switch(struct rfkill *rfkill)
> {
> + int error;
> +
> mutex_lock(&rfkill_mutex);
>
> + error = rfkill_check_duplicity(rfkill);
> + if (error < 0)
> + goto unlock_out;
> +
> rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
>
> list_add_tail(&rfkill->node, &rfkill_list);
>
> + error = 0;
> +unlock_out:
> mutex_unlock(&rfkill_mutex);
>
> - return 0;
> + return error;
> }
>
> static void rfkill_remove_switch(struct rfkill *rfkill)
rfkill_mutex and rfkill->mutex are too easy to confuse with each other.
Rename rfkill_mutex to rfkill_global_mutex, so that they are easier to tell
apart with just one glance.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Acked-by: Ivo van Doorn <[email protected]>
Cc: Michael Buesch <[email protected]>
---
net/rfkill/rfkill.c | 38 ++++++++++++++++++++------------------
1 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 88368c4..22da6e8 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -37,7 +37,7 @@ MODULE_DESCRIPTION("RF switch support");
MODULE_LICENSE("GPL");
static LIST_HEAD(rfkill_list); /* list of registered rf switches */
-static DEFINE_MUTEX(rfkill_mutex);
+static DEFINE_MUTEX(rfkill_global_mutex);
static unsigned int rfkill_default_state = RFKILL_STATE_UNBLOCKED;
module_param_named(default_state, rfkill_default_state, uint, 0444);
@@ -234,7 +234,7 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
* unless a specific switch is claimed by userspace (in which case,
* that switch is left alone) or suspended.
*
- * Caller must have acquired rfkill_mutex.
+ * Caller must have acquired rfkill_global_mutex.
*/
static void __rfkill_switch_all(const enum rfkill_type type,
const enum rfkill_state state)
@@ -263,14 +263,14 @@ static void __rfkill_switch_all(const enum rfkill_type type,
* @type: type of interfaces to be affected
* @state: the new state
*
- * Acquires rfkill_mutex and calls __rfkill_switch_all(@type, @state).
+ * Acquires rfkill_global_mutex and calls __rfkill_switch_all(@type, @state).
* Please refer to __rfkill_switch_all() for details.
*/
void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
{
- mutex_lock(&rfkill_mutex);
+ mutex_lock(&rfkill_global_mutex);
__rfkill_switch_all(type, state);
- mutex_unlock(&rfkill_mutex);
+ mutex_unlock(&rfkill_global_mutex);
}
EXPORT_SYMBOL(rfkill_switch_all);
@@ -278,7 +278,7 @@ EXPORT_SYMBOL(rfkill_switch_all);
* rfkill_epo - emergency power off all transmitters
*
* This kicks all non-suspended rfkill devices to RFKILL_STATE_SOFT_BLOCKED,
- * ignoring everything in its path but rfkill_mutex and rfkill->mutex.
+ * ignoring everything in its path but rfkill_global_mutex and rfkill->mutex.
*
* The global state before the EPO is saved and can be restored later
* using rfkill_restore_states().
@@ -288,7 +288,8 @@ void rfkill_epo(void)
struct rfkill *rfkill;
int i;
- mutex_lock(&rfkill_mutex);
+ mutex_lock(&rfkill_global_mutex);
+
list_for_each_entry(rfkill, &rfkill_list, node) {
mutex_lock(&rfkill->mutex);
rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
@@ -300,7 +301,7 @@ void rfkill_epo(void)
rfkill_global_states[i].current_state =
RFKILL_STATE_SOFT_BLOCKED;
}
- mutex_unlock(&rfkill_mutex);
+ mutex_unlock(&rfkill_global_mutex);
}
EXPORT_SYMBOL_GPL(rfkill_epo);
@@ -315,10 +316,11 @@ void rfkill_restore_states(void)
{
int i;
- mutex_lock(&rfkill_mutex);
+ mutex_lock(&rfkill_global_mutex);
+
for (i = 0; i < RFKILL_TYPE_MAX; i++)
__rfkill_switch_all(i, rfkill_global_states[i].default_state);
- mutex_unlock(&rfkill_mutex);
+ mutex_unlock(&rfkill_global_mutex);
}
EXPORT_SYMBOL_GPL(rfkill_restore_states);
@@ -470,7 +472,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
* Take the global lock to make sure the kernel is not in
* the middle of rfkill_switch_all
*/
- error = mutex_lock_interruptible(&rfkill_mutex);
+ error = mutex_lock_interruptible(&rfkill_global_mutex);
if (error)
return error;
@@ -485,7 +487,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
rfkill->user_claim = !!claim;
}
- mutex_unlock(&rfkill_mutex);
+ mutex_unlock(&rfkill_global_mutex);
return error ? error : count;
}
@@ -622,7 +624,7 @@ static int rfkill_add_switch(struct rfkill *rfkill)
{
int error;
- mutex_lock(&rfkill_mutex);
+ mutex_lock(&rfkill_global_mutex);
error = rfkill_check_duplicity(rfkill);
if (error < 0)
@@ -643,16 +645,16 @@ static int rfkill_add_switch(struct rfkill *rfkill)
error = 0;
unlock_out:
- mutex_unlock(&rfkill_mutex);
+ mutex_unlock(&rfkill_global_mutex);
return error;
}
static void rfkill_remove_switch(struct rfkill *rfkill)
{
- mutex_lock(&rfkill_mutex);
+ mutex_lock(&rfkill_global_mutex);
list_del_init(&rfkill->node);
- mutex_unlock(&rfkill_mutex);
+ mutex_unlock(&rfkill_global_mutex);
mutex_lock(&rfkill->mutex);
rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
@@ -845,7 +847,7 @@ int rfkill_set_default(enum rfkill_type type, enum rfkill_state state)
return -EINVAL;
}
- mutex_lock(&rfkill_mutex);
+ mutex_lock(&rfkill_global_mutex);
if (!test_and_set_bit(type, rfkill_states_lockdflt)) {
rfkill_global_states[type].default_state = state;
@@ -853,7 +855,7 @@ int rfkill_set_default(enum rfkill_type type, enum rfkill_state state)
} else
error = -EPERM;
- mutex_unlock(&rfkill_mutex);
+ mutex_unlock(&rfkill_global_mutex);
return error;
}
EXPORT_SYMBOL_GPL(rfkill_set_default);
--
1.5.6.3
On Sunday 03 August 2008, Johannes Berg wrote:
> On Sun, 2008-08-03 at 10:07 +0200, Ivo van Doorn wrote:
>
> > > diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> > > index ea872e5..d5f95cb 100644
> > > --- a/net/rfkill/rfkill.c
> > > +++ b/net/rfkill/rfkill.c
> > > @@ -76,6 +76,7 @@ static BLOCKING_NOTIFIER_HEAD(rfkill_notifier_list);
> > > */
> > > int register_rfkill_notifier(struct notifier_block *nb)
> > > {
> > > + BUG_ON(!nb);
> >
> > Probably better:
> >
> > if (unlikely(!nb) {
> > BUG()
> > return -EINVAL;
> > }
>
> Heh, not really, in fact, it will most likely not even compile to any
> different code. Did you mean
>
> if (WARN_ON(!nb))
> return -EINVAL;
>
> maybe?
>
> BUG() never returns. Same for all the other places you pointed out.
Ah right, that doesn't sound too good. I don't think rfkill should
become a blocker like that. WARN_ON should be sufficient. :)
Ivo
On Sun, 03 Aug 2008, Ivo van Doorn wrote:
> > So, I went with BUG(). Given the above, do you still want me to WARN()
> > and return -EINVAL instead? I can certainly do that, it would be more
> > correct than what the core kernel is doing, anyway.
>
> No, if notify chain doesn't check, and convention is to use BUG() in this
> case, then BUG() is fine with me.
The convention is to just OOPS, which I find terribly disgusting :-) so we
really can do whatever you want.
I used BUG() because the kernel-doc for the core kernel functions state that
for now they always return zero, AND because almost all the time the drivers
will be registering static text pointers as notifier blocks, so passing NULL
to these functions would be a very rare mistake indeed.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Sat, 2008-08-02 at 15:11 -0300, Henrique de Moraes Holschuh wrote:
> Currently, rfkill would stand in the way of properly supporting wireless
> devices that are capable of waking the system up from sleep or hibernation
> when they receive a special wireless message.
>
> Since rfkill attempts to soft-block any transmitters during class suspend,
why does it interfere with suspend anyway?
johannes
While it is interesting to not add last-enum-markers because it allows gcc
to warn us of switch() statements missing a valid state, we really should
be handling memory corruption on a rfkill state with default clauses,
anyway.
So add RFKILL_STATE_MAX and use it where applicable. It makes for safer
code in the long run.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
---
include/linux/rfkill.h | 1 +
net/rfkill/rfkill.c | 11 ++++++++---
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index e92d8e9..4cd64b0 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -49,6 +49,7 @@ enum rfkill_state {
RFKILL_STATE_SOFT_BLOCKED = 0, /* Radio output blocked */
RFKILL_STATE_UNBLOCKED = 1, /* Radio output allowed */
RFKILL_STATE_HARD_BLOCKED = 2, /* Output blocked, non-overrideable */
+ RFKILL_STATE_MAX, /* marker for last valid state */
};
/*
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 5320210..ea872e5 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -201,6 +201,8 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
* BLOCK even a transmitter that is already in state
* RFKILL_STATE_HARD_BLOCKED */
break;
+ default:
+ return -EINVAL;
}
if (force || state != rfkill->state) {
@@ -234,6 +236,9 @@ static void __rfkill_switch_all(const enum rfkill_type type,
{
struct rfkill *rfkill;
+ if (unlikely(state >= RFKILL_STATE_MAX))
+ return;
+
rfkill_global_states[type].current_state = state;
list_for_each_entry(rfkill, &rfkill_list, node) {
if ((!rfkill->user_claim) && (rfkill->type == type)) {
@@ -329,9 +334,7 @@ int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state)
{
enum rfkill_state oldstate;
- if (state != RFKILL_STATE_SOFT_BLOCKED &&
- state != RFKILL_STATE_UNBLOCKED &&
- state != RFKILL_STATE_HARD_BLOCKED)
+ if (unlikely(state >= RFKILL_STATE_MAX))
return -EINVAL;
mutex_lock(&rfkill->mutex);
@@ -735,6 +738,8 @@ int __must_check rfkill_register(struct rfkill *rfkill)
return -EINVAL;
if (rfkill->type >= RFKILL_TYPE_MAX)
return -EINVAL;
+ if (rfkill->state >= RFKILL_STATE_MAX)
+ return -EINVAL;
snprintf(dev->bus_id, sizeof(dev->bus_id),
"rfkill%ld", (long)atomic_inc_return(&rfkill_no) - 1);
--
1.5.6.3
On Sun, 03 Aug 2008, Tomas Winkler wrote:
> > I think that handler was added by Dmitry, but I see no real reason for
> > issuing the BLOCK event during suspend. However the handlers should
> > be used to prevent state changes to drivers after they have been suspended.
>
> Sounds reasonable.
Added to TODO list to remove the toggle_radio() calls on the suspend path.
Note that rfkill will still call the toggle_radio() callback on the resume
path with whatever state the radio was before suspend, because we want the
state set by the user to be preserved across sleeps of any sort, and that's
not the wireless driver's business to ensure, but rather, rfkill's.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Sun, 2008-08-03 at 10:07 +0200, Ivo van Doorn wrote:
> > diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> > index ea872e5..d5f95cb 100644
> > --- a/net/rfkill/rfkill.c
> > +++ b/net/rfkill/rfkill.c
> > @@ -76,6 +76,7 @@ static BLOCKING_NOTIFIER_HEAD(rfkill_notifier_list);
> > */
> > int register_rfkill_notifier(struct notifier_block *nb)
> > {
> > + BUG_ON(!nb);
>
> Probably better:
>
> if (unlikely(!nb) {
> BUG()
> return -EINVAL;
> }
Heh, not really, in fact, it will most likely not even compile to any
different code. Did you mean
if (WARN_ON(!nb))
return -EINVAL;
maybe?
BUG() never returns. Same for all the other places you pointed out.
johannes
On Sun, Aug 3, 2008 at 9:25 PM, Henrique de Moraes Holschuh
<[email protected]> wrote:
> On Sun, 03 Aug 2008, Tomas Winkler wrote:
>> > All the radio-is-allowed-to-transmit decisions are rfkill's. The driver is
>> > not allowed to override those. This is done to present a uniform behaviour
>> > and interface to the system's user (and any instance of rfkill doing
>> > something the user wouldn't expect to the radio is to be considered a major
>> > bug). rfkill is supposed to represend the will of the system's user
>> > regarding permission to transmit energy out of wireless transmitters.
>>
>> May point is that there are radio event out of scope rfkill so the
>> driver although obey rfkill system
>
> Err, no. If it involves energy emission, if rfkill forbade it [which is to
> be taken as the user forbade it], the driver must not, EVER, cause it to
> happen. There are no exceptions.
I didn't mean those kind of events :) iwlwifi driver has proper FCC
certification.
>
> This is a safety thing, not a convenience thing.
>
>> > Sure. I was wondering about drivers that *don't* have it, if any, out of
>> > the potential set of drivers that should be using rfkill (it is not a matter
>> > of those who are using rfkill right now).
>>
>> I think we are aligned in general.
>
> I may still have it as optional, but I will switch the default behaviour
> around. It will likely be useful for someone, and it is less than 10 LOC.
I make an effort to remove it's a mess.
Tomas
On Sun, 03 Aug 2008, Ivo van Doorn wrote:
> I rather see it merged into the other patch, backporting shouldn't really
> be kept in mind for development for the next kernel.
> The WARN() macro can easily be backported, and it doesn't make a real difference
> if this patch is provided seperately or merged with the pevious one.
> Both patches will end up in the same backport package anyway ;)
Very well, I will merge the two patches.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Sun, 03 Aug 2008, Tomas Winkler wrote:
> > All the radio-is-allowed-to-transmit decisions are rfkill's. The driver is
> > not allowed to override those. This is done to present a uniform behaviour
> > and interface to the system's user (and any instance of rfkill doing
> > something the user wouldn't expect to the radio is to be considered a major
> > bug). rfkill is supposed to represend the will of the system's user
> > regarding permission to transmit energy out of wireless transmitters.
>
> May point is that there are radio event out of scope rfkill so the
> driver although obey rfkill system
Err, no. If it involves energy emission, if rfkill forbade it [which is to
be taken as the user forbade it], the driver must not, EVER, cause it to
happen. There are no exceptions.
This is a safety thing, not a convenience thing.
> > Sure. I was wondering about drivers that *don't* have it, if any, out of
> > the potential set of drivers that should be using rfkill (it is not a matter
> > of those who are using rfkill right now).
>
> I think we are aligned in general.
I may still have it as optional, but I will switch the default behaviour
around. It will likely be useful for someone, and it is less than 10 LOC.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Sat, 2008-08-02 at 16:27 -0300, Henrique de Moraes Holschuh wrote:
> Hi Johannes!
>
> On Sat, 02 Aug 2008, Johannes Berg wrote:
>
> > On Sat, 2008-08-02 at 15:11 -0300, Henrique de Moraes Holschuh wrote:
> > > Currently, rfkill would stand in the way of properly supporting wireless
> > > devices that are capable of waking the system up from sleep or hibernation
> > > when they receive a special wireless message.
> > >
> > > Since rfkill attempts to soft-block any transmitters during class suspend,
> >
> > why does it interfere with suspend anyway?
>
> The class makes sure that all transmitters are blocked on suspend. You'd
> have to ask Ivo for the reason, but AFAIK, it is for both safety and to help
> conserve power.
rfkill shouldn't be touching stuff during suspend.
In the OLPC libertas case, the radio may remain _ON_ during suspend,
because the OLPC machines are expected to suspend/resume many times per
second, and the radio must continue to participate in the mesh during
that time. The only case where the radio gets blocked is when the user
requests it or when regulations require it.
Suspend != block, and tying suspend and rfkill together really is a
policy decision. Thus, I don't agree that rfkill should block radios on
suspend.
Dan
On Mon, 04 Aug 2008, Dan Williams wrote:
> On Sat, 2008-08-02 at 16:27 -0300, Henrique de Moraes Holschuh wrote:
> > On Sat, 02 Aug 2008, Johannes Berg wrote:
> > > On Sat, 2008-08-02 at 15:11 -0300, Henrique de Moraes Holschuh wrote:
> > > > Currently, rfkill would stand in the way of properly supporting wireless
> > > > devices that are capable of waking the system up from sleep or hibernation
> > > > when they receive a special wireless message.
> > > >
> > > > Since rfkill attempts to soft-block any transmitters during class suspend,
> > >
> > > why does it interfere with suspend anyway?
> >
> > The class makes sure that all transmitters are blocked on suspend. You'd
> > have to ask Ivo for the reason, but AFAIK, it is for both safety and to help
> > conserve power.
>
> rfkill shouldn't be touching stuff during suspend.
rfkill shouldn't *need* to touch stuff during suspend. But for that to be
true, all drivers using rfkill need to properly suspend in the first place.
And that's more than just drivers/net/wireless.
> In the OLPC libertas case, the radio may remain _ON_ during suspend,
> because the OLPC machines are expected to suspend/resume many times per
> second, and the radio must continue to participate in the mesh during
> that time. The only case where the radio gets blocked is when the user
> requests it or when regulations require it.
Yes. And the fact that rfkill stood in the way of doing that is a bug.
However, even my first try of a patch would already allow libertas to
declare it doesn't want rfkill to bother it on suspend.
It is very clear some drivers don't want rfkill to block radios on suspend.
Really. So far, libertas and iwl* are on my list of "don't want" or "don't
need". From what I can see, PCI-based rt2xxx is also "don't need". And I
can assume everything in drivers/misc is "don't need" without too much risk
of being wrong.
But the OPPOSITE is not clear at all to me. I don't know whether the other
users of rfkill need a radio block on suspend or not. Unless someone can
look over *all* in-tree users of linux/rfkill.h and state that none of them
need it because all of them DO shutdown their devices on suspend, I will
have to ask the maintainers of every single one about it before I ask a
patch to be merged. I already looked, and I don't know enough to have a
definitive answer by myself.
> Suspend != block, and tying suspend and rfkill together really is a
> policy decision. Thus, I don't agree that rfkill should block radios on
> suspend.
If some drivers out there are relying on it to, we need to know that before
we remove it completely.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
Hi Johannes!
On Sat, 02 Aug 2008, Johannes Berg wrote:
> On Sat, 2008-08-02 at 15:11 -0300, Henrique de Moraes Holschuh wrote:
> > Currently, rfkill would stand in the way of properly supporting wireless
> > devices that are capable of waking the system up from sleep or hibernation
> > when they receive a special wireless message.
> >
> > Since rfkill attempts to soft-block any transmitters during class suspend,
>
> why does it interfere with suspend anyway?
The class makes sure that all transmitters are blocked on suspend. You'd
have to ask Ivo for the reason, but AFAIK, it is for both safety and to help
conserve power.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tue, 05 Aug 2008, Johannes Berg wrote:
> On Tue, 2008-08-05 at 14:50 +0200, Johannes Berg wrote:
> > On Tue, 2008-08-05 at 09:48 -0300, Henrique de Moraes Holschuh wrote:
> >
> > > Maybe on net/wireless, but I am really not going to trust that one for,
> > > e.g., serial and USB drivers (USB being a bus that is often not shut down
> > > during suspend because there are USB wake devices), unless you clearly state
> > > you DID review them all, and which ones.
> >
> > Look, this is _clearly_ the wrong thing to do, so unless you want your
> > patches blocked over it, just rip it out.
>
> You can of course keep the current _wrong_ behaviour of blocking the
> radio when suspending, but instead of adding extra API for the
> wake-on-wlan case you should work on fixing that.
>
> If you think that you then need to review all drivers, so be it. I don't
> think it makes sense because keeping the device alive requires
> additional code nobody in their right mind would write unless they knew
> they needed it, like libertas.
>
> As it stands, rfkill also does the wrong thing for rfkill. You don't
> have to fix it, of course, but don't add workarounds.
I have prepared a patch that does exactly what you wanted (remove BLOCK on
suspend altogether), in the grounds that all drivers should be trying to
properly suspend and resume by themselves as much as possible, anyway.
I have added the maintainers of the drivers that *might* want to do
something about it (i.e. take action to block their transmitters explicitly)
in the CC, and documented the source files that might object to the change
in the commit message.
It will be in my next patchset. I am NOT happy about doing things this way,
but I am just too tired of the issue to bother.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tue, 2008-08-05 at 17:44 -0300, Henrique de Moraes Holschuh wrote:
> It will be in my next patchset. I am NOT happy about doing things this way,
> but I am just too tired of the issue to bother.
As I said, I totally don't care what you do now, as long as you drop
patch 8/8.
johannes
On Tue, 2008-08-05 at 09:48 -0300, Henrique de Moraes Holschuh wrote:
> Maybe on net/wireless, but I am really not going to trust that one for,
> e.g., serial and USB drivers (USB being a bus that is often not shut down
> during suspend because there are USB wake devices), unless you clearly state
> you DID review them all, and which ones.
Look, this is _clearly_ the wrong thing to do, so unless you want your
patches blocked over it, just rip it out.
johannes
On Sun, Aug 3, 2008 at 4:44 PM, Henrique de Moraes Holschuh
<[email protected]> wrote:
> On Sun, 03 Aug 2008, Tomas Winkler wrote:
>> > I think that handler was added by Dmitry, but I see no real reason for
>> > issuing the BLOCK event during suspend. However the handlers should
>> > be used to prevent state changes to drivers after they have been suspended.
>>
>> Sounds reasonable.
>
> Added to TODO list to remove the toggle_radio() calls on the suspend path.
Great.
> Note that rfkill will still call the toggle_radio() callback on the resume
> path with whatever state the radio was before suspend, because we want the
> state set by the user to be preserved across sleeps of any sort, and that's
> not the wireless driver's business to ensure, but rather, rfkill's.
That's align with how I see it.
Thanks
Tomas
On Sun, 2008-08-03 at 10:21 -0300, Henrique de Moraes Holschuh wrote:
> On Sun, 03 Aug 2008, Johannes Berg wrote:
> > if (WARN_ON(!nb))
> > return -EINVAL;
>
> I could use the notation above instead of:
> if (foo) {
> WARN_ON(1);
> return -ERROR;
> }
>
> Ivo, which one you prefer? The if() with the condition and WARN on the
> branch, or the if(WARN_ON(condition)) ?
You really should either just BUG() as you guys seem to have decided on,
which is fine with me, or use if(WARN_ON()) because that'll also put in
an unlikely() etc.
johannes
Detect and abort with -EEXIST if rfkill_register is called twice on the
same rfkill struct. And WARN_ON(it) for good measure.
While at it, flag when we are adding the first switch of a type, we will
need that information later.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
Cc: Johannes Berg <[email protected]>
---
net/rfkill/rfkill.c | 29 ++++++++++++++++++++++++++++-
1 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 2ec6312..297bfd5 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -533,17 +533,44 @@ static struct class rfkill_class = {
.dev_uevent = rfkill_dev_uevent,
};
+static int rfkill_check_duplicity(const struct rfkill *rfkill)
+{
+ struct rfkill *p;
+ unsigned long seen[BITS_TO_LONGS(RFKILL_TYPE_MAX)];
+
+ memset(seen, 0, sizeof(seen));
+
+ list_for_each_entry(p, &rfkill_list, node) {
+ if (p == rfkill) {
+ WARN_ON(1);
+ return -EEXIST;
+ }
+ set_bit(p->type, seen);
+ }
+
+ /* 0: first switch of its kind */
+ return test_bit(rfkill->type, seen);
+}
+
static int rfkill_add_switch(struct rfkill *rfkill)
{
+ int error;
+
mutex_lock(&rfkill_mutex);
+ error = rfkill_check_duplicity(rfkill);
+ if (error < 0)
+ goto unlock_out;
+
rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
list_add_tail(&rfkill->node, &rfkill_list);
+ error = 0;
+unlock_out:
mutex_unlock(&rfkill_mutex);
- return 0;
+ return error;
}
static void rfkill_remove_switch(struct rfkill *rfkill)
--
1.5.6.3
Add a second set of global states, "rfkill_default_states", to track the
state that will be used when the first rfkill class of a given type is
registered, and also to save "undo" information when rfkill_epo is called.
Add a new exported function, rfkill_set_default(), which can be used by
platform drivers to restore radio state saved by the platform across
reboots or shutdown.
Also, fix rfkill_epo to properly update rfkill_states, but still preserve a
copy of the state so that we can undo the effect of rfkill_epo later if we
want to. Add rfkill_restore_states() to restore rfkill_states from the
copy.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
---
include/linux/rfkill.h | 1 +
net/rfkill/rfkill-input.h | 1 +
net/rfkill/rfkill.c | 127 ++++++++++++++++++++++++++++++++++++++++----
3 files changed, 117 insertions(+), 12 deletions(-)
diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index 741d1a6..aa3c7d5 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -116,6 +116,7 @@ int rfkill_register(struct rfkill *rfkill);
void rfkill_unregister(struct rfkill *rfkill);
int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state);
+int rfkill_set_default(enum rfkill_type type, enum rfkill_state state);
/**
* rfkill_state_complement - return complementar state
diff --git a/net/rfkill/rfkill-input.h b/net/rfkill/rfkill-input.h
index f63d050..bbfa646 100644
--- a/net/rfkill/rfkill-input.h
+++ b/net/rfkill/rfkill-input.h
@@ -13,5 +13,6 @@
void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state);
void rfkill_epo(void);
+void rfkill_restore_states(void);
#endif /* __RFKILL_INPUT_H */
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 297bfd5..c1901fb 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -44,7 +44,13 @@ module_param_named(default_state, rfkill_default_state, uint, 0444);
MODULE_PARM_DESC(default_state,
"Default initial state for all radio types, 0 = radio off");
-static enum rfkill_state rfkill_states[RFKILL_TYPE_MAX];
+struct rfkill_gsw_state {
+ enum rfkill_state current_state;
+ enum rfkill_state default_state;
+};
+
+static struct rfkill_gsw_state rfkill_global_states[RFKILL_TYPE_MAX];
+static unsigned long rfkill_states_lockdflt[BITS_TO_LONGS(RFKILL_TYPE_MAX)];
static BLOCKING_NOTIFIER_HEAD(rfkill_notifier_list);
@@ -213,22 +219,22 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
}
/**
- * rfkill_switch_all - Toggle state of all switches of given type
+ * __rfkill_switch_all - Toggle state of all switches of given type
* @type: type of interfaces to be affected
* @state: the new state
*
* This function toggles the state of all switches of given type,
* unless a specific switch is claimed by userspace (in which case,
* that switch is left alone) or suspended.
+ *
+ * Caller must have acquired rfkill_mutex.
*/
-void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
+static void __rfkill_switch_all(const enum rfkill_type type,
+ const enum rfkill_state state)
{
struct rfkill *rfkill;
- mutex_lock(&rfkill_mutex);
-
- rfkill_states[type] = state;
-
+ rfkill_global_states[type].current_state = state;
list_for_each_entry(rfkill, &rfkill_list, node) {
if ((!rfkill->user_claim) && (rfkill->type == type)) {
mutex_lock(&rfkill->mutex);
@@ -236,7 +242,20 @@ void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
mutex_unlock(&rfkill->mutex);
}
}
+}
+/**
+ * rfkill_switch_all - Toggle state of all switches of given type
+ * @type: type of interfaces to be affected
+ * @state: the new state
+ *
+ * Acquires rfkill_mutex and calls __rfkill_switch_all(@type, @state).
+ * Please refer to __rfkill_switch_all() for details.
+ */
+void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
+{
+ mutex_lock(&rfkill_mutex);
+ __rfkill_switch_all(type, state);
mutex_unlock(&rfkill_mutex);
}
EXPORT_SYMBOL(rfkill_switch_all);
@@ -246,10 +265,14 @@ EXPORT_SYMBOL(rfkill_switch_all);
*
* This kicks all non-suspended rfkill devices to RFKILL_STATE_SOFT_BLOCKED,
* ignoring everything in its path but rfkill_mutex and rfkill->mutex.
+ *
+ * The global state before the EPO is saved and can be restored later
+ * using rfkill_restore_states().
*/
void rfkill_epo(void)
{
struct rfkill *rfkill;
+ int i;
mutex_lock(&rfkill_mutex);
list_for_each_entry(rfkill, &rfkill_list, node) {
@@ -257,11 +280,35 @@ void rfkill_epo(void)
rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
mutex_unlock(&rfkill->mutex);
}
+ for (i = 0; i < RFKILL_TYPE_MAX; i++) {
+ rfkill_global_states[i].default_state =
+ rfkill_global_states[i].current_state;
+ rfkill_global_states[i].current_state =
+ RFKILL_STATE_SOFT_BLOCKED;
+ }
mutex_unlock(&rfkill_mutex);
}
EXPORT_SYMBOL_GPL(rfkill_epo);
/**
+ * rfkill_restore_states - restore global states
+ *
+ * Restore (and sync switches to) the global state from the
+ * states in rfkill_default_states. This can undo the effects of
+ * a call to rfkill_epo().
+ */
+void rfkill_restore_states(void)
+{
+ int i;
+
+ mutex_lock(&rfkill_mutex);
+ for (i = 0; i < RFKILL_TYPE_MAX; i++)
+ __rfkill_switch_all(i, rfkill_global_states[i].default_state);
+ mutex_unlock(&rfkill_mutex);
+}
+EXPORT_SYMBOL_GPL(rfkill_restore_states);
+
+/**
* rfkill_force_state - Force the internal rfkill radio state
* @rfkill: pointer to the rfkill class to modify.
* @state: the current radio state the class should be forced to.
@@ -414,8 +461,8 @@ static ssize_t rfkill_claim_store(struct device *dev,
if (!claim) {
mutex_lock(&rfkill->mutex);
rfkill_toggle_radio(rfkill,
- rfkill_states[rfkill->type],
- 0);
+ rfkill_global_states[rfkill->type].current_state,
+ 0);
mutex_unlock(&rfkill->mutex);
}
rfkill->user_claim = !!claim;
@@ -562,7 +609,16 @@ static int rfkill_add_switch(struct rfkill *rfkill)
if (error < 0)
goto unlock_out;
- rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
+ if (!error) {
+ /* lock default after first use */
+ set_bit(rfkill->type, rfkill_states_lockdflt);
+ rfkill_global_states[rfkill->type].current_state =
+ rfkill_global_states[rfkill->type].default_state;
+ }
+
+ rfkill_toggle_radio(rfkill,
+ rfkill_global_states[rfkill->type].current_state,
+ 0);
list_add_tail(&rfkill->node, &rfkill_list);
@@ -718,6 +774,53 @@ void rfkill_unregister(struct rfkill *rfkill)
}
EXPORT_SYMBOL(rfkill_unregister);
+/**
+ * rfkill_set_default - set initial value for a switch type
+ * @type - the type of switch to set the default state of
+ * @state - the new default state for that group of switches
+ *
+ * Sets the initial state rfkill should use for a given type.
+ * The following initial states are allowed: RFKILL_STATE_SOFT_BLOCKED
+ * and RFKILL_STATE_UNBLOCKED.
+ *
+ * This function is meant to be used by platform drivers for platforms
+ * that can save switch state across power down/reboot.
+ *
+ * The default state for each switch type can be changed exactly once.
+ * After a switch of that type is registered, the default state cannot
+ * be changed anymore. This guards against multiple drivers it the
+ * same platform trying to set the initial switch default state, which
+ * is not allowed.
+ *
+ * Returns -EPERM if the state has already been set once or is in use,
+ * so drivers likely want to either ignore or at most printk(KERN_NOTICE)
+ * if this function returns -EPERM.
+ *
+ * Returns 0 if the new default state was set, or an error if it
+ * could not be set.
+ */
+int rfkill_set_default(enum rfkill_type type, enum rfkill_state state)
+{
+ int error;
+
+ if (type >= RFKILL_TYPE_MAX ||
+ (state != RFKILL_STATE_SOFT_BLOCKED &&
+ state != RFKILL_STATE_UNBLOCKED))
+ return -EINVAL;
+
+ mutex_lock(&rfkill_mutex);
+
+ if (!test_and_set_bit(type, rfkill_states_lockdflt)) {
+ rfkill_global_states[type].default_state = state;
+ error = 0;
+ } else
+ error = -EPERM;
+
+ mutex_unlock(&rfkill_mutex);
+ return error;
+}
+EXPORT_SYMBOL_GPL(rfkill_set_default);
+
/*
* Rfkill module initialization/deinitialization.
*/
@@ -731,8 +834,8 @@ static int __init rfkill_init(void)
rfkill_default_state != RFKILL_STATE_UNBLOCKED)
return -EINVAL;
- for (i = 0; i < ARRAY_SIZE(rfkill_states); i++)
- rfkill_states[i] = rfkill_default_state;
+ for (i = 0; i < RFKILL_TYPE_MAX; i++)
+ rfkill_global_states[i].default_state = rfkill_default_state;
error = class_register(&rfkill_class);
if (error) {
--
1.5.6.3
On Sun, Aug 3, 2008 at 6:55 AM, Henrique de Moraes Holschuh
<[email protected]> wrote:
> On Sun, 03 Aug 2008, Tomas Winkler wrote:
>> >> why does it interfere with suspend anyway?
>> >
>> > The class makes sure that all transmitters are blocked on suspend. You'd
>> > have to ask Ivo for the reason, but AFAIK, it is for both safety and to help
>> > conserve power.
>>
>> This one is also on my list to remove. Suspend/Hibernate Resume
>> definitelly doesn't mean to rfkill radio on and off.. Driver should
>
> I *certainly* don't want the chip active during S3 or S4 unless I want WoWL
> active, so I do agree that just killing the radio is not enough.
>
>> This only creates conflicts as both driver and rfkill system are
>> trying to bring radio down
>
> No conflicts. The wireless driver gets the request to shut the radio down
> BEFORE its suspend method is called (the class suspends first). It will
> also get the request to enable/disable (whatever state it was in before the
> suspend) the radio AFTER its resume method is called (the class resumes
> after).
At least from iwlwifi perspective killing radio is not the same
operation as shutting down the card.
The intention is different if nothing it's really redundant operation
Tomas
>> RFKILL IMHO shell track rfkill switches states and not to invent
>
> RFKILL is not about tracking, it is about *controlling*.
I think driver controls the radio why need to add another entity for
that. I don't like this definition, But that's just wording I hope.
> Without the patch, rfkill always disables radios on suspend. If they were
> enabled before the suspend, it will try to enable them on resume. If they
> were disabled before the suspend, it will try to disable them on resume.
>
> That's well part of rfkill's role. It might not be *needed* at all though,
> and if it is not, it is best to stop doing it. If it is needed just in a
> few situations, we need to make it configurable (that's what my patch does).
I rather remove it, If the driver has D3 operation it should move card
to operation
which is much more complex that just not killing radio so it seams useless.
Tomas
rfkill is not a small, mere detail in wireless support. Once it starts
supporting rfkill and users start counting on that support, a wireless
device is at risk of operating in dangerous conditions should rfkill
support fail to properly activate.
Therefore, add the required __must_check annotations on some key functions
of the rfkill API, for which the wireless drivers absolutely MUST handle
the failure mode safely in order to avoid a potentially dangerous situation
where the wireless transmitter is left enabled when the user don't want it
to.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
Cc: Matthew Garrett <[email protected]>
---
include/linux/rfkill.h | 5 +++--
net/rfkill/rfkill.c | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index aa3c7d5..e92d8e9 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -110,9 +110,10 @@ struct rfkill {
};
#define to_rfkill(d) container_of(d, struct rfkill, dev)
-struct rfkill *rfkill_allocate(struct device *parent, enum rfkill_type type);
+struct rfkill * __must_check rfkill_allocate(struct device *parent,
+ enum rfkill_type type);
void rfkill_free(struct rfkill *rfkill);
-int rfkill_register(struct rfkill *rfkill);
+int __must_check rfkill_register(struct rfkill *rfkill);
void rfkill_unregister(struct rfkill *rfkill);
int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state);
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index c1901fb..5320210 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -653,7 +653,8 @@ static void rfkill_remove_switch(struct rfkill *rfkill)
* NOTE: If registration fails the structure shoudl be freed by calling
* rfkill_free() otherwise rfkill_unregister() should be used.
*/
-struct rfkill *rfkill_allocate(struct device *parent, enum rfkill_type type)
+struct rfkill * __must_check rfkill_allocate(struct device *parent,
+ enum rfkill_type type)
{
struct rfkill *rfkill;
struct device *dev;
@@ -724,7 +725,7 @@ static void rfkill_led_trigger_unregister(struct rfkill *rfkill)
* structure needs to be registered. Immediately from registration the
* switch driver should be able to service calls to toggle_radio.
*/
-int rfkill_register(struct rfkill *rfkill)
+int __must_check rfkill_register(struct rfkill *rfkill)
{
static atomic_t rfkill_no = ATOMIC_INIT(0);
struct device *dev = &rfkill->dev;
--
1.5.6.3
Currently, rfkill would stand in the way of properly supporting wireless
devices that are capable of waking the system up from sleep or hibernation
when they receive a special wireless message.
Since rfkill attempts to soft-block any transmitters during class suspend,
it would be difficult for the wireless device driver to know whether it is
allowed to keep the transmitter active or not, and since the state of the
transmitter might impact on functionality needed by the device to be able
to know it has to wake the system, this is not good.
In order to properly support these devices, add a flag that requests the
class suspend handlers to NOT attempt to block the transmitter on suspend.
This way, the wireless device driver can be sure that if it is in any of
the blocked states when entering suspend it MUST keep its transmitter
disabled (even if that breaks wake-on-wireless-packet).
Note that if the transmitter does not need to be functioning for
wake-on-wireless-packet, the new flag should not be used. If the
wake-on-wireless-packet is not enabled for the current suspend or
hibernation cycle, and the wireless driver requested that rfkill do not
block the transmitter, it MUST block the transmitter by itself when the
system attempts to enter the suspend/hibernation state.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
---
Documentation/rfkill.txt | 29 ++++++++++++++++++++++++++++-
include/linux/rfkill.h | 4 ++++
net/rfkill/rfkill.c | 3 ++-
3 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
index 6fcb306..888d756 100644
--- a/Documentation/rfkill.txt
+++ b/Documentation/rfkill.txt
@@ -341,6 +341,8 @@ key that does nothing by itself, as well as any hot key that is type-specific
3.1 Guidelines for wireless device drivers
------------------------------------------
+(in this text, rfkill->foo means the foo field of struct rfkill).
+
1. Each independent transmitter in a wireless device (usually there is only one
transmitter per device) should have a SINGLE rfkill class attached to it.
@@ -364,10 +366,35 @@ when possible) the overall transmitter rfkill state, not of a particular rfkill
line.
5. During suspend, the rfkill class will attempt to soft-block the radio
-through a call to rfkill->toggle_radio, and will try to restore its previous
+through a call to the rfkill->toggle_radio callback (unless the
+rfkill->no_block_on_pm_sleep flag set), and will try to restore its previous
state during resume. After a rfkill class is suspended, it will *not* call
rfkill->toggle_radio until it is resumed.
+rfkill->no_block_on_pm_sleep is to be used by devices that need to handle
+suspend by themselves (e.g. due to wake-on-wireless-packet functionality) in
+such a way that the automated soft-block would get in the way.
+
+The wireless device driver MUST NOT leave the transmitter enabled during
+suspend and hibernation unless:
+
+ 5.1. The transmitter has to be enabled for the
+ wake-on-wireless-packet functionality to work, and that
+ functionality is enabled for this suspend/hibernation cycle.
+
+AND
+
+ 5.2. The device was not on a user-requested BLOCKED state before
+ the suspend (i.e. the driver must NOT unblock a device, not even
+ to support wake-on-wireless-packet).
+
+In other words, there is absolutely no allowed scenario where a driver can
+automatically take action to unblock a rfkill controller (obviously, this deals
+with scenarios where soft-blocking or both soft and hard blocking is happening.
+Scenarios where hardware rfkill lines are the only ones blocking the
+transmitter are outside of this rule, since the wireless device driver does not
+control its input hardware rfkill lines in the first place).
+
Example of a WLAN wireless driver connected to the rfkill subsystem:
--------------------------------------------------------------------
diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index 4cd64b0..97b767b 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -84,6 +84,8 @@ enum rfkill_state {
* @dev: Device structure integrating the switch into device tree.
* @node: Used to place switch into list of all switches known to the
* the system.
+ * @no_block_on_pm_sleep: Whether the rfkill class should block
+ * the wireless transmitter during suspend.
*
* This structure represents a RF switch located on a network device.
*/
@@ -108,6 +110,8 @@ struct rfkill {
struct device dev;
struct list_head node;
+
+ bool no_block_on_pm_sleep;
};
#define to_rfkill(d) container_of(d, struct rfkill, dev)
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 22da6e8..e92828b 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -514,7 +514,8 @@ static int rfkill_suspend(struct device *dev, pm_message_t state)
struct rfkill *rfkill = to_rfkill(dev);
if (dev->power.power_state.event != state.event) {
- if (state.event & PM_EVENT_SLEEP) {
+ if (state.event & PM_EVENT_SLEEP &&
+ !rfkill->no_block_on_pm_sleep) {
/* Stop transmitter, keep state, no notifies */
update_rfkill_state(rfkill);
--
1.5.6.3
On Tue, 2008-08-05 at 14:50 +0200, Johannes Berg wrote:
> On Tue, 2008-08-05 at 09:48 -0300, Henrique de Moraes Holschuh wrote:
>
> > Maybe on net/wireless, but I am really not going to trust that one for,
> > e.g., serial and USB drivers (USB being a bus that is often not shut down
> > during suspend because there are USB wake devices), unless you clearly state
> > you DID review them all, and which ones.
>
> Look, this is _clearly_ the wrong thing to do, so unless you want your
> patches blocked over it, just rip it out.
You can of course keep the current _wrong_ behaviour of blocking the
radio when suspending, but instead of adding extra API for the
wake-on-wlan case you should work on fixing that.
If you think that you then need to review all drivers, so be it. I don't
think it makes sense because keeping the device alive requires
additional code nobody in their right mind would write unless they knew
they needed it, like libertas.
As it stands, rfkill also does the wrong thing for rfkill. You don't
have to fix it, of course, but don't add workarounds.
johannes
On Saturday 02 August 2008, Henrique de Moraes Holschuh wrote:
> While it is interesting to not add last-enum-markers because it allows gcc
> to warn us of switch() statements missing a valid state, we really should
> be handling memory corruption on a rfkill state with default clauses,
> anyway.
>
> So add RFKILL_STATE_MAX and use it where applicable. It makes for safer
> code in the long run.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
Acked-by: Ivo van Doorn <[email protected]>
> ---
> include/linux/rfkill.h | 1 +
> net/rfkill/rfkill.c | 11 ++++++++---
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
> index e92d8e9..4cd64b0 100644
> --- a/include/linux/rfkill.h
> +++ b/include/linux/rfkill.h
> @@ -49,6 +49,7 @@ enum rfkill_state {
> RFKILL_STATE_SOFT_BLOCKED = 0, /* Radio output blocked */
> RFKILL_STATE_UNBLOCKED = 1, /* Radio output allowed */
> RFKILL_STATE_HARD_BLOCKED = 2, /* Output blocked, non-overrideable */
> + RFKILL_STATE_MAX, /* marker for last valid state */
> };
>
> /*
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index 5320210..ea872e5 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -201,6 +201,8 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
> * BLOCK even a transmitter that is already in state
> * RFKILL_STATE_HARD_BLOCKED */
> break;
> + default:
> + return -EINVAL;
> }
>
> if (force || state != rfkill->state) {
> @@ -234,6 +236,9 @@ static void __rfkill_switch_all(const enum rfkill_type type,
> {
> struct rfkill *rfkill;
>
> + if (unlikely(state >= RFKILL_STATE_MAX))
> + return;
> +
> rfkill_global_states[type].current_state = state;
> list_for_each_entry(rfkill, &rfkill_list, node) {
> if ((!rfkill->user_claim) && (rfkill->type == type)) {
> @@ -329,9 +334,7 @@ int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state)
> {
> enum rfkill_state oldstate;
>
> - if (state != RFKILL_STATE_SOFT_BLOCKED &&
> - state != RFKILL_STATE_UNBLOCKED &&
> - state != RFKILL_STATE_HARD_BLOCKED)
> + if (unlikely(state >= RFKILL_STATE_MAX))
> return -EINVAL;
>
> mutex_lock(&rfkill->mutex);
> @@ -735,6 +738,8 @@ int __must_check rfkill_register(struct rfkill *rfkill)
> return -EINVAL;
> if (rfkill->type >= RFKILL_TYPE_MAX)
> return -EINVAL;
> + if (rfkill->state >= RFKILL_STATE_MAX)
> + return -EINVAL;
>
> snprintf(dev->bus_id, sizeof(dev->bus_id),
> "rfkill%ld", (long)atomic_inc_return(&rfkill_no) - 1);
On Saturday 02 August 2008, Henrique de Moraes Holschuh wrote:
> BUG_ON and WARN_ON the heck out of buggy drivers calling into the rfkill
> subsystem.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
> Cc: Johannes Berg <[email protected]>
> ---
> net/rfkill/rfkill.c | 33 +++++++++++++++++++++++++--------
> 1 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index ea872e5..d5f95cb 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -76,6 +76,7 @@ static BLOCKING_NOTIFIER_HEAD(rfkill_notifier_list);
> */
> int register_rfkill_notifier(struct notifier_block *nb)
> {
> + BUG_ON(!nb);
Probably better:
if (unlikely(!nb) {
BUG()
return -EINVAL;
}
> return blocking_notifier_chain_register(&rfkill_notifier_list, nb);
> }
> EXPORT_SYMBOL_GPL(register_rfkill_notifier);
> @@ -91,6 +92,7 @@ EXPORT_SYMBOL_GPL(register_rfkill_notifier);
> */
> int unregister_rfkill_notifier(struct notifier_block *nb)
> {
> + BUG_ON(!nb);
if (unlikely(!nb) {
BUG()
return -EINVAL;
}
> return blocking_notifier_chain_unregister(&rfkill_notifier_list, nb);
> }
> EXPORT_SYMBOL_GPL(unregister_rfkill_notifier);
> @@ -202,6 +204,7 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
> * RFKILL_STATE_HARD_BLOCKED */
> break;
> default:
> + WARN_ON(1);
> return -EINVAL;
> }
>
> @@ -236,8 +239,10 @@ static void __rfkill_switch_all(const enum rfkill_type type,
> {
> struct rfkill *rfkill;
>
> - if (unlikely(state >= RFKILL_STATE_MAX))
> + if (unlikely(state >= RFKILL_STATE_MAX || type >= RFKILL_TYPE_MAX)) {
> + WARN_ON(1);
> return;
> + }
>
> rfkill_global_states[type].current_state = state;
> list_for_each_entry(rfkill, &rfkill_list, node) {
> @@ -334,8 +339,11 @@ int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state)
> {
> enum rfkill_state oldstate;
>
> - if (unlikely(state >= RFKILL_STATE_MAX))
> + BUG_ON(!rfkill);
if (unlikely(!rfkill) {
BUG()
return -EINVAL;
}
> + if (unlikely(state >= RFKILL_STATE_MAX)) {
> + WARN_ON(1);
> return -EINVAL;
> + }
>
> mutex_lock(&rfkill->mutex);
>
> @@ -662,6 +670,11 @@ struct rfkill * __must_check rfkill_allocate(struct device *parent,
> struct rfkill *rfkill;
> struct device *dev;
>
> + if (type >= RFKILL_TYPE_MAX) {
> + WARN_ON(1);
> + return NULL;
> + }
> +
> rfkill = kzalloc(sizeof(struct rfkill), GFP_KERNEL);
> if (!rfkill)
> return NULL;
> @@ -734,12 +747,13 @@ int __must_check rfkill_register(struct rfkill *rfkill)
> struct device *dev = &rfkill->dev;
> int error;
>
> - if (!rfkill->toggle_radio)
> - return -EINVAL;
> - if (rfkill->type >= RFKILL_TYPE_MAX)
> - return -EINVAL;
> - if (rfkill->state >= RFKILL_STATE_MAX)
> + if (unlikely(!rfkill ||
> + !rfkill->toggle_radio ||
> + rfkill->type >= RFKILL_TYPE_MAX ||
> + rfkill->state >= RFKILL_STATE_MAX)) {
> + WARN_ON(1);
> return -EINVAL;
> + }
>
> snprintf(dev->bus_id, sizeof(dev->bus_id),
> "rfkill%ld", (long)atomic_inc_return(&rfkill_no) - 1);
> @@ -773,6 +787,7 @@ EXPORT_SYMBOL(rfkill_register);
> */
> void rfkill_unregister(struct rfkill *rfkill)
> {
> + BUG_ON(!rfkill);
if (unlikely(!rfkill) {
BUG()
return -EINVAL;
}
> device_del(&rfkill->dev);
> rfkill_remove_switch(rfkill);
> rfkill_led_trigger_unregister(rfkill);
> @@ -811,8 +826,10 @@ int rfkill_set_default(enum rfkill_type type, enum rfkill_state state)
>
> if (type >= RFKILL_TYPE_MAX ||
> (state != RFKILL_STATE_SOFT_BLOCKED &&
> - state != RFKILL_STATE_UNBLOCKED))
> + state != RFKILL_STATE_UNBLOCKED)) {
> + WARN_ON(1);
> return -EINVAL;
> + }
>
> mutex_lock(&rfkill_mutex);
>
On Sat, Aug 2, 2008 at 10:27 PM, Henrique de Moraes Holschuh
<[email protected]> wrote:
> Hi Johannes!
>
> On Sat, 02 Aug 2008, Johannes Berg wrote:
>
>> On Sat, 2008-08-02 at 15:11 -0300, Henrique de Moraes Holschuh wrote:
>> > Currently, rfkill would stand in the way of properly supporting wireless
>> > devices that are capable of waking the system up from sleep or hibernation
>> > when they receive a special wireless message.
>> >
>> > Since rfkill attempts to soft-block any transmitters during class suspend,
>>
>> why does it interfere with suspend anyway?
>
> The class makes sure that all transmitters are blocked on suspend. You'd
> have to ask Ivo for the reason, but AFAIK, it is for both safety and to help
> conserve power.
This one is also on my list to remove. Suspend/Hibernate Resume
definitelly doesn't mean to rfkill radio on and off.. Driver should
bring down the NIC up and down. anyway nless it supports some D3
working condition as iwlwiifi HW )not enabled under Linux).
This only creates conflicts as both driver and rfkill system are
trying to bring radio down
RFKILL IMHO shell track rfkill switches states and not to invent
them. If the radio was SW switched off before NIC was suspend it
shell be switched off also in resume. That's should be role of rfkill
system, probably with some help from user space.
Tomas
Tomas
On Sunday 03 August 2008, Henrique de Moraes Holschuh wrote:
> On Sun, 03 Aug 2008, Ivo van Doorn wrote:
> > > BUG() never returns. Same for all the other places you pointed out.
> >
> > Ah right, that doesn't sound too good. I don't think rfkill should
> > become a blocker like that. WARN_ON should be sufficient. :)
>
> It would, if anyone ever tested the return from notify chain registering.
> But the core kernel doesn't do any checking when registering notifiers to
> the chains, and always either return zero or OOPS outright when it attempts
> to dereference a NULL pointer (at least on 2.6.25)...
>
> So, I went with BUG(). Given the above, do you still want me to WARN() and
> return -EINVAL instead? I can certainly do that, it would be more correct
> than what the core kernel is doing, anyway.
No, if notify chain doesn't check, and convention is to use BUG() in this case,
then BUG() is fine with me.
Ivo
On Saturday 02 August 2008, Henrique de Moraes Holschuh wrote:
> Hi Johannes!
>
> On Sat, 02 Aug 2008, Johannes Berg wrote:
>
> > On Sat, 2008-08-02 at 15:11 -0300, Henrique de Moraes Holschuh wrote:
> > > Currently, rfkill would stand in the way of properly supporting wireless
> > > devices that are capable of waking the system up from sleep or hibernation
> > > when they receive a special wireless message.
> > >
> > > Since rfkill attempts to soft-block any transmitters during class suspend,
> >
> > why does it interfere with suspend anyway?
>
> The class makes sure that all transmitters are blocked on suspend. You'd
> have to ask Ivo for the reason, but AFAIK, it is for both safety and to help
> conserve power.
I think that handler was added by Dmitry, but I see no real reason for
issuing the BLOCK event during suspend. However the handlers should
be used to prevent state changes to drivers after they have been suspended.
Ivo
On Sun, 03 Aug 2008, Tomas Winkler wrote:
> >> why does it interfere with suspend anyway?
> >
> > The class makes sure that all transmitters are blocked on suspend. You'd
> > have to ask Ivo for the reason, but AFAIK, it is for both safety and to help
> > conserve power.
>
> This one is also on my list to remove. Suspend/Hibernate Resume
> definitelly doesn't mean to rfkill radio on and off.. Driver should
I *certainly* don't want the chip active during S3 or S4 unless I want WoWL
active, so I do agree that just killing the radio is not enough.
> This only creates conflicts as both driver and rfkill system are
> trying to bring radio down
No conflicts. The wireless driver gets the request to shut the radio down
BEFORE its suspend method is called (the class suspends first). It will
also get the request to enable/disable (whatever state it was in before the
suspend) the radio AFTER its resume method is called (the class resumes
after).
> RFKILL IMHO shell track rfkill switches states and not to invent
RFKILL is not about tracking, it is about *controlling*.
> them. If the radio was SW switched off before NIC was suspend it
> shell be switched off also in resume. That's should be role of rfkill
> system, probably with some help from user space.
Without the patch, rfkill always disables radios on suspend. If they were
enabled before the suspend, it will try to enable them on resume. If they
were disabled before the suspend, it will try to disable them on resume.
That's well part of rfkill's role. It might not be *needed* at all though,
and if it is not, it is best to stop doing it. If it is needed just in a
few situations, we need to make it configurable (that's what my patch does).
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Mon, 2008-08-04 at 20:35 -0300, Henrique de Moraes Holschuh wrote:
> And it is already damn clear that what we currently have (rfkill always
> blocks on suspend) is not the correct way to go about it. WHAT I want to
> know now is whether there are any drivers out there which need the current
> behaviour.
No. There are no drivers that rely on that. Seriously, all drivers
implement some form of suspend by simply turning off the hardware.
There's no reason to believe that rfkill is needed to turn off the radio
in any way when drivers except libertas turn off the hardware anyway.
johannes
On Tue, 05 Aug 2008, Johannes Berg wrote:
> On Mon, 2008-08-04 at 20:35 -0300, Henrique de Moraes Holschuh wrote:
> > And it is already damn clear that what we currently have (rfkill always
> > blocks on suspend) is not the correct way to go about it. WHAT I want to
> > know now is whether there are any drivers out there which need the current
> > behaviour.
>
> No. There are no drivers that rely on that. Seriously, all drivers
Did you review them all? Including what is outside of drivers/net/wireless?
> implement some form of suspend by simply turning off the hardware.
Maybe on net/wireless, but I am really not going to trust that one for,
e.g., serial and USB drivers (USB being a bus that is often not shut down
during suspend because there are USB wake devices), unless you clearly state
you DID review them all, and which ones.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Sun, 03 Aug 2008, Tomas Winkler wrote:
> At least from iwlwifi perspective killing radio is not the same
> operation as shutting down the card.
It almost never is. Which is why I said we should not bother if nobody
needs that.
> The intention is different if nothing it's really redundant operation
On proper drivers that do proper power management? Certainly :-)
> > RFKILL is not about tracking, it is about *controlling*.
>
> I think driver controls the radio why need to add another entity for
> that. I don't like this definition, But that's just wording I hope.
All the radio-is-allowed-to-transmit decisions are rfkill's. The driver is
not allowed to override those. This is done to present a uniform behaviour
and interface to the system's user (and any instance of rfkill doing
something the user wouldn't expect to the radio is to be considered a major
bug). rfkill is supposed to represend the will of the system's user
regarding permission to transmit energy out of wireless transmitters.
Not even WoWL is an exception, if your radio is blocked before suspend, you
are not allowed to enable the transmitter [to get WoWL to work -- I sure
hope this is not necessary, but it could be]. I'd expect a printk
complaining about it, though :-)
> I rather remove it, If the driver has D3 operation it should move card
> to operation
Sure. I was wondering about drivers that *don't* have it, if any, out of
the potential set of drivers that should be using rfkill (it is not a matter
of those who are using rfkill right now).
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
BUG_ON and WARN_ON the heck out of buggy drivers calling into the rfkill
subsystem.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
Cc: Johannes Berg <[email protected]>
---
net/rfkill/rfkill.c | 33 +++++++++++++++++++++++++--------
1 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index ea872e5..d5f95cb 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -76,6 +76,7 @@ static BLOCKING_NOTIFIER_HEAD(rfkill_notifier_list);
*/
int register_rfkill_notifier(struct notifier_block *nb)
{
+ BUG_ON(!nb);
return blocking_notifier_chain_register(&rfkill_notifier_list, nb);
}
EXPORT_SYMBOL_GPL(register_rfkill_notifier);
@@ -91,6 +92,7 @@ EXPORT_SYMBOL_GPL(register_rfkill_notifier);
*/
int unregister_rfkill_notifier(struct notifier_block *nb)
{
+ BUG_ON(!nb);
return blocking_notifier_chain_unregister(&rfkill_notifier_list, nb);
}
EXPORT_SYMBOL_GPL(unregister_rfkill_notifier);
@@ -202,6 +204,7 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
* RFKILL_STATE_HARD_BLOCKED */
break;
default:
+ WARN_ON(1);
return -EINVAL;
}
@@ -236,8 +239,10 @@ static void __rfkill_switch_all(const enum rfkill_type type,
{
struct rfkill *rfkill;
- if (unlikely(state >= RFKILL_STATE_MAX))
+ if (unlikely(state >= RFKILL_STATE_MAX || type >= RFKILL_TYPE_MAX)) {
+ WARN_ON(1);
return;
+ }
rfkill_global_states[type].current_state = state;
list_for_each_entry(rfkill, &rfkill_list, node) {
@@ -334,8 +339,11 @@ int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state)
{
enum rfkill_state oldstate;
- if (unlikely(state >= RFKILL_STATE_MAX))
+ BUG_ON(!rfkill);
+ if (unlikely(state >= RFKILL_STATE_MAX)) {
+ WARN_ON(1);
return -EINVAL;
+ }
mutex_lock(&rfkill->mutex);
@@ -662,6 +670,11 @@ struct rfkill * __must_check rfkill_allocate(struct device *parent,
struct rfkill *rfkill;
struct device *dev;
+ if (type >= RFKILL_TYPE_MAX) {
+ WARN_ON(1);
+ return NULL;
+ }
+
rfkill = kzalloc(sizeof(struct rfkill), GFP_KERNEL);
if (!rfkill)
return NULL;
@@ -734,12 +747,13 @@ int __must_check rfkill_register(struct rfkill *rfkill)
struct device *dev = &rfkill->dev;
int error;
- if (!rfkill->toggle_radio)
- return -EINVAL;
- if (rfkill->type >= RFKILL_TYPE_MAX)
- return -EINVAL;
- if (rfkill->state >= RFKILL_STATE_MAX)
+ if (unlikely(!rfkill ||
+ !rfkill->toggle_radio ||
+ rfkill->type >= RFKILL_TYPE_MAX ||
+ rfkill->state >= RFKILL_STATE_MAX)) {
+ WARN_ON(1);
return -EINVAL;
+ }
snprintf(dev->bus_id, sizeof(dev->bus_id),
"rfkill%ld", (long)atomic_inc_return(&rfkill_no) - 1);
@@ -773,6 +787,7 @@ EXPORT_SYMBOL(rfkill_register);
*/
void rfkill_unregister(struct rfkill *rfkill)
{
+ BUG_ON(!rfkill);
device_del(&rfkill->dev);
rfkill_remove_switch(rfkill);
rfkill_led_trigger_unregister(rfkill);
@@ -811,8 +826,10 @@ int rfkill_set_default(enum rfkill_type type, enum rfkill_state state)
if (type >= RFKILL_TYPE_MAX ||
(state != RFKILL_STATE_SOFT_BLOCKED &&
- state != RFKILL_STATE_UNBLOCKED))
+ state != RFKILL_STATE_UNBLOCKED)) {
+ WARN_ON(1);
return -EINVAL;
+ }
mutex_lock(&rfkill_mutex);
--
1.5.6.3
On Saturday 02 August 2008, Henrique de Moraes Holschuh wrote:
> Add a second set of global states, "rfkill_default_states", to track the
> state that will be used when the first rfkill class of a given type is
> registered, and also to save "undo" information when rfkill_epo is called.
>
> Add a new exported function, rfkill_set_default(), which can be used by
> platform drivers to restore radio state saved by the platform across
> reboots or shutdown.
>
> Also, fix rfkill_epo to properly update rfkill_states, but still preserve a
> copy of the state so that we can undo the effect of rfkill_epo later if we
> want to. Add rfkill_restore_states() to restore rfkill_states from the
> copy.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
Acked-by: Ivo van Doorn <[email protected]>
> ---
> include/linux/rfkill.h | 1 +
> net/rfkill/rfkill-input.h | 1 +
> net/rfkill/rfkill.c | 127 ++++++++++++++++++++++++++++++++++++++++----
> 3 files changed, 117 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
> index 741d1a6..aa3c7d5 100644
> --- a/include/linux/rfkill.h
> +++ b/include/linux/rfkill.h
> @@ -116,6 +116,7 @@ int rfkill_register(struct rfkill *rfkill);
> void rfkill_unregister(struct rfkill *rfkill);
>
> int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state);
> +int rfkill_set_default(enum rfkill_type type, enum rfkill_state state);
>
> /**
> * rfkill_state_complement - return complementar state
> diff --git a/net/rfkill/rfkill-input.h b/net/rfkill/rfkill-input.h
> index f63d050..bbfa646 100644
> --- a/net/rfkill/rfkill-input.h
> +++ b/net/rfkill/rfkill-input.h
> @@ -13,5 +13,6 @@
>
> void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state);
> void rfkill_epo(void);
> +void rfkill_restore_states(void);
>
> #endif /* __RFKILL_INPUT_H */
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index 297bfd5..c1901fb 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -44,7 +44,13 @@ module_param_named(default_state, rfkill_default_state, uint, 0444);
> MODULE_PARM_DESC(default_state,
> "Default initial state for all radio types, 0 = radio off");
>
> -static enum rfkill_state rfkill_states[RFKILL_TYPE_MAX];
> +struct rfkill_gsw_state {
> + enum rfkill_state current_state;
> + enum rfkill_state default_state;
> +};
> +
> +static struct rfkill_gsw_state rfkill_global_states[RFKILL_TYPE_MAX];
> +static unsigned long rfkill_states_lockdflt[BITS_TO_LONGS(RFKILL_TYPE_MAX)];
>
> static BLOCKING_NOTIFIER_HEAD(rfkill_notifier_list);
>
> @@ -213,22 +219,22 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
> }
>
> /**
> - * rfkill_switch_all - Toggle state of all switches of given type
> + * __rfkill_switch_all - Toggle state of all switches of given type
> * @type: type of interfaces to be affected
> * @state: the new state
> *
> * This function toggles the state of all switches of given type,
> * unless a specific switch is claimed by userspace (in which case,
> * that switch is left alone) or suspended.
> + *
> + * Caller must have acquired rfkill_mutex.
> */
> -void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
> +static void __rfkill_switch_all(const enum rfkill_type type,
> + const enum rfkill_state state)
> {
> struct rfkill *rfkill;
>
> - mutex_lock(&rfkill_mutex);
> -
> - rfkill_states[type] = state;
> -
> + rfkill_global_states[type].current_state = state;
> list_for_each_entry(rfkill, &rfkill_list, node) {
> if ((!rfkill->user_claim) && (rfkill->type == type)) {
> mutex_lock(&rfkill->mutex);
> @@ -236,7 +242,20 @@ void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
> mutex_unlock(&rfkill->mutex);
> }
> }
> +}
>
> +/**
> + * rfkill_switch_all - Toggle state of all switches of given type
> + * @type: type of interfaces to be affected
> + * @state: the new state
> + *
> + * Acquires rfkill_mutex and calls __rfkill_switch_all(@type, @state).
> + * Please refer to __rfkill_switch_all() for details.
> + */
> +void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
> +{
> + mutex_lock(&rfkill_mutex);
> + __rfkill_switch_all(type, state);
> mutex_unlock(&rfkill_mutex);
> }
> EXPORT_SYMBOL(rfkill_switch_all);
> @@ -246,10 +265,14 @@ EXPORT_SYMBOL(rfkill_switch_all);
> *
> * This kicks all non-suspended rfkill devices to RFKILL_STATE_SOFT_BLOCKED,
> * ignoring everything in its path but rfkill_mutex and rfkill->mutex.
> + *
> + * The global state before the EPO is saved and can be restored later
> + * using rfkill_restore_states().
> */
> void rfkill_epo(void)
> {
> struct rfkill *rfkill;
> + int i;
>
> mutex_lock(&rfkill_mutex);
> list_for_each_entry(rfkill, &rfkill_list, node) {
> @@ -257,11 +280,35 @@ void rfkill_epo(void)
> rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
> mutex_unlock(&rfkill->mutex);
> }
> + for (i = 0; i < RFKILL_TYPE_MAX; i++) {
> + rfkill_global_states[i].default_state =
> + rfkill_global_states[i].current_state;
> + rfkill_global_states[i].current_state =
> + RFKILL_STATE_SOFT_BLOCKED;
> + }
> mutex_unlock(&rfkill_mutex);
> }
> EXPORT_SYMBOL_GPL(rfkill_epo);
>
> /**
> + * rfkill_restore_states - restore global states
> + *
> + * Restore (and sync switches to) the global state from the
> + * states in rfkill_default_states. This can undo the effects of
> + * a call to rfkill_epo().
> + */
> +void rfkill_restore_states(void)
> +{
> + int i;
> +
> + mutex_lock(&rfkill_mutex);
> + for (i = 0; i < RFKILL_TYPE_MAX; i++)
> + __rfkill_switch_all(i, rfkill_global_states[i].default_state);
> + mutex_unlock(&rfkill_mutex);
> +}
> +EXPORT_SYMBOL_GPL(rfkill_restore_states);
> +
> +/**
> * rfkill_force_state - Force the internal rfkill radio state
> * @rfkill: pointer to the rfkill class to modify.
> * @state: the current radio state the class should be forced to.
> @@ -414,8 +461,8 @@ static ssize_t rfkill_claim_store(struct device *dev,
> if (!claim) {
> mutex_lock(&rfkill->mutex);
> rfkill_toggle_radio(rfkill,
> - rfkill_states[rfkill->type],
> - 0);
> + rfkill_global_states[rfkill->type].current_state,
> + 0);
> mutex_unlock(&rfkill->mutex);
> }
> rfkill->user_claim = !!claim;
> @@ -562,7 +609,16 @@ static int rfkill_add_switch(struct rfkill *rfkill)
> if (error < 0)
> goto unlock_out;
>
> - rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
> + if (!error) {
> + /* lock default after first use */
> + set_bit(rfkill->type, rfkill_states_lockdflt);
> + rfkill_global_states[rfkill->type].current_state =
> + rfkill_global_states[rfkill->type].default_state;
> + }
> +
> + rfkill_toggle_radio(rfkill,
> + rfkill_global_states[rfkill->type].current_state,
> + 0);
>
> list_add_tail(&rfkill->node, &rfkill_list);
>
> @@ -718,6 +774,53 @@ void rfkill_unregister(struct rfkill *rfkill)
> }
> EXPORT_SYMBOL(rfkill_unregister);
>
> +/**
> + * rfkill_set_default - set initial value for a switch type
> + * @type - the type of switch to set the default state of
> + * @state - the new default state for that group of switches
> + *
> + * Sets the initial state rfkill should use for a given type.
> + * The following initial states are allowed: RFKILL_STATE_SOFT_BLOCKED
> + * and RFKILL_STATE_UNBLOCKED.
> + *
> + * This function is meant to be used by platform drivers for platforms
> + * that can save switch state across power down/reboot.
> + *
> + * The default state for each switch type can be changed exactly once.
> + * After a switch of that type is registered, the default state cannot
> + * be changed anymore. This guards against multiple drivers it the
> + * same platform trying to set the initial switch default state, which
> + * is not allowed.
> + *
> + * Returns -EPERM if the state has already been set once or is in use,
> + * so drivers likely want to either ignore or at most printk(KERN_NOTICE)
> + * if this function returns -EPERM.
> + *
> + * Returns 0 if the new default state was set, or an error if it
> + * could not be set.
> + */
> +int rfkill_set_default(enum rfkill_type type, enum rfkill_state state)
> +{
> + int error;
> +
> + if (type >= RFKILL_TYPE_MAX ||
> + (state != RFKILL_STATE_SOFT_BLOCKED &&
> + state != RFKILL_STATE_UNBLOCKED))
> + return -EINVAL;
> +
> + mutex_lock(&rfkill_mutex);
> +
> + if (!test_and_set_bit(type, rfkill_states_lockdflt)) {
> + rfkill_global_states[type].default_state = state;
> + error = 0;
> + } else
> + error = -EPERM;
> +
> + mutex_unlock(&rfkill_mutex);
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(rfkill_set_default);
> +
> /*
> * Rfkill module initialization/deinitialization.
> */
> @@ -731,8 +834,8 @@ static int __init rfkill_init(void)
> rfkill_default_state != RFKILL_STATE_UNBLOCKED)
> return -EINVAL;
>
> - for (i = 0; i < ARRAY_SIZE(rfkill_states); i++)
> - rfkill_states[i] = rfkill_default_state;
> + for (i = 0; i < RFKILL_TYPE_MAX; i++)
> + rfkill_global_states[i].default_state = rfkill_default_state;
>
> error = class_register(&rfkill_class);
> if (error) {
On Tuesday 05 August 2008, John W. Linville wrote:
> On Tue, Aug 05, 2008 at 09:03:29AM -0400, Dan Williams wrote:
> > On Mon, 2008-08-04 at 20:35 -0300, Henrique de Moraes Holschuh wrote:
>
> > > And it is already damn clear that what we currently have (rfkill always
> > > blocks on suspend) is not the correct way to go about it. WHAT I want to
> > > know now is whether there are any drivers out there which need the current
> > > behaviour.
> >
> > Ah! I seem to have misunderstood you. If some drivers _do_ need the
> > current block-on-suspend behavior, I feel like that should be an
> > internal driver decision that rfkill shouldn't need to be aware of.
> > Drivers know how to suspend themselves; we shouldn't expect rfkill to
> > know how certain hardware needs to suspend.
>
> I agree with Dan. Blocking and suspending should be separate operations.
Like I said earlier, the main thing rfkill should do is to prevent the callback
function being used while the device is suspended. And I definately agree
on the statement that drivers are in charge to do what should be done for
suspend.
Ivo
On Sun, Aug 3, 2008 at 4:52 PM, Henrique de Moraes Holschuh
<[email protected]> wrote:
> On Sun, 03 Aug 2008, Tomas Winkler wrote:
>> At least from iwlwifi perspective killing radio is not the same
>> operation as shutting down the card.
>
> It almost never is. Which is why I said we should not bother if nobody
> needs that.
>
>> The intention is different if nothing it's really redundant operation
>
> On proper drivers that do proper power management? Certainly :-)
>
>> > RFKILL is not about tracking, it is about *controlling*.
>>
>> I think driver controls the radio why need to add another entity for
>> that. I don't like this definition, But that's just wording I hope.
>
> All the radio-is-allowed-to-transmit decisions are rfkill's. The driver is
> not allowed to override those. This is done to present a uniform behaviour
> and interface to the system's user (and any instance of rfkill doing
> something the user wouldn't expect to the radio is to be considered a major
> bug). rfkill is supposed to represend the will of the system's user
> regarding permission to transmit energy out of wireless transmitters.
May point is that there are radio event out of scope rfkill so the
driver although obey rfkill system
requests but rfkill switch doesn't control the radio, the driver does.
But it's really just point of view.
>
> Not even WoWL is an exception, if your radio is blocked before suspend, you
> are not allowed to enable the transmitter [to get WoWL to work -- I sure
> hope this is not necessary, but it could be]. I'd expect a printk
> complaining about it, though :-)
>
>> I rather remove it, If the driver has D3 operation it should move card
>> to operation
>
> Sure. I was wondering about drivers that *don't* have it, if any, out of
> the potential set of drivers that should be using rfkill (it is not a matter
> of those who are using rfkill right now).
I think we are aligned in general.
Thanks
Tomas
On Saturday 02 August 2008, Henrique de Moraes Holschuh wrote:
> rfkill is not a small, mere detail in wireless support. Once it starts
> supporting rfkill and users start counting on that support, a wireless
> device is at risk of operating in dangerous conditions should rfkill
> support fail to properly activate.
>
> Therefore, add the required __must_check annotations on some key functions
> of the rfkill API, for which the wireless drivers absolutely MUST handle
> the failure mode safely in order to avoid a potentially dangerous situation
> where the wireless transmitter is left enabled when the user don't want it
> to.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
> Cc: Matthew Garrett <[email protected]>
Acked-by: Ivo van Doorn <[email protected]>
> ---
> include/linux/rfkill.h | 5 +++--
> net/rfkill/rfkill.c | 5 +++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
> index aa3c7d5..e92d8e9 100644
> --- a/include/linux/rfkill.h
> +++ b/include/linux/rfkill.h
> @@ -110,9 +110,10 @@ struct rfkill {
> };
> #define to_rfkill(d) container_of(d, struct rfkill, dev)
>
> -struct rfkill *rfkill_allocate(struct device *parent, enum rfkill_type type);
> +struct rfkill * __must_check rfkill_allocate(struct device *parent,
> + enum rfkill_type type);
> void rfkill_free(struct rfkill *rfkill);
> -int rfkill_register(struct rfkill *rfkill);
> +int __must_check rfkill_register(struct rfkill *rfkill);
> void rfkill_unregister(struct rfkill *rfkill);
>
> int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state);
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index c1901fb..5320210 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -653,7 +653,8 @@ static void rfkill_remove_switch(struct rfkill *rfkill)
> * NOTE: If registration fails the structure shoudl be freed by calling
> * rfkill_free() otherwise rfkill_unregister() should be used.
> */
> -struct rfkill *rfkill_allocate(struct device *parent, enum rfkill_type type)
> +struct rfkill * __must_check rfkill_allocate(struct device *parent,
> + enum rfkill_type type)
> {
> struct rfkill *rfkill;
> struct device *dev;
> @@ -724,7 +725,7 @@ static void rfkill_led_trigger_unregister(struct rfkill *rfkill)
> * structure needs to be registered. Immediately from registration the
> * switch driver should be able to service calls to toggle_radio.
> */
> -int rfkill_register(struct rfkill *rfkill)
> +int __must_check rfkill_register(struct rfkill *rfkill)
> {
> static atomic_t rfkill_no = ATOMIC_INIT(0);
> struct device *dev = &rfkill->dev;
On Sun, 03 Aug 2008, Johannes Berg wrote:
> if (WARN_ON(!nb))
> return -EINVAL;
I could use the notation above instead of:
if (foo) {
WARN_ON(1);
return -ERROR;
}
Ivo, which one you prefer? The if() with the condition and WARN on the
branch, or the if(WARN_ON(condition)) ?
> BUG() never returns. Same for all the other places you pointed out.
Yes. And I used BUG() on the notify chain calls, because the primitives in
the kernel code are not doing proper error checking anyway (for speed, I
suppose)... they just OOPS.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Sun, Aug 3, 2008 at 11:12 AM, Ivo van Doorn <[email protected]> wrote:
> On Saturday 02 August 2008, Henrique de Moraes Holschuh wrote:
>> Hi Johannes!
>>
>> On Sat, 02 Aug 2008, Johannes Berg wrote:
>>
>> > On Sat, 2008-08-02 at 15:11 -0300, Henrique de Moraes Holschuh wrote:
>> > > Currently, rfkill would stand in the way of properly supporting wireless
>> > > devices that are capable of waking the system up from sleep or hibernation
>> > > when they receive a special wireless message.
>> > >
>> > > Since rfkill attempts to soft-block any transmitters during class suspend,
>> >
>> > why does it interfere with suspend anyway?
>>
>> The class makes sure that all transmitters are blocked on suspend. You'd
>> have to ask Ivo for the reason, but AFAIK, it is for both safety and to help
>> conserve power.
>
> I think that handler was added by Dmitry, but I see no real reason for
> issuing the BLOCK event during suspend. However the handlers should
> be used to prevent state changes to drivers after they have been suspended.
Sounds reasonable.
Thanks
Tomas
On Sun, 03 Aug 2008, Ivo van Doorn wrote:
> > BUG() never returns. Same for all the other places you pointed out.
>
> Ah right, that doesn't sound too good. I don't think rfkill should
> become a blocker like that. WARN_ON should be sufficient. :)
It would, if anyone ever tested the return from notify chain registering.
But the core kernel doesn't do any checking when registering notifiers to
the chains, and always either return zero or OOPS outright when it attempts
to dereference a NULL pointer (at least on 2.6.25)...
So, I went with BUG(). Given the above, do you still want me to WARN() and
return -EINVAL instead? I can certainly do that, it would be more correct
than what the core kernel is doing, anyway.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh