[GIT PATCH] RFC: next batch of rfkill changes

[GIT PATCH] RFC: next batch of rfkill changes

[Henrique de Moraes Holschuh]

This is my next batch of rfkill changes.  They add features and address
some shortcomings of rfkill-input, mostly.

Please comment.  One thing that is bothering me in the last patch is
what happens during suspend/resume to pending delayed rfkill state
changes.  Are pending scheduled jobs executed, cancelled, or frozen to
fire up when the system resumes itself?

Henrique de Moraes Holschuh (6):
      rfkill: detect bogus double-registering
      rfkill: add default global states
      rfkill: add master_switch_mode functionality
      rfkill: add EPO lock to rfkill-input
      rfkill: rename rfkill_mutex to rfkill_global_mutex
      rfkill: rate-limit rfkill-input workqueue usage

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

[PATCH] rfkill: detect bogus double-registering

[Henrique de Moraes Holschuh]

Detect and abort with -EEXIST if rfkill_register is called twice on the
same rfkill struct.

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 <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
---
 net/rfkill/rfkill.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index c6f2f38..0a78e32 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -509,17 +509,42 @@ 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)
+			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.2

Re: [PATCH] rfkill: detect bogus double-registering

[Johannes Berg]

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


> +	list_for_each_entry(p, &rfkill_list, node) {
> +		if (p == rfkill)
> +			return -EEXIST;
> +		set_bit(p->type, &seen);

You should WARN_ON so it can get fixed.

johannes

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

Re: [PATCH] rfkill: detect bogus double-registering

[Henrique de Moraes Holschuh]

On Wed, 23 Jul 2008, Johannes Berg wrote:
> > +	list_for_each_entry(p, &rfkill_list, node) {
> > +		if (p == rfkill)
> > +			return -EEXIST;
> > +		set_bit(p->type, &seen);
> 
> You should WARN_ON so it can get fixed.

I'd rather make rfkill_register __must_check, actually.  With or without
WARN_ON.

In fact, lots of stuff in rfkill should be __must_check.  Ivo, what do you
think?  Add __must_check?  Add WARN_ON?  Add both?

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

Re: [PATCH] rfkill: detect bogus double-registering

[Johannes Berg]

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

On Wed, 2008-07-23 at 12:27 -0300, Henrique de Moraes Holschuh wrote:
> On Wed, 23 Jul 2008, Johannes Berg wrote:
> > > +	list_for_each_entry(p, &rfkill_list, node) {
> > > +		if (p == rfkill)
> > > +			return -EEXIST;
> > > +		set_bit(p->type, &seen);
> > 
> > You should WARN_ON so it can get fixed.
> 
> I'd rather make rfkill_register __must_check, actually.  With or without
> WARN_ON.
> 
> In fact, lots of stuff in rfkill should be __must_check.  Ivo, what do you
> think?  Add __must_check?  Add WARN_ON?  Add both?

Do both then. __must_check annotations get ignored regularly, or
"handled" like this:

if (...)
	// nothing we can do.

johannes

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

Re: [PATCH] rfkill: detect bogus double-registering

[Henrique de Moraes Holschuh]

On Wed, 23 Jul 2008, Johannes Berg wrote:
> On Wed, 2008-07-23 at 12:27 -0300, Henrique de Moraes Holschuh wrote:
> > On Wed, 23 Jul 2008, Johannes Berg wrote:
> > > > +	list_for_each_entry(p, &rfkill_list, node) {
> > > > +		if (p == rfkill)
> > > > +			return -EEXIST;
> > > > +		set_bit(p->type, &seen);
> > > 
> > > You should WARN_ON so it can get fixed.
> > 
> > I'd rather make rfkill_register __must_check, actually.  With or without
> > WARN_ON.
> > 
> > In fact, lots of stuff in rfkill should be __must_check.  Ivo, what do you
> > think?  Add __must_check?  Add WARN_ON?  Add both?
> 
> Do both then. __must_check annotations get ignored regularly, or
> "handled" like this:
> 
> if (...)
> 	// nothing we can do.

Ah, that gives you heavy spiked LART rights to the head of whomever does it
if he is wrong about it.  You should fail to load the module or something
like that if you are going to run half-broken without rfkill support.

I will add the WARN_ON.  __must_check will wait for Ivo's reply, as you
said, it is often ignored.

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

Re: [PATCH] rfkill: detect bogus double-registering

[Ivo van Doorn]

On Wednesday 23 July 2008, Henrique de Moraes Holschuh wrote:
> On Wed, 23 Jul 2008, Johannes Berg wrote:
> > On Wed, 2008-07-23 at 12:27 -0300, Henrique de Moraes Holschuh wrote:
> > > On Wed, 23 Jul 2008, Johannes Berg wrote:
> > > > > +	list_for_each_entry(p, &rfkill_list, node) {
> > > > > +		if (p == rfkill)
> > > > > +			return -EEXIST;
> > > > > +		set_bit(p->type, &seen);
> > > > 
> > > > You should WARN_ON so it can get fixed.
> > > 
> > > I'd rather make rfkill_register __must_check, actually.  With or without
> > > WARN_ON.
> > > 
> > > In fact, lots of stuff in rfkill should be __must_check.  Ivo, what do you
> > > think?  Add __must_check?  Add WARN_ON?  Add both?
> > 
> > Do both then. __must_check annotations get ignored regularly, or
> > "handled" like this:
> > 
> > if (...)
> > 	// nothing we can do.
> 
> Ah, that gives you heavy spiked LART rights to the head of whomever does it
> if he is wrong about it.  You should fail to load the module or something
> like that if you are going to run half-broken without rfkill support.
> 
> I will add the WARN_ON.  __must_check will wait for Ivo's reply, as you
> said, it is often ignored.

I usually dislike WARN_ON, but like Johannes said, this one is a bug in the driver
which must be fixed. When you submit the final version of the patch having
the WARN_ON would be best.

I am however a fan on sparse annotation and using __must_check, but since you
already stated that more functions might need it, it doesn't have to be in this patch
and could wait for a later patch. (But it is fine if you want to add it in this patch as well,
off course. ;) )

Ivo

[PATCH] rfkill: add default global states

[Henrique de Moraes Holschuh]

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 <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
---
 include/linux/rfkill.h    |    1 +
 net/rfkill/rfkill-input.h |    1 +
 net/rfkill/rfkill.c       |  105 ++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 100 insertions(+), 7 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 0a78e32..eaaabbd 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -45,6 +45,8 @@ MODULE_PARM_DESC(default_state,
 		 "Default initial state for all radio types, 0 = radio off");
 
 static enum rfkill_state rfkill_states[RFKILL_TYPE_MAX];
+static enum rfkill_state rfkill_default_states[RFKILL_TYPE_MAX];
+static unsigned long rfkill_states_lockdflt[BITS_TO_LONGS(RFKILL_TYPE_MAX)];
 
 static BLOCKING_NOTIFIER_HEAD(rfkill_notifier_list);
 
@@ -198,22 +200,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).
+ *
+ * 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;
-
 	list_for_each_entry(rfkill, &rfkill_list, node) {
 		if ((!rfkill->user_claim) && (rfkill->type == type)) {
 			mutex_lock(&rfkill->mutex);
@@ -221,7 +223,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);
@@ -231,10 +246,14 @@ EXPORT_SYMBOL(rfkill_switch_all);
  *
  * This kicks all 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) {
@@ -242,11 +261,33 @@ 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_default_states[i] = rfkill_states[i];
+		rfkill_states[i] = 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_default_states[i]);
+	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.
@@ -536,6 +577,13 @@ static int rfkill_add_switch(struct rfkill *rfkill)
 	if (error < 0)
 		goto unlock_out;
 
+	if (!error) {
+		/* lock default after first use */
+		set_bit(rfkill->type, &rfkill_states_lockdflt);
+		rfkill_states[rfkill->type] =
+				rfkill_default_states[rfkill->type];
+	}
+
 	rfkill_toggle_radio(rfkill, rfkill_states[rfkill->type], 0);
 
 	list_add_tail(&rfkill->node, &rfkill_list);
@@ -689,6 +737,49 @@ 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.  Only "soft"
+ * states are allowed, i.e. 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.
+ *
+ * Returns -EPERM if the state has already been set once or is in use,
+ * so drivers likely want to ignore -EPERM returns from this function.
+ *
+ * Returns 0 if the state was set, or another error if it was not 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_default_states[type] = state;
+		error = 0;
+	} else
+		error = -EPERM;
+
+	mutex_unlock(&rfkill_mutex);
+	return error;
+}
+EXPORT_SYMBOL_GPL(rfkill_set_default);
+
 /*
  * Rfkill module initialization/deinitialization.
  */
@@ -702,8 +793,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 < ARRAY_SIZE(rfkill_default_states); i++)
+		rfkill_default_states[i] = rfkill_default_state;
 
 	error = class_register(&rfkill_class);
 	if (error) {
-- 
1.5.6.2

Re: [PATCH] rfkill: add default global states

[Ivo van Doorn]

>  #endif /* __RFKILL_INPUT_H */
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index 0a78e32..eaaabbd 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -45,6 +45,8 @@ MODULE_PARM_DESC(default_state,
>  		 "Default initial state for all radio types, 0 = radio off");
>  
>  static enum rfkill_state rfkill_states[RFKILL_TYPE_MAX];
> +static enum rfkill_state rfkill_default_states[RFKILL_TYPE_MAX];
> +static unsigned long rfkill_states_lockdflt[BITS_TO_LONGS(RFKILL_TYPE_MAX)];

I have a slight distaste for using multiple arrays where each variable is a different
variable for the same object. Perhaps it is time to move it into a structure:

struct .. {
	enum rfkill_state current
	enum rfkill_state default
};

static struct .. rfkill_states[RFKILL_TYPE_MAX];

It probably won't optimize anything, but I think it looks a bit nicer.

Just a suggestion, if it easier to keep it as multiple arrays I would still ack it. ;)

>  	list_add_tail(&rfkill->node, &rfkill_list);
> @@ -689,6 +737,49 @@ 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.  Only "soft"
> + * states are allowed, i.e. 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.

As sideinfo: I found a hardware bug in a broadcom rfkill key a few days ago
which would not work well with this function:

* laptop was powered on, HW rfkill was set to allow WiFi
* laptop halted, something about a missing power supply and a battery which was drained
	without any userspace tool checking the battery status. ;)
* laptop was powered on and booted,  HW rkfill decided to block WiFi.

In any case not really important for this patch, but just for information. ;)

>   * Rfkill module initialization/deinitialization.
>   */
> @@ -702,8 +793,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 < ARRAY_SIZE(rfkill_default_states); i++)
> +		rfkill_default_states[i] = rfkill_default_state;

rfkill_states[] is now no longer initialized, I guess that is a bad thing ;)

The rest of the patch looks good.

Ivo

Re: [PATCH] rfkill: add default global states

[Henrique de Moraes Holschuh]

On Wed, 23 Jul 2008, Ivo van Doorn wrote:
> >  static enum rfkill_state rfkill_states[RFKILL_TYPE_MAX];
> > +static enum rfkill_state rfkill_default_states[RFKILL_TYPE_MAX];
> > +static unsigned long rfkill_states_lockdflt[BITS_TO_LONGS(RFKILL_TYPE_MAX)];
> 
> I have a slight distaste for using multiple arrays where each variable is a different
> variable for the same object. Perhaps it is time to move it into a structure:
> 
> struct .. {
> 	enum rfkill_state current
> 	enum rfkill_state default
> };
> 
> static struct .. rfkill_states[RFKILL_TYPE_MAX];
> 
> It probably won't optimize anything, but I think it looks a bit nicer.
> 
> Just a suggestion, if it easier to keep it as multiple arrays I would still ack it. ;)

Sure, I can do that.  It doesn't work well for the bitfields, though (wastes
a LOT of memory), so I will keep the bitfields as separate ulong arrays if
that's OK with you?

> > + * 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.  Only "soft"
> > + * states are allowed, i.e. 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.
> 
> As sideinfo: I found a hardware bug in a broadcom rfkill key a few days ago
> which would not work well with this function:
> 
> * laptop was powered on, HW rfkill was set to allow WiFi
> * laptop halted, something about a missing power supply and a battery which was drained
> 	without any userspace tool checking the battery status. ;)
> * laptop was powered on and booted,  HW rkfill decided to block WiFi.

Heh.  Well, it is a hardware failure mode, it will be up to the broadcomm
device driver to figure out when NOT to use rfkill_set_default() ;-)

I just found out I can do that (store WWAN and bluetooth state) across
power cycles on thinkpads as well.  I wonder what kind of weird failures
will be hidden behind THAT... :)

> In any case not really important for this patch, but just for information. ;)

Heh, sure :-)  At least the failure mode for the hardware was safely handled
(it HW-killed the radio instead of unblocking it :p).

> >   * Rfkill module initialization/deinitialization.
> >   */
> > @@ -702,8 +793,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 < ARRAY_SIZE(rfkill_default_states); i++)
> > +		rfkill_default_states[i] = rfkill_default_state;
> 
> rfkill_states[] is now no longer initialized, I guess that is a bad thing ;)

It is initialized from the [current] default state on first use... look at
the hunk for rfkill_add_switch()  :)

I could double-initialize it, but since the kernel will already zero it for
us anyway, and we need to copy the default state to the current state on
first use (so that rfkill_set_default() is easy to implement...) I would
just be triple-initializing something :p

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

Re: [PATCH] rfkill: add default global states

[Ivo van Doorn]

On Wednesday 23 July 2008, Henrique de Moraes Holschuh wrote:
> On Wed, 23 Jul 2008, Ivo van Doorn wrote:
> > >  static enum rfkill_state rfkill_states[RFKILL_TYPE_MAX];
> > > +static enum rfkill_state rfkill_default_states[RFKILL_TYPE_MAX];
> > > +static unsigned long rfkill_states_lockdflt[BITS_TO_LONGS(RFKILL_TYPE_MAX)];
> > 
> > I have a slight distaste for using multiple arrays where each variable is a different
> > variable for the same object. Perhaps it is time to move it into a structure:
> > 
> > struct .. {
> > 	enum rfkill_state current
> > 	enum rfkill_state default
> > };
> > 
> > static struct .. rfkill_states[RFKILL_TYPE_MAX];
> > 
> > It probably won't optimize anything, but I think it looks a bit nicer.
> > 
> > Just a suggestion, if it easier to keep it as multiple arrays I would still ack it. ;)
> 
> Sure, I can do that.  It doesn't work well for the bitfields, though (wastes
> a LOT of memory), so I will keep the bitfields as separate ulong arrays if
> that's OK with you?

Sure sounds good.

> > > + * 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.  Only "soft"
> > > + * states are allowed, i.e. 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.
> > 
> > As sideinfo: I found a hardware bug in a broadcom rfkill key a few days ago
> > which would not work well with this function:
> > 
> > * laptop was powered on, HW rfkill was set to allow WiFi
> > * laptop halted, something about a missing power supply and a battery which was drained
> > 	without any userspace tool checking the battery status. ;)
> > * laptop was powered on and booted,  HW rkfill decided to block WiFi.
> 
> Heh.  Well, it is a hardware failure mode, it will be up to the broadcomm
> device driver to figure out when NOT to use rfkill_set_default() ;-)

Well even the driver can't do much about it, it is only triggered when the power
is completely drained and it simply cuts the power. On a regular halt/reboot the state
is preserved.
So it is something no driver can actually detect..

> I just found out I can do that (store WWAN and bluetooth state) across
> power cycles on thinkpads as well.  I wonder what kind of weird failures
> will be hidden behind THAT... :)
> 
> > In any case not really important for this patch, but just for information. ;)
> 
> Heh, sure :-)  At least the failure mode for the hardware was safely handled
> (it HW-killed the radio instead of unblocking it :p).

True. :)

> > >   * Rfkill module initialization/deinitialization.
> > >   */
> > > @@ -702,8 +793,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 < ARRAY_SIZE(rfkill_default_states); i++)
> > > +		rfkill_default_states[i] = rfkill_default_state;
> > 
> > rfkill_states[] is now no longer initialized, I guess that is a bad thing ;)
> 
> It is initialized from the [current] default state on first use... look at
> the hunk for rfkill_add_switch()  :)

Ah ok, I missed that part.

> I could double-initialize it, but since the kernel will already zero it for
> us anyway, and we need to copy the default state to the current state on
> first use (so that rfkill_set_default() is easy to implement...) I would
> just be triple-initializing something :p

Nah, single initialization is good enough. ;)

Ivo

[PATCH] rfkill: add master_switch_mode functionality

[Henrique de Moraes Holschuh]

Let the user configure what rfkill-input should do upon EV_SW SW_RFKILL_ALL
ON.

The mode of operation can be set through the master_switch_mode parameter.
master_switch_mode 0 does nothing.  1 tries to restore the state before the
last EV_SW SW_RFKILL_ALL OFF, or the default states of the switches if no
EV_SW SW_RFKILL_ALL OFF ever happened.  2 tries to unblock all switches
(default).

Note that the default mode of operation is unchanged.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
---
 net/rfkill/rfkill-input.c |  129 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 107 insertions(+), 22 deletions(-)

diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
index 827f178..273fb38 100644
--- a/net/rfkill/rfkill-input.c
+++ b/net/rfkill/rfkill-input.c
@@ -23,6 +23,18 @@ MODULE_AUTHOR("Dmitry Torokhov <dtor@mail.ru>");
 MODULE_DESCRIPTION("Input layer to RF switch connector");
 MODULE_LICENSE("GPL");
 
+enum rfkill_input_master_mode {
+	RFKILL_INPUT_MASTER_DONOTHING = 0,
+	RFKILL_INPUT_MASTER_RESTORE = 1,
+	RFKILL_INPUT_MASTER_UNBLOCKALL = 2,
+};
+
+static enum rfkill_input_master_mode rfkill_master_switch_mode =
+					RFKILL_INPUT_MASTER_UNBLOCKALL;
+module_param_named(master_switch_mode, rfkill_master_switch_mode, uint, 0);
+MODULE_PARM_DESC(master_switch_mode,
+	"SW_RFKILL_ALL ON should: 0=do nothing; 1=restore; 2=unblock all");
+
 struct rfkill_task {
 	struct work_struct work;
 	enum rfkill_type type;
@@ -32,27 +44,77 @@ struct rfkill_task {
 	enum rfkill_state desired_state; /* on/off */
 };
 
-static void rfkill_task_handler(struct work_struct *work)
+enum rfkill_global_sched_op {
+	RFKILL_GLOBAL_OP_EPO = 0,
+	RFKILL_GLOBAL_OP_RESTORE,
+};
+
+struct rfkill_global_task {
+	struct work_struct work;
+	struct mutex mutex; /* ensures that task is serialized */
+	spinlock_t lock;
+	enum rfkill_global_sched_op op;
+	int op_count; /* avoid race window */
+};
+
+static void rfkill_global_task_handler(struct work_struct *work)
 {
-	struct rfkill_task *task = container_of(work, struct rfkill_task, work);
+	struct rfkill_global_task *task =
+			container_of(work, struct rfkill_global_task, work);
+	enum rfkill_global_sched_op op;
 
 	mutex_lock(&task->mutex);
 
-	rfkill_switch_all(task->type, task->desired_state);
+	spin_lock_irq(&task->lock);
+	if (task->op_count) {
+		op = task->op;
+		task->op_count = 0;
+		spin_unlock_irq(&task->lock);
+
+		switch (op) {
+		case RFKILL_GLOBAL_OP_EPO:
+			rfkill_epo();
+			break;
+		case RFKILL_GLOBAL_OP_RESTORE:
+			rfkill_restore_states();
+			break;
+		default:
+			BUG();
+		}
+	} else
+		/* got scheduled on race window, nothing to do */
+		spin_unlock_irq(&task->lock);
 
 	mutex_unlock(&task->mutex);
 }
 
-static void rfkill_task_epo_handler(struct work_struct *work)
+static struct rfkill_global_task rfkill_global_task = {
+	.work = __WORK_INITIALIZER(rfkill_global_task.work,
+				rfkill_global_task_handler),
+	.mutex =  __MUTEX_INITIALIZER(rfkill_global_task.mutex),
+	.lock = __SPIN_LOCK_UNLOCKED(rfkill_global_task.lock),
+};
+
+static void rfkill_schedule_global_op(enum rfkill_global_sched_op op)
 {
-	rfkill_epo();
-}
+	unsigned long flags;
 
-static DECLARE_WORK(epo_work, rfkill_task_epo_handler);
+	spin_lock_irqsave(&rfkill_global_task.lock, flags);
+	rfkill_global_task.op = op;
+	rfkill_global_task.op_count = 1;
+	schedule_work(&rfkill_global_task.work);
+	spin_unlock_irqrestore(&rfkill_global_task.lock, flags);
+}
 
-static void rfkill_schedule_epo(void)
+static void rfkill_task_handler(struct work_struct *work)
 {
-	schedule_work(&epo_work);
+	struct rfkill_task *task = container_of(work, struct rfkill_task, work);
+
+	mutex_lock(&task->mutex);
+
+	rfkill_switch_all(task->type, task->desired_state);
+
+	mutex_unlock(&task->mutex);
 }
 
 static void rfkill_schedule_set(struct rfkill_task *task,
@@ -60,7 +122,7 @@ static void rfkill_schedule_set(struct rfkill_task *task,
 {
 	unsigned long flags;
 
-	if (unlikely(work_pending(&epo_work)))
+	if (unlikely(work_pending(&rfkill_global_task.work)))
 		return;
 
 	spin_lock_irqsave(&task->lock, flags);
@@ -78,7 +140,7 @@ static void rfkill_schedule_toggle(struct rfkill_task *task)
 {
 	unsigned long flags;
 
-	if (unlikely(work_pending(&epo_work)))
+	if (unlikely(work_pending(&rfkill_global_task.work)))
 		return;
 
 	spin_lock_irqsave(&task->lock, flags);
@@ -114,18 +176,33 @@ static void rfkill_schedule_evsw_rfkillall(int state)
 	/* EVERY radio type. state != 0 means radios ON */
 	/* handle EPO (emergency power off) through shortcut */
 	if (state) {
-		rfkill_schedule_set(&rfkill_wwan,
-				    RFKILL_STATE_UNBLOCKED);
-		rfkill_schedule_set(&rfkill_wimax,
-				    RFKILL_STATE_UNBLOCKED);
-		rfkill_schedule_set(&rfkill_uwb,
-				    RFKILL_STATE_UNBLOCKED);
-		rfkill_schedule_set(&rfkill_bt,
-				    RFKILL_STATE_UNBLOCKED);
-		rfkill_schedule_set(&rfkill_wlan,
-				    RFKILL_STATE_UNBLOCKED);
+		switch (rfkill_master_switch_mode) {
+		case RFKILL_INPUT_MASTER_UNBLOCKALL:
+			rfkill_schedule_set(&rfkill_wwan,
+					    RFKILL_STATE_UNBLOCKED);
+			rfkill_schedule_set(&rfkill_wimax,
+					    RFKILL_STATE_UNBLOCKED);
+			rfkill_schedule_set(&rfkill_uwb,
+					    RFKILL_STATE_UNBLOCKED);
+			rfkill_schedule_set(&rfkill_bt,
+					    RFKILL_STATE_UNBLOCKED);
+			rfkill_schedule_set(&rfkill_wlan,
+					    RFKILL_STATE_UNBLOCKED);
+			break;
+		case RFKILL_INPUT_MASTER_RESTORE:
+			rfkill_schedule_global_op(RFKILL_GLOBAL_OP_RESTORE);
+			break;
+		case RFKILL_INPUT_MASTER_DONOTHING:
+			break;
+		default:
+			printk(KERN_ERR
+				"rfkill-input: Illegal value for "
+				"master_switch_mode parameter: %d\n",
+				rfkill_master_switch_mode);
+			break;
+		}
 	} else
-		rfkill_schedule_epo();
+		rfkill_schedule_global_op(RFKILL_GLOBAL_OP_EPO);
 }
 
 static void rfkill_event(struct input_handle *handle, unsigned int type,
@@ -255,6 +332,14 @@ static struct input_handler rfkill_handler = {
 
 static int __init rfkill_handler_init(void)
 {
+	switch (rfkill_master_switch_mode) {
+	case RFKILL_INPUT_MASTER_DONOTHING:
+	case RFKILL_INPUT_MASTER_RESTORE:
+	case RFKILL_INPUT_MASTER_UNBLOCKALL:
+		break;
+	default:
+		return -EINVAL;
+	}
 	return input_register_handler(&rfkill_handler);
 }
 
-- 
1.5.6.2

Re: [PATCH] rfkill: add master_switch_mode functionality

[Ivo van Doorn]

On Wednesday 23 July 2008, Henrique de Moraes Holschuh wrote:
> Let the user configure what rfkill-input should do upon EV_SW SW_RFKILL_ALL
> ON.
> 
> The mode of operation can be set through the master_switch_mode parameter.
> master_switch_mode 0 does nothing.  1 tries to restore the state before the
> last EV_SW SW_RFKILL_ALL OFF, or the default states of the switches if no
> EV_SW SW_RFKILL_ALL OFF ever happened.  2 tries to unblock all switches
> (default).
> 
> Note that the default mode of operation is unchanged.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>
> ---
>  net/rfkill/rfkill-input.c |  129 +++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 107 insertions(+), 22 deletions(-)
> 
> diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
> index 827f178..273fb38 100644
> --- a/net/rfkill/rfkill-input.c
> +++ b/net/rfkill/rfkill-input.c
> @@ -23,6 +23,18 @@ MODULE_AUTHOR("Dmitry Torokhov <dtor@mail.ru>");
>  MODULE_DESCRIPTION("Input layer to RF switch connector");
>  MODULE_LICENSE("GPL");
>  
> +enum rfkill_input_master_mode {
> +	RFKILL_INPUT_MASTER_DONOTHING = 0,
> +	RFKILL_INPUT_MASTER_RESTORE = 1,
> +	RFKILL_INPUT_MASTER_UNBLOCKALL = 2,

RFKILL_INPUT_MASTER_LAST, /* Internal */

See below for reason. :)

>  	spin_lock_irqsave(&task->lock, flags);
> @@ -114,18 +176,33 @@ static void rfkill_schedule_evsw_rfkillall(int state)
>  	/* EVERY radio type. state != 0 means radios ON */
>  	/* handle EPO (emergency power off) through shortcut */
>  	if (state) {
> -		rfkill_schedule_set(&rfkill_wwan,
> -				    RFKILL_STATE_UNBLOCKED);
> -		rfkill_schedule_set(&rfkill_wimax,
> -				    RFKILL_STATE_UNBLOCKED);
> -		rfkill_schedule_set(&rfkill_uwb,
> -				    RFKILL_STATE_UNBLOCKED);
> -		rfkill_schedule_set(&rfkill_bt,
> -				    RFKILL_STATE_UNBLOCKED);
> -		rfkill_schedule_set(&rfkill_wlan,
> -				    RFKILL_STATE_UNBLOCKED);
> +		switch (rfkill_master_switch_mode) {
> +		case RFKILL_INPUT_MASTER_UNBLOCKALL:
> +			rfkill_schedule_set(&rfkill_wwan,
> +					    RFKILL_STATE_UNBLOCKED);
> +			rfkill_schedule_set(&rfkill_wimax,
> +					    RFKILL_STATE_UNBLOCKED);
> +			rfkill_schedule_set(&rfkill_uwb,
> +					    RFKILL_STATE_UNBLOCKED);
> +			rfkill_schedule_set(&rfkill_bt,
> +					    RFKILL_STATE_UNBLOCKED);
> +			rfkill_schedule_set(&rfkill_wlan,
> +					    RFKILL_STATE_UNBLOCKED);
> +			break;
> +		case RFKILL_INPUT_MASTER_RESTORE:
> +			rfkill_schedule_global_op(RFKILL_GLOBAL_OP_RESTORE);
> +			break;
> +		case RFKILL_INPUT_MASTER_DONOTHING:
> +			break;
> +		default:
> +			printk(KERN_ERR
> +				"rfkill-input: Illegal value for "
> +				"master_switch_mode parameter: %d\n",
> +				rfkill_master_switch_mode);

You probably need to move this printk below to rfkill_handler_init()
since there it will return -EINVAL so it won't reach this function.

> +			break;
> +		}
>  	} else
> -		rfkill_schedule_epo();
> +		rfkill_schedule_global_op(RFKILL_GLOBAL_OP_EPO);
>  }
>  
>  static void rfkill_event(struct input_handle *handle, unsigned int type,
> @@ -255,6 +332,14 @@ static struct input_handler rfkill_handler = {
>  
>  static int __init rfkill_handler_init(void)
>  {
> +	switch (rfkill_master_switch_mode) {
> +	case RFKILL_INPUT_MASTER_DONOTHING:
> +	case RFKILL_INPUT_MASTER_RESTORE:
> +	case RFKILL_INPUT_MASTER_UNBLOCKALL:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

if (rfkill_master_switch_mode >= RFKILL_INPUT_MASTER_LAST)
	return -EINVAL

This will make sure new entries in the enum are automatically handled.
Note that your next patch doesn't update the above switch statement,
and thus always returns -EINVAL when those values are used.

>  	return input_register_handler(&rfkill_handler);
>  }
>  

Ivo

Re: [PATCH] rfkill: add master_switch_mode functionality

[Henrique de Moraes Holschuh]

On Wed, 23 Jul 2008, Ivo van Doorn wrote:
> > +enum rfkill_input_master_mode {
> > +	RFKILL_INPUT_MASTER_DONOTHING = 0,
> > +	RFKILL_INPUT_MASTER_RESTORE = 1,
> > +	RFKILL_INPUT_MASTER_UNBLOCKALL = 2,
> 
> RFKILL_INPUT_MASTER_LAST, /* Internal */
> 
> See below for reason. :)

[...]

> >  static int __init rfkill_handler_init(void)
> >  {
> > +	switch (rfkill_master_switch_mode) {
> > +	case RFKILL_INPUT_MASTER_DONOTHING:
> > +	case RFKILL_INPUT_MASTER_RESTORE:
> > +	case RFKILL_INPUT_MASTER_UNBLOCKALL:
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> if (rfkill_master_switch_mode >= RFKILL_INPUT_MASTER_LAST)
> 	return -EINVAL

It will generate the same code :-)  I can certainly do it that way, but if I
do, every switch() statement will HAVE to handle either default or
RFKILL_INPUT_MASTER_LAST...  which is bad, as you don't get the warning that
you forgot to handle something in other code paths that don't need default:
handling.

That reminds me that I should kill the default: clause in
rfkill_schedule_evsw_rfkillall().  It is there because an earlier version of
the patch would let userspace modify the module paramenter.  That didn't
play out well (if we want to do that, we should do it in some other way),
and I forgot to remove the default handling when I made the attribute
read-only.

> This will make sure new entries in the enum are automatically handled.
> Note that your next patch doesn't update the above switch statement,

My next patch doesn't deal with RFKILL_INPUT_MASTER_* :-) I guess you
misread.

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

[PATCH] rfkill: add EPO lock to rfkill-input

[Henrique de Moraes Holschuh]

Add software-based sanity to rfkill-input so that it can reproduce what
hardware-based EPO switches do, and lock down any further attempts to
UNBLOCK radios until the switch is deactivated.

This makes rfkill and rfkill-input extend the operation of an existing
wireless master kill switch that issues EV_SW SW_RFKILL_ALL OFF events to
all wireless devices in the system, even those that are not under hardware
or firmware control.

Since this is the expected operational behaviour for the master switch, the
EPO lock functionality is not optional.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
---
 net/rfkill/rfkill-input.c |   46 +++++++++++++-------------------------------
 net/rfkill/rfkill-input.h |    1 +
 net/rfkill/rfkill.c       |   36 ++++++++++++++++++++++++++++++++--
 3 files changed, 48 insertions(+), 35 deletions(-)

diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
index 273fb38..8e6781d 100644
--- a/net/rfkill/rfkill-input.c
+++ b/net/rfkill/rfkill-input.c
@@ -47,6 +47,8 @@ struct rfkill_task {
 enum rfkill_global_sched_op {
 	RFKILL_GLOBAL_OP_EPO = 0,
 	RFKILL_GLOBAL_OP_RESTORE,
+	RFKILL_GLOBAL_OP_UNLOCK,
+	RFKILL_GLOBAL_OP_UNBLOCK,
 };
 
 struct rfkill_global_task {
@@ -62,6 +64,7 @@ static void rfkill_global_task_handler(struct work_struct *work)
 	struct rfkill_global_task *task =
 			container_of(work, struct rfkill_global_task, work);
 	enum rfkill_global_sched_op op;
+	unsigned int i;
 
 	mutex_lock(&task->mutex);
 
@@ -78,6 +81,14 @@ static void rfkill_global_task_handler(struct work_struct *work)
 		case RFKILL_GLOBAL_OP_RESTORE:
 			rfkill_restore_states();
 			break;
+		case RFKILL_GLOBAL_OP_UNLOCK:
+			rfkill_set_state_lock(0);
+			break;
+		case RFKILL_GLOBAL_OP_UNBLOCK:
+			rfkill_set_state_lock(0);
+			for (i = 0; i < RFKILL_TYPE_MAX; i++)
+				rfkill_switch_all(i, RFKILL_STATE_UNBLOCKED);
+			break;
 		default:
 			BUG();
 		}
@@ -117,25 +128,6 @@ static void rfkill_task_handler(struct work_struct *work)
 	mutex_unlock(&task->mutex);
 }
 
-static void rfkill_schedule_set(struct rfkill_task *task,
-				enum rfkill_state desired_state)
-{
-	unsigned long flags;
-
-	if (unlikely(work_pending(&rfkill_global_task.work)))
-		return;
-
-	spin_lock_irqsave(&task->lock, flags);
-
-	if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
-		task->desired_state = desired_state;
-		task->last = jiffies;
-		schedule_work(&task->work);
-	}
-
-	spin_unlock_irqrestore(&task->lock, flags);
-}
-
 static void rfkill_schedule_toggle(struct rfkill_task *task)
 {
 	unsigned long flags;
@@ -169,30 +161,19 @@ static DEFINE_RFKILL_TASK(rfkill_wlan, RFKILL_TYPE_WLAN);
 static DEFINE_RFKILL_TASK(rfkill_bt, RFKILL_TYPE_BLUETOOTH);
 static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);
 static DEFINE_RFKILL_TASK(rfkill_wimax, RFKILL_TYPE_WIMAX);
-static DEFINE_RFKILL_TASK(rfkill_wwan, RFKILL_TYPE_WWAN);
 
 static void rfkill_schedule_evsw_rfkillall(int state)
 {
-	/* EVERY radio type. state != 0 means radios ON */
-	/* handle EPO (emergency power off) through shortcut */
 	if (state) {
 		switch (rfkill_master_switch_mode) {
 		case RFKILL_INPUT_MASTER_UNBLOCKALL:
-			rfkill_schedule_set(&rfkill_wwan,
-					    RFKILL_STATE_UNBLOCKED);
-			rfkill_schedule_set(&rfkill_wimax,
-					    RFKILL_STATE_UNBLOCKED);
-			rfkill_schedule_set(&rfkill_uwb,
-					    RFKILL_STATE_UNBLOCKED);
-			rfkill_schedule_set(&rfkill_bt,
-					    RFKILL_STATE_UNBLOCKED);
-			rfkill_schedule_set(&rfkill_wlan,
-					    RFKILL_STATE_UNBLOCKED);
+			rfkill_schedule_global_op(RFKILL_GLOBAL_OP_UNBLOCK);
 			break;
 		case RFKILL_INPUT_MASTER_RESTORE:
 			rfkill_schedule_global_op(RFKILL_GLOBAL_OP_RESTORE);
 			break;
 		case RFKILL_INPUT_MASTER_DONOTHING:
+			rfkill_schedule_global_op(RFKILL_GLOBAL_OP_UNLOCK);
 			break;
 		default:
 			printk(KERN_ERR
@@ -347,6 +328,7 @@ static void __exit rfkill_handler_exit(void)
 {
 	input_unregister_handler(&rfkill_handler);
 	flush_scheduled_work();
+	rfkill_set_state_lock(0);
 }
 
 module_init(rfkill_handler_init);
diff --git a/net/rfkill/rfkill-input.h b/net/rfkill/rfkill-input.h
index bbfa646..6a8c8a2 100644
--- a/net/rfkill/rfkill-input.h
+++ b/net/rfkill/rfkill-input.h
@@ -14,5 +14,6 @@
 void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state);
 void rfkill_epo(void);
 void rfkill_restore_states(void);
+void rfkill_set_state_lock(int lock);
 
 #endif /* __RFKILL_INPUT_H */
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index eaaabbd..2265bc5 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -47,6 +47,7 @@ MODULE_PARM_DESC(default_state,
 static enum rfkill_state rfkill_states[RFKILL_TYPE_MAX];
 static enum rfkill_state rfkill_default_states[RFKILL_TYPE_MAX];
 static unsigned long rfkill_states_lockdflt[BITS_TO_LONGS(RFKILL_TYPE_MAX)];
+static int rfkill_state_lock;
 
 static BLOCKING_NOTIFIER_HEAD(rfkill_notifier_list);
 
@@ -232,11 +233,15 @@ static void __rfkill_switch_all(const enum rfkill_type type,
  *
  * Acquires rfkill_mutex and calls __rfkill_switch_all(@type, @state).
  * Please refer to __rfkill_switch_all() for details.
+ *
+ * Does nothing if rfkill_set_state_lock() was used to lock state
+ * changes.
  */
 void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
 {
 	mutex_lock(&rfkill_mutex);
-	__rfkill_switch_all(type, state);
+	if (!rfkill_state_lock)
+		__rfkill_switch_all(type, state);
 	mutex_unlock(&rfkill_mutex);
 }
 EXPORT_SYMBOL(rfkill_switch_all);
@@ -256,6 +261,8 @@ void rfkill_epo(void)
 	int i;
 
 	mutex_lock(&rfkill_mutex);
+
+	rfkill_state_lock = 1;
 	list_for_each_entry(rfkill, &rfkill_list, node) {
 		mutex_lock(&rfkill->mutex);
 		rfkill_toggle_radio(rfkill, RFKILL_STATE_SOFT_BLOCKED, 1);
@@ -281,6 +288,8 @@ void rfkill_restore_states(void)
 	int i;
 
 	mutex_lock(&rfkill_mutex);
+
+	rfkill_state_lock = 0;
 	for (i = 0; i < RFKILL_TYPE_MAX; i++)
 		__rfkill_switch_all(i, rfkill_default_states[i]);
 	mutex_unlock(&rfkill_mutex);
@@ -288,6 +297,22 @@ void rfkill_restore_states(void)
 EXPORT_SYMBOL_GPL(rfkill_restore_states);
 
 /**
+ * rfkill_set_state_lock - lock or unlock state changes
+ * @lock: 0 unlocks, non-zero locks state changes
+ *
+ * Used by rfkill-input to set initial state of rfkill_state_lock,
+ * based on the initial state of input devices, and to manually
+ * unlock when unloaded.
+ */
+void rfkill_set_state_lock(int lock)
+{
+	mutex_lock(&rfkill_mutex);
+	rfkill_state_lock = !!lock;
+	mutex_unlock(&rfkill_mutex);
+}
+EXPORT_SYMBOL_GPL(rfkill_set_state_lock);
+
+/**
  * 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.
@@ -391,7 +416,12 @@ static ssize_t rfkill_state_store(struct device *dev,
 
 	if (mutex_lock_interruptible(&rfkill->mutex))
 		return -ERESTARTSYS;
-	error = rfkill_toggle_radio(rfkill, state, 0);
+
+	if (!rfkill_state_lock)
+		error = rfkill_toggle_radio(rfkill, state, 0);
+	else
+		error = -EPERM;
+
 	mutex_unlock(&rfkill->mutex);
 
 	return error ? error : count;
@@ -429,7 +459,7 @@ static ssize_t rfkill_claim_store(struct device *dev,
 		return error;
 
 	if (rfkill->user_claim != claim) {
-		if (!claim) {
+		if (!claim && !rfkill_state_lock) {
 			mutex_lock(&rfkill->mutex);
 			rfkill_toggle_radio(rfkill,
 					    rfkill_states[rfkill->type],
-- 
1.5.6.2

Re: [PATCH] rfkill: add EPO lock to rfkill-input

[Ivo van Doorn]

On Wednesday 23 July 2008, Henrique de Moraes Holschuh wrote:
> Add software-based sanity to rfkill-input so that it can reproduce what
> hardware-based EPO switches do, and lock down any further attempts to
> UNBLOCK radios until the switch is deactivated.
> 
> This makes rfkill and rfkill-input extend the operation of an existing
> wireless master kill switch that issues EV_SW SW_RFKILL_ALL OFF events to
> all wireless devices in the system, even those that are not under hardware
> or firmware control.
> 
> Since this is the expected operational behaviour for the master switch, the
> EPO lock functionality is not optional.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>
> ---
>  net/rfkill/rfkill-input.c |   46 +++++++++++++-------------------------------
>  net/rfkill/rfkill-input.h |    1 +
>  net/rfkill/rfkill.c       |   36 ++++++++++++++++++++++++++++++++--
>  3 files changed, 48 insertions(+), 35 deletions(-)
> 
> diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
> index 273fb38..8e6781d 100644
> --- a/net/rfkill/rfkill-input.c
> +++ b/net/rfkill/rfkill-input.c
> @@ -47,6 +47,8 @@ struct rfkill_task {
>  enum rfkill_global_sched_op {
>  	RFKILL_GLOBAL_OP_EPO = 0,
>  	RFKILL_GLOBAL_OP_RESTORE,
> +	RFKILL_GLOBAL_OP_UNLOCK,
> +	RFKILL_GLOBAL_OP_UNBLOCK,
>  };

As mentioned in the previous patch "rfkill: add master_switch_mode functionality"
the above 2 new enums aren't allowed because they are blocked in the module
init function.

>  static void rfkill_schedule_toggle(struct rfkill_task *task)
>  {
>  	unsigned long flags;
> @@ -169,30 +161,19 @@ static DEFINE_RFKILL_TASK(rfkill_wlan, RFKILL_TYPE_WLAN);
>  static DEFINE_RFKILL_TASK(rfkill_bt, RFKILL_TYPE_BLUETOOTH);
>  static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);
>  static DEFINE_RFKILL_TASK(rfkill_wimax, RFKILL_TYPE_WIMAX);
> -static DEFINE_RFKILL_TASK(rfkill_wwan, RFKILL_TYPE_WWAN);

Are all RFKILL_TYPE_WWAN users gone?
In that case the define should disappear completely.

Ivo

Re: [PATCH] rfkill: add EPO lock to rfkill-input

[Henrique de Moraes Holschuh]

On Wed, 23 Jul 2008, Ivo van Doorn wrote:
> > --- a/net/rfkill/rfkill-input.c
> > +++ b/net/rfkill/rfkill-input.c
> > @@ -47,6 +47,8 @@ struct rfkill_task {
> >  enum rfkill_global_sched_op {
> >  	RFKILL_GLOBAL_OP_EPO = 0,
> >  	RFKILL_GLOBAL_OP_RESTORE,
> > +	RFKILL_GLOBAL_OP_UNLOCK,
> > +	RFKILL_GLOBAL_OP_UNBLOCK,
> >  };
> 
> As mentioned in the previous patch "rfkill: add master_switch_mode functionality"
> the above 2 new enums aren't allowed because they are blocked in the module
> init function.

Hmm? The GLOBAL_OP are internal stuff for the driver, and
master_switch_mode=2 does work, I did test :)  I will reread the code and
reply to you on the previous patch.

> >  static void rfkill_schedule_toggle(struct rfkill_task *task)
> >  {
> >  	unsigned long flags;
> > @@ -169,30 +161,19 @@ static DEFINE_RFKILL_TASK(rfkill_wlan, RFKILL_TYPE_WLAN);
> >  static DEFINE_RFKILL_TASK(rfkill_bt, RFKILL_TYPE_BLUETOOTH);
> >  static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);
> >  static DEFINE_RFKILL_TASK(rfkill_wimax, RFKILL_TYPE_WIMAX);
> > -static DEFINE_RFKILL_TASK(rfkill_wwan, RFKILL_TYPE_WWAN);
> 
> Are all RFKILL_TYPE_WWAN users gone?
> In that case the define should disappear completely.

No, we do have users of WWAN.  Thinkpad-acpi for one.  What we don't have is
any specific KEY_WWAN or SW_WWAN event, so rfkill-input doesn't need to
special case it like it does for the others.  And the new RFKILL_ALL/EPO
code handles every switch type in a single for() loop, so I didn't need to
special-case it anymore.

I didn't add a KEY_WWAN because I don't know exactly of anyone who needs it
(thinkpads actually want a KEY_WIRELESS that is used to cycle through
various states for WLAN, WWAN, BLUETOOTH and UWB, or to bring up a GUI or
somesuch).

However, if you guys think it is best, we can certainly ask Dmitry for
KEY_WWAN and add it to rfkill-input.  Anyone with a laptop with a
UMTS/EDGE/GPRS radio might want to map one of his keys to that keycode and
have rfkill-input handle it.

Unfortunately, we can't share KEY_WIMAX.  We could DROP KEY_WIMAX in favor
of KEY_WWAN and have rfkill-input handle KEY_WWAN as both a WiMAX and WWAN
event (WiMAX is actually an element of the WWAN set)...  but the proper fix
for that is a bit more complicated (add superclasses/groups, make WWAN a
superclass and add WIMAX inside it, make WLAN a superclass, create a WPAN
superclass with bluetooth and UWB inside it) and I really don't feel like
coding that one right now :(   rfkill-input really would benefit from that,
user-interface wise, as we usually want to rfkill an entire set of devices
regardless of their "technology".

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

Re: [PATCH] rfkill: add EPO lock to rfkill-input

[Ivo van Doorn]

On Wednesday 23 July 2008, Henrique de Moraes Holschuh wrote:
> On Wed, 23 Jul 2008, Ivo van Doorn wrote:
> > > --- a/net/rfkill/rfkill-input.c
> > > +++ b/net/rfkill/rfkill-input.c
> > > @@ -47,6 +47,8 @@ struct rfkill_task {
> > >  enum rfkill_global_sched_op {
> > >  	RFKILL_GLOBAL_OP_EPO = 0,
> > >  	RFKILL_GLOBAL_OP_RESTORE,
> > > +	RFKILL_GLOBAL_OP_UNLOCK,
> > > +	RFKILL_GLOBAL_OP_UNBLOCK,
> > >  };
> > 
> > As mentioned in the previous patch "rfkill: add master_switch_mode functionality"
> > the above 2 new enums aren't allowed because they are blocked in the module
> > init function.
> 
> Hmm? The GLOBAL_OP are internal stuff for the driver, and
> master_switch_mode=2 does work, I did test :)  I will reread the code and
> reply to you on the previous patch.

*sigh* forget my statement, I am mixing up
RFKILL_INPUT_MASTER_* and RFKILL_GLOBAL_OP_*

I'll grab some coffee to wake up. :)

> > >  static void rfkill_schedule_toggle(struct rfkill_task *task)
> > >  {
> > >  	unsigned long flags;
> > > @@ -169,30 +161,19 @@ static DEFINE_RFKILL_TASK(rfkill_wlan, RFKILL_TYPE_WLAN);
> > >  static DEFINE_RFKILL_TASK(rfkill_bt, RFKILL_TYPE_BLUETOOTH);
> > >  static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);
> > >  static DEFINE_RFKILL_TASK(rfkill_wimax, RFKILL_TYPE_WIMAX);
> > > -static DEFINE_RFKILL_TASK(rfkill_wwan, RFKILL_TYPE_WWAN);
> > 
> > Are all RFKILL_TYPE_WWAN users gone?
> > In that case the define should disappear completely.
> 
> No, we do have users of WWAN.  Thinkpad-acpi for one.  What we don't have is
> any specific KEY_WWAN or SW_WWAN event, so rfkill-input doesn't need to
> special case it like it does for the others.  And the new RFKILL_ALL/EPO
> code handles every switch type in a single for() loop, so I didn't need to
> special-case it anymore.
> 
> I didn't add a KEY_WWAN because I don't know exactly of anyone who needs it
> (thinkpads actually want a KEY_WIRELESS that is used to cycle through
> various states for WLAN, WWAN, BLUETOOTH and UWB, or to bring up a GUI or
> somesuch).
> 
> However, if you guys think it is best, we can certainly ask Dmitry for
> KEY_WWAN and add it to rfkill-input.  Anyone with a laptop with a
> UMTS/EDGE/GPRS radio might want to map one of his keys to that keycode and
> have rfkill-input handle it.

I don't think Dmitry will be very happy with new KEY_* defines, if I recall correctly
there aren't that many availble slots left.

> Unfortunately, we can't share KEY_WIMAX.  We could DROP KEY_WIMAX in favor
> of KEY_WWAN and have rfkill-input handle KEY_WWAN as both a WiMAX and WWAN
> event (WiMAX is actually an element of the WWAN set)...  but the proper fix
> for that is a bit more complicated (add superclasses/groups, make WWAN a
> superclass and add WIMAX inside it, make WLAN a superclass, create a WPAN
> superclass with bluetooth and UWB inside it) and I really don't feel like
> coding that one right now :(   rfkill-input really would benefit from that,
> user-interface wise, as we usually want to rfkill an entire set of devices
> regardless of their "technology".

Those superclasses and groups sound like a complete mess, (or perhaps not, but
just very complicated to get right). If nobody needs the KEY_WWAN lets keep it
out for now, and see later what to do about it.

We should promote userspace tools to control rfkill rather then rfkill-input anyway. ;)

Ivo

[PATCH] rfkill: rename rfkill_mutex to rfkill_global_mutex

[Henrique de Moraes Holschuh]

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 <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
Cc: Michael Buesch <mb@bu3sch.de>
---
 net/rfkill/rfkill.c |   40 ++++++++++++++++++++--------------------
 1 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 2265bc5..d6e16e1 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);
@@ -209,7 +209,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).
  *
- * 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)
@@ -231,7 +231,7 @@ 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.
  *
  * Does nothing if rfkill_set_state_lock() was used to lock state
@@ -239,10 +239,10 @@ static void __rfkill_switch_all(const enum rfkill_type type,
  */
 void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
 {
-	mutex_lock(&rfkill_mutex);
+	mutex_lock(&rfkill_global_mutex);
 	if (!rfkill_state_lock)
 		__rfkill_switch_all(type, state);
-	mutex_unlock(&rfkill_mutex);
+	mutex_unlock(&rfkill_global_mutex);
 }
 EXPORT_SYMBOL(rfkill_switch_all);
 
@@ -250,7 +250,7 @@ EXPORT_SYMBOL(rfkill_switch_all);
  * rfkill_epo - emergency power off all transmitters
  *
  * This kicks all rfkill devices to RFKILL_STATE_SOFT_BLOCKED, ignoring
- * everything in its path but rfkill_mutex and rfkill->mutex.
+ * 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().
@@ -260,7 +260,7 @@ void rfkill_epo(void)
 	struct rfkill *rfkill;
 	int i;
 
-	mutex_lock(&rfkill_mutex);
+	mutex_lock(&rfkill_global_mutex);
 
 	rfkill_state_lock = 1;
 	list_for_each_entry(rfkill, &rfkill_list, node) {
@@ -272,7 +272,7 @@ void rfkill_epo(void)
 		rfkill_default_states[i] = rfkill_states[i];
 		rfkill_states[i] = RFKILL_STATE_SOFT_BLOCKED;
 	}
-	mutex_unlock(&rfkill_mutex);
+	mutex_unlock(&rfkill_global_mutex);
 }
 EXPORT_SYMBOL_GPL(rfkill_epo);
 
@@ -287,12 +287,12 @@ void rfkill_restore_states(void)
 {
 	int i;
 
-	mutex_lock(&rfkill_mutex);
+	mutex_lock(&rfkill_global_mutex);
 
 	rfkill_state_lock = 0;
 	for (i = 0; i < RFKILL_TYPE_MAX; i++)
 		__rfkill_switch_all(i, rfkill_default_states[i]);
-	mutex_unlock(&rfkill_mutex);
+	mutex_unlock(&rfkill_global_mutex);
 }
 EXPORT_SYMBOL_GPL(rfkill_restore_states);
 
@@ -306,9 +306,9 @@ EXPORT_SYMBOL_GPL(rfkill_restore_states);
  */
 void rfkill_set_state_lock(int lock)
 {
-	mutex_lock(&rfkill_mutex);
+	mutex_lock(&rfkill_global_mutex);
 	rfkill_state_lock = !!lock;
-	mutex_unlock(&rfkill_mutex);
+	mutex_unlock(&rfkill_global_mutex);
 }
 EXPORT_SYMBOL_GPL(rfkill_set_state_lock);
 
@@ -454,7 +454,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;
 
@@ -469,7 +469,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;
 }
@@ -601,7 +601,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)
@@ -620,16 +620,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);
@@ -797,7 +797,7 @@ int rfkill_set_default(enum rfkill_type type, enum rfkill_state state)
 	     state != RFKILL_STATE_UNBLOCKED))
 		return -EINVAL;
 
-	mutex_lock(&rfkill_mutex);
+	mutex_lock(&rfkill_global_mutex);
 
 	if (!test_and_set_bit(type, &rfkill_states_lockdflt)) {
 		rfkill_default_states[type] = state;
@@ -805,7 +805,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.2

Re: [PATCH] rfkill: rename rfkill_mutex to rfkill_global_mutex

[Ivo van Doorn]

On Wednesday 23 July 2008, Henrique de Moraes Holschuh wrote:
> 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 <hmh@hmh.eng.br>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>
> Cc: Michael Buesch <mb@bu3sch.de>

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

> ---
>  net/rfkill/rfkill.c |   40 ++++++++++++++++++++--------------------
>  1 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index 2265bc5..d6e16e1 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);
> @@ -209,7 +209,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).
>   *
> - * 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)
> @@ -231,7 +231,7 @@ 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.
>   *
>   * Does nothing if rfkill_set_state_lock() was used to lock state
> @@ -239,10 +239,10 @@ static void __rfkill_switch_all(const enum rfkill_type type,
>   */
>  void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
>  {
> -	mutex_lock(&rfkill_mutex);
> +	mutex_lock(&rfkill_global_mutex);
>  	if (!rfkill_state_lock)
>  		__rfkill_switch_all(type, state);
> -	mutex_unlock(&rfkill_mutex);
> +	mutex_unlock(&rfkill_global_mutex);
>  }
>  EXPORT_SYMBOL(rfkill_switch_all);
>  
> @@ -250,7 +250,7 @@ EXPORT_SYMBOL(rfkill_switch_all);
>   * rfkill_epo - emergency power off all transmitters
>   *
>   * This kicks all rfkill devices to RFKILL_STATE_SOFT_BLOCKED, ignoring
> - * everything in its path but rfkill_mutex and rfkill->mutex.
> + * 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().
> @@ -260,7 +260,7 @@ void rfkill_epo(void)
>  	struct rfkill *rfkill;
>  	int i;
>  
> -	mutex_lock(&rfkill_mutex);
> +	mutex_lock(&rfkill_global_mutex);
>  
>  	rfkill_state_lock = 1;
>  	list_for_each_entry(rfkill, &rfkill_list, node) {
> @@ -272,7 +272,7 @@ void rfkill_epo(void)
>  		rfkill_default_states[i] = rfkill_states[i];
>  		rfkill_states[i] = RFKILL_STATE_SOFT_BLOCKED;
>  	}
> -	mutex_unlock(&rfkill_mutex);
> +	mutex_unlock(&rfkill_global_mutex);
>  }
>  EXPORT_SYMBOL_GPL(rfkill_epo);
>  
> @@ -287,12 +287,12 @@ void rfkill_restore_states(void)
>  {
>  	int i;
>  
> -	mutex_lock(&rfkill_mutex);
> +	mutex_lock(&rfkill_global_mutex);
>  
>  	rfkill_state_lock = 0;
>  	for (i = 0; i < RFKILL_TYPE_MAX; i++)
>  		__rfkill_switch_all(i, rfkill_default_states[i]);
> -	mutex_unlock(&rfkill_mutex);
> +	mutex_unlock(&rfkill_global_mutex);
>  }
>  EXPORT_SYMBOL_GPL(rfkill_restore_states);
>  
> @@ -306,9 +306,9 @@ EXPORT_SYMBOL_GPL(rfkill_restore_states);
>   */
>  void rfkill_set_state_lock(int lock)
>  {
> -	mutex_lock(&rfkill_mutex);
> +	mutex_lock(&rfkill_global_mutex);
>  	rfkill_state_lock = !!lock;
> -	mutex_unlock(&rfkill_mutex);
> +	mutex_unlock(&rfkill_global_mutex);
>  }
>  EXPORT_SYMBOL_GPL(rfkill_set_state_lock);
>  
> @@ -454,7 +454,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;
>  
> @@ -469,7 +469,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;
>  }
> @@ -601,7 +601,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)
> @@ -620,16 +620,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);
> @@ -797,7 +797,7 @@ int rfkill_set_default(enum rfkill_type type, enum rfkill_state state)
>  	     state != RFKILL_STATE_UNBLOCKED))
>  		return -EINVAL;
>  
> -	mutex_lock(&rfkill_mutex);
> +	mutex_lock(&rfkill_global_mutex);
>  
>  	if (!test_and_set_bit(type, &rfkill_states_lockdflt)) {
>  		rfkill_default_states[type] = state;
> @@ -805,7 +805,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);

[PATCH] rfkill: rate-limit rfkill-input workqueue usage

[Henrique de Moraes Holschuh]

Limit the number of rfkill-input global operations per second.  It lacked
the limiter that non-global operations (state changes) had.  This way, a
rogue input event generator cannot force rfkill-input to hog the workqueue
too much.

Rework the limiter code so that newer state change requests (rfkill input
events) will override older ones that haven't been acted upon yet.  It used
to ignore new ones that were past the rate limit.

The hardcoded limit is to process requests only once every 200ms.  Limits
are separate for each switch type and for the global operations (used by
the master switch).

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 net/rfkill/rfkill-input.c |   43 ++++++++++++++++++++++++++++---------------
 1 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
index 8e6781d..be3edda 100644
--- a/net/rfkill/rfkill-input.c
+++ b/net/rfkill/rfkill-input.c
@@ -23,6 +23,9 @@ MODULE_AUTHOR("Dmitry Torokhov <dtor@mail.ru>");
 MODULE_DESCRIPTION("Input layer to RF switch connector");
 MODULE_LICENSE("GPL");
 
+/* Delay (in ms) between consecutive switch ops */
+#define	RFKILL_OPS_DELAY 200UL
+
 enum rfkill_input_master_mode {
 	RFKILL_INPUT_MASTER_DONOTHING = 0,
 	RFKILL_INPUT_MASTER_RESTORE = 1,
@@ -36,7 +39,7 @@ MODULE_PARM_DESC(master_switch_mode,
 	"SW_RFKILL_ALL ON should: 0=do nothing; 1=restore; 2=unblock all");
 
 struct rfkill_task {
-	struct work_struct work;
+	struct delayed_work dwork;
 	enum rfkill_type type;
 	struct mutex mutex; /* ensures that task is serialized */
 	spinlock_t lock; /* for accessing last and desired state */
@@ -52,17 +55,18 @@ enum rfkill_global_sched_op {
 };
 
 struct rfkill_global_task {
-	struct work_struct work;
+	struct delayed_work dwork;
 	struct mutex mutex; /* ensures that task is serialized */
 	spinlock_t lock;
+	unsigned long last; /* last schedule */
 	enum rfkill_global_sched_op op;
 	int op_count; /* avoid race window */
 };
 
 static void rfkill_global_task_handler(struct work_struct *work)
 {
-	struct rfkill_global_task *task =
-			container_of(work, struct rfkill_global_task, work);
+	struct rfkill_global_task *task = container_of(work,
+			struct rfkill_global_task, dwork.work);
 	enum rfkill_global_sched_op op;
 	unsigned int i;
 
@@ -100,12 +104,18 @@ static void rfkill_global_task_handler(struct work_struct *work)
 }
 
 static struct rfkill_global_task rfkill_global_task = {
-	.work = __WORK_INITIALIZER(rfkill_global_task.work,
+	.dwork = __DELAYED_WORK_INITIALIZER(rfkill_global_task.dwork,
 				rfkill_global_task_handler),
 	.mutex =  __MUTEX_INITIALIZER(rfkill_global_task.mutex),
 	.lock = __SPIN_LOCK_UNLOCKED(rfkill_global_task.lock),
 };
 
+static unsigned long rfkill_ratelimit(const unsigned long last)
+{
+	const unsigned long delay = msecs_to_jiffies(RFKILL_OPS_DELAY);
+	return (time_after(jiffies, last + delay)) ? 0 : delay;
+};
+
 static void rfkill_schedule_global_op(enum rfkill_global_sched_op op)
 {
 	unsigned long flags;
@@ -113,13 +123,18 @@ static void rfkill_schedule_global_op(enum rfkill_global_sched_op op)
 	spin_lock_irqsave(&rfkill_global_task.lock, flags);
 	rfkill_global_task.op = op;
 	rfkill_global_task.op_count = 1;
-	schedule_work(&rfkill_global_task.work);
+	if (likely(!delayed_work_pending(&rfkill_global_task.dwork))) {
+		schedule_delayed_work(&rfkill_global_task.dwork,
+				rfkill_ratelimit(rfkill_global_task.last));
+		rfkill_global_task.last = jiffies;
+	}
 	spin_unlock_irqrestore(&rfkill_global_task.lock, flags);
 }
 
 static void rfkill_task_handler(struct work_struct *work)
 {
-	struct rfkill_task *task = container_of(work, struct rfkill_task, work);
+	struct rfkill_task *task = container_of(work,
+					struct rfkill_task, dwork.work);
 
 	mutex_lock(&task->mutex);
 
@@ -132,24 +147,22 @@ static void rfkill_schedule_toggle(struct rfkill_task *task)
 {
 	unsigned long flags;
 
-	if (unlikely(work_pending(&rfkill_global_task.work)))
+	if (unlikely(delayed_work_pending(&rfkill_global_task.dwork)))
 		return;
 
 	spin_lock_irqsave(&task->lock, flags);
-
-	if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
-		task->desired_state =
-				rfkill_state_complement(task->desired_state);
+	task->desired_state = rfkill_state_complement(task->desired_state);
+	if (likely(!delayed_work_pending(&task->dwork))) {
+		schedule_delayed_work(&task->dwork,
+					rfkill_ratelimit(task->last));
 		task->last = jiffies;
-		schedule_work(&task->work);
 	}
-
 	spin_unlock_irqrestore(&task->lock, flags);
 }
 
 #define DEFINE_RFKILL_TASK(n, t)				\
 	struct rfkill_task n = {				\
-		.work = __WORK_INITIALIZER(n.work,		\
+		.dwork = __DELAYED_WORK_INITIALIZER(n.dwork,	\
 				rfkill_task_handler),		\
 		.type = t,					\
 		.mutex = __MUTEX_INITIALIZER(n.mutex),		\
-- 
1.5.6.2

Re: [PATCH] rfkill: rate-limit rfkill-input workqueue usage

[Ivo van Doorn]

On Wednesday 23 July 2008, Henrique de Moraes Holschuh wrote:
> Limit the number of rfkill-input global operations per second.  It lacked
> the limiter that non-global operations (state changes) had.  This way, a
> rogue input event generator cannot force rfkill-input to hog the workqueue
> too much.
> 
> Rework the limiter code so that newer state change requests (rfkill input
> events) will override older ones that haven't been acted upon yet.  It used
> to ignore new ones that were past the rate limit.
> 
> The hardcoded limit is to process requests only once every 200ms.  Limits
> are separate for each switch type and for the global operations (used by
> the master switch).
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>

Good idea. :)

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

> ---
>  net/rfkill/rfkill-input.c |   43 ++++++++++++++++++++++++++++---------------
>  1 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
> index 8e6781d..be3edda 100644
> --- a/net/rfkill/rfkill-input.c
> +++ b/net/rfkill/rfkill-input.c
> @@ -23,6 +23,9 @@ MODULE_AUTHOR("Dmitry Torokhov <dtor@mail.ru>");
>  MODULE_DESCRIPTION("Input layer to RF switch connector");
>  MODULE_LICENSE("GPL");
>  
> +/* Delay (in ms) between consecutive switch ops */
> +#define	RFKILL_OPS_DELAY 200UL
> +
>  enum rfkill_input_master_mode {
>  	RFKILL_INPUT_MASTER_DONOTHING = 0,
>  	RFKILL_INPUT_MASTER_RESTORE = 1,
> @@ -36,7 +39,7 @@ MODULE_PARM_DESC(master_switch_mode,
>  	"SW_RFKILL_ALL ON should: 0=do nothing; 1=restore; 2=unblock all");
>  
>  struct rfkill_task {
> -	struct work_struct work;
> +	struct delayed_work dwork;
>  	enum rfkill_type type;
>  	struct mutex mutex; /* ensures that task is serialized */
>  	spinlock_t lock; /* for accessing last and desired state */
> @@ -52,17 +55,18 @@ enum rfkill_global_sched_op {
>  };
>  
>  struct rfkill_global_task {
> -	struct work_struct work;
> +	struct delayed_work dwork;
>  	struct mutex mutex; /* ensures that task is serialized */
>  	spinlock_t lock;
> +	unsigned long last; /* last schedule */
>  	enum rfkill_global_sched_op op;
>  	int op_count; /* avoid race window */
>  };
>  
>  static void rfkill_global_task_handler(struct work_struct *work)
>  {
> -	struct rfkill_global_task *task =
> -			container_of(work, struct rfkill_global_task, work);
> +	struct rfkill_global_task *task = container_of(work,
> +			struct rfkill_global_task, dwork.work);
>  	enum rfkill_global_sched_op op;
>  	unsigned int i;
>  
> @@ -100,12 +104,18 @@ static void rfkill_global_task_handler(struct work_struct *work)
>  }
>  
>  static struct rfkill_global_task rfkill_global_task = {
> -	.work = __WORK_INITIALIZER(rfkill_global_task.work,
> +	.dwork = __DELAYED_WORK_INITIALIZER(rfkill_global_task.dwork,
>  				rfkill_global_task_handler),
>  	.mutex =  __MUTEX_INITIALIZER(rfkill_global_task.mutex),
>  	.lock = __SPIN_LOCK_UNLOCKED(rfkill_global_task.lock),
>  };
>  
> +static unsigned long rfkill_ratelimit(const unsigned long last)
> +{
> +	const unsigned long delay = msecs_to_jiffies(RFKILL_OPS_DELAY);
> +	return (time_after(jiffies, last + delay)) ? 0 : delay;
> +};
> +
>  static void rfkill_schedule_global_op(enum rfkill_global_sched_op op)
>  {
>  	unsigned long flags;
> @@ -113,13 +123,18 @@ static void rfkill_schedule_global_op(enum rfkill_global_sched_op op)
>  	spin_lock_irqsave(&rfkill_global_task.lock, flags);
>  	rfkill_global_task.op = op;
>  	rfkill_global_task.op_count = 1;
> -	schedule_work(&rfkill_global_task.work);
> +	if (likely(!delayed_work_pending(&rfkill_global_task.dwork))) {
> +		schedule_delayed_work(&rfkill_global_task.dwork,
> +				rfkill_ratelimit(rfkill_global_task.last));
> +		rfkill_global_task.last = jiffies;
> +	}
>  	spin_unlock_irqrestore(&rfkill_global_task.lock, flags);
>  }
>  
>  static void rfkill_task_handler(struct work_struct *work)
>  {
> -	struct rfkill_task *task = container_of(work, struct rfkill_task, work);
> +	struct rfkill_task *task = container_of(work,
> +					struct rfkill_task, dwork.work);
>  
>  	mutex_lock(&task->mutex);
>  
> @@ -132,24 +147,22 @@ static void rfkill_schedule_toggle(struct rfkill_task *task)
>  {
>  	unsigned long flags;
>  
> -	if (unlikely(work_pending(&rfkill_global_task.work)))
> +	if (unlikely(delayed_work_pending(&rfkill_global_task.dwork)))
>  		return;
>  
>  	spin_lock_irqsave(&task->lock, flags);
> -
> -	if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
> -		task->desired_state =
> -				rfkill_state_complement(task->desired_state);
> +	task->desired_state = rfkill_state_complement(task->desired_state);
> +	if (likely(!delayed_work_pending(&task->dwork))) {
> +		schedule_delayed_work(&task->dwork,
> +					rfkill_ratelimit(task->last));
>  		task->last = jiffies;
> -		schedule_work(&task->work);
>  	}
> -
>  	spin_unlock_irqrestore(&task->lock, flags);
>  }
>  
>  #define DEFINE_RFKILL_TASK(n, t)				\
>  	struct rfkill_task n = {				\
> -		.work = __WORK_INITIALIZER(n.work,		\
> +		.dwork = __DELAYED_WORK_INITIALIZER(n.dwork,	\
>  				rfkill_task_handler),		\
>  		.type = t,					\
>  		.mutex = __MUTEX_INITIALIZER(n.mutex),		\

Re: [PATCH] rfkill: rate-limit rfkill-input workqueue usage

[Dmitry Torokhov]

On Wed, Jul 23, 2008 at 08:46:58PM +0200, Ivo van Doorn wrote:
> On Wednesday 23 July 2008, Henrique de Moraes Holschuh wrote:
> > Limit the number of rfkill-input global operations per second.  It lacked
> > the limiter that non-global operations (state changes) had.  This way, a
> > rogue input event generator cannot force rfkill-input to hog the workqueue
> > too much.
> > 
> > Rework the limiter code so that newer state change requests (rfkill input
> > events) will override older ones that haven't been acted upon yet.  It used
> > to ignore new ones that were past the rate limit.

This was done to deal with potential jitter from button devices.

In the proposed implementation, if button bounces and generates 2 or
more press/release pairs and we manage to schedule and execure first
task (which is scheduled with delay 0) before the last press/release
arrives we will schedule an extra work and toggle the switch again as
far as I can see.

> >  
> > @@ -132,24 +147,22 @@ static void rfkill_schedule_toggle(struct rfkill_task *task)
> >  {
> >  	unsigned long flags;
> >  
> > -	if (unlikely(work_pending(&rfkill_global_task.work)))
> > +	if (unlikely(delayed_work_pending(&rfkill_global_task.dwork)))

What if the global task comletes executing right at this moment?

> >  		return;
> >  
> >  	spin_lock_irqsave(&task->lock, flags);
> > -
> > -	if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
> > -		task->desired_state =
> > -				rfkill_state_complement(task->desired_state);
> > +	task->desired_state = rfkill_state_complement(task->desired_state);
> > +	if (likely(!delayed_work_pending(&task->dwork))) {
> > +		schedule_delayed_work(&task->dwork,
> > +					rfkill_ratelimit(task->last));
> >  		task->last = jiffies;
> > -		schedule_work(&task->work);
> >  	}
> > -
> >  	spin_unlock_irqrestore(&task->lock, flags);
> >  }
> >  

-- 
Dmitry

Re: [PATCH] rfkill: rate-limit rfkill-input workqueue usage

[Henrique de Moraes Holschuh]

On Wed, 23 Jul 2008, Dmitry Torokhov wrote:
> On Wed, Jul 23, 2008 at 08:46:58PM +0200, Ivo van Doorn wrote:
> > On Wednesday 23 July 2008, Henrique de Moraes Holschuh wrote:
> > > Limit the number of rfkill-input global operations per second.  It lacked
> > > the limiter that non-global operations (state changes) had.  This way, a
> > > rogue input event generator cannot force rfkill-input to hog the workqueue
> > > too much.
> > > 
> > > Rework the limiter code so that newer state change requests (rfkill input
> > > events) will override older ones that haven't been acted upon yet.  It used
> > > to ignore new ones that were past the rate limit.
> 
> This was done to deal with potential jitter from button devices.

I'd argue that should be handled in the input device driver (note: I didn't
say input layer, although it could certainly add debouncing helpers if we
don't want to duplicate code in drivers).

IMHO, you should NOT send the result of a lack of proper debouncing to the
input layer.  When you do, you send duplicated events to every input device
handler, userspace, etc.

> In the proposed implementation, if button bounces and generates 2 or
> more press/release pairs and we manage to schedule and execure first
> task (which is scheduled with delay 0) before the last press/release
> arrives we will schedule an extra work and toggle the switch again as
> far as I can see.

You are correct.  I didn't know the previous behaviour was there to ignore
rapid press of buttons.  It looked like anti-DoS standard limiting to me
(needed, since userspace can inject events).

Well there are two possible ways to deal with this.  Three actually.  The
first is to ignore the issue in rfkill-input and input layer, and tell
anything that generate events for badly-behaved buttons to deal with it.
The second, is to fix it in the input layer, so that we don't have to fix it
in every handler.  The third is to fix it in the handlers.

I'd rather either the driver or the input layer dealt with this issue, so as
not to propagate bogus events to the rest of the system...

Or did I misunderstand the issue?

> > > @@ -132,24 +147,22 @@ static void rfkill_schedule_toggle(struct rfkill_task *task)
> > >  {
> > >  	unsigned long flags;
> > >  
> > > -	if (unlikely(work_pending(&rfkill_global_task.work)))
> > > +	if (unlikely(delayed_work_pending(&rfkill_global_task.dwork)))
> 
> What if the global task comletes executing right at this moment?

If delayed_work_pending() is set, then it has not begun executing yet.  That
bit is cleared by the workqueue stuff right before the delayed work handler
is called.

But I missed a test. I should check delayed_work_pending() and if it is set,
check rfkill_global_task.op_count.  If both are set, we are sure we ARE going
to execute a global operation in the near future, and therefore we should
clobber regular toggles.

Would that fix the issue with rfkill_schedule_toggle versus
rfkill_global_task?

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

Re: [PATCH] rfkill: rate-limit rfkill-input workqueue usage

[Dmitry Torokhov]

On Wed, Jul 23, 2008 at 05:27:55PM -0300, Henrique de Moraes Holschuh wrote:
> On Wed, 23 Jul 2008, Dmitry Torokhov wrote:
> > On Wed, Jul 23, 2008 at 08:46:58PM +0200, Ivo van Doorn wrote:
> > > On Wednesday 23 July 2008, Henrique de Moraes Holschuh wrote:
> > > > Limit the number of rfkill-input global operations per second.  It lacked
> > > > the limiter that non-global operations (state changes) had.  This way, a
> > > > rogue input event generator cannot force rfkill-input to hog the workqueue
> > > > too much.
> > > > 
> > > > Rework the limiter code so that newer state change requests (rfkill input
> > > > events) will override older ones that haven't been acted upon yet.  It used
> > > > to ignore new ones that were past the rate limit.
> > 
> > This was done to deal with potential jitter from button devices.
> 
> I'd argue that should be handled in the input device driver (note: I didn't
> say input layer, although it could certainly add debouncing helpers if we
> don't want to duplicate code in drivers).
> 
> IMHO, you should NOT send the result of a lack of proper debouncing to the
> input layer.  When you do, you send duplicated events to every input device
> handler, userspace, etc.
> 
> > In the proposed implementation, if button bounces and generates 2 or
> > more press/release pairs and we manage to schedule and execure first
> > task (which is scheduled with delay 0) before the last press/release
> > arrives we will schedule an extra work and toggle the switch again as
> > far as I can see.
> 
> You are correct.  I didn't know the previous behaviour was there to ignore
> rapid press of buttons.  It looked like anti-DoS standard limiting to me
> (needed, since userspace can inject events).
> 
> Well there are two possible ways to deal with this.  Three actually.  The
> first is to ignore the issue in rfkill-input and input layer, and tell
> anything that generate events for badly-behaved buttons to deal with it.
> The second, is to fix it in the input layer, so that we don't have to fix it
> in every handler.  The third is to fix it in the handlers.
> 
> I'd rather either the driver or the input layer dealt with this issue, so as
> not to propagate bogus events to the rest of the system...
> 
> Or did I misunderstand the issue?
> 

Yes, we can try to make drivers behave. Originally I was trying to be
nice to simple drivers and take care of the jitter for them.

I do not think the input core is suited to deal with the jitter since
many devices support several buttons to be pressed at the same time so
we'd need separate timers for pretty much every event possible.

> > > > @@ -132,24 +147,22 @@ static void rfkill_schedule_toggle(struct rfkill_task *task)
> > > >  {
> > > >  	unsigned long flags;
> > > >  
> > > > -	if (unlikely(work_pending(&rfkill_global_task.work)))
> > > > +	if (unlikely(delayed_work_pending(&rfkill_global_task.dwork)))
> > 
> > What if the global task comletes executing right at this moment?
> 
> If delayed_work_pending() is set, then it has not begun executing yet.  That
> bit is cleared by the workqueue stuff right before the delayed work handler
> is called.
> 
> But I missed a test. I should check delayed_work_pending() and if it is set,
> check rfkill_global_task.op_count.  If both are set, we are sure we ARE going
> to execute a global operation in the near future, and therefore we should
> clobber regular toggles.
> 
> Would that fix the issue with rfkill_schedule_toggle versus
> rfkill_global_task?
> 

Not sure... consider:

CPU1:					CPU2:
 check work pending - yes
 hardware interrupt			 start executing global work
 ....					 global work done
 return from HW handler
 return immediately from
	rfkill_schedule_toggle

-> event lost.

-- 
Dmitry

Re: [GIT PATCH] RFC: next batch of rfkill changes

[Henrique de Moraes Holschuh]

On Tue, 22 Jul 2008, Henrique de Moraes Holschuh wrote:
> This is my next batch of rfkill changes.  They add features and address
> some shortcomings of rfkill-input, mostly.
> 
> Please comment.  One thing that is bothering me in the last patch is
> what happens during suspend/resume to pending delayed rfkill state
> changes.  Are pending scheduled jobs executed, cancelled, or frozen to
> fire up when the system resumes itself?
> 
> Henrique de Moraes Holschuh (6):
>       rfkill: detect bogus double-registering
>       rfkill: add default global states
>       rfkill: add master_switch_mode functionality
>       rfkill: add EPO lock to rfkill-input
>       rfkill: rename rfkill_mutex to rfkill_global_mutex
>       rfkill: rate-limit rfkill-input workqueue usage

I forgot to give the -n option to git format-patch and didn't notice it
before I sent the batch away, so the ordering is missing.  The above
shortlog has the correct patch sequence.

Since this is just for comments, I will not resend the whole stack right
now just to get the ordering in the subject lines.

Sorry about that.

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

Re: [GIT PATCH] RFC: next batch of rfkill changes

[Ivo van Doorn]

On Wednesday 23 July 2008, Henrique de Moraes Holschuh wrote:
> 
> This is my next batch of rfkill changes.  They add features and address
> some shortcomings of rfkill-input, mostly.
> 
> Please comment.  One thing that is bothering me in the last patch is
> what happens during suspend/resume to pending delayed rfkill state
> changes.  Are pending scheduled jobs executed, cancelled, or frozen to
> fire up when the system resumes itself?

When using the kernel workqueue, I *think* they are frozen until the system
resumes. When using a private workqueue you should use the freezable
workqueue allocation.

But in this case you are using the kernel workqueue, so I assume it is
scheduled untill after resume.

> Henrique de Moraes Holschuh (6):
>       rfkill: detect bogus double-registering
>       rfkill: add default global states
>       rfkill: add master_switch_mode functionality
>       rfkill: add EPO lock to rfkill-input
>       rfkill: rename rfkill_mutex to rfkill_global_mutex
>       rfkill: rate-limit rfkill-input workqueue usage
> 

Ivo

Re: [GIT PATCH] RFC: next batch of rfkill changes

[Henrique de Moraes Holschuh]

On Wed, 23 Jul 2008, Ivo van Doorn wrote:
> On Wednesday 23 July 2008, Henrique de Moraes Holschuh wrote:
> > This is my next batch of rfkill changes.  They add features and address
> > some shortcomings of rfkill-input, mostly.
> > 
> > Please comment.  One thing that is bothering me in the last patch is
> > what happens during suspend/resume to pending delayed rfkill state
> > changes.  Are pending scheduled jobs executed, cancelled, or frozen to
> > fire up when the system resumes itself?
> 
> When using the kernel workqueue, I *think* they are frozen until the system
> resumes. When using a private workqueue you should use the freezable
> workqueue allocation.
> 
> But in this case you are using the kernel workqueue, so I assume it is
> scheduled untill after resume.

I will look into handling that in the next version, then.  Or as a separate
patch.

I wonder if we should switch to a private workqueue?  We are not being
exactly lightweight users of schedule_foo in rfkill-input, and I'd really
hate to hang the main workqueue if something in rfkill/rfkill-input decides
to deadlock.

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

Re: [GIT PATCH] RFC: next batch of rfkill changes

[John W. Linville]

On Tue, Jul 22, 2008 at 10:04:00PM -0300, Henrique de Moraes Holschuh wrote:
> 
> This is my next batch of rfkill changes.  They add features and address
> some shortcomings of rfkill-input, mostly.
> 
> Please comment.  One thing that is bothering me in the last patch is
> what happens during suspend/resume to pending delayed rfkill state
> changes.  Are pending scheduled jobs executed, cancelled, or frozen to
> fire up when the system resumes itself?
> 
> Henrique de Moraes Holschuh (6):
>       rfkill: detect bogus double-registering
>       rfkill: add default global states
>       rfkill: add master_switch_mode functionality
>       rfkill: add EPO lock to rfkill-input
>       rfkill: rename rfkill_mutex to rfkill_global_mutex
>       rfkill: rate-limit rfkill-input workqueue usage

FYI...because you included RFC in the subject I won't be merging
these until I hear otherwise from you.

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [GIT PATCH] RFC: next batch of rfkill changes

[Henrique de Moraes Holschuh]

On Fri, 01 Aug 2008, John W. Linville wrote:
> On Tue, Jul 22, 2008 at 10:04:00PM -0300, Henrique de Moraes Holschuh wrote:
> > This is my next batch of rfkill changes.  They add features and address
> > some shortcomings of rfkill-input, mostly.
> > 
> > Please comment.  One thing that is bothering me in the last patch is
> > what happens during suspend/resume to pending delayed rfkill state
> > changes.  Are pending scheduled jobs executed, cancelled, or frozen to
> > fire up when the system resumes itself?
> > 
> > Henrique de Moraes Holschuh (6):
> >       rfkill: detect bogus double-registering
> >       rfkill: add default global states
> >       rfkill: add master_switch_mode functionality
> >       rfkill: add EPO lock to rfkill-input
> >       rfkill: rename rfkill_mutex to rfkill_global_mutex
> >       rfkill: rate-limit rfkill-input workqueue usage
> 
> FYI...because you included RFC in the subject I won't be merging
> these until I hear otherwise from you.

That's fine, they are not for merging yet :-)  I have modified some of them
a lot, added others, and have not found a good way to do the last one
(rate-limiting) properly yet. I will post another "RFC" batch sooner or
later, probably during the weekend.

This is all 2.6.28 material anyway, so there is no hurry to merge early on
wireless-testing, it is best to merge right.  If I find anything that looks
like a somewhat urgent fix, I will post it separately for merging in 2.6.27.

When some of them patch are finally good for merging, I will send a stack
just with those and ask for a merge explicitly.

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