[RFC] b43: A patch for control of the radio LED using rfkill

[RFC] b43: A patch for control of the radio LED using rfkill

[Larry Finger]

The changes below, along with Henriques patch "[PATCH] rfkill: update
LEDs for all state changes", and the reversion of commit bc19d6e make
the wireless LED toggle correctly.

I'll let the experts discuss whether the changes below have problems
of the sort that were discussed in the context of the regression I
found.

Larry

Index: wireless-testing/drivers/net/wireless/b43/rfkill.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/b43/rfkill.c
+++ wireless-testing/drivers/net/wireless/b43/rfkill.c
@@ -44,13 +44,11 @@ static bool b43_is_hw_radio_enabled(stru
 	return 0;
 }
 
-/* The poll callback for the hardware button. */
-static void b43_rfkill_poll(struct input_polled_dev *poll_dev)
+static void b43_rfkill_update(struct b43_wldev *dev)
 {
-	struct b43_wldev *dev = poll_dev->private;
 	struct b43_wl *wl = dev->wl;
 	bool enabled;
-	bool report_change = 0;
+	struct b43_rfkill *rfk = &(dev->wl->rfkill);
 
 	mutex_lock(&wl->mutex);
 	if (unlikely(b43_status(dev) < B43_STAT_INITIALIZED)) {
@@ -60,18 +58,40 @@ static void b43_rfkill_poll(struct input
 	enabled = b43_is_hw_radio_enabled(dev);
 	if (unlikely(enabled != dev->radio_hw_enable)) {
 		dev->radio_hw_enable = enabled;
-		report_change = 1;
 		b43info(wl, "Radio hardware status changed to %s\n",
 			enabled ? "ENABLED" : "DISABLED");
+		if (!enabled)
+			rfkill_force_state(rfk->rfkill,
+					   RFKILL_STATE_HARD_BLOCKED);
+		else {
+			if (!dev->phy.radio_on)
+				rfkill_force_state(rfk->rfkill,
+						   RFKILL_STATE_SOFT_BLOCKED);
+			else
+				rfkill_force_state(rfk->rfkill,
+						   RFKILL_STATE_UNBLOCKED);
+		}
 	}
 	mutex_unlock(&wl->mutex);
+}
 
-	/* send the radio switch event to the system - note both a key press
-	 * and a release are required */
-	if (unlikely(report_change)) {
-		input_report_key(poll_dev->input, KEY_WLAN, 1);
-		input_report_key(poll_dev->input, KEY_WLAN, 0);
-	}
+static void b43_rfkill_poll(unsigned long data)
+{
+	struct b43_rfkill *rfkill = (struct b43_rfkill *)data;
+	schedule_work(&rfkill->poll_queue);
+}
+
+static void b43_rfkill_work(struct work_struct *work)
+{
+	struct b43_rfkill *rfk = container_of(work, struct b43_rfkill,
+					      poll_queue);
+	struct b43_wl *wl = container_of(rfk, struct b43_wl, rfkill);
+	struct b43_wldev *dev = wl->current_dev;
+
+	b43_rfkill_update(dev);
+	rfk->poll_timer.function = b43_rfkill_poll;
+	rfk->poll_timer.expires = round_jiffies(jiffies + HZ);
+	add_timer(&rfk->poll_timer);
 }
 
 /* Called when the RFKILL toggled in software. */
@@ -141,48 +161,23 @@ void b43_rfkill_init(struct b43_wldev *d
 	rfk->rfkill->toggle_radio = b43_rfkill_soft_toggle;
 	rfk->rfkill->user_claim_unsupported = 1;
 
-	rfk->poll_dev = input_allocate_polled_device();
-	if (!rfk->poll_dev) {
-		rfkill_free(rfk->rfkill);
-		goto err_freed_rfk;
-	}
-
-	rfk->poll_dev->private = dev;
-	rfk->poll_dev->poll = b43_rfkill_poll;
-	rfk->poll_dev->poll_interval = 1000; /* msecs */
-
-	rfk->poll_dev->input->name = rfk->name;
-	rfk->poll_dev->input->id.bustype = BUS_HOST;
-	rfk->poll_dev->input->id.vendor = dev->dev->bus->boardinfo.vendor;
-	rfk->poll_dev->input->evbit[0] = BIT(EV_KEY);
-	set_bit(KEY_WLAN, rfk->poll_dev->input->keybit);
-
 	err = rfkill_register(rfk->rfkill);
 	if (err)
-		goto err_free_polldev;
+		goto err_free_rfk;
 
-#ifdef CONFIG_RFKILL_INPUT_MODULE
-	/* B43 RF-kill isn't useful without the rfkill-input subsystem.
-	 * Try to load the module. */
-	err = request_module("rfkill-input");
-	if (err)
-		b43warn(wl, "Failed to load the rfkill-input module. "
-			"The built-in radio LED will not work.\n");
-#endif /* CONFIG_RFKILL_INPUT */
+	rfk->registered = 1;
 
-	err = input_register_polled_device(rfk->poll_dev);
-	if (err)
-		goto err_unreg_rfk;
+	INIT_WORK(&rfk->poll_queue, b43_rfkill_work);
 
-	rfk->registered = 1;
+	init_timer(&rfk->poll_timer);
+	rfk->poll_timer.function = b43_rfkill_poll;
+	rfk->poll_timer.expires = round_jiffies(jiffies + HZ);
+	rfk->poll_timer.data = (unsigned long)rfk;
+	add_timer(&rfk->poll_timer);
 
 	return;
-err_unreg_rfk:
-	rfkill_unregister(rfk->rfkill);
-err_free_polldev:
-	input_free_polled_device(rfk->poll_dev);
-	rfk->poll_dev = NULL;
-err_freed_rfk:
+err_free_rfk:
+	rfkill_free(rfk->rfkill);
 	rfk->rfkill = NULL;
 out_error:
 	rfk->registered = 0;
@@ -197,9 +192,8 @@ void b43_rfkill_exit(struct b43_wldev *d
 		return;
 	rfk->registered = 0;
 
-	input_unregister_polled_device(rfk->poll_dev);
+	del_timer(&rfk->poll_timer);
+
 	rfkill_unregister(rfk->rfkill);
-	input_free_polled_device(rfk->poll_dev);
-	rfk->poll_dev = NULL;
 	rfk->rfkill = NULL;
 }
Index: wireless-testing/drivers/net/wireless/b43/rfkill.h
===================================================================
--- wireless-testing.orig/drivers/net/wireless/b43/rfkill.h
+++ wireless-testing/drivers/net/wireless/b43/rfkill.h
@@ -7,14 +7,14 @@ struct b43_wldev;
 #ifdef CONFIG_B43_RFKILL
 
 #include <linux/rfkill.h>
-#include <linux/input-polldev.h>
-
 
 struct b43_rfkill {
 	/* The RFKILL subsystem data structure */
 	struct rfkill *rfkill;
-	/* The poll device for the RFKILL input button */
-	struct input_polled_dev *poll_dev;
+	/* The timer list for the RFKILL polling */
+	struct timer_list poll_timer;
+	/* Workqueue for the RFKILL polling */
+	struct work_struct poll_queue;
 	/* Did initialization succeed? Used for freeing. */
 	bool registered;
 	/* The unique name of this rfkill switch */

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

Hi,

> @@ -60,18 +58,40 @@ static void b43_rfkill_poll(struct input
>  	enabled = b43_is_hw_radio_enabled(dev);
>  	if (unlikely(enabled != dev->radio_hw_enable)) {
>  		dev->radio_hw_enable = enabled;
> -		report_change = 1;
>  		b43info(wl, "Radio hardware status changed to %s\n",
>  			enabled ? "ENABLED" : "DISABLED");
> +		if (!enabled)
> +			rfkill_force_state(rfk->rfkill,
> +					   RFKILL_STATE_HARD_BLOCKED);
> +		else {
> +			if (!dev->phy.radio_on)
> +				rfkill_force_state(rfk->rfkill,
> +						   RFKILL_STATE_SOFT_BLOCKED);
> +			else
> +				rfkill_force_state(rfk->rfkill,
> +						   RFKILL_STATE_UNBLOCKED);
> +		}

Is dev->phy.radio_on set when mac80211 has send an instruction
to the driver to enable the radio (start() or config() callback)
or does it represent the key state in the hardware?

If it is something coming from mac80211, then you do not want
to send a SOFT_BLOCKED event since that will cause all other radios
to be switched off simply because the b43 interface has not been
enabled.

Off course when it represents the key state in the hardware then the
code would be fine...

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Larry Finger]

Ivo van Doorn wrote:

> Is dev->phy.radio_on set when mac80211 has send an instruction
> to the driver to enable the radio (start() or config() callback)
> or does it represent the key state in the hardware?
> 
> If it is something coming from mac80211, then you do not want
> to send a SOFT_BLOCKED event since that will cause all other radios
> to be switched off simply because the b43 interface has not been
> enabled.
> 
> Off course when it represents the key state in the hardware then the
> code would be fine...

The state comes from mac80211 and is set in the config() callback.

What state should be sent at the point when the hardware block is
removed? It seems to me that forcing an UNBLOCKED state gives the
wrong result. Perhaps RFKILL does need to have 4 states so that an
RFKILL_STATE_HW_UNBLOCKED state can be transmitted.

Larry

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Larry Finger wrote:
> Ivo van Doorn wrote:
> 
> > Is dev->phy.radio_on set when mac80211 has send an instruction
> > to the driver to enable the radio (start() or config() callback)
> > or does it represent the key state in the hardware?
> > 
> > If it is something coming from mac80211, then you do not want
> > to send a SOFT_BLOCKED event since that will cause all other radios
> > to be switched off simply because the b43 interface has not been
> > enabled.
> > 
> > Off course when it represents the key state in the hardware then the
> > code would be fine...
> 
> The state comes from mac80211 and is set in the config() callback.
> 
> What state should be sent at the point when the hardware block is
> removed? It seems to me that forcing an UNBLOCKED state gives the
> wrong result. Perhaps RFKILL does need to have 4 states so that an
> RFKILL_STATE_HW_UNBLOCKED state can be transmitted.

When the key is pressed to unblock the device, then simply send state
RFKILL_STATE_UNBLOCKED to rfkill. That will generate a trigger to the
driver to enable the radio, and at that time you can check if mac80211
wants the radio on or not (aka you check dev->phy.radio_on or the mac80211
config structure).

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> On Thursday 18 September 2008, Larry Finger wrote:
> > Ivo van Doorn wrote:
> > > Is dev->phy.radio_on set when mac80211 has send an instruction
> > > to the driver to enable the radio (start() or config() callback)
> > > or does it represent the key state in the hardware?
> > > 
> > > If it is something coming from mac80211, then you do not want
> > > to send a SOFT_BLOCKED event since that will cause all other radios
> > > to be switched off simply because the b43 interface has not been
> > > enabled.
> > > 
> > > Off course when it represents the key state in the hardware then the
> > > code would be fine...
> > 
> > The state comes from mac80211 and is set in the config() callback.
> > 
> > What state should be sent at the point when the hardware block is
> > removed? It seems to me that forcing an UNBLOCKED state gives the

The *REAL* state of the hardware.  rfkill_force_state() is used to match the
rfkill idea of the radio state with whatever is REALLY in the hardware.

I guess it would be a damn good idea to make that clear in the docs, I will
send in a patch for that.

> > wrong result. Perhaps RFKILL does need to have 4 states so that an
> > RFKILL_STATE_HW_UNBLOCKED state can be transmitted.

It looks like I need an API that makes the conversion from HW:SW ->
rfkill_state easier...

The problem is that it really boils down to something like this:

static inline enum rfkill_state translate_to_rfkill_state(
		const bool is_any_hw_rfkill_line_active_RIGHT_NOW,
		const bool is_any_sw_rfkill_line_active_RIGHT_NOW) {

	if (is_any_hw_rfkill_line_active_RIGHT_NOW)
		return RFKILL_STATE_HARD_BLOCKED;
	else if (is_any_sw_rfkill_line_active_RIGHT_NOW)
		return RFKILL_STATE_SOFT_BLOCKED;
	else
		return RFKILL_STATE_UNBLOCKED;
}

and send that to rfkill_force_state().

I don't see why we couldn't add a rfkill_translate_and_force_state()
function that does the above and calls rfkill_force_state() on the result.
But I somehow doubt that will really help much.

The key issue seems to be to make it clear to everyone that whatever you use
to synthesize the status you are going to give to rfkill_force_state() must
be the current state of the hardware.

> When the key is pressed to unblock the device, then simply send state
> RFKILL_STATE_UNBLOCKED to rfkill. That will generate a trigger to the

You can only do THAT if the software rfkill lines were also unblocked.  If
any of the software rfkill lines are still active, the transmitter is still
blocked and the correct target state is SOFT_BLOCKED.

In other words: the radio might be double-blocked both in HW and SW.  When
you unblock the HW, you may transition to either SOFT_BLOCKED or UNBLOCKED,
depending on wether the SW rfkill line is still active.

I don't know enough about the mac80211 layer and b43 to know whether
dev->phy.radio_on matches the hardware or not.

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

> I don't know enough about the mac80211 layer and b43 to know whether
> dev->phy.radio_on matches the hardware or not.

It is the software request from mac80211 which ultimately comes from the user.

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > I don't know enough about the mac80211 layer and b43 to know whether
> > dev->phy.radio_on matches the hardware or not.
> 
> It is the software request from mac80211 which ultimately comes from the user.

It is incorrect, then.  Must be the real, current, state of the SW rfkill
line.

-- 
  "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: clarify usage of rfkill_force_state() and rfkill->get_state()

[Henrique de Moraes Holschuh]

rfkill_force_state() is used to update the rfkill core's idea of what is
really happening RIGHT NOW to the transmitter hardware in a PUSH model.

rfkill->get_state() does the same, in a PULL model.

Neither of them change the real hardware radio state through a call to
rfkill->toggle_radio() or anything of the sort, so they must deal with the
real, current state of the hardware.

Change some documentation to make that more clear (I hope).

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
---
 Documentation/rfkill.txt |   10 ++++++++++
 include/linux/rfkill.h   |    3 +++
 net/rfkill/rfkill.c      |    3 +++
 3 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
index b65f079..0143f1e 100644
--- a/Documentation/rfkill.txt
+++ b/Documentation/rfkill.txt
@@ -461,6 +461,16 @@ mandatory when the device has a hardware rfkill line, or when something else
 like the firmware could cause its state to be changed without going through the
 rfkill class.
 
+**** ATTENTION ****
+Both the state returned by the rfkill->get_state() hook, and the state
+passed to rfkill_force_state() MUST BE THE REAL, CURRENT STATE OF THE
+HARDWARE TRANSMITTER.  Never use for these functions the "desired" state,
+"user requested" state, or anything of the sort.
+
+Also, be warned that rfkill_force_state() cannot be called from atomic context,
+so interrupt handlers will have to make use of a workqueue to do it.
+**** ATTENTION ****
+
 Some hardware provides events when its status changes.  In these cases, it is
 best for the driver to not provide a get_state hook, and instead register the
 rfkill class *already* with the correct status, and keep it updated using
diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index 4cd64b0..92b9c72 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -80,6 +80,9 @@ enum rfkill_state {
  *      may be called from atomic context, should return 0 on success.
  *      Either this handler OR judicious use of rfkill_force_state() is
  *      MANDATORY for any driver capable of RFKILL_STATE_HARD_BLOCKED.
+ *      THIS HOOK MUST RETURN THE CURRENT, REAL STATE OF THE HARDWARE
+ *      TRANSMITTER, and not the "desired state", "user requested state"
+ *      or anything of the sort.
  * @led_trigger: A LED trigger for this button's LED.
  * @dev: Device structure integrating the switch into device tree.
  * @node: Used to place switch into list of all switches known to the
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index f949a48..64a53f1 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -333,6 +333,9 @@ EXPORT_SYMBOL_GPL(rfkill_restore_states);
  * a notification by the firmware/hardware of the current *real*
  * state of the radio rfkill switch.
  *
+ * The state passed to this function MUST MATCH THE CURRENT,
+ * REAL HARDWARE STATE OF THE TRANSMITTER.
+ *
  * Devices which are subject to external changes on their rfkill
  * state (such as those caused by a hardware rfkill line) MUST
  * have their driver arrange to call rfkill_force_state() as soon
-- 
1.5.6.5

Re: [PATCH] rfkill: clarify usage of rfkill_force_state() and rfkill->get_state()

[Ivo van Doorn]

On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> rfkill_force_state() is used to update the rfkill core's idea of what is
> really happening RIGHT NOW to the transmitter hardware in a PUSH model.
> 
> rfkill->get_state() does the same, in a PULL model.
> 
> Neither of them change the real hardware radio state through a call to
> rfkill->toggle_radio() or anything of the sort, so they must deal with the
> real, current state of the hardware.
> 
> Change some documentation to make that more clear (I hope).
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>

See my other mail I just send out.
But I don't quite agree on this change of rfkill event interpretation.

Ivo

> Cc: Larry Finger <Larry.Finger@lwfinger.net>
> ---
>  Documentation/rfkill.txt |   10 ++++++++++
>  include/linux/rfkill.h   |    3 +++
>  net/rfkill/rfkill.c      |    3 +++
>  3 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
> index b65f079..0143f1e 100644
> --- a/Documentation/rfkill.txt
> +++ b/Documentation/rfkill.txt
> @@ -461,6 +461,16 @@ mandatory when the device has a hardware rfkill line, or when something else
>  like the firmware could cause its state to be changed without going through the
>  rfkill class.
>  
> +**** ATTENTION ****
> +Both the state returned by the rfkill->get_state() hook, and the state
> +passed to rfkill_force_state() MUST BE THE REAL, CURRENT STATE OF THE
> +HARDWARE TRANSMITTER.  Never use for these functions the "desired" state,
> +"user requested" state, or anything of the sort.
> +
> +Also, be warned that rfkill_force_state() cannot be called from atomic context,
> +so interrupt handlers will have to make use of a workqueue to do it.
> +**** ATTENTION ****
> +
>  Some hardware provides events when its status changes.  In these cases, it is
>  best for the driver to not provide a get_state hook, and instead register the
>  rfkill class *already* with the correct status, and keep it updated using
> diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
> index 4cd64b0..92b9c72 100644
> --- a/include/linux/rfkill.h
> +++ b/include/linux/rfkill.h
> @@ -80,6 +80,9 @@ enum rfkill_state {
>   *      may be called from atomic context, should return 0 on success.
>   *      Either this handler OR judicious use of rfkill_force_state() is
>   *      MANDATORY for any driver capable of RFKILL_STATE_HARD_BLOCKED.
> + *      THIS HOOK MUST RETURN THE CURRENT, REAL STATE OF THE HARDWARE
> + *      TRANSMITTER, and not the "desired state", "user requested state"
> + *      or anything of the sort.
>   * @led_trigger: A LED trigger for this button's LED.
>   * @dev: Device structure integrating the switch into device tree.
>   * @node: Used to place switch into list of all switches known to the
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index f949a48..64a53f1 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -333,6 +333,9 @@ EXPORT_SYMBOL_GPL(rfkill_restore_states);
>   * a notification by the firmware/hardware of the current *real*
>   * state of the radio rfkill switch.
>   *
> + * The state passed to this function MUST MATCH THE CURRENT,
> + * REAL HARDWARE STATE OF THE TRANSMITTER.
> + *
>   * Devices which are subject to external changes on their rfkill
>   * state (such as those caused by a hardware rfkill line) MUST
>   * have their driver arrange to call rfkill_force_state() as soon

Re: [PATCH] rfkill: clarify usage of rfkill_force_state() and rfkill->get_state()

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> > rfkill_force_state() is used to update the rfkill core's idea of what is
> > really happening RIGHT NOW to the transmitter hardware in a PUSH model.
> > 
> > rfkill->get_state() does the same, in a PULL model.
> > 
> > Neither of them change the real hardware radio state through a call to
> > rfkill->toggle_radio() or anything of the sort, so they must deal with the
> > real, current state of the hardware.
> > 
> > Change some documentation to make that more clear (I hope).
> > 
> > Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> > Cc: Ivo van Doorn <IvDoorn@gmail.com>
> 
> See my other mail I just send out.

I did, and just replied to it.

But only now do I think I realised what you meant:  That even if the driver
tries to keep set txpower off separate from rfkill, if it uses the hardware
soft-rfkill bit to implement both, it cannot use that to feed information to
rfkill_force_state() directly.

Argh.

> But I don't quite agree on this change of rfkill event interpretation.

Well, there is no intended change in interpretation, but I don't know how to
word it in a way that means "the real current hardware state as far as
rfkill is concerned".

Because suppose it is a driver doing txpower off AND software rfkill using
the *same* hardware bit (a sigle software rfkill bit).

Now it must do something like this in pseudo-code:

	1. if (the bit is disabled (i.e. SW rfkill is NOT ACTIVE)) {
		rfkill-SW-status = disabled;
	   }  else if (the bit is enabled (i.e. SW rfkill is ACTIVE)) {
		if (tx power off is NOT ACTIVE)
			rfkill-SW-status = enabled;
		else
			rfkill-SW-status = whatever the user asked
	   }

THEN, it should use rfkill-sw-status, along with the hw rfkill line status,
to synthesize the state it must pass to rfkill_force_status().

ICK.  Of course, if the driver has another way to implement txpower off that
does not clash with sw rfkill, the above is unneeded.

How would you put that into words for the rfkill documentation?

-- 
  "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: clarify usage of rfkill_force_state() and rfkill->get_state()

[Johannes Berg]

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

On Thu, 2008-09-18 at 13:43 -0300, Henrique de Moraes Holschuh wrote:

> Now it must do something like this in pseudo-code:
> 
> 	1. if (the bit is disabled (i.e. SW rfkill is NOT ACTIVE)) {
> 		rfkill-SW-status = disabled;
> 	   }  else if (the bit is enabled (i.e. SW rfkill is ACTIVE)) {
> 		if (tx power off is NOT ACTIVE)
> 			rfkill-SW-status = enabled;
> 		else
> 			rfkill-SW-status = whatever the user asked
> 	   }
> 
> THEN, it should use rfkill-sw-status, along with the hw rfkill line status,
> to synthesize the state it must pass to rfkill_force_status().
> 
> ICK.  Of course, if the driver has another way to implement txpower off that
> does not clash with sw rfkill, the above is unneeded.

Why are we not handling soft-rfkill in mac80211 entirely?

johannes

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

Re: [PATCH] rfkill: clarify usage of rfkill_force_state() and rfkill->get_state()

[Ivo van Doorn]

On Thursday 18 September 2008, Johannes Berg wrote:
> On Thu, 2008-09-18 at 13:43 -0300, Henrique de Moraes Holschuh wrote:
> 
> > Now it must do something like this in pseudo-code:
> > 
> > 	1. if (the bit is disabled (i.e. SW rfkill is NOT ACTIVE)) {
> > 		rfkill-SW-status = disabled;
> > 	   }  else if (the bit is enabled (i.e. SW rfkill is ACTIVE)) {
> > 		if (tx power off is NOT ACTIVE)
> > 			rfkill-SW-status = enabled;
> > 		else
> > 			rfkill-SW-status = whatever the user asked
> > 	   }
> > 
> > THEN, it should use rfkill-sw-status, along with the hw rfkill line status,
> > to synthesize the state it must pass to rfkill_force_status().
> > 
> > ICK.  Of course, if the driver has another way to implement txpower off that
> > does not clash with sw rfkill, the above is unneeded.
> 
> Why are we not handling soft-rfkill in mac80211 entirely?

Ideal situation would indeed be that mac80211 registers a rfkill structure
and listens to rfkill events. This would help drivers by only needing to
register a rfkill structure for state-change events without any need for
listeners.

I was considering such a patch some time ago, but needed to figure out
how to work with the state-override capabilities (HW_BLOCK and SOFT_BLOCK)
and didn't work on it any further since.

Ivo

Re: [PATCH] rfkill: clarify usage of rfkill_force_state() and rfkill->get_state()

[Johannes Berg]

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

On Thu, 2008-09-18 at 19:32 +0200, Ivo van Doorn wrote:

> Ideal situation would indeed be that mac80211 registers a rfkill structure
> and listens to rfkill events. This would help drivers by only needing to
> register a rfkill structure for state-change events without any need for
> listeners.

Yup.

> I was considering such a patch some time ago, but needed to figure out
> how to work with the state-override capabilities (HW_BLOCK and SOFT_BLOCK)
> and didn't work on it any further since.

So make the struct part of the hw structure? Then drivers can just use
that to force hard events. Or actually, no, don't do this, make a new
mac80211 call:

	ieee80211_inform_hardblocked(BLOCK/OPEN)

which makes sure we can also try to not associate in this case in the
future...

johannes

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

Re: [PATCH] rfkill: clarify usage of rfkill_force_state() and rfkill->get_state()

[Ivo van Doorn]

On Thursday 18 September 2008, Johannes Berg wrote:
> On Thu, 2008-09-18 at 19:32 +0200, Ivo van Doorn wrote:
> 
> > Ideal situation would indeed be that mac80211 registers a rfkill structure
> > and listens to rfkill events. This would help drivers by only needing to
> > register a rfkill structure for state-change events without any need for
> > listeners.
> 
> Yup.
> 
> > I was considering such a patch some time ago, but needed to figure out
> > how to work with the state-override capabilities (HW_BLOCK and SOFT_BLOCK)
> > and didn't work on it any further since.
> 
> So make the struct part of the hw structure? Then drivers can just use
> that to force hard events. Or actually, no, don't do this, make a new
> mac80211 call:
> 
> 	ieee80211_inform_hardblocked(BLOCK/OPEN)
> 
> which makes sure we can also try to not associate in this case in the
> future...

Yeah, unfortunately that wasn't the probablematic part. ;)
Anyway when I have some time available I'll see if I can sort it out and
make it work. But that will not be for another week or 2.

Ivo

Re: [PATCH] rfkill: clarify usage of rfkill_force_state() and rfkill->get_state()

[Ivo van Doorn]

On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> > > rfkill_force_state() is used to update the rfkill core's idea of what is
> > > really happening RIGHT NOW to the transmitter hardware in a PUSH model.
> > > 
> > > rfkill->get_state() does the same, in a PULL model.
> > > 
> > > Neither of them change the real hardware radio state through a call to
> > > rfkill->toggle_radio() or anything of the sort, so they must deal with the
> > > real, current state of the hardware.
> > > 
> > > Change some documentation to make that more clear (I hope).
> > > 
> > > Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> > > Cc: Ivo van Doorn <IvDoorn@gmail.com>
> > 
> > See my other mail I just send out.
> 
> I did, and just replied to it.
> 
> But only now do I think I realised what you meant:  That even if the driver
> tries to keep set txpower off separate from rfkill, if it uses the hardware
> soft-rfkill bit to implement both, it cannot use that to feed information to
> rfkill_force_state() directly.
> 
> Argh.

Indeed, you need to differentiate between RFKILL and RADIO states.

> > But I don't quite agree on this change of rfkill event interpretation.
> 
> Well, there is no intended change in interpretation, but I don't know how to
> word it in a way that means "the real current hardware state as far as
> rfkill is concerned".

Well my interpretation is that rfkill events and thus rfkill_force_state() calls
should be done for RFKILL state changes only. And RADIO state should be
ignored since they don't matter for rfkill.

> Because suppose it is a driver doing txpower off AND software rfkill using
> the *same* hardware bit (a sigle software rfkill bit).
>
> Now it must do something like this in pseudo-code:
> 
> 	1. if (the bit is disabled (i.e. SW rfkill is NOT ACTIVE)) {
> 		rfkill-SW-status = disabled;
> 	   }  else if (the bit is enabled (i.e. SW rfkill is ACTIVE)) {
> 		if (tx power off is NOT ACTIVE)
> 			rfkill-SW-status = enabled;
> 		else
> 			rfkill-SW-status = whatever the user asked
> 	   }
> 
> THEN, it should use rfkill-sw-status, along with the hw rfkill line status,
> to synthesize the state it must pass to rfkill_force_status().
> 
> ICK.  Of course, if the driver has another way to implement txpower off that
> does not clash with sw rfkill, the above is unneeded.
> 
> How would you put that into words for the rfkill documentation?

The driver is required to keep track of the userspace configuration settings,
when rfkill sends BLOCK to driver it should disable the radio, when rfkill
send UNBLOCK to the driver it should restore to the userspace configuration
settings (which can either be an enabled or disabled radio).

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Michael Buesch]

On Thursday 18 September 2008 16:21:33 Henrique de Moraes Holschuh wrote:
> The problem is that it really boils down to something like this:
> 
> static inline enum rfkill_state translate_to_rfkill_state(
> 		const bool is_any_hw_rfkill_line_active_RIGHT_NOW,
> 		const bool is_any_sw_rfkill_line_active_RIGHT_NOW) {
> 
> 	if (is_any_hw_rfkill_line_active_RIGHT_NOW)
> 		return RFKILL_STATE_HARD_BLOCKED;
> 	else if (is_any_sw_rfkill_line_active_RIGHT_NOW)
> 		return RFKILL_STATE_SOFT_BLOCKED;
> 	else
> 		return RFKILL_STATE_UNBLOCKED;
> }
> 
> and send that to rfkill_force_state().

I agree with that. We must call that from the paths that change the software
state of the b43 radio and the hardware rfkill state notifiers in the b43 driver.

-- 
Greetings Michael.

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Michael Buesch]

On Thursday 18 September 2008 15:53:28 Ivo van Doorn wrote:
> On Thursday 18 September 2008, Larry Finger wrote:
> > Ivo van Doorn wrote:
> > 
> > > Is dev->phy.radio_on set when mac80211 has send an instruction
> > > to the driver to enable the radio (start() or config() callback)
> > > or does it represent the key state in the hardware?
> > > 
> > > If it is something coming from mac80211, then you do not want
> > > to send a SOFT_BLOCKED event since that will cause all other radios
> > > to be switched off simply because the b43 interface has not been
> > > enabled.
> > > 
> > > Off course when it represents the key state in the hardware then the
> > > code would be fine...
> > 
> > The state comes from mac80211 and is set in the config() callback.
> > 
> > What state should be sent at the point when the hardware block is
> > removed? It seems to me that forcing an UNBLOCKED state gives the
> > wrong result. Perhaps RFKILL does need to have 4 states so that an
> > RFKILL_STATE_HW_UNBLOCKED state can be transmitted.
> 
> When the key is pressed to unblock the device, then simply send state
> RFKILL_STATE_UNBLOCKED to rfkill. That will generate a trigger to the
> driver to enable the radio,

The radio is enabled in _hardware_. There doesn't need to be any trigger.
It's already on. In fact, such a trigger (done through rfkill->toggle callback)
would deadlock wl->mutex with the current patch.

> and at that time you can check if mac80211 
> wants the radio on or not (aka you check dev->phy.radio_on or the mac80211
> config structure).

No!
Software and hardware rfkill states are completely different and _independent_
states on the b43 hardware. You set the software state from the mac80211 callback
no matter what hardware state the device is in. You just have to announce the
state correctly to the rfkill subsystem if mac80211 changed the software
state.

Do not change any software state from within the hardware state change handler.
This will blow up.

-- 
Greetings Michael.

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Michael Buesch wrote:
> On Thursday 18 September 2008 15:53:28 Ivo van Doorn wrote:
> > On Thursday 18 September 2008, Larry Finger wrote:
> > > Ivo van Doorn wrote:
> > > 
> > > > Is dev->phy.radio_on set when mac80211 has send an instruction
> > > > to the driver to enable the radio (start() or config() callback)
> > > > or does it represent the key state in the hardware?
> > > > 
> > > > If it is something coming from mac80211, then you do not want
> > > > to send a SOFT_BLOCKED event since that will cause all other radios
> > > > to be switched off simply because the b43 interface has not been
> > > > enabled.
> > > > 
> > > > Off course when it represents the key state in the hardware then the
> > > > code would be fine...
> > > 
> > > The state comes from mac80211 and is set in the config() callback.
> > > 
> > > What state should be sent at the point when the hardware block is
> > > removed? It seems to me that forcing an UNBLOCKED state gives the
> > > wrong result. Perhaps RFKILL does need to have 4 states so that an
> > > RFKILL_STATE_HW_UNBLOCKED state can be transmitted.
> > 
> > When the key is pressed to unblock the device, then simply send state
> > RFKILL_STATE_UNBLOCKED to rfkill. That will generate a trigger to the
> > driver to enable the radio,
> 
> The radio is enabled in _hardware_. There doesn't need to be any trigger.
> It's already on. In fact, such a trigger (done through rfkill->toggle callback)
> would deadlock wl->mutex with the current patch.

This depends on the hardware, for b43 the toggle() callback might not be needed,
but for rt2x00 it does (Since the key press will only raise a GPIO bit and does not
affect the radio in any way).

> > and at that time you can check if mac80211 
> > wants the radio on or not (aka you check dev->phy.radio_on or the mac80211
> > config structure).
> 
> No!
> Software and hardware rfkill states are completely different and _independent_
> states on the b43 hardware. You set the software state from the mac80211 callback
> no matter what hardware state the device is in. You just have to announce the
> state correctly to the rfkill subsystem if mac80211 changed the software
> state.

Well no actually, when the radio state (software rfkill state in your words) is changed,
it shouldn't be send to rfkill at all. rfkill should only be informed about the hardware
rfkill state changes.

> Do not change any software state from within the hardware state change handler.
> This will blow up.

When you use userspace tools this won't happen since the hardware state change handler
will send an uevent to userspace which might act upon that, and will eventually send an
instruction back to the hardware, but that goes through a different thread.

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Michael Buesch]

On Thursday 18 September 2008 19:48:27 Ivo van Doorn wrote:
> Well no actually, when the radio state (software rfkill state in your words)

No, "radio state" is _not_ "software rfkill state" in my words.
It's an independent state.
The actual physical radio state is a combined state of the two sw and hw state bits.
If either bit blocks the radio, it's physically blocked. We cannot toggle the hw bit
from software.

> it shouldn't be send to rfkill at all. rfkill should only be informed about the hardware
> rfkill state changes.

So that's fine. We just revert the patch that caused all the trouble and we will
gain _exactly_ that.

> > Do not change any software state from within the hardware state change handler.
> > This will blow up.
> 
> When you use userspace tools this won't happen since the hardware state change handler
> will send an uevent to userspace which might act upon that, and will eventually send an
> instruction back to the hardware, but that goes through a different thread.

It will semantically blow up. See the initial mail from Larry with the regression report.

-- 
Greetings Michael.

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Michael Buesch wrote:
> On Thursday 18 September 2008 19:48:27 Ivo van Doorn wrote:
> > Well no actually, when the radio state (software rfkill state in your words)
> 
> No, "radio state" is _not_ "software rfkill state" in my words.
> It's an independent state.
> The actual physical radio state is a combined state of the two sw and hw state bits.
> If either bit blocks the radio, it's physically blocked. We cannot toggle the hw bit
> from software.

Ah ok. In that case b43 should do:

send HW_BLOCK when the hardware rfkill state is set to block
send SOFT_BLOCK when the software rfkill state is set to block

But it shouldn't (and that change was the start of this discussion) send SOFT_BLOCK
when mac80211 disabled the radio.

> > it shouldn't be send to rfkill at all. rfkill should only be informed about the hardware
> > rfkill state changes.
> 
> So that's fine. We just revert the patch that caused all the trouble and we will
> gain _exactly_ that.
>
> > > Do not change any software state from within the hardware state change handler.
> > > This will blow up.
> > 
> > When you use userspace tools this won't happen since the hardware state change handler
> > will send an uevent to userspace which might act upon that, and will eventually send an
> > instruction back to the hardware, but that goes through a different thread.
> 
> It will semantically blow up. See the initial mail from Larry with the regression report.

Ok.

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Michael Buesch]

On Thursday 18 September 2008 20:10:35 Ivo van Doorn wrote:
> On Thursday 18 September 2008, Michael Buesch wrote:
> > On Thursday 18 September 2008 19:48:27 Ivo van Doorn wrote:
> > > Well no actually, when the radio state (software rfkill state in your words)
> > 
> > No, "radio state" is _not_ "software rfkill state" in my words.
> > It's an independent state.
> > The actual physical radio state is a combined state of the two sw and hw state bits.
> > If either bit blocks the radio, it's physically blocked. We cannot toggle the hw bit
> > from software.
> 
> Ah ok. In that case b43 should do:
> 
> send HW_BLOCK when the hardware rfkill state is set to block
> send SOFT_BLOCK when the software rfkill state is set to block
> 
> But it shouldn't (and that change was the start of this discussion) send SOFT_BLOCK
> when mac80211 disabled the radio.

I'm kind of confused. You say one thing and revert it right in the next sentence.

-- 
Greetings Michael.

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Michael Buesch wrote:
> On Thursday 18 September 2008 20:10:35 Ivo van Doorn wrote:
> > On Thursday 18 September 2008, Michael Buesch wrote:
> > > On Thursday 18 September 2008 19:48:27 Ivo van Doorn wrote:
> > > > Well no actually, when the radio state (software rfkill state in your words)
> > > 
> > > No, "radio state" is _not_ "software rfkill state" in my words.
> > > It's an independent state.
> > > The actual physical radio state is a combined state of the two sw and hw state bits.
> > > If either bit blocks the radio, it's physically blocked. We cannot toggle the hw bit
> > > from software.
> > 
> > Ah ok. In that case b43 should do:
> > 
> > send HW_BLOCK when the hardware rfkill state is set to block
> > send SOFT_BLOCK when the software rfkill state is set to block
> > 
> > But it shouldn't (and that change was the start of this discussion) send SOFT_BLOCK
> > when mac80211 disabled the radio.
> 
> I'm kind of confused. You say one thing and revert it right in the next sentence.

Ehm now you are confusing me.
You state that software rfkill state is not the state as requested by mac80211.

Since that is not the case b43 has 2 different methods to indicate the RFKILL state,
one is the hardware rfkill state and the second is the software rfkill state.
On top of that mac80211 sends configuration commands to b43 to set the radio state.

Configuration commands from mac80211 should _not_ trigger rfkill events since they
don't represent the RFKILL state but the RADIO state.

The RFKILL states as provided by the hardware do trigger rfkill state changes.

So apparently in b43 hardware you have 3 settings:
* radio enabled/disabled
* sw rfkill enabled/disabled
* hw rfkill enabled/disabled

The latter 2 provide RFKILL state changes when they are changed, but the first
one is the state as requested by mac80211 and thus should not generate events.

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Michael Buesch]

On Thursday 18 September 2008 20:23:21 Ivo van Doorn wrote:
> On Thursday 18 September 2008, Michael Buesch wrote:
> > On Thursday 18 September 2008 20:10:35 Ivo van Doorn wrote:
> > > On Thursday 18 September 2008, Michael Buesch wrote:
> > > > On Thursday 18 September 2008 19:48:27 Ivo van Doorn wrote:
> > > > > Well no actually, when the radio state (software rfkill state in your words)
> > > > 
> > > > No, "radio state" is _not_ "software rfkill state" in my words.
> > > > It's an independent state.
> > > > The actual physical radio state is a combined state of the two sw and hw state bits.
> > > > If either bit blocks the radio, it's physically blocked. We cannot toggle the hw bit
> > > > from software.
> > > 
> > > Ah ok. In that case b43 should do:
> > > 
> > > send HW_BLOCK when the hardware rfkill state is set to block
> > > send SOFT_BLOCK when the software rfkill state is set to block
> > > 
> > > But it shouldn't (and that change was the start of this discussion) send SOFT_BLOCK
> > > when mac80211 disabled the radio.
> > 
> > I'm kind of confused. You say one thing and revert it right in the next sentence.
> 
> Ehm now you are confusing me.
> You state that software rfkill state is not the state as requested by mac80211.


Nono. I will try to explain it once again. It's really easy.

Think of b43 having two independent registers for rfkill. (the actual hardware
is different, but that doesn't matter. It's a driver internal implementation detail).
One of these registers is readonly and it does indicate the physical rfkill button state.
If that register indicates BLOCK, we can't do anything about it. The radio is blocked.
However, we can still write to the second register and turn the radio off through
that, too. We can also write to the second register to turn the radio on, _but_ it won't
physically turn on until the physical button is pressed and the first register changes
to Unblocked.
So you say that rfkill should only be notified about changes in the first read-only
hardware intication bit? If that's the case, it would be possible to turn the radio off
through mac80211 calls (using the read-write register), but still have rfkill think it's
unblocked.

-- 
Greetings Michael.

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Johannes Berg]

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

On Thu, 2008-09-18 at 20:34 +0200, Michael Buesch wrote:

> So you say that rfkill should only be notified about changes in the first read-only
> hardware intication bit? If that's the case, it would be possible to turn the radio off
> through mac80211 calls (using the read-write register), but still have rfkill think it's
> unblocked.

This is why I'm saying it needs to be integrated with mac80211. Anything
else is stupid because it would force drivers to keep track of all three
states.

johannes

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

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Michael Buesch wrote:
> Nono. I will try to explain it once again. It's really easy.

It is also a textbook design for rfkill input lines, and handled in the
obvious way by rfkill.  If it is not, I don't get what Ivo means either.

> Think of b43 having two independent registers for rfkill. (the actual hardware
> is different, but that doesn't matter. It's a driver internal implementation detail).

> One of these registers is readonly and it does indicate the physical rfkill button state.
> If that register indicates BLOCK, we can't do anything about it. The radio is blocked.
> However, we can still write to the second register and turn the radio off through
> that, too. We can also write to the second register to turn the radio on, _but_ it won't
> physically turn on until the physical button is pressed and the first register changes
> to Unblocked.

Then you use the standard function I posted elsewhere in this thread:

if (any_of_the_hw_rfkill_lines_is_active_RIGHT_NOW)
	return RFKILL_STATE_HARD_BLOCKED;
else if (any_of_the_sw_rfkill_lines_is_active_RIGHT_NOW)
	return RFKILL_STATE_SOFT_BLOCKED;
else
	return RFKILL_STATE_UNBLOCKED;

You can use that as your rfkill->get_state() if you need it, and it is what
you should send to rfkill_force_state when
"any_of_the_hw_rfkill_lines_is_active_RIGHT_NOW" or
"any_of_the_sw_rfkill_lines_is_active_RIGHT_NOW" could have changed.

The devil is in the details of how you get
any_of_the_hw_rfkill_lines_is_active_RIGHT_NOW, and
any_of_the_sw_rfkill_lines_is_active_RIGHT_NOW.

You have to make sure "set txpower off" and such non-rfkill stuff doesn't
interfere with them, and Ivo will be happy :-)   You should also query the
hardware to make sure nothing poked it while you were looking elsewhere, but
that is impossible if the hardware is write-only.

I posted a sample code fragment to do that too, for drivers that implement
txpower off and rfkill SOFT_BLOCKED through the same hardware register.

> So you say that rfkill should only be notified about changes in the first read-only
> hardware intication bit? If that's the case, it would be possible to turn the radio off

Well, it only needs to be notified of changes due to the SW bit, if they can
happen because of external factors (such as laptop firmware).

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Michael Buesch wrote:
> On Thursday 18 September 2008 20:23:21 Ivo van Doorn wrote:
> > On Thursday 18 September 2008, Michael Buesch wrote:
> > > On Thursday 18 September 2008 20:10:35 Ivo van Doorn wrote:
> > > > On Thursday 18 September 2008, Michael Buesch wrote:
> > > > > On Thursday 18 September 2008 19:48:27 Ivo van Doorn wrote:
> > > > > > Well no actually, when the radio state (software rfkill state in your words)
> > > > > 
> > > > > No, "radio state" is _not_ "software rfkill state" in my words.
> > > > > It's an independent state.
> > > > > The actual physical radio state is a combined state of the two sw and hw state bits.
> > > > > If either bit blocks the radio, it's physically blocked. We cannot toggle the hw bit
> > > > > from software.
> > > > 
> > > > Ah ok. In that case b43 should do:
> > > > 
> > > > send HW_BLOCK when the hardware rfkill state is set to block
> > > > send SOFT_BLOCK when the software rfkill state is set to block
> > > > 
> > > > But it shouldn't (and that change was the start of this discussion) send SOFT_BLOCK
> > > > when mac80211 disabled the radio.
> > > 
> > > I'm kind of confused. You say one thing and revert it right in the next sentence.
> > 
> > Ehm now you are confusing me.
> > You state that software rfkill state is not the state as requested by mac80211.
> 
> 
> Nono. I will try to explain it once again. It's really easy.
> 
> Think of b43 having two independent registers for rfkill. (the actual hardware
> is different, but that doesn't matter. It's a driver internal implementation detail).
> One of these registers is readonly and it does indicate the physical rfkill button state.
> If that register indicates BLOCK, we can't do anything about it. The radio is blocked.

Which qualifies for a HW_BLOCK signal

> However, we can still write to the second register and turn the radio off through
> that, too. We can also write to the second register to turn the radio on, _but_ it won't
> physically turn on until the physical button is pressed and the first register changes
> to Unblocked.

Since the hardware doesn't toggle this field when a key is pressed, it is more the state
of the radio as configured by mac80211. And thus shouldn't generate RFKILL events.

> So you say that rfkill should only be notified about changes in the first read-only
> hardware intication bit? If that's the case, it would be possible to turn the radio off
> through mac80211 calls (using the read-write register), but still have rfkill think it's
> unblocked.

Right. Because how I see the rfkill layer is that it doesn't represent the RADIO state
but the RFKILL state (and uses that to control the RADIO state).

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> This depends on the hardware, for b43 the toggle() callback might not be needed,
> but for rt2x00 it does (Since the key press will only raise a GPIO bit and does not
> affect the radio in any way).

Hmm... please correct me if I misunderstood, but wouldn't that mean that
rt2x00 does not have a hardware rfkill line at all, and that instead it has
a GPIO pin that is used to communicate the desire to software-rfkill the
transmitter...  and the driver needs to do everything.

If the above is wrong, don't bother with the rest of this email.

If the above is correct, you'd not have HARD_BLOCKED at all in rt2x00, just
SOFT_BLOCK.  Unless you somehow emulated HARD_BLOCKED.

Without a real hardware rfkill line, the only way to have HARD_BLOCKED is
through software emulation inside the driver, but that is only safe if you
are absolutely SURE no firmware or other crap will poke at the hardware
behind your back.

And if you don't emulate HARD_BLOCKED, you have to handle the input device
inside the rt2x00 driver, because the world outside (and that includes the
rfkill core and everything else) sure as heck won't know if you SOFT_BLOCKED
because of that GPIO pin, or because of something else.

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > This depends on the hardware, for b43 the toggle() callback might not be needed,
> > but for rt2x00 it does (Since the key press will only raise a GPIO bit and does not
> > affect the radio in any way).
> 
> Hmm... please correct me if I misunderstood, but wouldn't that mean that
> rt2x00 does not have a hardware rfkill line at all, and that instead it has
> a GPIO pin that is used to communicate the desire to software-rfkill the
> transmitter...  and the driver needs to do everything.

That is correct.

> If the above is wrong, don't bother with the rest of this email.
> 
> If the above is correct, you'd not have HARD_BLOCKED at all in rt2x00, just
> SOFT_BLOCK.  Unless you somehow emulated HARD_BLOCKED.

True, and that is why rt2x00 is generating the SOFT_BLOCK. :)

> Without a real hardware rfkill line, the only way to have HARD_BLOCKED is
> through software emulation inside the driver, but that is only safe if you
> are absolutely SURE no firmware or other crap will poke at the hardware
> behind your back.

I am sure of that.

> And if you don't emulate HARD_BLOCKED, you have to handle the input device
> inside the rt2x00 driver, because the world outside (and that includes the
> rfkill core and everything else) sure as heck won't know if you SOFT_BLOCKED
> because of that GPIO pin, or because of something else.

Currently rt2x00 polls the GPIO pin every second (no interrupts are raised when the
GPIO pin is toggled) and uses rfkill_force_state(SOFT_BLOCK/UNBLOCK) to rfkill

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> > On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > > This depends on the hardware, for b43 the toggle() callback might not be needed,
> > > but for rt2x00 it does (Since the key press will only raise a GPIO bit and does not
> > > affect the radio in any way).
> > 
> > Hmm... please correct me if I misunderstood, but wouldn't that mean that
> > rt2x00 does not have a hardware rfkill line at all, and that instead it has
> > a GPIO pin that is used to communicate the desire to software-rfkill the
> > transmitter...  and the driver needs to do everything.
> 
> That is correct.

Yeah, thanks <deity> I noticed this post :-)  That will make it MUCH easier
for me to sync with you and actually grok what you are saying.

> > And if you don't emulate HARD_BLOCKED, you have to handle the input device
> > inside the rt2x00 driver, because the world outside (and that includes the
> > rfkill core and everything else) sure as heck won't know if you SOFT_BLOCKED
> > because of that GPIO pin, or because of something else.
> 
> Currently rt2x00 polls the GPIO pin every second (no interrupts are raised when the
> GPIO pin is toggled) and uses rfkill_force_state(SOFT_BLOCK/UNBLOCK) to rfkill

I just sent a post about it.  If you get a bit that tells you HW RFKILL
ACTIVE/INACTIVE (as opposed to "please toggle it"), you don't need to care
about whatever else is reading that thing.  You can always register an input
device, and issue EV_SW SW_WLAN or EV_SW SW_RFKILL_ALL (I'd sugest making it
configurable).  But do NOT issue EV_KEY, THAT one breaks if anything else is
also listening to that input signal.

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> > > On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > > > This depends on the hardware, for b43 the toggle() callback might not be needed,
> > > > but for rt2x00 it does (Since the key press will only raise a GPIO bit and does not
> > > > affect the radio in any way).
> > > 
> > > Hmm... please correct me if I misunderstood, but wouldn't that mean that
> > > rt2x00 does not have a hardware rfkill line at all, and that instead it has
> > > a GPIO pin that is used to communicate the desire to software-rfkill the
> > > transmitter...  and the driver needs to do everything.
> > 
> > That is correct.
> 
> Yeah, thanks <deity> I noticed this post :-)  That will make it MUCH easier
> for me to sync with you and actually grok what you are saying.

Hehe :)

> > > And if you don't emulate HARD_BLOCKED, you have to handle the input device
> > > inside the rt2x00 driver, because the world outside (and that includes the
> > > rfkill core and everything else) sure as heck won't know if you SOFT_BLOCKED
> > > because of that GPIO pin, or because of something else.
> > 
> > Currently rt2x00 polls the GPIO pin every second (no interrupts are raised when the
> > GPIO pin is toggled) and uses rfkill_force_state(SOFT_BLOCK/UNBLOCK) to rfkill
> 
> I just sent a post about it.  If you get a bit that tells you HW RFKILL
> ACTIVE/INACTIVE (as opposed to "please toggle it"), you don't need to care
> about whatever else is reading that thing.  You can always register an input
> device, and issue EV_SW SW_WLAN or EV_SW SW_RFKILL_ALL (I'd sugest making it
> configurable).  But do NOT issue EV_KEY, THAT one breaks if anything else is
> also listening to that input signal.

Ok.

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Michael Buesch]

On Thursday 18 September 2008 15:47:42 Larry Finger wrote:
> Ivo van Doorn wrote:
> 
> > Is dev->phy.radio_on set when mac80211 has send an instruction
> > to the driver to enable the radio (start() or config() callback)
> > or does it represent the key state in the hardware?
> > 
> > If it is something coming from mac80211, then you do not want
> > to send a SOFT_BLOCKED event since that will cause all other radios
> > to be switched off simply because the b43 interface has not been
> > enabled.
> > 
> > Off course when it represents the key state in the hardware then the
> > code would be fine...
> 
> The state comes from mac80211 and is set in the config() callback.
> 
> What state should be sent at the point when the hardware block is
> removed? It seems to me that forcing an UNBLOCKED state gives the
> wrong result. Perhaps RFKILL does need to have 4 states so that an
> RFKILL_STATE_HW_UNBLOCKED state can be transmitted.

If sw is unblocked, but hw is still blocked, you must not announce
unblocked state to rfkill.


-- 
Greetings Michael.

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Michael Buesch wrote:
> On Thursday 18 September 2008 15:47:42 Larry Finger wrote:
> > Ivo van Doorn wrote:
> > 
> > > Is dev->phy.radio_on set when mac80211 has send an instruction
> > > to the driver to enable the radio (start() or config() callback)
> > > or does it represent the key state in the hardware?
> > > 
> > > If it is something coming from mac80211, then you do not want
> > > to send a SOFT_BLOCKED event since that will cause all other radios
> > > to be switched off simply because the b43 interface has not been
> > > enabled.
> > > 
> > > Off course when it represents the key state in the hardware then the
> > > code would be fine...
> > 
> > The state comes from mac80211 and is set in the config() callback.
> > 
> > What state should be sent at the point when the hardware block is
> > removed? It seems to me that forcing an UNBLOCKED state gives the
> > wrong result. Perhaps RFKILL does need to have 4 states so that an
> > RFKILL_STATE_HW_UNBLOCKED state can be transmitted.
> 
> If sw is unblocked, but hw is still blocked, you must not announce
> unblocked state to rfkill.

Well from my perspective:
Note that 'sw' is the RADIO state as requested by mac80211 and
'hw' is the RFKILL state as indicated by the hardware

radio: block, rfkill: block => BLOCK
radio: block, rfkill: unblock => UNBLOCK
radio: unblock, rfkill: block => BLOCK
radio: unblock, rfkill: unblock => UNBLOCK

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Johannes Berg]

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

On Thu, 2008-09-18 at 19:42 +0200, Ivo van Doorn wrote:

> Well from my perspective:
> Note that 'sw' is the RADIO state as requested by mac80211 and
> 'hw' is the RFKILL state as indicated by the hardware
> 
> radio: block, rfkill: block => BLOCK
> radio: block, rfkill: unblock => UNBLOCK
> radio: unblock, rfkill: block => BLOCK
> radio: unblock, rfkill: unblock => UNBLOCK

Can't that crap just live in rfkill?

rfkill_announce_softstate(OPEN/BLOCKED)
rfkill_announce_hardstate(OPEN/BLOCKED)

and all the other junk happens there. And make sure this can be called
from interrupts.

johannes

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

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Johannes Berg wrote:
> On Thu, 2008-09-18 at 19:42 +0200, Ivo van Doorn wrote:
> 
> > Well from my perspective:
> > Note that 'sw' is the RADIO state as requested by mac80211 and
> > 'hw' is the RFKILL state as indicated by the hardware
> > 
> > radio: block, rfkill: block => BLOCK
> > radio: block, rfkill: unblock => UNBLOCK
> > radio: unblock, rfkill: block => BLOCK
> > radio: unblock, rfkill: unblock => UNBLOCK
> 
> Can't that crap just live in rfkill?

Rfkill is intended to listen for RFKILL state changes and report
those to userspace. _Optionally_ it can handle it internally through
rfkill-input which will call toggle() to change the radio state.

It is not intended as the library through which all radio state changes
should go through. Because then we must change mac80211 to not
send the enable_radio command to the drivers, but should directly send
it to rfkill.

> rfkill_announce_softstate(OPEN/BLOCKED)
> rfkill_announce_hardstate(OPEN/BLOCKED)
> 
> and all the other junk happens there. And make sure this can be called
> from interrupts.

I think that limitation came from the notifier_block functions, but
Henrique knows more about those details.

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > rfkill_announce_softstate(OPEN/BLOCKED)
> > rfkill_announce_hardstate(OPEN/BLOCKED)

We could, but it wouldn't help much.  WHEN you are allowed to call these
(well, mostly rfkill_announce_softstat) is the real problem.

And if you are forced to give both soft and hard states at once when you
update the rfkill core state, you guard yourself against stuff messing with
the rfkill state of your hardware behind your driver's back.

> > and all the other junk happens there. And make sure this can be called
> > from interrupts.
> 
> I think that limitation came from the notifier_block functions, but
> Henrique knows more about those details.

No, it would not be that trivial, but it would be doable.   Were that the
only problem, I would have done that already.

The problem is in the locking of rfkill->state, that is, rfkill->mutex.

Changing rfkill->mutex to a spinlock would work, but the issue here is how
much time rfkill->mutex spends locked.  It can stay locked for a while, the
longest path in the rfkill core includes a call to rfkill->get_state() plus
a call to rfkill->toggle_radio().

And we have no idea what get_state() and toggle_radio() might need to do, or
how much time would they take to do it.

If someone could help with it, it would be really nice if we could somehow
make unlocked access to rfkill->state safe...

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Michael Buesch]

On Thursday 18 September 2008 19:42:28 Ivo van Doorn wrote:
> On Thursday 18 September 2008, Michael Buesch wrote:
> > On Thursday 18 September 2008 15:47:42 Larry Finger wrote:
> > > Ivo van Doorn wrote:
> > > 
> > > > Is dev->phy.radio_on set when mac80211 has send an instruction
> > > > to the driver to enable the radio (start() or config() callback)
> > > > or does it represent the key state in the hardware?
> > > > 
> > > > If it is something coming from mac80211, then you do not want
> > > > to send a SOFT_BLOCKED event since that will cause all other radios
> > > > to be switched off simply because the b43 interface has not been
> > > > enabled.
> > > > 
> > > > Off course when it represents the key state in the hardware then the
> > > > code would be fine...
> > > 
> > > The state comes from mac80211 and is set in the config() callback.
> > > 
> > > What state should be sent at the point when the hardware block is
> > > removed? It seems to me that forcing an UNBLOCKED state gives the
> > > wrong result. Perhaps RFKILL does need to have 4 states so that an
> > > RFKILL_STATE_HW_UNBLOCKED state can be transmitted.
> > 
> > If sw is unblocked, but hw is still blocked, you must not announce
> > unblocked state to rfkill.
> 
> Well from my perspective:
> Note that 'sw' is the RADIO state as requested by mac80211 and
> 'hw' is the RFKILL state as indicated by the hardware
> 
> radio: block, rfkill: block => BLOCK
> radio: block, rfkill: unblock => UNBLOCK

If the radio is physically blocked by the hardware rfkill switch,
why on earth would we want to announce unblocked state?
Or did I get your table wrong?

(Still note that software and hardware states are _independent_ in b43.
Think of it as two independent bits that can each turn the radio off, where
the hardware state bit is readonly and is triggered by the hw-button).

-- 
Greetings Michael.

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Michael Buesch wrote:
> On Thursday 18 September 2008 19:42:28 Ivo van Doorn wrote:
> > On Thursday 18 September 2008, Michael Buesch wrote:
> > > On Thursday 18 September 2008 15:47:42 Larry Finger wrote:
> > > > Ivo van Doorn wrote:
> > > > 
> > > > > Is dev->phy.radio_on set when mac80211 has send an instruction
> > > > > to the driver to enable the radio (start() or config() callback)
> > > > > or does it represent the key state in the hardware?
> > > > > 
> > > > > If it is something coming from mac80211, then you do not want
> > > > > to send a SOFT_BLOCKED event since that will cause all other radios
> > > > > to be switched off simply because the b43 interface has not been
> > > > > enabled.
> > > > > 
> > > > > Off course when it represents the key state in the hardware then the
> > > > > code would be fine...
> > > > 
> > > > The state comes from mac80211 and is set in the config() callback.
> > > > 
> > > > What state should be sent at the point when the hardware block is
> > > > removed? It seems to me that forcing an UNBLOCKED state gives the
> > > > wrong result. Perhaps RFKILL does need to have 4 states so that an
> > > > RFKILL_STATE_HW_UNBLOCKED state can be transmitted.
> > > 
> > > If sw is unblocked, but hw is still blocked, you must not announce
> > > unblocked state to rfkill.
> > 
> > Well from my perspective:
> > Note that 'sw' is the RADIO state as requested by mac80211 and
> > 'hw' is the RFKILL state as indicated by the hardware
> > 
> > radio: block, rfkill: block => BLOCK
> > radio: block, rfkill: unblock => UNBLOCK
> 
> If the radio is physically blocked by the hardware rfkill switch,
> why on earth would we want to announce unblocked state?
> Or did I get your table wrong?

You get the table wrong.
When the user presses the RFKILL key, the RFKILL state is set to unblock.
When the user does 'ifconfig wlan0 up' or 'iwconfig wlan0 txpower on'
then the RADIO state is set to unblock.

>From your mail I understood that your interpretation of the SW RFKILL
was "radio state as requested by mac80211" and HW RFKILL was
"radio state as requested by RFKILL register in b43".

> (Still note that software and hardware states are _independent_ in b43.
> Think of it as two independent bits that can each turn the radio off, where
> the hardware state bit is readonly and is triggered by the hw-button).

I am, and this is what I am trying to preserve in rfkill. That it _does_
differentiate between the 2 states.

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> If it is something coming from mac80211, then you do not want
> to send a SOFT_BLOCKED event since that will cause all other radios
> to be switched off simply because the b43 interface has not been
> enabled.

Drivers ARE supposed to be able to set their radio state to their heart's
content, without messing with any other devices.  There are no constraints
to calls to rfkill_force_state(), other than the current issue that it must
not be done from an atomic context.

OTOH, rfkill_force_state() does NOT change anything in the radio, so you
must always call it to match the REAL state of the hardware.

What CAN mess with other radios are INPUT EVENTS.  Which b43 has no business
generating by itself by default, since it is used both on platforms where it
is just a wireless device, and also on platforms where its hardware rfkill
input line is THE only way to know the state of THE hardware slider RFKILL
switch.

And, frankly, it shouldn't be the job of b43 to care to know what platform
is it in.

Now, with Larry's patch, b43 has no input device mess dragging it down
anymore.  So, any platforms that need input events from b43 are to generate
them in userspace, or through a separate kernel module.   We already have
all the APIs needed for that, and they work.

This *DOES* mean the fixed b43 will be noticed by userspace, since it won't
issue KEY_WLAN anymore.   However, that has been broken in rfkill (the
input-polldev stuff) and b43 (use of EV_KEY instead of EV_SW) since day one
anyway, so I personally can't see that as anything but a required bug fix
that userspace would have to adapt to.

If this is much too confusing, I am all ears on how we could improve it.

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > If it is something coming from mac80211, then you do not want
> > to send a SOFT_BLOCKED event since that will cause all other radios
> > to be switched off simply because the b43 interface has not been
> > enabled.
> 
> Drivers ARE supposed to be able to set their radio state to their heart's
> content, without messing with any other devices.  There are no constraints
> to calls to rfkill_force_state(), other than the current issue that it must
> not be done from an atomic context.

My main point was that when the radio is not enabled because the user
did something like "iwconfig wlan0 txpower off" then this is not an rfkill
SOFT_BLOCKED event. Since that command has nothing to do with the
entire rfkill layer.

When you consider such commands as rfkill events you get wrong behavior
because it would trigger a SOFT_BLOCK in rfkill which will be send to all
registered drivers who can disable their radio off as well. And that is
definately not what you want...

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> > On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > > If it is something coming from mac80211, then you do not want
> > > to send a SOFT_BLOCKED event since that will cause all other radios
> > > to be switched off simply because the b43 interface has not been
> > > enabled.
> > 
> > Drivers ARE supposed to be able to set their radio state to their heart's
> > content, without messing with any other devices.  There are no constraints
> > to calls to rfkill_force_state(), other than the current issue that it must
> > not be done from an atomic context.
> 
> My main point was that when the radio is not enabled because the user
> did something like "iwconfig wlan0 txpower off" then this is not an rfkill
> SOFT_BLOCKED event. Since that command has nothing to do with the
> entire rfkill layer.

We had some threads about it, and I don't recall any real conclusions if we
should have "tx power off" be treated the same way as a software rfkill
block request would, or not.  I believe it ended up as "do whatever you want
in your driver" (i.e. no real conclusion).

So, the rfkill core is not even aware of any tx power changes, unless the
wireless device drivers decides to make it so by actively feeding it new
states through rfkill_force_state() when the tx power changes from off to
something else, and from something else to off.

rfkill will work right whether you do it or not...

> When you consider such commands as rfkill events you get wrong behavior
> because it would trigger a SOFT_BLOCK in rfkill which will be send to all
> registered drivers who can disable their radio off as well. And that is
> definately not what you want...

...because this is incorrect.

No transmitter-specific rfkill events cause this, at least as far as the
rfkill core is concerned.  They are sent to userspace and the notify chain,
yes.  And either userspace or another kernel module might, if it is feeling
wrong on the head, change that event into a request to mess with all radios.

But right now, none will.  rfkill-input only cares about INPUT EVENTS, but
the rfkill core never *issues* any input events, ever.

Unless b43 is still issuing INPUT EVENTS even after the patch?  It
shouldn't.

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> > > On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > > > If it is something coming from mac80211, then you do not want
> > > > to send a SOFT_BLOCKED event since that will cause all other radios
> > > > to be switched off simply because the b43 interface has not been
> > > > enabled.
> > > 
> > > Drivers ARE supposed to be able to set their radio state to their heart's
> > > content, without messing with any other devices.  There are no constraints
> > > to calls to rfkill_force_state(), other than the current issue that it must
> > > not be done from an atomic context.
> > 
> > My main point was that when the radio is not enabled because the user
> > did something like "iwconfig wlan0 txpower off" then this is not an rfkill
> > SOFT_BLOCKED event. Since that command has nothing to do with the
> > entire rfkill layer.
> 
> We had some threads about it, and I don't recall any real conclusions if we
> should have "tx power off" be treated the same way as a software rfkill
> block request would, or not.  I believe it ended up as "do whatever you want
> in your driver" (i.e. no real conclusion).
> 
> So, the rfkill core is not even aware of any tx power changes, unless the
> wireless device drivers decides to make it so by actively feeding it new
> states through rfkill_force_state() when the tx power changes from off to
> something else, and from something else to off.
> 
> rfkill will work right whether you do it or not...

Well as far as I am concerned, userspace configuration actions are not rfkill events
and thus should rfkill not raise them as such. rfkill events are triggered by
buttons/keys/switches/sliders whatever the manufacturer considered as a nice idea
to control the wireless device.

Abusing rfkill to make userspace commands like "iwconfig txpower on/off" configurable
by triggering rfkill events is just plain wrong and will mess up userspace tools/notifier chain
listeners or whatever other tool is listening to rfkill.

The rfkill layer was added for a specific reason:
	Control the wireless radios when the rfkill buttons/keys/switches/sliders was pressed.
This was needed for areas where no RF radios are allowed (i.e. airplanes).

Apparently we are now changing it to a global library where is displays the state of
all radios grouped by type, and are basically providing a new interface for the user to
where all configuration changes done in userspace result into new events reported to
userspace.

When the userspace tool listens to rfkill events it is not, or most accurate, should not
be interested in the events from userspace (which it probably caused itself)
but from the events coming from the hardware.

If the user has userspace tools which listen to rfkill events, and the user performs
a "iwconfig wlan0 txpower off" he does not want the userspace tool to listen to
the event and raises a "hey somebody killed a radio, lets kill all other radios as well"

Or should the user now first disable his rfkill-listener daemon before applying
configuration actions to his wireless interface just to prevent events from occuring?

> > When you consider such commands as rfkill events you get wrong behavior
> > because it would trigger a SOFT_BLOCK in rfkill which will be send to all
> > registered drivers who can disable their radio off as well. And that is
> > definately not what you want...
> 
> ...because this is incorrect.
> 
> No transmitter-specific rfkill events cause this, at least as far as the
> rfkill core is concerned.  They are sent to userspace and the notify chain,
> yes.  And either userspace or another kernel module might, if it is feeling
> wrong on the head, change that event into a request to mess with all radios.
> 
> But right now, none will.  rfkill-input only cares about INPUT EVENTS, but
> the rfkill core never *issues* any input events, ever.

True, but I am not talking about rfkill-input alone.
I am talking about whatever rfkill event listener, and that can be either rfkill-input
notify chain listener or a uevent listener.
They are all allowed to perform actions based on the rfkill events.
 
> Unless b43 is still issuing INPUT EVENTS even after the patch?  It
> shouldn't.

That part is being removed with this patch.

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> > We had some threads about it, and I don't recall any real conclusions if we
> > should have "tx power off" be treated the same way as a software rfkill
> > block request would, or not.  I believe it ended up as "do whatever you want
> > in your driver" (i.e. no real conclusion).
> > 
> > So, the rfkill core is not even aware of any tx power changes, unless the
> > wireless device drivers decides to make it so by actively feeding it new
> > states through rfkill_force_state() when the tx power changes from off to
> > something else, and from something else to off.
> > 
> > rfkill will work right whether you do it or not...
> 
> Well as far as I am concerned, userspace configuration actions are not rfkill events
> and thus should rfkill not raise them as such. rfkill events are triggered by
> buttons/keys/switches/sliders whatever the manufacturer considered as a nice idea
> to control the wireless device.

Actually, rfkill-related INPUT events are triggered by buttons/keys
/switches/sliders.  rfkill events are sent over the uevent system and the
rfkill notifier chain, triggered by rfkill controllers changing state (and
they exist for on-screen-display support and such stuff).

I find it best to make real sure we are talking about INPUT events or
non-input events in any rfkill context, because they really are very
different for the rfkill core.

> Abusing rfkill to make userspace commands like "iwconfig txpower on/off" configurable
> by triggering rfkill events is just plain wrong and will mess up userspace tools/notifier chain
> listeners or whatever other tool is listening to rfkill.

I really don't care either way.  Currently the rfkill core will just work no
matter what is done re. tx power on/off, and I really don't know which way
would be better for the user.

I also have no idea if something in userspace will be confused to see an
rfkill event telling it a radio entered SOFT BLOCK when it issues an txpower
off.  If they do, they're hosed anyway, because the radio could have entered
the SOFT_BLOCK state for any other reason in that time window...

However, I still think it *is* a lot more complex for drivers that *happen
to already implement tx power off using the hardware support for software
rfkill lines* to keep the two things separate.  I have no idea if any
drivers do so.  The rfkill core can deal with it either way, as long as it
is properly informed of any relevant rfkill state changes.

> The rfkill layer was added for a specific reason:
> 	Control the wireless radios when the rfkill buttons/keys/switches/sliders was pressed.
> This was needed for areas where no RF radios are allowed (i.e. airplanes).

Yeah, but in order to do that properly, we had to make it a complete
read/write, push and pull interface to the rfkill controllers (because of
pesky firmware that changes it behind your back, etc) and do a clean,
complete separation from rfkill state propagation (rfkill uevents, notify
chain, rfkill->get_state/rfkill_force_state API) and rfkill command
propagation (input events).

That's done and finished, as far as I am concerned.  Maybe some enhancements
like make it poll()'able or somesuch might happen, but that's it as far as I
am concerned.

> Apparently we are now changing it to a global library where is displays the state of
> all radios grouped by type, and are basically providing a new interface for the user to
> where all configuration changes done in userspace result into new events reported to
> userspace.

Hmm?  I am working on the addition of enough userspace interface to let
rfkill-input be implemented in userspace, nothing more.   I can certainly
drop that work if you want me to, but I had the impression that you liked
the idea.

As for the "all configuration changes done in userspace result into new
events reported to userspace", well, I already explained the bit about set
txpower off above.  If you mean something else that I did or said, please
explain.

> If the user has userspace tools which listen to rfkill events, and the user performs
> a "iwconfig wlan0 txpower off" he does not want the userspace tool to listen to
> the event and raises a "hey somebody killed a radio, lets kill all other radios as well"

If any tool is doing something that braindamaged, get rid of the tool.

> Or should the user now first disable his rfkill-listener daemon before applying
> configuration actions to his wireless interface just to prevent events from occuring?

No rfkill-listener daemon should be ever doing something as retarded as
shutting down all other radios because a single radio was shutdown in a
general way.

The only scenario I know of that might need something *CLOSE* to that is
this one (which is part of the design guidelines of the rfkill core to
support):

1. The code has detected that it is running on a specifc platform where the
EV_SW SW_RFKILL_ALL button is tied *directly* to, say, b43 and you cannot
read that state from anywhere else.

   * Note that this is for specific platforms, and not the general case.

2. The code will then listen to b43 rfkill uevents, and ONLY FOR TRANSITIONS
THAT INVOLVE ENTERING OR EXITING RFKILL_STATE_HARD_BLOCKED, it will generate
EV_SW SW_RFKILL_ALL events.  Alternatively, you can use EV_SW SW_WLAN if you
want it to be type-based, etc.

   * Note that only changes involving a hardware rfkill line are of concern,
     simple changes between UNBLOCKED and SOFT_BLOCKED are ignored.

3. Something else (be it rfkill-input or some userspace thing) traps that
EV_SW *input* event, and orders rfkill to change the state of a set of
radios (all of them for SW_RFKILL_ALL, just WLAN for SW_WLAN, etc).

   * Note that the proper layer separation is kept, so input events are
     reacted to the same way, no matter where they were generated.

That would certainly ignore any rfkill state changes done through software,
no matter the source of the change (firmware, userland poking at sysfs,
rfkill-input acting upon a KEY_WLAN event, etc), and regardless of the
wireless driver separating set txtpower off from soft-blocking or not.  So
it would't be toggling the state of all (or all WLAN,etc) radios
erratically.

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> > > We had some threads about it, and I don't recall any real conclusions if we
> > > should have "tx power off" be treated the same way as a software rfkill
> > > block request would, or not.  I believe it ended up as "do whatever you want
> > > in your driver" (i.e. no real conclusion).
> > > 
> > > So, the rfkill core is not even aware of any tx power changes, unless the
> > > wireless device drivers decides to make it so by actively feeding it new
> > > states through rfkill_force_state() when the tx power changes from off to
> > > something else, and from something else to off.
> > > 
> > > rfkill will work right whether you do it or not...
> > 
> > Well as far as I am concerned, userspace configuration actions are not rfkill events
> > and thus should rfkill not raise them as such. rfkill events are triggered by
> > buttons/keys/switches/sliders whatever the manufacturer considered as a nice idea
> > to control the wireless device.
> 
> Actually, rfkill-related INPUT events are triggered by buttons/keys
> /switches/sliders.  rfkill events are sent over the uevent system and the
> rfkill notifier chain, triggered by rfkill controllers changing state (and
> they exist for on-screen-display support and such stuff).

Unfortunately this is no longer completely the case since you qualified
several drivers as "no valid input device" and those are not using the input devices
anymore. 

Among those drivers are rt2x00, and I believe b43 as well (hence the patch send by Larry
to remove the input device).

Those drivers completely rely on rfkill_force_state() to send the RFKILL events
to userspace.

> I find it best to make real sure we are talking about INPUT events or
> non-input events in any rfkill context, because they really are very
> different for the rfkill core.

True, and that is where the problems are now coming from...

> > Abusing rfkill to make userspace commands like "iwconfig txpower on/off" configurable
> > by triggering rfkill events is just plain wrong and will mess up userspace tools/notifier chain
> > listeners or whatever other tool is listening to rfkill.
> 
> I really don't care either way.  Currently the rfkill core will just work no
> matter what is done re. tx power on/off, and I really don't know which way
> would be better for the user.

Rfkill is supposed to listen to the RFKILL status in the hardware and not the RADIO status,
at the same time it is used to control the RADIO status (and not the RFKILL status).
This means that having a radio enabled is completely different then having the RFKILL blocked

For rt2x00 the following is possible:
 * Radio enabled
 * RFKILL status set to blocked

The device will happily send and receive data while the RFKILL status is set to disabled,
hence it needs rfkill to disable the radio. And thus it sends SOFT_BLOCKED to rfkill which
is the valid status to indicate the RFKILL status is set to blocked and thus the RADIO should
be disabled.

What your new suggestion is that there is no difference between:
 * Pressing the RFKILL key on the laptop
 * Running "iwconfig wlan0 txpower off"

While both have a different meaning and should be threated differently. And since this is
the RFKILL layer, we should only concern ourselves with the RFKILL status and RFKILL events
and not userspace configuration.

The only possible userspace configuration which matters for RFKILL is that the SOFT_BLOCK
can be lifted (while the HW_BLOCK cannot).

After that it is up to rfkill-input (which use was killed because some drivers were no longer valid
input devices), or any userspace tool to listen to the events and perform the right action:
Which is killing all wireless radios because we most likely entered an area where Wireless Radios
are not allowed.

> I also have no idea if something in userspace will be confused to see an
> rfkill event telling it a radio entered SOFT BLOCK when it issues an txpower
> off.  If they do, they're hosed anyway, because the radio could have entered
> the SOFT_BLOCK state for any other reason in that time window...
> 
> However, I still think it *is* a lot more complex for drivers that *happen
> to already implement tx power off using the hardware support for software
> rfkill lines* to keep the two things separate.  I have no idea if any
> drivers do so.  The rfkill core can deal with it either way, as long as it
> is properly informed of any relevant rfkill state changes.

Your are mixing the RFKILL and RADIO status. They are different and on only
for some hardware they are the same. But such hardware uses the HW_BLOCK status
to indicate the RADIO status cannot be set to a state which does not match the RADIO state.

> > The rfkill layer was added for a specific reason:
> > 	Control the wireless radios when the rfkill buttons/keys/switches/sliders was pressed.
> > This was needed for areas where no RF radios are allowed (i.e. airplanes).
> 
> Yeah, but in order to do that properly, we had to make it a complete
> read/write, push and pull interface to the rfkill controllers (because of
> pesky firmware that changes it behind your back, etc) and do a clean,
> complete separation from rfkill state propagation (rfkill uevents, notify
> chain, rfkill->get_state/rfkill_force_state API) and rfkill command
> propagation (input events).

That is right, and those changes were correct. However that doesn't mean rfkill
should be used to combine the RFKILL and RADIO state.

> That's done and finished, as far as I am concerned.  Maybe some enhancements
> like make it poll()'able or somesuch might happen, but that's it as far as I
> am concerned.
>
> > Apparently we are now changing it to a global library where is displays the state of
> > all radios grouped by type, and are basically providing a new interface for the user to
> > where all configuration changes done in userspace result into new events reported to
> > userspace.
> 
> Hmm?  I am working on the addition of enough userspace interface to let
> rfkill-input be implemented in userspace, nothing more.   I can certainly
> drop that work if you want me to, but I had the impression that you liked
> the idea.

Userspace implementation is fine, but only when it reacts to actual RFKILL events.
userspace shouldn't care when the RADIO state changed, because those are
actions done by userspace itself.

> As for the "all configuration changes done in userspace result into new
> events reported to userspace", well, I already explained the bit about set
> txpower off above.  If you mean something else that I did or said, please
> explain.

* User a executes command: iwconfig wlan0 txpower off
* Driver for wlan0 switches radio off
* Driver for wlan0 reports SOFT_BLOCKED to rfkill
* rfkill sees state change, triggers notifier_block and uevent
* userpace gets the rfkill event

> > If the user has userspace tools which listen to rfkill events, and the user performs
> > a "iwconfig wlan0 txpower off" he does not want the userspace tool to listen to
> > the event and raises a "hey somebody killed a radio, lets kill all other radios as well"
> 
> If any tool is doing something that braindamaged, get rid of the tool.

Well than it means it won't perform the task of disabling wireless hardware while
in an airplane...

> > Or should the user now first disable his rfkill-listener daemon before applying
> > configuration actions to his wireless interface just to prevent events from occuring?
> 
> No rfkill-listener daemon should be ever doing something as retarded as
> shutting down all other radios because a single radio was shutdown in a
> general way.

Exactly, so why raise it as rfkill event then?

> The only scenario I know of that might need something *CLOSE* to that is
> this one (which is part of the design guidelines of the rfkill core to
> support):
> 
> 1. The code has detected that it is running on a specifc platform where the
> EV_SW SW_RFKILL_ALL button is tied *directly* to, say, b43 and you cannot
> read that state from anywhere else.
> 
>    * Note that this is for specific platforms, and not the general case.
> 
> 2. The code will then listen to b43 rfkill uevents, and ONLY FOR TRANSITIONS
> THAT INVOLVE ENTERING OR EXITING RFKILL_STATE_HARD_BLOCKED, it will generate
> EV_SW SW_RFKILL_ALL events.  Alternatively, you can use EV_SW SW_WLAN if you
> want it to be type-based, etc.
> 
>    * Note that only changes involving a hardware rfkill line are of concern,
>      simple changes between UNBLOCKED and SOFT_BLOCKED are ignored.
> 
> 3. Something else (be it rfkill-input or some userspace thing) traps that
> EV_SW *input* event, and orders rfkill to change the state of a set of
> radios (all of them for SW_RFKILL_ALL, just WLAN for SW_WLAN, etc).
> 
>    * Note that the proper layer separation is kept, so input events are
>      reacted to the same way, no matter where they were generated.
> 
> That would certainly ignore any rfkill state changes done through software,
> no matter the source of the change (firmware, userland poking at sysfs,
> rfkill-input acting upon a KEY_WLAN event, etc), and regardless of the
> wireless driver separating set txtpower off from soft-blocking or not.  So
> it would't be toggling the state of all (or all WLAN,etc) radios
> erratically.

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > Actually, rfkill-related INPUT events are triggered by buttons/keys
> > /switches/sliders.  rfkill events are sent over the uevent system and the
> > rfkill notifier chain, triggered by rfkill controllers changing state (and
> > they exist for on-screen-display support and such stuff).
> 
> Unfortunately this is no longer completely the case since you qualified
> several drivers as "no valid input device" and those are not using the input devices
> anymore. 

Passing rfkill state around using the input layer is broken, and caused real
issues.  That is what cannot be done, that is what was fixed in the new API.
But that does not preclude, e.g., b43, from also exporting input events...
*as long as* it is done correctly.

The real issue is, how will b43 and rt2x00 *know* if they are in a platform
where there is an input device directly tied to their hardware rfkill line?

How are they supposed to know whether that input device is a "rfkill all"
button, or a "wlan rfkill button"?

Either DMI lists, or configuration through module parameters, or runtime
bringuing up/down the input device upon userspace commands would br needed
for that.

And I said as much in the past.  But it is an horrible way of doing things,
so I have also provided a way to kick that from the wireless device driver
to userspace, or to a different platform module.

But you can *certainly* still do it in b43 and rt2x00, if you want to.
We might even add rfkill core *helpers* to do it, I suppose.  I just don't
see that as being the best way to go about it, but it would work.

> Those drivers completely rely on rfkill_force_state() to send the RFKILL events
> to userspace.

That's correct.

And they would now rely on something listening to either userspace events or
attaching itself to the rfkill notify chain to issue input events, in those
platforms that want it.

If you want me to write an rfkill-inputgen module that could be configured
to do what b43 and rt2x00 won't do anymore, I can add that to the queue.  It
should be simple enough, generic, and it would be optional.

Or one could just rework input device support for b43 and rt2x00, provide an
"enable_input_event_generation" module paramenter, add a
"generate_rfkill_all_event" boolean (that selects EV_SW SW_RFKILL_ALL or a
new EV_SW SW_WLAN), and that's it.

Note that you must be using the rfkill_force_state() for the above to work,
and you want to do it before you send the input events to be on the safe
side.  You must also only issue EV_SW events.  EV_KEY is not safe, as it can
cause state ping-pong.

If all the above care is taken (use only EV_SW events, make sure
rfkill->state is current), it will work, and it is supported.

> Rfkill is supposed to listen to the RFKILL status in the hardware and not the RADIO status,

Agreed.  But issues will happen when RADIO status becomes mixed with RFKILL
status because of some particular device requirement, or because of the way
that device's driver is implemented.

> at the same time it is used to control the RADIO status (and not the RFKILL status).

As far as I am concerned, rfkill [the rfkill class] is to control the
"wireless transmitter is allowed to emit energy" fact, and just THAT.

The other parts of the rfkill core exist to make that control system-wide
based on input events (like KEY_WLAN, etc).

> This means that having a radio enabled is completely different then having the RFKILL blocked
> 
> For rt2x00 the following is possible:
>  * Radio enabled
>  * RFKILL status set to blocked

We agree on that.  As long as that means the transmitter is not emitting
energy, a radio is [rfkill blocked] but it can very well be still [radio
enabled], with the interface still UP, and even receiving packets depending
on the technology and the hardware.

All a [rfkill blocked] device MUST NOT do is emit energy through its
wireless transmitter.  Anything else is fair game.

> What your new suggestion is that there is no difference between:
>  * Pressing the RFKILL key on the laptop
>  * Running "iwconfig wlan0 txpower off"

*If* the driver implements it that way.  I am not PROPOSING that drivers
implement it that way.  I was pretty clear that I do not really prefer it
either way.

In fact, I sent a reply about how it might go in order for the driver to
still differentiate between txpower off and RFKILL SOFT_BLOCK, even if it
has to use the same hardware bit for both operations.

It is more work for the driver developer, but if that's how it should be
done, I certainly don't mind.  I do think it would be best if all drivers
did it the same way, though, but I really, *really* don't feel strongly
about it at all.

> While both have a different meaning and should be threated differently. And since this is
> the RFKILL layer, we should only concern ourselves with the RFKILL status and RFKILL events
> and not userspace configuration.

Agreed.

> The only possible userspace configuration which matters for RFKILL is that the SOFT_BLOCK
> can be lifted (while the HW_BLOCK cannot).

Agreed.

> After that it is up to rfkill-input (which use was killed because some drivers were no longer valid
> input devices), or any userspace tool to listen to the events and perform the right action:
> Which is killing all wireless radios because we most likely entered an area where Wireless Radios
> are not allowed.

See above.  If you feel we need to have an in-kernel way to generate the
input-events that is not inside platform-specific devices and still in the
kernel, I am well prepared to write a generic one.  But I want an
acknowledgement that it is what you need, first.  I don't want to do that
work and drop it later because I misunderstood you.

> > Hmm?  I am working on the addition of enough userspace interface to let
> > rfkill-input be implemented in userspace, nothing more.   I can certainly
> > drop that work if you want me to, but I had the impression that you liked
> > the idea.
> 
> Userspace implementation is fine, but only when it reacts to actual RFKILL events.
> userspace shouldn't care when the RADIO state changed, because those are
> actions done by userspace itself.

That just means the wireless drivers need to know when something should make
it to rfkill_force_state() or not.   But I *do* need some help on how to
best word that in the docs.

> > As for the "all configuration changes done in userspace result into new
> > events reported to userspace", well, I already explained the bit about set
> > txpower off above.  If you mean something else that I did or said, please
> > explain.
> 
> * User a executes command: iwconfig wlan0 txpower off
> * Driver for wlan0 switches radio off
> * Driver for wlan0 reports SOFT_BLOCKED to rfkill

Ok, then the docs need to make it clear that it should not say SOFT_BLOCKED
to rfkill if it is SOFT_BLOCKED *just because* of txpower off.

And it has to be damn careful to never go out of SOFT_BLOCKED because of a
txpower <non-off> if it was SOFT_BLOCKED because of RKFILL, etc.

> * rfkill sees state change, triggers notifier_block and uevent
> * userpace gets the rfkill event

Yeah. The problem there is the "Driver for wlan0 reports SOFT_BLOCKED to
rfkill".  As I said, I was NOT aware of any consensus about how THAT should
be handled, but I have no strong feelings about it at all.

I am certainly not going to suggest (anymore) that anyone do it that way,
since you feel strongly against it.  But I do not *know* how to properly
word the docs to discourage it without making them terribly complicated.
Please help me with THAT.

I do think a post to linux-wireless about this issue IS warranted. It does
mean some drivers need changes, and this detail is currently hidden inside
this thread that I don't expect everyone to be reading.  But I will also
leave that for you to handle, since you're the one who feels strongly about
it.

> > > Or should the user now first disable his rfkill-listener daemon before applying
> > > configuration actions to his wireless interface just to prevent events from occuring?
> > 
> > No rfkill-listener daemon should be ever doing something as retarded as
> > shutting down all other radios because a single radio was shutdown in a
> > general way.
> 
> Exactly, so why raise it as rfkill event then?

I am confused.  I don't know if I understand what you mean with this.

We generate events when rfkill controllers change state, so that OSD applets
can tell you about it.   It can also be used to generate input events in
some situations.

We do not generate them ONLY to convert them to input events once in a blue
moon.

Indeed, something might still want to shutdown radios because of an specific
rfkill event in a specific situation.  That is very different from what you
were talking about, at least from my PoV.   That's why I wrote the stuff
below:

> > The only scenario I know of that might need something *CLOSE* to that is
> > this one (which is part of the design guidelines of the rfkill core to
> > support):
> > 
> > 1. The code has detected that it is running on a specifc platform where the
> > EV_SW SW_RFKILL_ALL button is tied *directly* to, say, b43 and you cannot
> > read that state from anywhere else.
> > 
> >    * Note that this is for specific platforms, and not the general case.
> > 
> > 2. The code will then listen to b43 rfkill uevents, and ONLY FOR TRANSITIONS
> > THAT INVOLVE ENTERING OR EXITING RFKILL_STATE_HARD_BLOCKED, it will generate
> > EV_SW SW_RFKILL_ALL events.  Alternatively, you can use EV_SW SW_WLAN if you
> > want it to be type-based, etc.
> > 
> >    * Note that only changes involving a hardware rfkill line are of concern,
> >      simple changes between UNBLOCKED and SOFT_BLOCKED are ignored.
> > 
> > 3. Something else (be it rfkill-input or some userspace thing) traps that
> > EV_SW *input* event, and orders rfkill to change the state of a set of
> > radios (all of them for SW_RFKILL_ALL, just WLAN for SW_WLAN, etc).
> > 
> >    * Note that the proper layer separation is kept, so input events are
> >      reacted to the same way, no matter where they were generated.
> > 
> > That would certainly ignore any rfkill state changes done through software,
> > no matter the source of the change (firmware, userland poking at sysfs,
> > rfkill-input acting upon a KEY_WLAN event, etc), and regardless of the
> > wireless driver separating set txtpower off from soft-blocking or not.  So
> > it would't be toggling the state of all (or all WLAN,etc) radios
> > erratically.

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Johannes Berg]

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

On Thu, 2008-09-18 at 15:47 -0300, Henrique de Moraes Holschuh wrote:

> Passing rfkill state around using the input layer is broken, and caused real
> issues.  That is what cannot be done, that is what was fixed in the new API.
> But that does not preclude, e.g., b43, from also exporting input events...
> *as long as* it is done correctly.

[...]

I don't get it.

We only have a few things to control:

 * radio state for each device

and a few mechanisms:
 1) mac80211-internal (TPC, ..., iwconfig txpower off)
 2) per-hardware input button (soft)
 3) per-hardware rfkill button (hard)
 4) platform input buttons (soft)
 5) platform rfkill buttons (hard)

b43, for example, 1, 2 and 3 (where connected, this is unknown to the
driver) and may live on a platform that also has 4, 5 is very unlikely.

The way I see it, we should have about this architecture:

input layer        userspace      mac80211     driver     rfkill
  2 ----------------->|
                     (a)----------------------------------->|
  4 ----------------->|                                     |
                                      (b)<------------------|
                                       |
                                       +-------->(c)

                                      (e)<-------(d)
                                       |
                                       +------------------->|
                                                            |
                    (f)<------------------------------------|


(a) synthesize rfkill state for each driver out of the various input
    events you can get, depending on whether the platform button is
    bluetooth, wlan, all, ...
(b) take rfkill state and transform it into conf.radio_enabled along
    with the internal conf.radio_enabled state that mac80211 may decide
    on based on iwconfig txpower off. assume that users know what
    they're doing if they iwconfig txpower off, I think it's pretty
    pointless to have in light of rfkill and for all I care we can
    remove it
(c) take conf.radio_enabled and enable/disable radio
(d) notify mac80211 about _HARD_ rfkill state
(e) take that into account for internal state machine and report to
    rfkill subsystem
(f) display to user

Driver only has to follow conf.radio_enabled and inform mac80211 of the
hard state.

Where's the flaw?

johannes

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

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > > On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > > Actually, rfkill-related INPUT events are triggered by buttons/keys
> > > /switches/sliders.  rfkill events are sent over the uevent system and the
> > > rfkill notifier chain, triggered by rfkill controllers changing state (and
> > > they exist for on-screen-display support and such stuff).
> > 
> > Unfortunately this is no longer completely the case since you qualified
> > several drivers as "no valid input device" and those are not using the input devices
> > anymore. 
> 
> Passing rfkill state around using the input layer is broken, and caused real
> issues.  That is what cannot be done, that is what was fixed in the new API.
> But that does not preclude, e.g., b43, from also exporting input events...
> *as long as* it is done correctly.
> 
> The real issue is, how will b43 and rt2x00 *know* if they are in a platform
> where there is an input device directly tied to their hardware rfkill line?
> 
> How are they supposed to know whether that input device is a "rfkill all"
> button, or a "wlan rfkill button"?

We don't, that is the part where userspace tools would decide what to do.

> > Those drivers completely rely on rfkill_force_state() to send the RFKILL events
> > to userspace.
> 
> That's correct.
> 
> And they would now rely on something listening to either userspace events or
> attaching itself to the rfkill notify chain to issue input events, in those
> platforms that want it.
> 
> If you want me to write an rfkill-inputgen module that could be configured
> to do what b43 and rt2x00 won't do anymore, I can add that to the queue.  It
> should be simple enough, generic, and it would be optional.

I'm not sure. Any other driver developer have a strong opinion about this?

> Or one could just rework input device support for b43 and rt2x00, provide an
> "enable_input_event_generation" module paramenter, add a
> "generate_rfkill_all_event" boolean (that selects EV_SW SW_RFKILL_ALL or a
> new EV_SW SW_WLAN), and that's it.

Ugh, that sounds not-nice. ;)

> > Rfkill is supposed to listen to the RFKILL status in the hardware and not the RADIO status,
> 
> Agreed.  But issues will happen when RADIO status becomes mixed with RFKILL
> status because of some particular device requirement, or because of the way
> that device's driver is implemented.

I think those cases would be rare, and we need to find a solution for this in the
device driver. The current API of rfkill shouldn't be too hard for drivers to make
it usable for their implementation.

> > at the same time it is used to control the RADIO status (and not the RFKILL status).
> 
> As far as I am concerned, rfkill [the rfkill class] is to control the
> "wireless transmitter is allowed to emit energy" fact, and just THAT.

That is even a better wording. :)

> > This means that having a radio enabled is completely different then having the RFKILL blocked
> > 
> > For rt2x00 the following is possible:
> >  * Radio enabled
> >  * RFKILL status set to blocked
> 
> We agree on that.  As long as that means the transmitter is not emitting
> energy, a radio is [rfkill blocked] but it can very well be still [radio
> enabled], with the interface still UP, and even receiving packets depending
> on the technology and the hardware.
> 
> All a [rfkill blocked] device MUST NOT do is emit energy through its
> wireless transmitter.  Anything else is fair game.

Agreed.

> > What your new suggestion is that there is no difference between:
> >  * Pressing the RFKILL key on the laptop
> >  * Running "iwconfig wlan0 txpower off"
> 
> *If* the driver implements it that way.  I am not PROPOSING that drivers
> implement it that way.  I was pretty clear that I do not really prefer it
> either way.

Well I think we should try to be strict in this case, and just call it a rule. ;)

> > After that it is up to rfkill-input (which use was killed because some drivers were no longer valid
> > input devices), or any userspace tool to listen to the events and perform the right action:
> > Which is killing all wireless radios because we most likely entered an area where Wireless Radios
> > are not allowed.
> 
> See above.  If you feel we need to have an in-kernel way to generate the
> input-events that is not inside platform-specific devices and still in the
> kernel, I am well prepared to write a generic one.  But I want an
> acknowledgement that it is what you need, first.  I don't want to do that
> work and drop it later because I misunderstood you.

I have no strong feeling about this. This is something other developers might have
more input about before we decide one way or another.

> > > Hmm?  I am working on the addition of enough userspace interface to let
> > > rfkill-input be implemented in userspace, nothing more.   I can certainly
> > > drop that work if you want me to, but I had the impression that you liked
> > > the idea.
> > 
> > Userspace implementation is fine, but only when it reacts to actual RFKILL events.
> > userspace shouldn't care when the RADIO state changed, because those are
> > actions done by userspace itself.
> 
> That just means the wireless drivers need to know when something should make
> it to rfkill_force_state() or not.   But I *do* need some help on how to
> best word that in the docs.

Ok, I'll think of a good descriptive explanation which we can add to the docs.

> > > As for the "all configuration changes done in userspace result into new
> > > events reported to userspace", well, I already explained the bit about set
> > > txpower off above.  If you mean something else that I did or said, please
> > > explain.
> > 
> > * User a executes command: iwconfig wlan0 txpower off
> > * Driver for wlan0 switches radio off
> > * Driver for wlan0 reports SOFT_BLOCKED to rfkill
> 
> Ok, then the docs need to make it clear that it should not say SOFT_BLOCKED
> to rfkill if it is SOFT_BLOCKED *just because* of txpower off.

Right.

> And it has to be damn careful to never go out of SOFT_BLOCKED because of a
> txpower <non-off> if it was SOFT_BLOCKED because of RKFILL, etc.

Definately, if we are really clear that userspace configuration changes should not
trigger rfkill events we are also implying this. But it wouldn't hurt to make it extra
clear. :)

> > * rfkill sees state change, triggers notifier_block and uevent
> > * userpace gets the rfkill event
> 
> Yeah. The problem there is the "Driver for wlan0 reports SOFT_BLOCKED to
> rfkill".  As I said, I was NOT aware of any consensus about how THAT should
> be handled, but I have no strong feelings about it at all.
> 
> I am certainly not going to suggest (anymore) that anyone do it that way,
> since you feel strongly against it.  But I do not *know* how to properly
> word the docs to discourage it without making them terribly complicated.
> Please help me with THAT.

I will. :)

> I do think a post to linux-wireless about this issue IS warranted. It does
> mean some drivers need changes, and this detail is currently hidden inside
> this thread that I don't expect everyone to be reading.  But I will also
> leave that for you to handle, since you're the one who feels strongly about
> it.

b43 was the first driver I saw doing this, but once we have updated the documentation
I'll send out the email to linux-wireless. Reviewing all current rfkill users in the kernel
is also high on my todo list, so that could catch some drivers as well.

> > > > Or should the user now first disable his rfkill-listener daemon before applying
> > > > configuration actions to his wireless interface just to prevent events from occuring?
> > > 
> > > No rfkill-listener daemon should be ever doing something as retarded as
> > > shutting down all other radios because a single radio was shutdown in a
> > > general way.
> > 
> > Exactly, so why raise it as rfkill event then?
> 
> I am confused.  I don't know if I understand what you mean with this.
> 
> We generate events when rfkill controllers change state, so that OSD applets
> can tell you about it.   It can also be used to generate input events in
> some situations.
> 
> We do not generate them ONLY to convert them to input events once in a blue
> moon.
> 
> Indeed, something might still want to shutdown radios because of an specific
> rfkill event in a specific situation.  That is very different from what you
> were talking about, at least from my PoV.   That's why I wrote the stuff
> below:

I was refering to RFKILL events which would be triggered by the 'iwconfig wlan0 txpower off'
since those are not RFKILL events (triggered by a RFKILL device when the user presses
a RFKILL key).

> > > The only scenario I know of that might need something *CLOSE* to that is
> > > this one (which is part of the design guidelines of the rfkill core to
> > > support):
> > > 
> > > 1. The code has detected that it is running on a specifc platform where the
> > > EV_SW SW_RFKILL_ALL button is tied *directly* to, say, b43 and you cannot
> > > read that state from anywhere else.
> > > 
> > >    * Note that this is for specific platforms, and not the general case.
> > > 
> > > 2. The code will then listen to b43 rfkill uevents, and ONLY FOR TRANSITIONS
> > > THAT INVOLVE ENTERING OR EXITING RFKILL_STATE_HARD_BLOCKED, it will generate
> > > EV_SW SW_RFKILL_ALL events.  Alternatively, you can use EV_SW SW_WLAN if you
> > > want it to be type-based, etc.
> > > 
> > >    * Note that only changes involving a hardware rfkill line are of concern,
> > >      simple changes between UNBLOCKED and SOFT_BLOCKED are ignored.
> > > 
> > > 3. Something else (be it rfkill-input or some userspace thing) traps that
> > > EV_SW *input* event, and orders rfkill to change the state of a set of
> > > radios (all of them for SW_RFKILL_ALL, just WLAN for SW_WLAN, etc).
> > > 
> > >    * Note that the proper layer separation is kept, so input events are
> > >      reacted to the same way, no matter where they were generated.
> > > 
> > > That would certainly ignore any rfkill state changes done through software,
> > > no matter the source of the change (firmware, userland poking at sysfs,
> > > rfkill-input acting upon a KEY_WLAN event, etc), and regardless of the
> > > wireless driver separating set txtpower off from soft-blocking or not.  So
> > > it would't be toggling the state of all (or all WLAN,etc) radios
> > > erratically.
> 

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > > > As for the "all configuration changes done in userspace result into new
> > > > events reported to userspace", well, I already explained the bit about set
> > > > txpower off above.  If you mean something else that I did or said, please
> > > > explain.
> > > 
> > > * User a executes command: iwconfig wlan0 txpower off
> > > * Driver for wlan0 switches radio off
> > > * Driver for wlan0 reports SOFT_BLOCKED to rfkill
> > 
> > Ok, then the docs need to make it clear that it should not say SOFT_BLOCKED
> > to rfkill if it is SOFT_BLOCKED *just because* of txpower off.
> 
> Right.
> 
> > And it has to be damn careful to never go out of SOFT_BLOCKED because of a
> > txpower <non-off> if it was SOFT_BLOCKED because of RKFILL, etc.
> 
> Definately, if we are really clear that userspace configuration changes should not
> trigger rfkill events we are also implying this. But it wouldn't hurt to make it extra
> clear. :)

We are not fully sync'd yet :)  I *still* don't understand what this
"userspace configuration changes should not trigger rfkill events" thing.

Do you mean "userspace configuration changes, that are NOT rfkill
configuration changes, should not trigger rfkill events" ?

> > > > > Or should the user now first disable his rfkill-listener daemon before applying
> > > > > configuration actions to his wireless interface just to prevent events from occuring?
> > > > 
> > > > No rfkill-listener daemon should be ever doing something as retarded as
> > > > shutting down all other radios because a single radio was shutdown in a
> > > > general way.
> > > 
> > > Exactly, so why raise it as rfkill event then?
> > 
> > I am confused.  I don't know if I understand what you mean with this.
> > 
> > We generate events when rfkill controllers change state, so that OSD applets
> > can tell you about it.   It can also be used to generate input events in
> > some situations.
> > 
> > We do not generate them ONLY to convert them to input events once in a blue
> > moon.
> > 
> > Indeed, something might still want to shutdown radios because of an specific
> > rfkill event in a specific situation.  That is very different from what you
> > were talking about, at least from my PoV.   That's why I wrote the stuff
> > below:
> 
> I was refering to RFKILL events which would be triggered by the 'iwconfig wlan0 txpower off'
> since those are not RFKILL events (triggered by a RFKILL device when the user presses
> a RFKILL key).

Ah, ok.  I suppose that's what you mean up above as well.  We agree, then.

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Michael Buesch]

On Thursday 18 September 2008 16:24:52 Ivo van Doorn wrote:
> On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> > On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > > If it is something coming from mac80211, then you do not want
> > > to send a SOFT_BLOCKED event since that will cause all other radios
> > > to be switched off simply because the b43 interface has not been
> > > enabled.
> > 
> > Drivers ARE supposed to be able to set their radio state to their heart's
> > content, without messing with any other devices.  There are no constraints
> > to calls to rfkill_force_state(), other than the current issue that it must
> > not be done from an atomic context.
> 
> My main point was that when the radio is not enabled because the user
> did something like "iwconfig wlan0 txpower off" then this is not an rfkill
> SOFT_BLOCKED event. Since that command has nothing to do with the
> entire rfkill layer.
> 
> When you consider such commands as rfkill events you get wrong behavior
> because it would trigger a SOFT_BLOCK in rfkill which will be send to all
> registered drivers who can disable their radio off as well. And that is
> definately not what you want...

Well, if that's the definition of the API, we must not force rfkill
state to anything other than HW_BLOCKED or UNBLOCKED.
I dunno how the API is defined...

-- 
Greetings Michael.

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Michael Buesch wrote:
> On Thursday 18 September 2008 16:24:52 Ivo van Doorn wrote:
> > On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> > > On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > > > If it is something coming from mac80211, then you do not want
> > > > to send a SOFT_BLOCKED event since that will cause all other radios
> > > > to be switched off simply because the b43 interface has not been
> > > > enabled.
> > > 
> > > Drivers ARE supposed to be able to set their radio state to their heart's
> > > content, without messing with any other devices.  There are no constraints
> > > to calls to rfkill_force_state(), other than the current issue that it must
> > > not be done from an atomic context.
> > 
> > My main point was that when the radio is not enabled because the user
> > did something like "iwconfig wlan0 txpower off" then this is not an rfkill
> > SOFT_BLOCKED event. Since that command has nothing to do with the
> > entire rfkill layer.
> > 
> > When you consider such commands as rfkill events you get wrong behavior
> > because it would trigger a SOFT_BLOCK in rfkill which will be send to all
> > registered drivers who can disable their radio off as well. And that is
> > definately not what you want...
> 
> Well, if that's the definition of the API, we must not force rfkill
> state to anything other than HW_BLOCKED or UNBLOCKED.
> I dunno how the API is defined...

>From rfkill.h:
	RFKILL_STATE_SOFT_BLOCKED = 0,	/* Radio output blocked */
	RFKILL_STATE_UNBLOCKED    = 1,	/* Radio output allowed */
	RFKILL_STATE_HARD_BLOCKED = 2,	/* Output blocked, non-overrideable */

Since b43 has a rfkill mechanism that does switch of the radio when RFKILL is set to BLOCK
after a key press, it should send RFKILL_STATE_HARD_BLOCKED because rfkill cannot override
it.

rt2x00 hardware does not change the radio state when RFKILL is set to BLOCK after a key press,
the state is therefor overridable and it can send RFKILL_STATE_SOFT_BLOCKED to rfkill.

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Johannes Berg]

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

On Thu, 2008-09-18 at 19:52 +0200, Ivo van Doorn wrote:

> From rfkill.h:
> 	RFKILL_STATE_SOFT_BLOCKED = 0,	/* Radio output blocked */
> 	RFKILL_STATE_UNBLOCKED    = 1,	/* Radio output allowed */
> 	RFKILL_STATE_HARD_BLOCKED = 2,	/* Output blocked, non-overrideable */
> 
> Since b43 has a rfkill mechanism that does switch of the radio when RFKILL is set to BLOCK
> after a key press, it should send RFKILL_STATE_HARD_BLOCKED because rfkill cannot override
> it.
> 
> rt2x00 hardware does not change the radio state when RFKILL is set to BLOCK after a key press,
> the state is therefor overridable and it can send RFKILL_STATE_SOFT_BLOCKED to rfkill.

If rt2x00 has no meaning of "hardware blocked", why is the button not a
simple input device?

johannes

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

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Johannes Berg wrote:
> On Thu, 2008-09-18 at 19:52 +0200, Ivo van Doorn wrote:
> 
> > From rfkill.h:
> > 	RFKILL_STATE_SOFT_BLOCKED = 0,	/* Radio output blocked */
> > 	RFKILL_STATE_UNBLOCKED    = 1,	/* Radio output allowed */
> > 	RFKILL_STATE_HARD_BLOCKED = 2,	/* Output blocked, non-overrideable */
> > 
> > Since b43 has a rfkill mechanism that does switch of the radio when RFKILL is set to BLOCK
> > after a key press, it should send RFKILL_STATE_HARD_BLOCKED because rfkill cannot override
> > it.
> > 
> > rt2x00 hardware does not change the radio state when RFKILL is set to BLOCK after a key press,
> > the state is therefor overridable and it can send RFKILL_STATE_SOFT_BLOCKED to rfkill.
> 
> If rt2x00 has no meaning of "hardware blocked", why is the button not a
> simple input device?

Because I had that discussion with Henrique and that ended with a "it isn't a input device"...

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Johannes Berg]

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

On Thu, 2008-09-18 at 20:06 +0200, Ivo van Doorn wrote:
> On Thursday 18 September 2008, Johannes Berg wrote:
> > On Thu, 2008-09-18 at 19:52 +0200, Ivo van Doorn wrote:
> > 
> > > From rfkill.h:
> > > 	RFKILL_STATE_SOFT_BLOCKED = 0,	/* Radio output blocked */
> > > 	RFKILL_STATE_UNBLOCKED    = 1,	/* Radio output allowed */
> > > 	RFKILL_STATE_HARD_BLOCKED = 2,	/* Output blocked, non-overrideable */
> > > 
> > > Since b43 has a rfkill mechanism that does switch of the radio when RFKILL is set to BLOCK
> > > after a key press, it should send RFKILL_STATE_HARD_BLOCKED because rfkill cannot override
> > > it.
> > > 
> > > rt2x00 hardware does not change the radio state when RFKILL is set to BLOCK after a key press,
> > > the state is therefor overridable and it can send RFKILL_STATE_SOFT_BLOCKED to rfkill.
> > 
> > If rt2x00 has no meaning of "hardware blocked", why is the button not a
> > simple input device?
> 
> Because I had that discussion with Henrique and that ended with a "it isn't a input device"...

Why isn't it? If it doesn't affect the hardware itself it might as well
be connected to the ps/2 port for all I care.

johannes

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

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> On Thursday 18 September 2008, Johannes Berg wrote:
> > On Thu, 2008-09-18 at 19:52 +0200, Ivo van Doorn wrote:
> > > From rfkill.h:
> > > 	RFKILL_STATE_SOFT_BLOCKED = 0,	/* Radio output blocked */
> > > 	RFKILL_STATE_UNBLOCKED    = 1,	/* Radio output allowed */
> > > 	RFKILL_STATE_HARD_BLOCKED = 2,	/* Output blocked, non-overrideable */
> > > 
> > > Since b43 has a rfkill mechanism that does switch of the radio when RFKILL is set to BLOCK
> > > after a key press, it should send RFKILL_STATE_HARD_BLOCKED because rfkill cannot override
> > > it.
> > > 
> > > rt2x00 hardware does not change the radio state when RFKILL is set to BLOCK after a key press,
> > > the state is therefor overridable and it can send RFKILL_STATE_SOFT_BLOCKED to rfkill.
> > 
> > If rt2x00 has no meaning of "hardware blocked", why is the button not a
> > simple input device?
> 
> Because I had that discussion with Henrique and that ended with a "it isn't a input device"...

Because I NEVER UNDERSTOOD it was not a hardware rfkill line until a few
posts ago in this thread.  Argh.  My deepest apologies for that screw up.

Now that I do, my answer is "depends on how the platform used that input pin".

If it is directly connected to a switch (you get on/off from it) or a button
(you get "I have been pressed, please toggle the state"), it is an input
device.  [a]

If it is directly connected to some crap inside the platform, that is
controlled by firmware, it is NOT an input device.  [b]

And if it is always used in the same way, you can pick one of the two and go
with it.

If it is [b] and it is a *switch*, it won't cause any crap, you can just
always register an input device and go with it.

if it is [b] and it is a button, you CANNOT issue input events from it if
there is any driver driving the firmware's platform... or you would get
multiple events, which is a major issue for EV_KEY.

I hope it is a switch, and that you can just always provide an input device
that issues some sort of EV_SW event (if you need it, we ask Dmitry to add
EV_SW SW_WLAN).

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > On Thursday 18 September 2008, Johannes Berg wrote:
> > > On Thu, 2008-09-18 at 19:52 +0200, Ivo van Doorn wrote:
> > > > From rfkill.h:
> > > > 	RFKILL_STATE_SOFT_BLOCKED = 0,	/* Radio output blocked */
> > > > 	RFKILL_STATE_UNBLOCKED    = 1,	/* Radio output allowed */
> > > > 	RFKILL_STATE_HARD_BLOCKED = 2,	/* Output blocked, non-overrideable */
> > > > 
> > > > Since b43 has a rfkill mechanism that does switch of the radio when RFKILL is set to BLOCK
> > > > after a key press, it should send RFKILL_STATE_HARD_BLOCKED because rfkill cannot override
> > > > it.
> > > > 
> > > > rt2x00 hardware does not change the radio state when RFKILL is set to BLOCK after a key press,
> > > > the state is therefor overridable and it can send RFKILL_STATE_SOFT_BLOCKED to rfkill.
> > > 
> > > If rt2x00 has no meaning of "hardware blocked", why is the button not a
> > > simple input device?
> > 
> > Because I had that discussion with Henrique and that ended with a "it isn't a input device"...
> 
> Because I NEVER UNDERSTOOD it was not a hardware rfkill line until a few
> posts ago in this thread.  Argh.  My deepest apologies for that screw up.
> 
> Now that I do, my answer is "depends on how the platform used that input pin".
> 
> If it is directly connected to a switch (you get on/off from it) or a button
> (you get "I have been pressed, please toggle the state"), it is an input
> device.  [a]
> 
> If it is directly connected to some crap inside the platform, that is
> controlled by firmware, it is NOT an input device.  [b]

Ok, rt2x00 is definately [a]

> I hope it is a switch, and that you can just always provide an input device
> that issues some sort of EV_SW event (if you need it, we ask Dmitry to add
> EV_SW SW_WLAN).

Ok, I'll readd the input_polldev to rt2x00 again then. :)

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> > On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > > On Thursday 18 September 2008, Johannes Berg wrote:
> > > > On Thu, 2008-09-18 at 19:52 +0200, Ivo van Doorn wrote:
> > > > > From rfkill.h:
> > > > > 	RFKILL_STATE_SOFT_BLOCKED = 0,	/* Radio output blocked */
> > > > > 	RFKILL_STATE_UNBLOCKED    = 1,	/* Radio output allowed */
> > > > > 	RFKILL_STATE_HARD_BLOCKED = 2,	/* Output blocked, non-overrideable */
> > > > > 
> > > > > Since b43 has a rfkill mechanism that does switch of the radio when RFKILL is set to BLOCK
> > > > > after a key press, it should send RFKILL_STATE_HARD_BLOCKED because rfkill cannot override
> > > > > it.
> > > > > 
> > > > > rt2x00 hardware does not change the radio state when RFKILL is set to BLOCK after a key press,
> > > > > the state is therefor overridable and it can send RFKILL_STATE_SOFT_BLOCKED to rfkill.
> > > > 
> > > > If rt2x00 has no meaning of "hardware blocked", why is the button not a
> > > > simple input device?
> > > 
> > > Because I had that discussion with Henrique and that ended with a "it isn't a input device"...
> > 
> > Because I NEVER UNDERSTOOD it was not a hardware rfkill line until a few
> > posts ago in this thread.  Argh.  My deepest apologies for that screw up.
> > 
> > Now that I do, my answer is "depends on how the platform used that input pin".
> > 
> > If it is directly connected to a switch (you get on/off from it) or a button
> > (you get "I have been pressed, please toggle the state"), it is an input
> > device.  [a]
> > 
> > If it is directly connected to some crap inside the platform, that is
> > controlled by firmware, it is NOT an input device.  [b]
> 
> Ok, rt2x00 is definately [a]
> 
> > I hope it is a switch, and that you can just always provide an input device
> > that issues some sort of EV_SW event (if you need it, we ask Dmitry to add
> > EV_SW SW_WLAN).
> 
> Ok, I'll readd the input_polldev to rt2x00 again then. :)

But do use EV_SW if it is a switch, please :-)  Even if that means a need to
get EV_SW SW_WLAN out of Dmitry...  I don't know if rt2x000 wants SW_WLAN or
SW_RFKILL_ALL.

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > On Thursday 18 September 2008, Henrique de Moraes Holschuh wrote:
> > > On Thu, 18 Sep 2008, Ivo van Doorn wrote:
> > > > On Thursday 18 September 2008, Johannes Berg wrote:
> > > > > On Thu, 2008-09-18 at 19:52 +0200, Ivo van Doorn wrote:
> > > > > > From rfkill.h:
> > > > > > 	RFKILL_STATE_SOFT_BLOCKED = 0,	/* Radio output blocked */
> > > > > > 	RFKILL_STATE_UNBLOCKED    = 1,	/* Radio output allowed */
> > > > > > 	RFKILL_STATE_HARD_BLOCKED = 2,	/* Output blocked, non-overrideable */
> > > > > > 
> > > > > > Since b43 has a rfkill mechanism that does switch of the radio when RFKILL is set to BLOCK
> > > > > > after a key press, it should send RFKILL_STATE_HARD_BLOCKED because rfkill cannot override
> > > > > > it.
> > > > > > 
> > > > > > rt2x00 hardware does not change the radio state when RFKILL is set to BLOCK after a key press,
> > > > > > the state is therefor overridable and it can send RFKILL_STATE_SOFT_BLOCKED to rfkill.
> > > > > 
> > > > > If rt2x00 has no meaning of "hardware blocked", why is the button not a
> > > > > simple input device?
> > > > 
> > > > Because I had that discussion with Henrique and that ended with a "it isn't a input device"...
> > > 
> > > Because I NEVER UNDERSTOOD it was not a hardware rfkill line until a few
> > > posts ago in this thread.  Argh.  My deepest apologies for that screw up.
> > > 
> > > Now that I do, my answer is "depends on how the platform used that input pin".
> > > 
> > > If it is directly connected to a switch (you get on/off from it) or a button
> > > (you get "I have been pressed, please toggle the state"), it is an input
> > > device.  [a]
> > > 
> > > If it is directly connected to some crap inside the platform, that is
> > > controlled by firmware, it is NOT an input device.  [b]
> > 
> > Ok, rt2x00 is definately [a]
> > 
> > > I hope it is a switch, and that you can just always provide an input device
> > > that issues some sort of EV_SW event (if you need it, we ask Dmitry to add
> > > EV_SW SW_WLAN).
> > 
> > Ok, I'll readd the input_polldev to rt2x00 again then. :)
> 
> But do use EV_SW if it is a switch, please :-)

Sure.

> Even if that means a need to 
> get EV_SW SW_WLAN out of Dmitry...  I don't know if rt2x000 wants SW_WLAN or
> SW_RFKILL_ALL.

Not sure either, there is no way to determine which of the 2 should be used.

Oh an to make sure I get it completely right this time:
When I implement the input polldev, I no longer have to use rfkill_force_state() right?

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Fri, 19 Sep 2008, Ivo van Doorn wrote:
> > Even if that means a need to 
> > get EV_SW SW_WLAN out of Dmitry...  I don't know if rt2x000 wants SW_WLAN or
> > SW_RFKILL_ALL.
> 
> Not sure either, there is no way to determine which of the 2 should be used.

Should be an module parameter or runtime (sysfs?) parameter, then...

> Oh an to make sure I get it completely right this time:
> When I implement the input polldev, I no longer have to use rfkill_force_state() right?

Correct.  It has nothing to do with the rfkill class part of your driver at
all, since there is no effect on the hardware.  It is just an input device
that issues EV_SW events.

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Ivo van Doorn]

On Saturday 20 September 2008, Henrique de Moraes Holschuh wrote:
> On Fri, 19 Sep 2008, Ivo van Doorn wrote:
> > > Even if that means a need to 
> > > get EV_SW SW_WLAN out of Dmitry...  I don't know if rt2x000 wants SW_WLAN or
> > > SW_RFKILL_ALL.
> > 
> > Not sure either, there is no way to determine which of the 2 should be used.
> 
> Should be an module parameter or runtime (sysfs?) parameter, then...

Module parameter sounds best, I doubt people would want to change the value
at runtime (when it is just easier to do it at loadtime).

> > Oh an to make sure I get it completely right this time:
> > When I implement the input polldev, I no longer have to use rfkill_force_state() right?
> 
> Correct.  It has nothing to do with the rfkill class part of your driver at
> all, since there is no effect on the hardware.  It is just an input device
> that issues EV_SW events.

Ok, thanks. :)

I'll remove rfkill from rt2x00, implement input_polldev as seperate module,
move rfkill support into mac80211 to make mac80211 drivers automatically
detect the rfkill notifications.

Ivo

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Sat, 20 Sep 2008, Ivo van Doorn wrote:
> I'll remove rfkill from rt2x00, implement input_polldev as seperate module,
> move rfkill support into mac80211 to make mac80211 drivers automatically
> detect the rfkill notifications.

As long as the drivers based on mac80211 still know it is rfkill they're
dealing with (i.e. as long as they will know they MUST NOT emit energy from
the transmitter when they're soft-blocked), it sounds like a extremely good
move.

-- 
  "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: [RFC] b43: A patch for control of the radio LED using rfkill

[Michael Buesch]

On Monday 22 September 2008 05:01:37 Henrique de Moraes Holschuh wrote:
> As long as the drivers based on mac80211 still know it is rfkill they're
> dealing with (i.e. as long as they will know they MUST NOT emit energy from
> the transmitter when they're soft-blocked),

That's the only policy that makes sense. So I don't think it would be a problem.
IMO that's how the mac80211 config API is defined, currently.

-- 
Greetings Michael.

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Michael Buesch]

On Thursday 18 September 2008 07:07:51 Larry Finger wrote:
> Index: wireless-testing/drivers/net/wireless/b43/rfkill.c
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/b43/rfkill.c
> +++ wireless-testing/drivers/net/wireless/b43/rfkill.c
> @@ -44,13 +44,11 @@ static bool b43_is_hw_radio_enabled(stru
>  	return 0;
>  }
>  
> -/* The poll callback for the hardware button. */
> -static void b43_rfkill_poll(struct input_polled_dev *poll_dev)
> +static void b43_rfkill_update(struct b43_wldev *dev)
>  {
> -	struct b43_wldev *dev = poll_dev->private;
>  	struct b43_wl *wl = dev->wl;
>  	bool enabled;
> -	bool report_change = 0;
> +	struct b43_rfkill *rfk = &(dev->wl->rfkill);
>  
>  	mutex_lock(&wl->mutex);
>  	if (unlikely(b43_status(dev) < B43_STAT_INITIALIZED)) {
> @@ -60,18 +58,40 @@ static void b43_rfkill_poll(struct input
>  	enabled = b43_is_hw_radio_enabled(dev);
>  	if (unlikely(enabled != dev->radio_hw_enable)) {
>  		dev->radio_hw_enable = enabled;
> -		report_change = 1;
>  		b43info(wl, "Radio hardware status changed to %s\n",
>  			enabled ? "ENABLED" : "DISABLED");
> +		if (!enabled)
> +			rfkill_force_state(rfk->rfkill,
> +					   RFKILL_STATE_HARD_BLOCKED);
> +		else {
> +			if (!dev->phy.radio_on)
> +				rfkill_force_state(rfk->rfkill,
> +						   RFKILL_STATE_SOFT_BLOCKED);
> +			else
> +				rfkill_force_state(rfk->rfkill,
> +						   RFKILL_STATE_UNBLOCKED);
> +		}
>  	}
>  	mutex_unlock(&wl->mutex);

Can rfkill_force_state recurse into b43_rfkill_soft_toggle()?
input_report_key could do this in the past (Dunno if that's still true now),
so that's why it's done outside of the lock to avoid deadlocks.

Also keep in mind that N and LP(?) devices report state through interrupt.

> +}
>  
> -	/* send the radio switch event to the system - note both a key press
> -	 * and a release are required */
> -	if (unlikely(report_change)) {
> -		input_report_key(poll_dev->input, KEY_WLAN, 1);
> -		input_report_key(poll_dev->input, KEY_WLAN, 0);
> -	}
> +static void b43_rfkill_poll(unsigned long data)
> +{
> +	struct b43_rfkill *rfkill = (struct b43_rfkill *)data;
> +	schedule_work(&rfkill->poll_queue);
> +}
> +
> +static void b43_rfkill_work(struct work_struct *work)
> +{
> +	struct b43_rfkill *rfk = container_of(work, struct b43_rfkill,
> +					      poll_queue);
> +	struct b43_wl *wl = container_of(rfk, struct b43_wl, rfkill);
> +	struct b43_wldev *dev = wl->current_dev;
> +
> +	b43_rfkill_update(dev);
> +	rfk->poll_timer.function = b43_rfkill_poll;
> +	rfk->poll_timer.expires = round_jiffies(jiffies + HZ);
> +	add_timer(&rfk->poll_timer);
>  }

Why not using delayed work instead of reinventing the wheel?
Also please schedule it on the mac80211 workqueue to avoid possible
deadlocks with the networking core.

>  /* Called when the RFKILL toggled in software. */
> @@ -141,48 +161,23 @@ void b43_rfkill_init(struct b43_wldev *d
>  	rfk->rfkill->toggle_radio = b43_rfkill_soft_toggle;
>  	rfk->rfkill->user_claim_unsupported = 1;
>  
> -	rfk->poll_dev = input_allocate_polled_device();
> -	if (!rfk->poll_dev) {
> -		rfkill_free(rfk->rfkill);
> -		goto err_freed_rfk;
> -	}
> -
> -	rfk->poll_dev->private = dev;
> -	rfk->poll_dev->poll = b43_rfkill_poll;
> -	rfk->poll_dev->poll_interval = 1000; /* msecs */
> -
> -	rfk->poll_dev->input->name = rfk->name;
> -	rfk->poll_dev->input->id.bustype = BUS_HOST;
> -	rfk->poll_dev->input->id.vendor = dev->dev->bus->boardinfo.vendor;
> -	rfk->poll_dev->input->evbit[0] = BIT(EV_KEY);
> -	set_bit(KEY_WLAN, rfk->poll_dev->input->keybit);
> -
>  	err = rfkill_register(rfk->rfkill);
>  	if (err)
> -		goto err_free_polldev;
> +		goto err_free_rfk;
>  
> -#ifdef CONFIG_RFKILL_INPUT_MODULE
> -	/* B43 RF-kill isn't useful without the rfkill-input subsystem.
> -	 * Try to load the module. */
> -	err = request_module("rfkill-input");
> -	if (err)
> -		b43warn(wl, "Failed to load the rfkill-input module. "
> -			"The built-in radio LED will not work.\n");
> -#endif /* CONFIG_RFKILL_INPUT */
> +	rfk->registered = 1;
>  
> -	err = input_register_polled_device(rfk->poll_dev);
> -	if (err)
> -		goto err_unreg_rfk;
> +	INIT_WORK(&rfk->poll_queue, b43_rfkill_work);
>  
> -	rfk->registered = 1;
> +	init_timer(&rfk->poll_timer);
> +	rfk->poll_timer.function = b43_rfkill_poll;
> +	rfk->poll_timer.expires = round_jiffies(jiffies + HZ);
> +	rfk->poll_timer.data = (unsigned long)rfk;
> +	add_timer(&rfk->poll_timer);
>  
>  	return;
> -err_unreg_rfk:
> -	rfkill_unregister(rfk->rfkill);
> -err_free_polldev:
> -	input_free_polled_device(rfk->poll_dev);
> -	rfk->poll_dev = NULL;
> -err_freed_rfk:
> +err_free_rfk:
> +	rfkill_free(rfk->rfkill);
>  	rfk->rfkill = NULL;
>  out_error:
>  	rfk->registered = 0;
> @@ -197,9 +192,8 @@ void b43_rfkill_exit(struct b43_wldev *d
>  		return;
>  	rfk->registered = 0;
>  
> -	input_unregister_polled_device(rfk->poll_dev);
> +	del_timer(&rfk->poll_timer);
> +

cancel the work. I'm not sure whether to do it here, however.
One must take care to avoid deadlocks for wl->mutex.
I don't know from the top of my head whether b43_rfkill_exit is called
with the mutex locked or not. However, in case it is, dropping and
reaquireing the lock is not an option.

>  	rfkill_unregister(rfk->rfkill);
> -	input_free_polled_device(rfk->poll_dev);
> -	rfk->poll_dev = NULL;
>  	rfk->rfkill = NULL;
>  }
> Index: wireless-testing/drivers/net/wireless/b43/rfkill.h
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/b43/rfkill.h
> +++ wireless-testing/drivers/net/wireless/b43/rfkill.h
> @@ -7,14 +7,14 @@ struct b43_wldev;
>  #ifdef CONFIG_B43_RFKILL
>  
>  #include <linux/rfkill.h>
> -#include <linux/input-polldev.h>
> -
>  
>  struct b43_rfkill {
>  	/* The RFKILL subsystem data structure */
>  	struct rfkill *rfkill;
> -	/* The poll device for the RFKILL input button */
> -	struct input_polled_dev *poll_dev;
> +	/* The timer list for the RFKILL polling */
> +	struct timer_list poll_timer;
> +	/* Workqueue for the RFKILL polling */
> +	struct work_struct poll_queue;
>  	/* Did initialization succeed? Used for freeing. */
>  	bool registered;
>  	/* The unique name of this rfkill switch */

-- 
Greetings Michael.

Re: [RFC] b43: A patch for control of the radio LED using rfkill

[Henrique de Moraes Holschuh]

On Thu, 18 Sep 2008, Michael Buesch wrote:
> Can rfkill_force_state recurse into b43_rfkill_soft_toggle()?

rfkill_force_state() will never call any of the rfkill struct hooks, ever.

So, unless you driver is hooked into the rfkill notifier chain (which is the
only possible way to recurse through the rfkill core when using
rfkill_force_state()), it will not recurse.

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