[GIT PATCH] rfkill support for r/w and r/o rfkill switches

[GIT PATCH] rfkill support for r/w and r/o rfkill switches

[Henrique de Moraes Holschuh]

This patch series (based on v2.6.25-rc8-mm2) has several improvements to
the rfkill class that I need for thinkpad-acpi, plus two fluff and
documentation fixes.

I'd appreciate comments, and if the patches are acceptable, that they are
sent to mainline early during the 2.6.26 merge window.  That way, I could
try for a thinkpad-acpi rfkill submission for 2.6.26 as well.

The thinkpad-acpi work that needs these patches is ready, and can be
looked at on the thinkpad-acpi git tree (devel branch):
http://repo.or.cz/w/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git

Note: new thinkpads have a read-only "type any" radio switch, plus
read-write WWAN and Bluetooth radio switches.   The embedded WLAN cards
have WLAN read-write switches (that should be handled by the ipw* and iwl*
drivers).  Very old thinkpads had ACPI-based WLAN radio switches, but
thinkpad-acpi does not support those.

Shortlog:
Henrique de Moraes Holschuh (8):
      rfkill: clarify meaning of rfkill states
      rfkill: fix minor typo in kernel doc
      rfkill: handle KEY_RADIO and SW_RADIO events
      rfkill: add read-write rfkill switch support
      rfkill: add read-only rfkill switch support
      rfkill: add the WWAN radio type
      rfkill: add an "any radio" switch type and functionality
      rfkill: add parameter to disable radios by default

Diffstat:
 include/linux/rfkill.h    |   18 ++++--
 net/rfkill/rfkill-input.c |   71 +++++++++++++++++++++++-
 net/rfkill/rfkill.c       |  134 ++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 207 insertions(+), 16 deletions(-)

-- 
  "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 1/8] rfkill: clarify meaning of rfkill states

[Henrique de Moraes Holschuh]

rfkill really should have been named rfswitch.  As it is, one can get
confused whether RFKILL_STATE_ON means the KILL switch is on (and
therefore, the radio is being *blocked* from operating), or whether it
means the RADIO rf output is on.

Clearly state that RFKILL_STATE_ON means the radio is *unblocked* from
operating (i.e. there is no rf killing going on).

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
Cc: John W. Linville <linville@tuxdriver.com>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 include/linux/rfkill.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index e3ab21d..ca89ae1 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -44,8 +44,8 @@ enum rfkill_type {
 };
 
 enum rfkill_state {
-	RFKILL_STATE_OFF	= 0,
-	RFKILL_STATE_ON		= 1,
+	RFKILL_STATE_OFF	= 0,	/* Radio output blocked */
+	RFKILL_STATE_ON		= 1,	/* Radio output active */
 };
 
 /**
@@ -53,7 +53,7 @@ enum rfkill_state {
  * @name: Name of the switch.
  * @type: Radio type which the button controls, the value stored
  *	here should be a value from enum rfkill_type.
- * @state: State of the switch (on/off).
+ * @state: State of the switch, "ON" means radio can operate.
  * @user_claim_unsupported: Whether the hardware supports exclusive
  *	RF-kill control by userspace. Set this before registering.
  * @user_claim: Set when the switch is controlled exlusively by userspace.
-- 
1.5.4.4

Re: [PATCH 1/8] rfkill: clarify meaning of rfkill states

[Dmitry Torokhov]

On Fri, Apr 11, 2008 at 05:37:17PM -0300, Henrique de Moraes Holschuh wrote:
> rfkill really should have been named rfswitch.  As it is, one can get
> confused whether RFKILL_STATE_ON means the KILL switch is on (and
> therefore, the radio is being *blocked* from operating), or whether it
> means the RADIO rf output is on.
> 
> Clearly state that RFKILL_STATE_ON means the radio is *unblocked* from
> operating (i.e. there is no rf killing going on).
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>
> Cc: John W. Linville <linville@tuxdriver.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>

I consider Ivo maintaining rfkill but for what it worth:

Acked-by: Dmitry Torokhov <dtor@mail.ru>

-- 
Dmitry

[PATCH 2/8] rfkill: fix minor typo in kernel doc

[Henrique de Moraes Holschuh]

Fix a minor typo in an exported function documentation

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

diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 140a0a8..1601e50 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -412,7 +412,7 @@ int rfkill_register(struct rfkill *rfkill)
 EXPORT_SYMBOL(rfkill_register);
 
 /**
- * rfkill_unregister - Uegister a rfkill structure.
+ * rfkill_unregister - Unregister a rfkill structure.
  * @rfkill: rfkill structure to be unregistered
  *
  * This function should be called by the network driver during device
-- 
1.5.4.4

[PATCH 3/8] rfkill: handle KEY_RADIO and SW_RADIO events

[Henrique de Moraes Holschuh]

The *_RADIO input events are related to all radios in a system.  There are
two: KEY_RADIO and SW_RADIO.

Teach rfkill-input how to handle them.  In particular, SW_RADIO is not a
toggle, but an absolute enable-or-disable command.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
Cc: John W. Linville <linville@tuxdriver.com>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 net/rfkill/rfkill-input.c |   57 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
index e4b051d..7d1e520 100644
--- a/net/rfkill/rfkill-input.c
+++ b/net/rfkill/rfkill-input.c
@@ -55,6 +55,22 @@ static void rfkill_task_handler(struct work_struct *work)
 	mutex_unlock(&task->mutex);
 }
 
+static void rfkill_schedule_set(struct rfkill_task *task,
+				enum rfkill_state desired_state)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&task->lock, flags);
+
+	if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
+		task->desired_state = desired_state;
+		task->last = jiffies;
+		schedule_work(&task->work);
+	}
+
+	spin_unlock_irqrestore(&task->lock, flags);
+}
+
 static void rfkill_schedule_toggle(struct rfkill_task *task)
 {
 	unsigned long flags;
@@ -87,9 +103,9 @@ static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);
 static DEFINE_RFKILL_TASK(rfkill_wimax, RFKILL_TYPE_WIMAX);
 
 static void rfkill_event(struct input_handle *handle, unsigned int type,
-			unsigned int code, int down)
+			unsigned int code, int data)
 {
-	if (type == EV_KEY && down == 1) {
+	if (type == EV_KEY && data == 1) {
 		switch (code) {
 		case KEY_WLAN:
 			rfkill_schedule_toggle(&rfkill_wlan);
@@ -103,6 +119,33 @@ static void rfkill_event(struct input_handle *handle, unsigned int type,
 		case KEY_WIMAX:
 			rfkill_schedule_toggle(&rfkill_wimax);
 			break;
+		case KEY_RADIO:
+			/* EVERY radio type */
+			rfkill_schedule_toggle(&rfkill_wimax);
+			rfkill_schedule_toggle(&rfkill_uwb);
+			rfkill_schedule_toggle(&rfkill_bt);
+			rfkill_schedule_toggle(&rfkill_wlan);
+			break;
+		default:
+			break;
+		}
+	} else if (type == EV_SW) {
+		switch (code) {
+		case SW_RADIO:
+			/* EVERY radio type. data != 0 means radios ON */
+			rfkill_schedule_set(&rfkill_wimax,
+					    (data)? RFKILL_STATE_ON:
+						    RFKILL_STATE_OFF);
+			rfkill_schedule_set(&rfkill_uwb,
+					    (data)? RFKILL_STATE_ON:
+						    RFKILL_STATE_OFF);
+			rfkill_schedule_set(&rfkill_bt,
+					    (data)? RFKILL_STATE_ON:
+						    RFKILL_STATE_OFF);
+			rfkill_schedule_set(&rfkill_wlan,
+					    (data)? RFKILL_STATE_ON:
+						    RFKILL_STATE_OFF);
+			break;
 		default:
 			break;
 		}
@@ -168,6 +211,16 @@ static const struct input_device_id rfkill_ids[] = {
 		.evbit = { BIT_MASK(EV_KEY) },
 		.keybit = { [BIT_WORD(KEY_WIMAX)] = BIT_MASK(KEY_WIMAX) },
 	},
+	{
+		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_KEYBIT,
+		.evbit = { BIT(EV_KEY) },
+		.keybit = { [BIT_WORD(KEY_RADIO)] = BIT_MASK(KEY_RADIO) },
+	},
+	{
+		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_SWBIT,
+		.evbit = { BIT(EV_SW) },
+		.swbit = { [BIT_WORD(SW_RADIO)] = BIT_MASK(SW_RADIO) },
+	},
 	{ }
 };
 
-- 
1.5.4.4

Re: [PATCH 3/8] rfkill: handle KEY_RADIO and SW_RADIO events

[Ivo van Doorn]

On Friday 11 April 2008, Henrique de Moraes Holschuh wrote:
> The *_RADIO input events are related to all radios in a system.  There are
> two: KEY_RADIO and SW_RADIO.
> 
> Teach rfkill-input how to handle them.  In particular, SW_RADIO is not a
> toggle, but an absolute enable-or-disable command.

Not sure what you are trying to achieve here,
who triggers the SW_RADIO and why?

> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>
> Cc: John W. Linville <linville@tuxdriver.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>
> ---
>  net/rfkill/rfkill-input.c |   57 +++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
> index e4b051d..7d1e520 100644
> --- a/net/rfkill/rfkill-input.c
> +++ b/net/rfkill/rfkill-input.c
> @@ -55,6 +55,22 @@ static void rfkill_task_handler(struct work_struct *work)
>  	mutex_unlock(&task->mutex);
>  }
>  
> +static void rfkill_schedule_set(struct rfkill_task *task,
> +				enum rfkill_state desired_state)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&task->lock, flags);
> +
> +	if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
> +		task->desired_state = desired_state;
> +		task->last = jiffies;
> +		schedule_work(&task->work);
> +	}
> +
> +	spin_unlock_irqrestore(&task->lock, flags);
> +}
> +
>  static void rfkill_schedule_toggle(struct rfkill_task *task)
>  {
>  	unsigned long flags;
> @@ -87,9 +103,9 @@ static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);
>  static DEFINE_RFKILL_TASK(rfkill_wimax, RFKILL_TYPE_WIMAX);
>  
>  static void rfkill_event(struct input_handle *handle, unsigned int type,
> -			unsigned int code, int down)
> +			unsigned int code, int data)
>  {
> -	if (type == EV_KEY && down == 1) {
> +	if (type == EV_KEY && data == 1) {
>  		switch (code) {
>  		case KEY_WLAN:
>  			rfkill_schedule_toggle(&rfkill_wlan);
> @@ -103,6 +119,33 @@ static void rfkill_event(struct input_handle *handle, unsigned int type,
>  		case KEY_WIMAX:
>  			rfkill_schedule_toggle(&rfkill_wimax);
>  			break;
> +		case KEY_RADIO:
> +			/* EVERY radio type */
> +			rfkill_schedule_toggle(&rfkill_wimax);
> +			rfkill_schedule_toggle(&rfkill_uwb);
> +			rfkill_schedule_toggle(&rfkill_bt);
> +			rfkill_schedule_toggle(&rfkill_wlan);
> +			break;
> +		default:
> +			break;
> +		}
> +	} else if (type == EV_SW) {
> +		switch (code) {
> +		case SW_RADIO:
> +			/* EVERY radio type. data != 0 means radios ON */
> +			rfkill_schedule_set(&rfkill_wimax,
> +					    (data)? RFKILL_STATE_ON:
> +						    RFKILL_STATE_OFF);
> +			rfkill_schedule_set(&rfkill_uwb,
> +					    (data)? RFKILL_STATE_ON:
> +						    RFKILL_STATE_OFF);
> +			rfkill_schedule_set(&rfkill_bt,
> +					    (data)? RFKILL_STATE_ON:
> +						    RFKILL_STATE_OFF);
> +			rfkill_schedule_set(&rfkill_wlan,
> +					    (data)? RFKILL_STATE_ON:
> +						    RFKILL_STATE_OFF);
> +			break;
>  		default:
>  			break;
>  		}
> @@ -168,6 +211,16 @@ static const struct input_device_id rfkill_ids[] = {
>  		.evbit = { BIT_MASK(EV_KEY) },
>  		.keybit = { [BIT_WORD(KEY_WIMAX)] = BIT_MASK(KEY_WIMAX) },
>  	},
> +	{
> +		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_KEYBIT,
> +		.evbit = { BIT(EV_KEY) },
> +		.keybit = { [BIT_WORD(KEY_RADIO)] = BIT_MASK(KEY_RADIO) },
> +	},
> +	{
> +		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_SWBIT,
> +		.evbit = { BIT(EV_SW) },
> +		.swbit = { [BIT_WORD(SW_RADIO)] = BIT_MASK(SW_RADIO) },
> +	},
>  	{ }
>  };
>  

Re: [PATCH 3/8] rfkill: handle KEY_RADIO and SW_RADIO events

[Henrique de Moraes Holschuh]

On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> On Friday 11 April 2008, Henrique de Moraes Holschuh wrote:
> > The *_RADIO input events are related to all radios in a system.  There are
> > two: KEY_RADIO and SW_RADIO.
> > 
> > Teach rfkill-input how to handle them.  In particular, SW_RADIO is not a
> > toggle, but an absolute enable-or-disable command.
> 
> Not sure what you are trying to achieve here,
> who triggers the SW_RADIO and why?

Thinkpad-acpi issues EV_SW SW_RADIO when the user changes a phisical
switch in the unit.  It is a switch, not a button: it has an ON
position, and an OFF position.

-- 
  "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 3/8] rfkill: handle KEY_RADIO and SW_RADIO events

[Ivo van Doorn]

On Saturday 12 April 2008, Henrique de Moraes Holschuh wrote:
> On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > On Friday 11 April 2008, Henrique de Moraes Holschuh wrote:
> > > The *_RADIO input events are related to all radios in a system.  There are
> > > two: KEY_RADIO and SW_RADIO.
> > > 
> > > Teach rfkill-input how to handle them.  In particular, SW_RADIO is not a
> > > toggle, but an absolute enable-or-disable command.
> > 
> > Not sure what you are trying to achieve here,
> > who triggers the SW_RADIO and why?
> 
> Thinkpad-acpi issues EV_SW SW_RADIO when the user changes a phisical
> switch in the unit.  It is a switch, not a button: it has an ON
> position, and an OFF position.

Ok, and such a switch is always intended to control all radios?
(in other words, it is the expected behavior it controls everything)

Ivo

Re: [PATCH 3/8] rfkill: handle KEY_RADIO and SW_RADIO events

[Henrique de Moraes Holschuh]

On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> On Saturday 12 April 2008, Henrique de Moraes Holschuh wrote:
> > On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > > On Friday 11 April 2008, Henrique de Moraes Holschuh wrote:
> > > > The *_RADIO input events are related to all radios in a system.  There are
> > > > two: KEY_RADIO and SW_RADIO.
> > > > 
> > > > Teach rfkill-input how to handle them.  In particular, SW_RADIO is not a
> > > > toggle, but an absolute enable-or-disable command.
> > > 
> > > Not sure what you are trying to achieve here,
> > > who triggers the SW_RADIO and why?
> > 
> > Thinkpad-acpi issues EV_SW SW_RADIO when the user changes a phisical
> > switch in the unit.  It is a switch, not a button: it has an ON
> > position, and an OFF position.
> 
> Ok, and such a switch is always intended to control all radios?
> (in other words, it is the expected behavior it controls everything)

In every device I have seen which had a non-type-specific radio switch?
Yes.

If we need type-specific switches (and NOT buttons/hot keys) for, e.g.,
Bluetooth, the correct thing to do is to add EV_SW SW_BLUETOOTH.  The
same goes for WWAN, UWB, etc.  But on laptops (which are the devices I
am dealing with), these switches (when they exist) are meant to block
*every* builtin radio and thus are not type-specific.

I can easily see someone designing a gadget with type-specific switches,
but since nobody asked for such support yet, we don't have anything but
SW_RADIO defined in the input layer right now.

So EV_SW SW_RADIO has "every radio" semanthics, just like KEY_RADIO.

-- 
  "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 3/8] rfkill: handle KEY_RADIO and SW_RADIO events

[Ivo van Doorn]

On Saturday 12 April 2008, Henrique de Moraes Holschuh wrote:
> On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > On Saturday 12 April 2008, Henrique de Moraes Holschuh wrote:
> > > On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > > > On Friday 11 April 2008, Henrique de Moraes Holschuh wrote:
> > > > > The *_RADIO input events are related to all radios in a system.  There are
> > > > > two: KEY_RADIO and SW_RADIO.
> > > > > 
> > > > > Teach rfkill-input how to handle them.  In particular, SW_RADIO is not a
> > > > > toggle, but an absolute enable-or-disable command.
> > > > 
> > > > Not sure what you are trying to achieve here,
> > > > who triggers the SW_RADIO and why?
> > > 
> > > Thinkpad-acpi issues EV_SW SW_RADIO when the user changes a phisical
> > > switch in the unit.  It is a switch, not a button: it has an ON
> > > position, and an OFF position.
> > 
> > Ok, and such a switch is always intended to control all radios?
> > (in other words, it is the expected behavior it controls everything)
> 
> In every device I have seen which had a non-type-specific radio switch?
> Yes.
> 
> If we need type-specific switches (and NOT buttons/hot keys) for, e.g.,
> Bluetooth, the correct thing to do is to add EV_SW SW_BLUETOOTH.  The
> same goes for WWAN, UWB, etc.  But on laptops (which are the devices I
> am dealing with), these switches (when they exist) are meant to block
> *every* builtin radio and thus are not type-specific.
> 
> I can easily see someone designing a gadget with type-specific switches,
> but since nobody asked for such support yet, we don't have anything but
> SW_RADIO defined in the input layer right now.
> 
> So EV_SW SW_RADIO has "every radio" semanthics, just like KEY_RADIO.

Ok, thanks for the explanation.
You can add this patch to my ACK list then. :)

Ivo

Re: [PATCH 3/8] rfkill: handle KEY_RADIO and SW_RADIO events

[Dmitry Torokhov]

Hi Henrique,

On Fri, Apr 11, 2008 at 05:37:19PM -0300, Henrique de Moraes Holschuh wrote:
> The *_RADIO input events are related to all radios in a system.  There are
> two: KEY_RADIO and SW_RADIO.
> 

KEY_RADIO is reserved for selecting radio input (as pooosed to TV, AUX,
etc) with a remote control. Rfkill woudl need a separate keycode if it
needs "all types of radios" event.

Btw, is there any devices in the wild that actually have separate
switches for different types of transmitters?

-- 
Dmitry

Re: [PATCH 3/8] rfkill: handle KEY_RADIO and SW_RADIO events

[Henrique de Moraes Holschuh]

On Sat, 12 Apr 2008, Dmitry Torokhov wrote:
> On Fri, Apr 11, 2008 at 05:37:19PM -0300, Henrique de Moraes Holschuh wrote:
> > The *_RADIO input events are related to all radios in a system.  There are
> > two: KEY_RADIO and SW_RADIO.
> > 
> 
> KEY_RADIO is reserved for selecting radio input (as pooosed to TV, AUX,
> etc) with a remote control. Rfkill woudl need a separate keycode if it
> needs "all types of radios" event.

Hmm, let me check where I got the wrong idea of using KEY_RADIO from,
because thinkpad-acpi already uses KEY_WLAN which means that at least in
the past I did know KEY_RADIO was not to be used for wireless data
communication devices like that...  jeez, looks like spontaneous brain
corruption on that topic hapenned to me sometime ago, and it came out in
a linux-thinkpad thread a few weeks ago.  The wrong semanthics for
KEY_RADIO seem to have stuck to my mind since then.  Drat, I *really*
apologise for this one.

This, of course, is a major NAK for this patch.  And I am considering
dropping the handling of KEY_<whatever replaces RADIO> completely from
it.  SW_RADIO (after a rename, see below) still needs to be handled,
though.

> Btw, is there any devices in the wild that actually have separate
> switches for different types of transmitters?

Separate switches?  I know of none.

Separate hot keys/buttons?  I haven't seen it, but check commit
90da11514562020ea7d697982f912ac949adc317's comment.  That was the commit
which added KEY_WLAN and KEY_BLUETOOTH, back in 2.6.18-rc.  Maybe ask
Lennart Poettering about it?

What I have seen in laptops is:

"Wireless Switch" (SW_RADIO -- this one has a bad name, might even be
what caused me to screw up with the KEY_RADIO thing, and it hints that
my brain corruption was deep rooted, since I was the one who came up
with SW_RADIO).   These switches do their plain best to remove all RF
output when in the "wireles off" position, and stop bothering RF output
when in the "wireless on" position.  Transitions from off->on are often
used as a hint to bring up wireless data communication services (such as
firing up wirelesss settings GUI pannels, etc).

"Wireless hot-key", which either brings up an GUI related to which
wireless data communication radios are active and their configuration,
or directly changes the set of active wireless data communication
radios.

One could easily argue that, should we make it possible for userspace to
interact with the global rfkill state for each rfkill switch type, all
handling of the "Wireless hot-key" should be done in userspace.  I will
go with that for now, and think about it some more.

New patches coming soon.  Please don't merge ANY of the patches in this
set, I will respin the entire series.

-- 
  "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 3/8] rfkill: handle KEY_RADIO and SW_RADIO events

[Ivo van Doorn]

On Saturday 12 April 2008, Henrique de Moraes Holschuh wrote:
> On Sat, 12 Apr 2008, Dmitry Torokhov wrote:
> > On Fri, Apr 11, 2008 at 05:37:19PM -0300, Henrique de Moraes Holschuh wrote:
> > > The *_RADIO input events are related to all radios in a system.  There are
> > > two: KEY_RADIO and SW_RADIO.
> > > 
> > 
> > KEY_RADIO is reserved for selecting radio input (as pooosed to TV, AUX,
> > etc) with a remote control. Rfkill woudl need a separate keycode if it
> > needs "all types of radios" event.
> 
> Hmm, let me check where I got the wrong idea of using KEY_RADIO from,
> because thinkpad-acpi already uses KEY_WLAN which means that at least in
> the past I did know KEY_RADIO was not to be used for wireless data
> communication devices like that...  jeez, looks like spontaneous brain
> corruption on that topic hapenned to me sometime ago, and it came out in
> a linux-thinkpad thread a few weeks ago.  The wrong semanthics for
> KEY_RADIO seem to have stuck to my mind since then.  Drat, I *really*
> apologise for this one.
> 
> This, of course, is a major NAK for this patch.  And I am considering
> dropping the handling of KEY_<whatever replaces RADIO> completely from
> it.  SW_RADIO (after a rename, see below) still needs to be handled,
> though.
> 
> > Btw, is there any devices in the wild that actually have separate
> > switches for different types of transmitters?
> 
> Separate switches?  I know of none.
> 
> Separate hot keys/buttons?  I haven't seen it, but check commit
> 90da11514562020ea7d697982f912ac949adc317's comment.  That was the commit
> which added KEY_WLAN and KEY_BLUETOOTH, back in 2.6.18-rc.  Maybe ask
> Lennart Poettering about it?

My laptop (Acer Ferrari 3200) features 2 keys, 1 for 802.11 and 1 for Bluetooth.
Both directly communicate to the hardware itself and only need rfkill for
notification purposes to userspace.

Ivo

Re: [PATCH 3/8] rfkill: handle KEY_RADIO and SW_RADIO events

[Carlos Corbacho]

On Saturday 12 April 2008 19:14:41 Ivo van Doorn wrote:
> > Separate hot keys/buttons?  I haven't seen it, but check commit
> > 90da11514562020ea7d697982f912ac949adc317's comment.  That was the commit
> > which added KEY_WLAN and KEY_BLUETOOTH, back in 2.6.18-rc.  Maybe ask
> > Lennart Poettering about it?
>
> My laptop (Acer Ferrari 3200) features 2 keys, 1 for 802.11 and 1 for
> Bluetooth. Both directly communicate to the hardware itself and only need
> rfkill for notification purposes to userspace.

The Ferrari's are the odd-one-out of Acer's lineup - these generally tend to 
just work out-of-the-box for the wireless and bluetooth buttons.

Most of the other Acer laptops just have software buttons (KEY_WLAN and 
KEY_BLUETOOTH), and need another driver to make them do anything (acer-wmi is 
one example), and the wireless card has no control over it's rfkill switch 
(though there are a few cases where this is apparently not the case).

Although the current wireless driver implementations of rfkill/ input polling 
have been causing me a bit of a headache lately, with no nice solution in 
site, because of the following (b43 does this, but I suspect rt2x00 may also 
do the same, so it's a slightly more generic problem). Now, I have a patch 
locally to add rfkill support to acer-wmi, but still keep seeing the 
following:

1) acer-wmi loads
2) Press KEY_WLAN
3) rfkill-input notifies all registered KEY_WLAN rfkill drivers that KEY_WLAN 
has been pressed.
4) acer-wmi toggles the wireless radio state
5) b43 notices this state change because of it's polling, then sends a 
KEY_WLAN event (even though b43 knows it can't control the radio on my 
laptop)
6) Goto 3.

And this continues in a loop. Removing the offending input_report_key() lines 
in the wireless driver stops this.

Ideally, I think we are going to need some way of saying that platform 
firmware level drivers (e.g. acer-wmi, thinkpad-acpi, etc) take priority over 
the wireless drivers, and that the drivers should _not_ send out these 
KEY_WLAN events if one of acer-wmi, etc are loaded (or find some other nice 
way of handling this).

-Carlos
-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

Re: [PATCH 3/8] rfkill: handle KEY_RADIO and SW_RADIO events

[Henrique de Moraes Holschuh]

On Sat, 12 Apr 2008, Carlos Corbacho wrote:
> Although the current wireless driver implementations of rfkill/ input polling 
> have been causing me a bit of a headache lately, with no nice solution in 
> site, because of the following (b43 does this, but I suspect rt2x00 may also 
> do the same, so it's a slightly more generic problem). Now, I have a patch 
> locally to add rfkill support to acer-wmi, but still keep seeing the 
> following:
> 
> 1) acer-wmi loads
> 2) Press KEY_WLAN
> 3) rfkill-input notifies all registered KEY_WLAN rfkill drivers that KEY_WLAN 
> has been pressed.

No.  It changes the global state for WLAN rfkill switches, and then it
causes rfkill to call the toggle_radio(new global state) hooks of all
these switches.  I know that's what you meant, but we better be explicit
about this.

There is NO notification of a key press, there is an explicit command to
change the radio switch state to an specific new state (it is not a
toggle command, even if the name of the hook is toggle_radio).

> 4) acer-wmi toggles the wireless radio state
> 5) b43 notices this state change because of it's polling, then sends a 
> KEY_WLAN event (even though b43 knows it can't control the radio on my 
> laptop)

b43 should not be issuing KEY_WLAN events in this case.  It should be
updating its rfkill->state (one of my patches add a proper way to do
this), but that's it.  See below.

> 6) Goto 3.
>
> And this continues in a loop. Removing the offending input_report_key() lines 
> in the wireless driver stops this.

No wonder it does, it is a Bug From Hell to use the input layer to
*report* events that happened and were already handled.  You use it to
*issue* events that *are still to be acted upon*, and that's only if you
are NOT the only one that will act upon them!  Sometimes it is not
trivial at all to know in which side of the fence you are.

And here things get difficult.  ipw2200 (or b43, or iwl*, etc) would
HAVE to know if its hardware rfkill input pin is supposed to be handled
as the canon way to know if rfkill is active or not, for example, not to
mention wheter it even makes sense for them to request that all other
radios change state (THAT is what issuing an input event means).

The simple, and IMHO any sane solution is to do the following:

1. Add a kernel notifier chain for b43, ipw*, iwl*, and other such
drivers to report any changes in their radio state (which is a product
of ALL their rfkill lines, software and hardware), and also changes in
their hardware rfkill input pins, using different events so that you
KNOW which ones are informing the world about input pin state changes,
and which one are just reports of a changed radio state.

2. NEVER issue input events from these drivers. At all. Just update the
rfkill switch state using rfkill_force_state, or let the pooling do it.

Then, we leave for userspace (hal + hal-info?) or a kernel platform
driver to decide if the kernel notification b43/ipw*/ilw*/etc issued
when it noticed its rfkill input pin changed state is to be made an
input event.

> Ideally, I think we are going to need some way of saying that platform 
> firmware level drivers (e.g. acer-wmi, thinkpad-acpi, etc) take priority over 
> the wireless drivers, and that the drivers should _not_ send out these 
> KEY_WLAN events if one of acer-wmi, etc are loaded (or find some other nice 
> way of handling this).

Better to have nothing but the platform firmware drivers issue such
input events.  Have all other drivers issue notification events instead
(which are not input events), and have something else that can be model
specific decide which notification events are to be made input 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

[PATCH 4/8] rfkill: add read-write rfkill switch support

[Henrique de Moraes Holschuh]

Currently, rfkill supports only write-only rfkill switches.  There is no
provision for querying the current switch state from the hardware/firmware.

This is bad on hardware where the switch state can change behind the
kernel's back (which is rather common).  There is no reason to keep kernel
state incorrect, when we have the possibility to match reality.

There is also the issue of read-only rfkill switches (support to be added
in a later patch), which absolutely requires support to read the current
state from the hardware in order to be implemented.

In order to implement the read/write functionality:

Add a get_state() hook that is called by the class every time it needs to
fetch the current state of the switch.  Add a call to this hook every time
the *current* state of the radio plays a role in a decision.

Also add a force_state() method that can be used to forcefully syncronize
the class' idea of the current state of the switch.  This allows for a
faster implementation of the read/write functionality, as a driver which
get events on switch changes can avoid the need for a get_state() hook.

If the get_state() hook is left as NULL, current behaviour is maintained,
so this change is fully backwards compatible with the current rfkill
drivers.

If the firmware is event driven, leave get_state() NULL in the driver, set
the initial state properly before registering the rfkill class, and use the
force_state() method in the driver to keep the class up-to-date.

get_state() can be called by the class from atomic context. It must not
sleep.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
Cc: John W. Linville <linville@tuxdriver.com>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 include/linux/rfkill.h |    5 ++++
 net/rfkill/rfkill.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index ca89ae1..844e961 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -61,6 +61,8 @@ enum rfkill_state {
  * @data: Pointer to the RF button drivers private data which will be
  *	passed along when toggling radio state.
  * @toggle_radio(): Mandatory handler to control state of the radio.
+ * @get_state(): handler to read current radio state from hardware,
+ *      may be called from atomic context, should return 0 on success.
  * @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
@@ -80,6 +82,7 @@ struct rfkill {
 
 	void *data;
 	int (*toggle_radio)(void *data, enum rfkill_state state);
+	int (*get_state)(void *data, enum rfkill_state *state);
 
 #ifdef CONFIG_RFKILL_LEDS
 	struct led_trigger led_trigger;
@@ -95,6 +98,8 @@ void rfkill_free(struct rfkill *rfkill);
 int rfkill_register(struct rfkill *rfkill);
 void rfkill_unregister(struct rfkill *rfkill);
 
+int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state);
+
 /**
  * rfkill_get_led_name - Get the LED trigger name for the button's LED.
  * This function might return a NULL pointer if registering of the
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 1601e50..88b0558 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -57,19 +57,39 @@ static void rfkill_led_trigger(struct rfkill *rfkill,
 #endif /* CONFIG_RFKILL_LEDS */
 }
 
+static void update_rfkill_state(struct rfkill *rfkill)
+{
+	enum rfkill_state newstate;
+
+	if (rfkill->get_state) {
+		mutex_lock(&rfkill->mutex);
+		if (!rfkill->get_state(rfkill->data, &newstate))
+			rfkill->state = newstate;
+		mutex_unlock(&rfkill->mutex);
+	}
+}
+
 static int rfkill_toggle_radio(struct rfkill *rfkill,
 				enum rfkill_state state)
 {
 	int retval = 0;
+	enum rfkill_state oldstate, newstate;
+
+	oldstate = rfkill->state;
+
+	if (rfkill->get_state &&
+	    !rfkill->get_state(rfkill->data, &newstate))
+		rfkill->state = newstate;
 
 	if (state != rfkill->state) {
 		retval = rfkill->toggle_radio(rfkill->data, state);
-		if (!retval) {
+		if (!retval)
 			rfkill->state = state;
-			rfkill_led_trigger(rfkill, state);
-		}
 	}
 
+	if (rfkill->state != oldstate)
+		rfkill_led_trigger(rfkill, rfkill->state);
+
 	return retval;
 }
 
@@ -100,6 +120,28 @@ void rfkill_switch_all(enum rfkill_type type, enum rfkill_state state)
 }
 EXPORT_SYMBOL(rfkill_switch_all);
 
+/**
+ * rfkill_force_state - Force the internal rfkill radio state
+ * @rfkill: pointer to the rfkill class to modify.
+ * @state: the current radio state the class should be forced to.
+ *
+ * This function updates the internal state of the radio cached
+ * by the rfkill class.  It should be used when the driver gets
+ * a notification by the firmware/hardware of the current *real*
+ * state of the radio rfkill switch.
+ *
+ * It may not be called from an atomic context.
+ */
+int rfkill_force_state(struct rfkill *rfkill, enum rfkill_state state)
+{
+	mutex_lock(&rfkill->mutex);
+	rfkill->state = state;
+	mutex_unlock(&rfkill->mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL(rfkill_force_state);
+
 static ssize_t rfkill_name_show(struct device *dev,
 				struct device_attribute *attr,
 				char *buf)
@@ -142,6 +184,7 @@ static ssize_t rfkill_state_show(struct device *dev,
 {
 	struct rfkill *rfkill = to_rfkill(dev);
 
+	update_rfkill_state(rfkill);
 	return sprintf(buf, "%d\n", rfkill->state);
 }
 
-- 
1.5.4.4

Re: [PATCH 4/8] rfkill: add read-write rfkill switch support

[Ivo van Doorn]

Hi,

> Currently, rfkill supports only write-only rfkill switches.  There is no
> provision for querying the current switch state from the hardware/firmware.

No that is by design, the input_polled_dev interface is there for polling.

> This is bad on hardware where the switch state can change behind the
> kernel's back (which is rather common).  There is no reason to keep kernel
> state incorrect, when we have the possibility to match reality.
> 
> There is also the issue of read-only rfkill switches (support to be added
> in a later patch), which absolutely requires support to read the current
> state from the hardware in order to be implemented.

See rt2x00 and b43 in driver-wireless for an example implementation
of pollable input device and rfkill

> In order to implement the read/write functionality:
> 
> Add a get_state() hook that is called by the class every time it needs to
> fetch the current state of the switch.  Add a call to this hook every time
> the *current* state of the radio plays a role in a decision.
> 
> Also add a force_state() method that can be used to forcefully syncronize
> the class' idea of the current state of the switch.  This allows for a
> faster implementation of the read/write functionality, as a driver which
> get events on switch changes can avoid the need for a get_state() hook.
> 
> If the get_state() hook is left as NULL, current behaviour is maintained,
> so this change is fully backwards compatible with the current rfkill
> drivers.
> 
> If the firmware is event driven, leave get_state() NULL in the driver, set
> the initial state properly before registering the rfkill class, and use the
> force_state() method in the driver to keep the class up-to-date.
> 
> get_state() can be called by the class from atomic context. It must not
> sleep.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>
> Cc: John W. Linville <linville@tuxdriver.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>

I'm going to NACK this patch.

Ivo

Re: [PATCH 4/8] rfkill: add read-write rfkill switch support

[Henrique de Moraes Holschuh]

On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > Currently, rfkill supports only write-only rfkill switches.  There is no
> > provision for querying the current switch state from the hardware/firmware.
> 
> No that is by design, the input_polled_dev interface is there for polling.

I have looked into this, now.  And I have to say I am not impressed with
the use of the input system for this.  My guess is that somehow the need
to issue state changes for "the stuff the user slides/presses" got
confused with the need to keep the state of the "hardware and software
that enables/disables radios", just because both are called "rfkill
switches".

The current flow for read/write switches (i.e. using input-polldev) is
this:

The players:
A: RFKILL class code: write-only with a state cache
B: rfkill-input
C: the code supporting the device registered with the rfkill class
D: something else, like a hardware signal line or firmware

The flow:
C: sets initial state, and registers rfkill device with A.
A: keeps state cache in sync with any changes that goes through it
D: toggles radio state directly, A doesn't know about it.
C: using pooling or some other method, finds out what D did.
C: issues an INPUT EVENT(!!) to force A to resync itself
B: traps the INPUT EVENT, and tells A to change radio state FOR ALL
   RADIOS of that type
A: toggles the radio to the state B wanted.

It is no surprise that b43 can't work right if it is using that flow.

Here's what is wrong with it:

  1. Usage of input events that matter to an entire group of drivers
     to syncronize something that is specific to a single device

  2. Dependency of an OPTIONAL input handler to do it, and let's not
     forget this can become a MAJOR mess if userspace decides to butt
     in, which it is indeed *expected* to do

  3. (some drivers like b43, not a rfkill class issue): the use of
     KEY_  events to set a desired state for a radio is wrong.  That
     event tells rfkill to toggle the radio state, and NOT to set it
     to a particular state.  One would need EV_SW events for this.

  4. Extremely heavy-weight, convoluted, complex information flow
     path, prone to races when there are multiple rfkill devices of
     the same type involved.

  5. Breaks completely if user_claim is used.

There might be more issues with this approach, but the above is damn
good enough reason to change the approach, already.

IMPORTANT: do note that there is nothing wrong with using input-polldev
to monitor a hardware GPIO pin or somesuch, and issue input events every
time there is a change in the state of a rfkill switch (in the "the
stuff the user slides/presses" sense).   That's not what I am talking
about.

> > This is bad on hardware where the switch state can change behind the
> > kernel's back (which is rather common).  There is no reason to keep kernel
> > state incorrect, when we have the possibility to match reality.
> > 
> > There is also the issue of read-only rfkill switches (support to be added
> > in a later patch), which absolutely requires support to read the current
> > state from the hardware in order to be implemented.
> 
> See rt2x00 and b43 in driver-wireless for an example implementation
> of pollable input device and rfkill

They're broken by design, at least in b43's case.  Someone (Carlos, I
believe?) posted an example of a ping-pong scenario using b43.

> I'm going to NACK this patch.

Given the above, would you ACK the idea that we implement a read path
for rfkill state (in the "hardware and software that enables/disables
radios" sense), which might as well be something very close to what this
patch did, AND leave input-polldev for the input device side of things?

-- 
  "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 4/8] rfkill: add read-write rfkill switch support

[Ivo van Doorn]

On Monday 14 April 2008, Henrique de Moraes Holschuh wrote:
> On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > > Currently, rfkill supports only write-only rfkill switches.  There is no
> > > provision for querying the current switch state from the hardware/firmware.
> > 
> > No that is by design, the input_polled_dev interface is there for polling.
> 
> I have looked into this, now.  And I have to say I am not impressed with
> the use of the input system for this.  My guess is that somehow the need
> to issue state changes for "the stuff the user slides/presses" got
> confused with the need to keep the state of the "hardware and software
> that enables/disables radios", just because both are called "rfkill
> switches".
>
> The current flow for read/write switches (i.e. using input-polldev) is
> this:
> 
> The players:
> A: RFKILL class code: write-only with a state cache
> B: rfkill-input
> C: the code supporting the device registered with the rfkill class
> D: something else, like a hardware signal line or firmware
> 
> The flow:
> C: sets initial state, and registers rfkill device with A.
> A: keeps state cache in sync with any changes that goes through it
> D: toggles radio state directly, A doesn't know about it.
> C: using pooling or some other method, finds out what D did.
> C: issues an INPUT EVENT(!!) to force A to resync itself
> B: traps the INPUT EVENT, and tells A to change radio state FOR ALL
>    RADIOS of that type
> A: toggles the radio to the state B wanted.
> 
> It is no surprise that b43 can't work right if it is using that flow.
> 
> Here's what is wrong with it:
> 
>   1. Usage of input events that matter to an entire group of drivers
>      to syncronize something that is specific to a single device
>
>   2. Dependency of an OPTIONAL input handler to do it, and let's not
>      forget this can become a MAJOR mess if userspace decides to butt
>      in, which it is indeed *expected* to do

The input handler is optional, because userspace *should* actually do all
the work. Userspace should listen to the registered input device and
it is userspace that should select the input device it wants to listen to.
(perhaps the laptop has 2 or more keys, but the user wants all interfaces
to be toggled when any key has been pressed).

>   3. (some drivers like b43, not a rfkill class issue): the use of
>      KEY_  events to set a desired state for a radio is wrong.  That
>      event tells rfkill to toggle the radio state, and NOT to set it
>      to a particular state.  One would need EV_SW events for this.

That is a rfkill issue due to lack of correct documentation and problems
with the rfkill implementation. I wouldn't blame the drivers for that. ;)

>   4. Extremely heavy-weight, convoluted, complex information flow
>      path, prone to races when there are multiple rfkill devices of
>      the same type involved.
> 
>   5. Breaks completely if user_claim is used.
> 
> There might be more issues with this approach, but the above is damn
> good enough reason to change the approach, already.

True.

> IMPORTANT: do note that there is nothing wrong with using input-polldev
> to monitor a hardware GPIO pin or somesuch, and issue input events every
> time there is a change in the state of a rfkill switch (in the "the
> stuff the user slides/presses" sense).   That's not what I am talking
> about.

Right.
But note that network hardware with such keys will have a driver that handles
both features. So such network drivers will have a input device and will send
a signal that the key was pressed regardless of the current state they are in.

> > > This is bad on hardware where the switch state can change behind the
> > > kernel's back (which is rather common).  There is no reason to keep kernel
> > > state incorrect, when we have the possibility to match reality.
> > > 
> > > There is also the issue of read-only rfkill switches (support to be added
> > > in a later patch), which absolutely requires support to read the current
> > > state from the hardware in order to be implemented.
> > 
> > See rt2x00 and b43 in driver-wireless for an example implementation
> > of pollable input device and rfkill
> 
> They're broken by design, at least in b43's case.  Someone (Carlos, I
> believe?) posted an example of a ping-pong scenario using b43.

Thats true. But was that a scenario where there were 2 keys for the same radio type?
Or was that a matter of the driver indicating a key pressed event while it has no
attached key to it, and only checked the register for the key status and the current radio
status and saw it was different?

For example rt2x00 does:
hasHardwareKey()
 yes -> register input_polldev, check key status
 no -> don't do anything.

Note that write-only support hasn't been implemented yet, so rt2x00 will do nothing
if the hardware doesn't have a hardware key attached to it.

> > I'm going to NACK this patch.
> 
> Given the above, would you ACK the idea that we implement a read path
> for rfkill state (in the "hardware and software that enables/disables
> radios" sense), which might as well be something very close to what this
> patch did, AND leave input-polldev for the input device side of things?

Depends on the implementation, I think we should at least have the following requirements:
 - _all_ wireless (wifi, bluetooth, etc) network drivers register themselves to rfkill
 - radio-key driver polls or listens to hardware events and sends them through the input layer
 - it must be caught by either:
	1) userspace (default): This can either run a lot of scripts of send a notification to rfkill
	2) rfkill-input: sens notification to rfkill
 - rfkill goes past all registered rfkill instances to toggle the status.

Note that the rfkill and rfkill-input description is incomplete since that is where this thread
is about. So the actions that those 2 layers should exactly do is not defined yet in this list.
However the first 2 points should be the case regardless of how rfkill handles things.

Ivo

Re: [PATCH 4/8] rfkill: add read-write rfkill switch support

[Dmitry Torokhov]

On Mon, Apr 14, 2008 at 02:00:41PM +0200, Ivo van Doorn wrote:
> On Monday 14 April 2008, Henrique de Moraes Holschuh wrote:
> > On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > > > Currently, rfkill supports only write-only rfkill switches.  There is no
> > > > provision for querying the current switch state from the hardware/firmware.
> > > 
> > > No that is by design, the input_polled_dev interface is there for polling.
> > 
> > I have looked into this, now.  And I have to say I am not impressed with
> > the use of the input system for this.  My guess is that somehow the need
> > to issue state changes for "the stuff the user slides/presses" got
> > confused with the need to keep the state of the "hardware and software
> > that enables/disables radios", just because both are called "rfkill
> > switches".
> >
> > The current flow for read/write switches (i.e. using input-polldev) is
> > this:
> > 
> > The players:
> > A: RFKILL class code: write-only with a state cache
> > B: rfkill-input
> > C: the code supporting the device registered with the rfkill class
> > D: something else, like a hardware signal line or firmware
> > 
> > The flow:
> > C: sets initial state, and registers rfkill device with A.
> > A: keeps state cache in sync with any changes that goes through it
> > D: toggles radio state directly, A doesn't know about it.
> > C: using pooling or some other method, finds out what D did.
> > C: issues an INPUT EVENT(!!) to force A to resync itself
> > B: traps the INPUT EVENT, and tells A to change radio state FOR ALL
> >    RADIOS of that type
> > A: toggles the radio to the state B wanted.
> > 
> > It is no surprise that b43 can't work right if it is using that flow.
> > 
> > Here's what is wrong with it:
> > 
> >   1. Usage of input events that matter to an entire group of drivers
> >      to syncronize something that is specific to a single device
> >
> >   2. Dependency of an OPTIONAL input handler to do it, and let's not
> >      forget this can become a MAJOR mess if userspace decides to butt
> >      in, which it is indeed *expected* to do
> 
> The input handler is optional, because userspace *should* actually do all
> the work. Userspace should listen to the registered input device and
> it is userspace that should select the input device it wants to listen to.
> (perhaps the laptop has 2 or more keys, but the user wants all interfaces
> to be toggled when any key has been pressed).
> 
> >   3. (some drivers like b43, not a rfkill class issue): the use of
> >      KEY_  events to set a desired state for a radio is wrong.  That
> >      event tells rfkill to toggle the radio state, and NOT to set it
> >      to a particular state.  One would need EV_SW events for this.
> 
> That is a rfkill issue due to lack of correct documentation and problems
> with the rfkill implementation. I wouldn't blame the drivers for that. ;)
> 
> >   4. Extremely heavy-weight, convoluted, complex information flow
> >      path, prone to races when there are multiple rfkill devices of
> >      the same type involved.
> > 
> >   5. Breaks completely if user_claim is used.
> > 

What exactly breaks with user_claim? I think the general issue is that
rfkill was originally not supposed to be used with switches that can't
be programmatically controlled. If we have a switch that mechanically
cuts off power to the RF transmitter rfkill can't relly do anything.
The input side (input polldev or other kind of button/switch/slider
inmplementation) still can issue KEY_WLAN or something to indicate that
user toggled the state and userspace or kernel may try to shut off the
rest of RF transmitters in the box but as far as this particlular
switch goes it is game over. I expected that here we'd just rely on
netif_carrier_* to notify the network side of things about interface
losing connection.

Unfortunately (or may be fortunately, I am not quite sure yet) rfkill
started to be used for mechanical type switches as well, disabling
user_claim option, so now we do need the "read-only" kind of rfkill
switch support that allows setting switch state directly from the
driver.

-- 
Dmitry

Re: [PATCH 4/8] rfkill: add read-write rfkill switch support

[Henrique de Moraes Holschuh]

On Mon, 14 Apr 2008, Dmitry Torokhov wrote:
> What exactly breaks with user_claim? I think the general issue is that

The same thing that happens if userspace is in charge and doesn't
cooperate or if rfkill-input is not loaded:  the rfkill driver state is
never updated (remember, it is about the radio controller, not the input
device).

> rfkill was originally not supposed to be used with switches that can't
> be programmatically controlled. If we have a switch that mechanically

The input layer does not provide a way for userspace to get access to
the current state of any switches easily (or at all?).  It just exports
events reporting these states sometimes.

We could fix it in the input subsystem, instead.  How difficult (and
welcome a change?) would it be for the input device infrastructure to
provide pollable (for good measure!) "state" attributes for every EV_SW
event an input device registers in evbits+swbits ?

If the input subsystem exports that information, we can define that
slave read-only radio controllers must NOT be registered as rfkill
devices, and drop support for read-only rfkill devices, as all master
rfkill devices (which are input devices by definition) would get their
state exported to userspace through the input layer.

However, that DOES mean userspace would need to look for rfkill state in
two places.  Input layer, to get the current state of the rfkill input
devices, and rfkill class to get the current state of the rfkill
radio controllers.

> The input side (input polldev or other kind of button/switch/slider
> inmplementation) still can issue KEY_WLAN or something to indicate that
> user toggled the state and userspace or kernel may try to shut off the
> rest of RF transmitters in the box but as far as this particlular
> switch goes it is game over. I expected that here we'd just rely on
> netif_carrier_* to notify the network side of things about interface
> losing connection.

Yes.  And that's the plan, actually.  But it DOES NOT work at all with
the current mess, which used input events to keep the internal
rfkill->state state in sync with the hardware.

> Unfortunately (or may be fortunately, I am not quite sure yet) rfkill

Let's decide this NOW, then.  But if we decide to not do it in rfkill,
you will have to add attributes to every input device which has
registered itself as a source of EV_SW events.

> started to be used for mechanical type switches as well, disabling
> user_claim option, so now we do need the "read-only" kind of rfkill
> switch support that allows setting switch state directly from the
> driver.

We always had read/write rfkill controllers, they were just not properly
supported at all, and that is the root cause of the mess.


Anyway, do we fix read-only rfkill input device support in the input
layer (add access to their state using sysfs and make it clear they are
NOT to be registered with rfkill class) or in the rfkill class (add
read-only rfkill controller support)?

I better make it clear that I have NO idea how to properly implement the
required changes in the input layer if we go that way so if it is left
for me to do that work, it won't be ready anytime soon and it might be
supbar, so I'd appreciate lots of help in that case.

-- 
  "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 4/8] rfkill: add read-write rfkill switch support

[Dmitry Torokhov]

On Mon, Apr 14, 2008 at 11:36:03AM -0300, Henrique de Moraes Holschuh wrote:
> On Mon, 14 Apr 2008, Dmitry Torokhov wrote:
> > What exactly breaks with user_claim? I think the general issue is that
> 
> The same thing that happens if userspace is in charge and doesn't
> cooperate or if rfkill-input is not loaded:  the rfkill driver state is
> never updated (remember, it is about the radio controller, not the input
> device).
> 

And it should not - setting user_claim means userspace controls the
transmitter state. If userspace decides to ignore KEY_* event and not
turn the radio on or off - that is fine. rfkill-input simply provides
default kernel policy for reacting to KEY_WLAN, KEY_BLUETOOTH, etc.
If there are drivers sense button presses and alter their actual
transmitter state immediately but then rely on rfkill-input to update
their sysfs rfkill attributes then such drivers should be fixed.

> > rfkill was originally not supposed to be used with switches that can't
> > be programmatically controlled. If we have a switch that mechanically
> 
> The input layer does not provide a way for userspace to get access to
> the current state of any switches easily (or at all?).  It just exports
> events reporting these states sometimes.
>

You can use EVIOCGSW to get state of switches. I'd expect applications
to use it when they open devices first time and then rely on event
delivery to monitor changes in state of input devices.

> We could fix it in the input subsystem, instead.  How difficult (and
> welcome a change?) would it be for the input device infrastructure to
> provide pollable (for good measure!) "state" attributes for every EV_SW
> event an input device registers in evbits+swbits ?
> 

Given the above I don't see the need.

> If the input subsystem exports that information, we can define that
> slave read-only radio controllers must NOT be registered as rfkill
> devices, and drop support for read-only rfkill devices, as all master
> rfkill devices (which are input devices by definition) would get their
> state exported to userspace through the input layer.
> 
> However, that DOES mean userspace would need to look for rfkill state in
> two places.  Input layer, to get the current state of the rfkill input
> devices, and rfkill class to get the current state of the rfkill
> radio controllers.
>

I think you outlined the problem in your other mail quite well. We
keep confusing button state with the transmitter state. Only for
buttons that are hardwired to transmitters the states are the same,
for programmatically controlled RF transmitters state may be
different. So if you need to see state of transmitters you should look
in /sys/class/rfkill and for input devices - hook into input devices.
 
> > The input side (input polldev or other kind of button/switch/slider
> > inmplementation) still can issue KEY_WLAN or something to indicate that
> > user toggled the state and userspace or kernel may try to shut off the
> > rest of RF transmitters in the box but as far as this particlular
> > switch goes it is game over. I expected that here we'd just rely on
> > netif_carrier_* to notify the network side of things about interface
> > losing connection.
> 
> Yes.  And that's the plan, actually.  But it DOES NOT work at all with
> the current mess, which used input events to keep the internal
> rfkill->state state in sync with the hardware.
> 
> > Unfortunately (or may be fortunately, I am not quite sure yet) rfkill
> 
> Let's decide this NOW, then.  But if we decide to not do it in rfkill,
> you will have to add attributes to every input device which has
> registered itself as a source of EV_SW events.
>

No, since they may not be tied directly to any RF transmitter.

> > started to be used for mechanical type switches as well, disabling
> > user_claim option, so now we do need the "read-only" kind of rfkill
> > switch support that allows setting switch state directly from the
> > driver.
> 
> We always had read/write rfkill controllers, they were just not properly
> supported at all, and that is the root cause of the mess.
> 
> 
> Anyway, do we fix read-only rfkill input device support in the input
> layer (add access to their state using sysfs and make it clear they are
> NOT to be registered with rfkill class) or in the rfkill class (add
> read-only rfkill controller support)?
> 
> I better make it clear that I have NO idea how to properly implement the
> required changes in the input layer if we go that way so if it is left
> for me to do that work, it won't be ready anytime soon and it might be
> supbar, so I'd appreciate lots of help in that case.
> 

Well, let's see.. Do we have any issues with transmitters that can be
controlled programmatically? Or is it the hardwired ones that give us
trouble at the moment? I think that exporting all of them in rfkill is
a good idea, even though originally I thought we would not need to do
that. Therefore we do need to allow hardwided drivers updating their
rfkill state directly. The only problem I see is that we need to monitor
this closely, so non-hardwired cases won't try to use this mechanism.

-- 
Dmitry

Re: [PATCH 4/8] rfkill: add read-write rfkill switch support

[Henrique de Moraes Holschuh]

On Mon, 14 Apr 2008, Dmitry Torokhov wrote:
> On Mon, Apr 14, 2008 at 11:36:03AM -0300, Henrique de Moraes Holschuh wrote:
> > On Mon, 14 Apr 2008, Dmitry Torokhov wrote:
> > > What exactly breaks with user_claim? I think the general issue is that
> > 
> > The same thing that happens if userspace is in charge and doesn't
> > cooperate or if rfkill-input is not loaded:  the rfkill driver state is
> > never updated (remember, it is about the radio controller, not the input
> > device).
> > 
> 
> And it should not - setting user_claim means userspace controls the
> transmitter state. If userspace decides to ignore KEY_* event and not

Nuh-uh.  Here's the root of the bug.  The rfkill controller knowing
whether its state is on or off for real has NOTHING to do with the user
telling it to change state.

Something changed the controller's state (the hardware, or the
firmware), and didn't do it using the rfkill class.  That's a fact.  The
rfkill class needs to know of that change through an rfkill->state
update, or it misbehaves (because some stuff takes decisions depending
on the contents of rfkill->state, AND that state is exposed to userspace
which is allowed to also take decisions based on it).  That's another
fact.

And none of it has to do with input events, or what the user wants done.
During this entire process, the kernel did not take ANY active instances
on what should happen to the radio controller.  It didn't change the
radio controller state.  It is just making sure the kernel doesn't think
it is ON when it is OFF, for a particular, specific, radio rfkill
controller.

I propose that we broadcast (through normal kernel notifier chains) that
the state has been changed, and if something is trying to enforce that
no radio rfkill controllers deviate from the global rfkill state for
that radio rfkill controller state, IT should tell the radios to go back
to the state it wants.  But IT is not the rfkill class, nor any input
devices or rfkill devices.  Maybe it is userspace, or rfkill-input, or
another platform-specific policy module.

> turn the radio on or off - that is fine. rfkill-input simply provides
> default kernel policy for reacting to KEY_WLAN, KEY_BLUETOOTH, etc.

So far so good, but it must not be used for something else entirely
(namely: keeping rfkill->state consistent to the real hardware state).
That's what I am talking about...

> If there are drivers sense button presses and alter their actual
> transmitter state immediately but then rely on rfkill-input to update
> their sysfs rfkill attributes then such drivers should be fixed.

That's the point, yes.  Currently *all* drivers have to do this :) and
that's why I did not add thinkpad-acpi rfkill support until I had enough
time to tack on rfkill itself to bring it up to speed with the extra
requirements stuff like thinkpad-acpi and b43 have.

> > > rfkill was originally not supposed to be used with switches that can't
> > > be programmatically controlled. If we have a switch that mechanically
> > 
> > The input layer does not provide a way for userspace to get access to
> > the current state of any switches easily (or at all?).  It just exports
> > events reporting these states sometimes.
> 
> You can use EVIOCGSW to get state of switches. I'd expect applications
> to use it when they open devices first time and then rely on event
> delivery to monitor changes in state of input devices.

Good to know.  But the usual consumers of those are shell scripts, do we
have a swiss-army-knife-for-the-input-layer utility a shell script can
use to do a EVIOCGSW call?  Or to wait for an event?

If we don't, but we provide one to be the companion of lsinput,
input-events and input-kbd, then I agree we can forego the sysfs
attribute.

But DO be warned that userspace developers will see that rfkill exports
a state attribute for the radio rfkill controllers, and will look for
something equally easy to use for the input devices.  There is a big
pontential for confusion and misuse of interfaces there, when you recall
that it will be COMMON to have the same module (driver) provide both a
rfkill controller through the rfkill class, and an input device.

> > We could fix it in the input subsystem, instead.  How difficult (and
> > welcome a change?) would it be for the input device infrastructure to
> > provide pollable (for good measure!) "state" attributes for every EV_SW
> > event an input device registers in evbits+swbits ?
> > 
> 
> Given the above I don't see the need.

So, we can get the required information using an IOCTL.  That means
rfkill class support for read-only devices is no longar mandatory.  It
is still not clear to me it is not *desired*, though.

The reasons are:
  1. Need to have an easy way to get that information for shell scripts,
     and IOCTLs are not (AFAIK).  This can be fixed by adding a shell-
     script helper to input-utils.

  2. It might be better to have all information directly related to
     rfkill behaviour (i.e. both rfkill controller state and rfkill
     input device state) in the same class, and present it in the same
     way to userspace.

Anyone has an opinion about the above?  Provided we can do it without
major complexity (we can, AFAIK, since I did get quite close to it with
the first patchset and the code in there is definately NOT complex), I
think both points could be addressed at the same time, and userspace
can get the information it wants in whichever way it is easier for the
application.

> > If the input subsystem exports that information, we can define that
> > slave read-only radio controllers must NOT be registered as rfkill
> > devices, and drop support for read-only rfkill devices, as all master
> > rfkill devices (which are input devices by definition) would get their
> > state exported to userspace through the input layer.
> > 
> > However, that DOES mean userspace would need to look for rfkill state in
> > two places.  Input layer, to get the current state of the rfkill input
> > devices, and rfkill class to get the current state of the rfkill
> > radio controllers.
> >
> 
> I think you outlined the problem in your other mail quite well. We
> keep confusing button state with the transmitter state. Only for
> buttons that are hardwired to transmitters the states are the same,
> for programmatically controlled RF transmitters state may be
> different. So if you need to see state of transmitters you should look
> in /sys/class/rfkill and for input devices - hook into input devices.

I am not so sure about that last sentence.  See above.  And I really
need to have a very clear and fixed picture of which way to go about
this before I start respinning the rfkill patches :)

> > Let's decide this NOW, then.  But if we decide to not do it in rfkill,
> > you will have to add attributes to every input device which has
> > registered itself as a source of EV_SW events.
> 
> No, since they may not be tied directly to any RF transmitter.

It is valuable information for userspace.  I export the same information
available through SW_TABLET_MODE for example, as a thinkpad-acpi
specific attribute.  Doing EVIOCGSW is not a very friendly interface for
userspace shell scripts right now.

So it *would* be useful.  Whether it is the right way to go about it
(instead of adding a new utility to input-utils to do the EVIOCGSW for
shell scripts and other script languages that are IOCTL-challenged) is
something else.

If you really don't want that exported over sysfs as well, and EVIOCGSW
is the only way to get that information, I feel the immediate need for
an input-ioctl utility for userspace shell scripting.

And there is the all important question about whether I should still
export that information in thinkpad-acpi over pollable attributes (which
are *really* easy to use) or not.  Other driver writers might want to
think about it as well.

> > I better make it clear that I have NO idea how to properly implement the
> > required changes in the input layer if we go that way so if it is left
> > for me to do that work, it won't be ready anytime soon and it might be
> > supbar, so I'd appreciate lots of help in that case.
> 
> Well, let's see.. Do we have any issues with transmitters that can be
> controlled programmatically? Or is it the hardwired ones that give us
> trouble at the moment? I think that exporting all of them in rfkill is

Both, I think.  Any rfkill controller which is not in complete,
exclusive control by the kernel, is currently a problem.  That means
stuff the firmware can change, and it also means any stuff directly tied
to a button or switch that the user can change.

The only things we are handling well right now is pure keys/buttons, and
rfkill radio controllers that are in full control by their kernel driver
(i.e. those with NO sideway control paths that are out of the control of
the rfkill device).

> a good idea, even though originally I thought we would not need to do
> that. Therefore we do need to allow hardwided drivers updating their
> rfkill state directly. The only problem I see is that we need to monitor
> this closely, so non-hardwired cases won't try to use this mechanism.

Well, I do propose that we export any, and *every* radio controller
state change through kernel notifier chains (and to userspace using
uevents).  Those notifications should include the rfkill type (and type
class if we add that), new switch state, and any other relevant
information (e.g. that the switch is a master switch -- those usually
need input events generated, whether the code that produced the
notification will also handle issuing an input event for it or not
(duplicate suppresion), etc).

We can monitor all rfkill radio controllers from userspace or
rfkill-input through those chains.

-- 
  "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 4/8] rfkill: add read-write rfkill switch support

[Dmitry Torokhov]

On Mon, Apr 14, 2008 at 01:33:57PM -0300, Henrique de Moraes Holschuh wrote:
> On Mon, 14 Apr 2008, Dmitry Torokhov wrote:
> > On Mon, Apr 14, 2008 at 11:36:03AM -0300, Henrique de Moraes Holschuh wrote:
> > > On Mon, 14 Apr 2008, Dmitry Torokhov wrote:
> > > > What exactly breaks with user_claim? I think the general issue is that
> > > 
> > > The same thing that happens if userspace is in charge and doesn't
> > > cooperate or if rfkill-input is not loaded:  the rfkill driver state is
> > > never updated (remember, it is about the radio controller, not the input
> > > device).
> > > 
> > 
> > And it should not - setting user_claim means userspace controls the
> > transmitter state. If userspace decides to ignore KEY_* event and not
> 
> Nuh-uh.  Here's the root of the bug.  The rfkill controller knowing
> whether its state is on or off for real has NOTHING to do with the user
> telling it to change state.
> 

Nuh-uh. Nothing changed the transmitter sate yet. We just got a
request from user to do something.

> Something changed the controller's state (the hardware, or the
> firmware), and didn't do it using the rfkill class.  That's a fact.

And this is a bug. How come hardware state changes without kernel not
beign aware? The only case is hard-wired switch. Here I agree that if
such switches are exported via rfkill to userspace the driver should
be able to set rfkill state to match its own. In all other cases, when
you can programmatically control RF state you shoudl defer the actual
switch until user[space] tells you to do so. I guess firmware could
work like a hard-wired switch, turnign radio off when battery goes
extremely low, so yes, I agree that allowing drivers to update their
state directly is required.

> The
> rfkill class needs to know of that change through an rfkill->state
> update, or it misbehaves (because some stuff takes decisions depending
> on the contents of rfkill->state, AND that state is exposed to userspace
> which is allowed to also take decisions based on it).  That's another
> fact.
> 
> And none of it has to do with input events, or what the user wants done.
> During this entire process, the kernel did not take ANY active instances
> on what should happen to the radio controller.  It didn't change the
> radio controller state.  It is just making sure the kernel doesn't think
> it is ON when it is OFF, for a particular, specific, radio rfkill
> controller.
> 
> I propose that we broadcast (through normal kernel notifier chains) that
> the state has been changed, and if something is trying to enforce that
> no radio rfkill controllers deviate from the global rfkill state for
> that radio rfkill controller state, IT should tell the radios to go back
> to the state it wants.  But IT is not the rfkill class, nor any input
> devices or rfkill devices.  Maybe it is userspace, or rfkill-input, or
> another platform-specific policy module.
>

I agree. Here you are talking about hardwired-like scenarios where
kernel did not get a chance to interfere with the RF toggle. Still, if
there was a button involved it needs to be broadcasted so that the
rest of RF transmitters can be adjusted according to the used
policy.

> > turn the radio on or off - that is fine. rfkill-input simply provides
> > default kernel policy for reacting to KEY_WLAN, KEY_BLUETOOTH, etc.
> 
> So far so good, but it must not be used for something else entirely
> (namely: keeping rfkill->state consistent to the real hardware state).
> That's what I am talking about...
> 

Here I agree.

> > If there are drivers sense button presses and alter their actual
> > transmitter state immediately but then rely on rfkill-input to update
> > their sysfs rfkill attributes then such drivers should be fixed.
> 
> That's the point, yes.  Currently *all* drivers have to do this :) and
> that's why I did not add thinkpad-acpi rfkill support until I had enough
> time to tack on rfkill itself to bring it up to speed with the extra
> requirements stuff like thinkpad-acpi and b43 have.

No, I dont believe all drivers have to do this. Ivo will correct me if
Im wrong but I believe he worked with drivers that dont necessarily
have the switch hard wired.

> 
> > > > rfkill was originally not supposed to be used with switches that can't
> > > > be programmatically controlled. If we have a switch that mechanically
> > > 
> > > The input layer does not provide a way for userspace to get access to
> > > the current state of any switches easily (or at all?).  It just exports
> > > events reporting these states sometimes.
> > 
> > You can use EVIOCGSW to get state of switches. I'd expect applications
> > to use it when they open devices first time and then rely on event
> > delivery to monitor changes in state of input devices.
> 
> Good to know.  But the usual consumers of those are shell scripts, do we
> have a swiss-army-knife-for-the-input-layer utility a shell script can
> use to do a EVIOCGSW call?  Or to wait for an event?
> 
> If we don't, but we provide one to be the companion of lsinput,
> input-events and input-kbd, then I agree we can forego the sysfs
> attribute.
> 
> But DO be warned that userspace developers will see that rfkill exports
> a state attribute for the radio rfkill controllers, and will look for
> something equally easy to use for the input devices.

Should I add attribute for every key on a keyboard just because some
userspace developers what to use shell scripts everywhere? key_a,
key_b, key_c and so on? No.

>  There is a big
> pontential for confusion and misuse of interfaces there, when you recall
> that it will be COMMON to have the same module (driver) provide both a
> rfkill controller through the rfkill class, and an input device.
> 

As soon as people will realize that button and RF transmitter are
different objects serving different purposes it all goes away. The
very same module also registers network device, why rfkill is so
different.

> > > We could fix it in the input subsystem, instead.  How difficult (and
> > > welcome a change?) would it be for the input device infrastructure to
> > > provide pollable (for good measure!) "state" attributes for every EV_SW
> > > event an input device registers in evbits+swbits ?
> > > 
> > 
> > Given the above I don't see the need.
> 
> So, we can get the required information using an IOCTL.  That means
> rfkill class support for read-only devices is no longar mandatory.  It
> is still not clear to me it is not *desired*, though.
> 
> The reasons are:
>   1. Need to have an easy way to get that information for shell scripts,
>      and IOCTLs are not (AFAIK).  This can be fixed by adding a shell-
>      script helper to input-utils.
> 
>   2. It might be better to have all information directly related to
>      rfkill behaviour (i.e. both rfkill controller state and rfkill
>      input device state) in the same class, and present it in the same
>      way to userspace.

No, we need to keep RF transmitetrs and buttons separate. I can assign
KEY_WLAN to one of the media keys on my laptop (that I dont use
otherwise). Does it make sense to "move" my keyboard to rfkill
vicinity? I dont think so. Its just a button.

> 
> Anyone has an opinion about the above?  Provided we can do it without
> major complexity (we can, AFAIK, since I did get quite close to it with
> the first patchset and the code in there is definately NOT complex), I
> think both points could be addressed at the same time, and userspace
> can get the information it wants in whichever way it is easier for the
> application.
> 
> > > If the input subsystem exports that information, we can define that
> > > slave read-only radio controllers must NOT be registered as rfkill
> > > devices, and drop support for read-only rfkill devices, as all master
> > > rfkill devices (which are input devices by definition) would get their
> > > state exported to userspace through the input layer.
> > > 
> > > However, that DOES mean userspace would need to look for rfkill state in
> > > two places.  Input layer, to get the current state of the rfkill input
> > > devices, and rfkill class to get the current state of the rfkill
> > > radio controllers.
> > >
> > 
> > I think you outlined the problem in your other mail quite well. We
> > keep confusing button state with the transmitter state. Only for
> > buttons that are hardwired to transmitters the states are the same,
> > for programmatically controlled RF transmitters state may be
> > different. So if you need to see state of transmitters you should look
> > in /sys/class/rfkill and for input devices - hook into input devices.
> 
> I am not so sure about that last sentence.  See above.  And I really
> need to have a very clear and fixed picture of which way to go about
> this before I start respinning the rfkill patches :)
> 
> > > Let's decide this NOW, then.  But if we decide to not do it in rfkill,
> > > you will have to add attributes to every input device which has
> > > registered itself as a source of EV_SW events.
> > 
> > No, since they may not be tied directly to any RF transmitter.
> 
> It is valuable information for userspace.  I export the same information
> available through SW_TABLET_MODE for example, as a thinkpad-acpi
> specific attribute.  Doing EVIOCGSW is not a very friendly interface for
> userspace shell scripts right now.
> 
> So it *would* be useful.  Whether it is the right way to go about it
> (instead of adding a new utility to input-utils to do the EVIOCGSW for
> shell scripts and other script languages that are IOCTL-challenged) is
> something else.
> 
> If you really don't want that exported over sysfs as well, and EVIOCGSW
> is the only way to get that information, I feel the immediate need for
> an input-ioctl utility for userspace shell scripting.
> 
> And there is the all important question about whether I should still
> export that information in thinkpad-acpi over pollable attributes (which
> are *really* easy to use) or not.  Other driver writers might want to
> think about it as well.

If we export switches then we need to export leds. And keys. And
axis... It all adds up very quickly and wastes kernel memory. Just
write ioctl utility for script-users if such is needed and be done
with it.

> 
> > > I better make it clear that I have NO idea how to properly implement the
> > > required changes in the input layer if we go that way so if it is left
> > > for me to do that work, it won't be ready anytime soon and it might be
> > > supbar, so I'd appreciate lots of help in that case.
> > 
> > Well, let's see.. Do we have any issues with transmitters that can be
> > controlled programmatically? Or is it the hardwired ones that give us
> > trouble at the moment? I think that exporting all of them in rfkill is
> 
> Both, I think.  Any rfkill controller which is not in complete,
> exclusive control by the kernel, is currently a problem.  That means
> stuff the firmware can change, and it also means any stuff directly tied
> to a button or switch that the user can change.
> 
> The only things we are handling well right now is pure keys/buttons, and
> rfkill radio controllers that are in full control by their kernel driver
> (i.e. those with NO sideway control paths that are out of the control of
> the rfkill device).
> 
> > a good idea, even though originally I thought we would not need to do
> > that. Therefore we do need to allow hardwided drivers updating their
> > rfkill state directly. The only problem I see is that we need to monitor
> > this closely, so non-hardwired cases won't try to use this mechanism.
> 
> Well, I do propose that we export any, and *every* radio controller
> state change through kernel notifier chains (and to userspace using
> uevents).  Those notifications should include the rfkill type (and type
> class if we add that), new switch state, and any other relevant
> information (e.g. that the switch is a master switch -- those usually
> need input events generated, whether the code that produced the
> notification will also handle issuing an input event for it or not
> (duplicate suppresion), etc).
> 

Notfiers are a good idea for RF transmitter state changes. Not so for
key/button notifiers because these can be ussued by regular PS/2 or
USB keyboards and other devices that have no idea about rfkill and
dont want to know.

> We can monitor all rfkill radio controllers from userspace or
> rfkill-input through those chains.

-- 
Dmitry

Re: [PATCH 4/8] rfkill: add read-write rfkill switch support

[Henrique de Moraes Holschuh]

On Mon, 14 Apr 2008, Dmitry Torokhov wrote:
> On Mon, Apr 14, 2008 at 01:33:57PM -0300, Henrique de Moraes Holschuh wrote:
> > On Mon, 14 Apr 2008, Dmitry Torokhov wrote:
> > > On Mon, Apr 14, 2008 at 11:36:03AM -0300, Henrique de Moraes Holschuh wrote:
> > > > On Mon, 14 Apr 2008, Dmitry Torokhov wrote:
> > > > > What exactly breaks with user_claim? I think the general issue is that
> > > > 
> > > > The same thing that happens if userspace is in charge and doesn't
> > > > cooperate or if rfkill-input is not loaded:  the rfkill driver state is
> > > > never updated (remember, it is about the radio controller, not the input
> > > > device).
> > > > 
> > > 
> > > And it should not - setting user_claim means userspace controls the
> > > transmitter state. If userspace decides to ignore KEY_* event and not
> > 
> > Nuh-uh.  Here's the root of the bug.  The rfkill controller knowing
> > whether its state is on or off for real has NOTHING to do with the user
> > telling it to change state.
> 
> Nuh-uh. Nothing changed the transmitter sate yet. We just got a
> request from user to do something.

We are talking about different things. Please re-read the last three or
four messages before this one, thread-wise.


> switch until user[space] tells you to do so. I guess firmware could
> work like a hard-wired switch, turnign radio off when battery goes
> extremely low, so yes, I agree that allowing drivers to update their
> state directly is required.

Ok, then we DO agree.  Let's drop this point and go to the next one...

> I agree. Here you are talking about hardwired-like scenarios where
> kernel did not get a chance to interfere with the RF toggle. Still, if

Actually, I am talking about ANY scenario where the kernel did not get a
chance to interfere with the RF toggle.

> there was a button involved it needs to be broadcasted so that the
> rest of RF transmitters can be adjusted according to the used
> policy.

We must only broadcast master events, these switches are often tied to
each other in a tree, you can't report events on the leaves, just the
root.  In a ThinkPad T60, you get an rfkill input device (that can be
read), which is hardwired into the WLAN card's rfkill input pin (that
can ALSO be read, and could potentially be registered as a rfkill
controller).  The WLAN card has also a second rfkill controller, which
is in a config register.

I kept that in mind, that's why I started talking about master and slave
rfkill switches.  Only the master ones can also be used as rfkill input
devices.

So yes, if there is a button involved, it must issue one (and only one)
input event.  We agree on this.

> > So far so good, but it must not be used for something else entirely
> > (namely: keeping rfkill->state consistent to the real hardware state).
> > That's what I am talking about...
> 
> Here I agree.

As far as I can see, we actually agree in most issues related to rfkill,
maybe even in everything... but our terminology is NOT in sync, so it is
not immediately obvious that we do.

> > > If there are drivers sense button presses and alter their actual
> > > transmitter state immediately but then rely on rfkill-input to update
> > > their sysfs rfkill attributes then such drivers should be fixed.
> > 
> > That's the point, yes.  Currently *all* drivers have to do this :) and
> > that's why I did not add thinkpad-acpi rfkill support until I had enough
> > time to tack on rfkill itself to bring it up to speed with the extra
> > requirements stuff like thinkpad-acpi and b43 have.
> 
> No, I dont believe all drivers have to do this. Ivo will correct me if
> Im wrong but I believe he worked with drivers that dont necessarily
> have the switch hard wired.

WHEN the kernel is the ONLY possible source of a radio controller state
change, it is not needed.  For all other cases, we are either ignoring
the status changes (i.e. the kernel's idea of the current controller
state can be wrong), or using the input layer to resync (b43 tries to do
this).

> Should I add attribute for every key on a keyboard just because some
> userspace developers what to use shell scripts everywhere? key_a,
> key_b, key_c and so on? No.

It is a rare application that needs to know if a key is currently
pressed or not.  And usually, it is assumed by the user that he should
not expect an application to know a key was depressed before it started,
nor are such applications usually written using shell script :-)

For switches, that's not rare at all.

> >  There is a big
> > pontential for confusion and misuse of interfaces there, when you recall
> > that it will be COMMON to have the same module (driver) provide both a
> > rfkill controller through the rfkill class, and an input device.
> 
> As soon as people will realize that button and RF transmitter are
> different objects serving different purposes it all goes away. The
> very same module also registers network device, why rfkill is so
> different.

It is not so different.  But you could make the handling of that
information more generic if you could get it using just the rfkill
interface.

> > > > We could fix it in the input subsystem, instead.  How difficult (and
> > > > welcome a change?) would it be for the input device infrastructure to
> > > > provide pollable (for good measure!) "state" attributes for every EV_SW
> > > > event an input device registers in evbits+swbits ?
> > > > 
> > > 
> > > Given the above I don't see the need.
> > 
> > So, we can get the required information using an IOCTL.  That means
> > rfkill class support for read-only devices is no longar mandatory.  It
> > is still not clear to me it is not *desired*, though.
> > 
> > The reasons are:
> >   1. Need to have an easy way to get that information for shell scripts,
> >      and IOCTLs are not (AFAIK).  This can be fixed by adding a shell-
> >      script helper to input-utils.
> > 
> >   2. It might be better to have all information directly related to
> >      rfkill behaviour (i.e. both rfkill controller state and rfkill
> >      input device state) in the same class, and present it in the same
> >      way to userspace.
> 
> No, we need to keep RF transmitetrs and buttons separate. I can assign
> KEY_WLAN to one of the media keys on my laptop (that I dont use
> otherwise). Does it make sense to "move" my keyboard to rfkill
> vicinity? I dont think so. Its just a button.

And it doesn't have much of an existence outside of when it generates an
input event.  You don't ask a button where it is ON or OFF, that's a
switch property.

But that's not true for a real switch. You can ask it whether it is in
the ON or OFF state (for two-state switches, they can have a lot more
than just two states, but that's not relevant for rfkill).

I can see why we'd not want to export that through rfkill now that you
brought external keyboards into the picture.  If I have an external
keyboard with a radio kill SWITCH (not key), I want it to work just like
if it were built-in the platform.

Which means it needs to be done in the input layer. Well, that gives me
my answer, in a rather definitive way too.

> > And there is the all important question about whether I should still
> > export that information in thinkpad-acpi over pollable attributes (which
> > are *really* easy to use) or not.  Other driver writers might want to
> > think about it as well.
> 
> If we export switches then we need to export leds. And keys. And
> axis... It all adds up very quickly and wastes kernel memory. Just
> write ioctl utility for script-users if such is needed and be done
> with it.

Actually, no, we don't.

AFAIK, input layer LEDs should go away, they belong in the LED class.

Keys have usage patterns that make it not necessary to export the
information of whether they are currently pressed or not through sysfs.
Utilities to query keyboard state are around for a long time as well.

Stuff that needs an axis position usually needs to track the axis (i.e.
they need a stream of data), so we are doing everyone a big favour if we
never ever let big bad stupid mistakes like that hdaps "position"
attribute's existence happen again.

As for writing the ioctl utility, it looks that it will be needed
without the sysfs attributes.  But who will write it?   Who will give it
a home?  Maybe util-linux would take it in, if we're very lucky...

> > Well, I do propose that we export any, and *every* radio controller
> > state change through kernel notifier chains (and to userspace using
> > uevents).  Those notifications should include the rfkill type (and type
> > class if we add that), new switch state, and any other relevant
> > information (e.g. that the switch is a master switch -- those usually
> > need input events generated, whether the code that produced the
> > notification will also handle issuing an input event for it or not
> > (duplicate suppresion), etc).
> 
> Notfiers are a good idea for RF transmitter state changes. Not so for
> key/button notifiers because these can be ussued by regular PS/2 or
> USB keyboards and other devices that have no idea about rfkill and
> dont want to know.

Agreed.

-- 
  "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 4/8] rfkill: add read-write rfkill switch support

[Carlos Corbacho]

On Monday 14 April 2008 13:00:41 Ivo van Doorn wrote:
> > > See rt2x00 and b43 in driver-wireless for an example implementation
> > > of pollable input device and rfkill
> >
> > They're broken by design, at least in b43's case.  Someone (Carlos, I
> > believe?) posted an example of a ping-pong scenario using b43.
>
> Thats true. But was that a scenario where there were 2 keys for the same
> radio type? Or was that a matter of the driver indicating a key pressed
> event while it has no attached key to it, and only checked the register for
> the key status and the current radio status and saw it was different?

Two part problem:

1) Bug in rfkill; rfkill is toggling all devices in the same state, and not 
actually checking the type.

(I submitted a patch for this on Sunday, although I sent it to linux-wireless 
before you sent the MAINTAINERS patch with netdev as the official list; but I 
did CC you on it regardless so you should have seen this. If not, let me know 
and I'll resend it).

2) The cycle-of-doom bug, as described earlier.

This still applies.

Yes, b43 can also be fixed not to emit a keycode if it cannot control the 
rfkill switch; but that doesn't solve the case of what happens if both the 
wireless driver and the platform driver can control the radio.

I agree wholeheartedly with Henrique here though - wireless drivers should 
_not_ be using key press events as a reporting mechanism.

-Carlos
-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

Re: [PATCH 4/8] rfkill: add read-write rfkill switch support

[Dmitry Torokhov]

On Mon, Apr 14, 2008 at 08:06:50PM +0100, Carlos Corbacho wrote:
> I agree wholeheartedly with Henrique here though - wireless drivers should 
> _not_ be using key press events as a reporting mechanism.
> 

Reporting mechanism for what exacly though? Depending on what you are
talking about we either agree or disagree here ;)

-- 
Dmitry

Re: [PATCH 4/8] rfkill: add read-write rfkill switch support

[Carlos Corbacho]

On Monday 14 April 2008 21:23:14 Dmitry Torokhov wrote:
> On Mon, Apr 14, 2008 at 08:06:50PM +0100, Carlos Corbacho wrote:
> > I agree wholeheartedly with Henrique here though - wireless drivers
> > should _not_ be using key press events as a reporting mechanism.
>
> Reporting mechanism for what exacly though? Depending on what you are
> talking about we either agree or disagree here ;)

Reporting state change in this case. For instance, KEY_WLAN means exactly 
that - a user pressed a key that was mapped to KEY_WLAN. It's not a generic 
notifier for any and all wireless events, state changes, etc.

If wireless drivers want to report a state change, then we need a proper 
mechanism for that, as has already been discussed by Henrique.

-Carlos
-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

Re: [PATCH 4/8] rfkill: add read-write rfkill switch support

[Dmitry Torokhov]

On Tue, Apr 15, 2008 at 08:27:49AM +0100, Carlos Corbacho wrote:
> On Monday 14 April 2008 21:23:14 Dmitry Torokhov wrote:
> > On Mon, Apr 14, 2008 at 08:06:50PM +0100, Carlos Corbacho wrote:
> > > I agree wholeheartedly with Henrique here though - wireless drivers
> > > should _not_ be using key press events as a reporting mechanism.
> >
> > Reporting mechanism for what exacly though? Depending on what you are
> > talking about we either agree or disagree here ;)
> 
> Reporting state change in this case. For instance, KEY_WLAN means exactly 
> that - a user pressed a key that was mapped to KEY_WLAN. It's not a generic 
> notifier for any and all wireless events, state changes, etc.
> 
> If wireless drivers want to report a state change, then we need a proper 
> mechanism for that, as has already been discussed by Henrique.
> 

I think everyone is in a violent agreement here but prefers to keep
talking to appear that they not agree ;)

-- 
Dmitry

Re: [PATCH 4/8] rfkill: add read-write rfkill switch support

[Ivo van Doorn]

On Monday 14 April 2008, Carlos Corbacho wrote:
> On Monday 14 April 2008 13:00:41 Ivo van Doorn wrote:
> > > > See rt2x00 and b43 in driver-wireless for an example implementation
> > > > of pollable input device and rfkill
> > >
> > > They're broken by design, at least in b43's case.  Someone (Carlos, I
> > > believe?) posted an example of a ping-pong scenario using b43.
> >
> > Thats true. But was that a scenario where there were 2 keys for the same
> > radio type? Or was that a matter of the driver indicating a key pressed
> > event while it has no attached key to it, and only checked the register for
> > the key status and the current radio status and saw it was different?
> 
> Two part problem:
> 
> 1) Bug in rfkill; rfkill is toggling all devices in the same state, and not 
> actually checking the type.
> 
> (I submitted a patch for this on Sunday, although I sent it to linux-wireless 
> before you sent the MAINTAINERS patch with netdev as the official list; but I 
> did CC you on it regardless so you should have seen this. If not, let me know 
> and I'll resend it).

Completely overlooked the patch.
I found it in my archives and just acked it. :)

Ivo

> 2) The cycle-of-doom bug, as described earlier.
> 
> This still applies.
> 
> Yes, b43 can also be fixed not to emit a keycode if it cannot control the 
> rfkill switch; but that doesn't solve the case of what happens if both the 
> wireless driver and the platform driver can control the radio.
> 
> I agree wholeheartedly with Henrique here though - wireless drivers should 
> _not_ be using key press events as a reporting mechanism.
> 
> -Carlos

Re: [PATCH 4/8] rfkill: add read-write rfkill switch support

[Henrique de Moraes Holschuh]

On Mon, 14 Apr 2008, Ivo van Doorn wrote:
> Completely overlooked the patch.
> I found it in my archives and just acked it. :)

Unless you want extra trouble and work on your hands when merge time
comes, I would really appreciate a tree with all pending rfkill patches
applied to base my work on.   Right now, I am using 2.6.25-rc8-mm2.

May I suggest you open a git tree somewhere that we can use to track the
rfkill pending patches that are not in mainline yet? :-)

If you don't have a git server in mind already, I suggest http://repo.or.cz

-- 
  "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 4/8] rfkill: add read-write rfkill switch support

[Ivo Van Doorn]

On Mon, Apr 14, 2008 at 11:46 PM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Mon, 14 Apr 2008, Ivo van Doorn wrote:
>  > Completely overlooked the patch.
>  > I found it in my archives and just acked it. :)
>
>  Unless you want extra trouble and work on your hands when merge time
>  comes, I would really appreciate a tree with all pending rfkill patches
>  applied to base my work on.   Right now, I am using 2.6.25-rc8-mm2.

I've asked john to push the patch to 2.6.25 as well.

>  May I suggest you open a git tree somewhere that we can use to track the
>  rfkill pending patches that are not in mainline yet? :-)

Sure, not a problem. Let me see from which tree it will be best to branch from.

>  If you don't have a git server in mind already, I suggest http://repo.or.cz

I've an account on kernel.org. :)

Ivo

[PATCH 5/8] rfkill: add read-only rfkill switch support

[Henrique de Moraes Holschuh]

Some devices (notably laptops) have read-only rfkill switches.  Those
devices are slider or rocker switches that are *physically* manipulated by
the user, most of the time tied to hardware or firmware functions that
automatically rf-kill and/or hot-unplug radio devices when in the "radios
off" position.

Software must not (and in fact, cannot) attempt to change the state of any
such switch.  These switches exist because of international regulations
regarding radio emission devices on airplanes and other sensitive areas.
They must never be overriden.

While one can easily report the *change* of state of these switches using
the input layer EV_SW SW_RADIO event, typically userspace needs to have
immediate access of the current switch state.  It makes sense to have all
radio kill switches grouped under the rfkill sysfs interface instead of
every driver doing its own thing.

Make the toggle_radio() hook no longer be mandatory.  read-only rfkill
switches are those who have their toggle_radio() hook set to NULL.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
Cc: John W. Linville <linville@tuxdriver.com>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 include/linux/rfkill.h |    3 ++-
 net/rfkill/rfkill.c    |   18 +++++++++++++-----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index 844e961..931d32b 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -60,7 +60,8 @@ enum rfkill_state {
  * @mutex: Guards switch state transitions
  * @data: Pointer to the RF button drivers private data which will be
  *	passed along when toggling radio state.
- * @toggle_radio(): Mandatory handler to control state of the radio.
+ * @toggle_radio(): handler to control state of the radio. Must be
+ *	NULL for read-only switches.
  * @get_state(): handler to read current radio state from hardware,
  *      may be called from atomic context, should return 0 on success.
  * @led_trigger: A LED trigger for this button's LED.
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 88b0558..d149d94 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -81,7 +81,7 @@ static int rfkill_toggle_radio(struct rfkill *rfkill,
 	    !rfkill->get_state(rfkill->data, &newstate))
 		rfkill->state = newstate;
 
-	if (state != rfkill->state) {
+	if (rfkill->toggle_radio && state != rfkill->state) {
 		retval = rfkill->toggle_radio(rfkill->data, state);
 		if (!retval)
 			rfkill->state = state;
@@ -196,6 +196,9 @@ static ssize_t rfkill_state_store(struct device *dev,
 	unsigned int state = simple_strtoul(buf, NULL, 0);
 	int error;
 
+	if (!rfkill->toggle_radio)
+		return -EACCES;
+
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
 
@@ -278,7 +281,8 @@ static int rfkill_suspend(struct device *dev, pm_message_t state)
 		if (state.event & PM_EVENT_SLEEP) {
 			mutex_lock(&rfkill->mutex);
 
-			if (rfkill->state == RFKILL_STATE_ON)
+			if (rfkill->state == RFKILL_STATE_ON &&
+			    rfkill->toggle_radio)
 				rfkill->toggle_radio(rfkill->data,
 						     RFKILL_STATE_OFF);
 
@@ -298,7 +302,8 @@ static int rfkill_resume(struct device *dev)
 	if (dev->power.power_state.event != PM_EVENT_ON) {
 		mutex_lock(&rfkill->mutex);
 
-		if (rfkill->state == RFKILL_STATE_ON)
+		if (rfkill->state == RFKILL_STATE_ON &&
+		    rfkill->toggle_radio)
 			rfkill->toggle_radio(rfkill->data, RFKILL_STATE_ON);
 
 		mutex_unlock(&rfkill->mutex);
@@ -427,11 +432,14 @@ int rfkill_register(struct rfkill *rfkill)
 	struct device *dev = &rfkill->dev;
 	int error;
 
-	if (!rfkill->toggle_radio)
-		return -EINVAL;
 	if (rfkill->type >= RFKILL_TYPE_MAX)
 		return -EINVAL;
 
+	if (!rfkill->toggle_radio) {
+		rfkill->user_claim_unsupported = 1;
+		rfkill->user_claim = 0;
+	}
+
 	snprintf(dev->bus_id, sizeof(dev->bus_id),
 		 "rfkill%ld", (long)atomic_inc_return(&rfkill_no) - 1);
 
-- 
1.5.4.4

[PATCH 6/8] rfkill: add the WWAN radio type

[Henrique de Moraes Holschuh]

Unfortunately, instead of adding a generic Wireless WAN type, a technology-
specific type (WiMAX) was added.  That's useless for other WWAN devices,
such as EDGE, UMTS, X-RTT and other such radios.

Add a WWAN rfkill type for generic wireless WAN devices.  No keys are added
as most devices use KEY_RADIO for WWAN control and need no specific keycode
added.

Signed-off-by: Inaky Perez-Gonzalez <inaky@linux.intel.com>
Cc: Iñaky Pérez-González <inaky.perez-gonzalez@intel.com>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
Cc: John W. Linville <linville@tuxdriver.com>
Cc: David S. Miller <davem@davemloft.net>
---
 include/linux/rfkill.h    |    2 ++
 net/rfkill/rfkill-input.c |    5 +++++
 net/rfkill/rfkill.c       |    3 +++
 3 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index 931d32b..7650517 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -34,12 +34,14 @@
  * RFKILL_TYPE_BLUETOOTH: switch is on a bluetooth device.
  * RFKILL_TYPE_UWB: switch is on a ultra wideband device.
  * RFKILL_TYPE_WIMAX: switch is on a WiMAX device.
+ * RFKILL_TYPE_WWAN: switch is on a wireless WAN device.
  */
 enum rfkill_type {
 	RFKILL_TYPE_WLAN ,
 	RFKILL_TYPE_BLUETOOTH,
 	RFKILL_TYPE_UWB,
 	RFKILL_TYPE_WIMAX,
+	RFKILL_TYPE_WWAN,
 	RFKILL_TYPE_MAX,
 };
 
diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
index 7d1e520..675651b 100644
--- a/net/rfkill/rfkill-input.c
+++ b/net/rfkill/rfkill-input.c
@@ -101,6 +101,7 @@ static DEFINE_RFKILL_TASK(rfkill_wlan, RFKILL_TYPE_WLAN);
 static DEFINE_RFKILL_TASK(rfkill_bt, RFKILL_TYPE_BLUETOOTH);
 static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);
 static DEFINE_RFKILL_TASK(rfkill_wimax, RFKILL_TYPE_WIMAX);
+static DEFINE_RFKILL_TASK(rfkill_wwan, RFKILL_TYPE_WIMAX);
 
 static void rfkill_event(struct input_handle *handle, unsigned int type,
 			unsigned int code, int data)
@@ -121,6 +122,7 @@ static void rfkill_event(struct input_handle *handle, unsigned int type,
 			break;
 		case KEY_RADIO:
 			/* EVERY radio type */
+			rfkill_schedule_toggle(&rfkill_wwan);
 			rfkill_schedule_toggle(&rfkill_wimax);
 			rfkill_schedule_toggle(&rfkill_uwb);
 			rfkill_schedule_toggle(&rfkill_bt);
@@ -133,6 +135,9 @@ static void rfkill_event(struct input_handle *handle, unsigned int type,
 		switch (code) {
 		case SW_RADIO:
 			/* EVERY radio type. data != 0 means radios ON */
+			rfkill_schedule_set(&rfkill_wwan,
+					    (data)? RFKILL_STATE_ON:
+						    RFKILL_STATE_OFF);
 			rfkill_schedule_set(&rfkill_wimax,
 					    (data)? RFKILL_STATE_ON:
 						    RFKILL_STATE_OFF);
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index d149d94..56241a4 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -171,6 +171,9 @@ static ssize_t rfkill_type_show(struct device *dev,
 	case RFKILL_TYPE_WIMAX:
 		type = "wimax";
 		break;
+	case RFKILL_TYPE_WWAN:
+		type = "wwan";
+		break;
 	default:
 		BUG();
 	}
-- 
1.5.4.4

Re: [PATCH 6/8] rfkill: add the WWAN radio type

[Inaky Perez-Gonzalez]

On Friday 11 April 2008, Henrique de Moraes Holschuh wrote:
> Unfortunately, instead of adding a generic Wireless WAN type, a technology-
> specific type (WiMAX) was added.  That's useless for other WWAN devices,
> such as EDGE, UMTS, X-RTT and other such radios.
> 
> Add a WWAN rfkill type for generic wireless WAN devices.  No keys are added
> as most devices use KEY_RADIO for WWAN control and need no specific keycode
> added.

I know it is easier to complain than to submit code, but at this
point, shouldn't we make this dynamic? [so that the interested technology
that provides an rfkill switch registers it?].

Something that given a technology name registers a dynamic key and type
number that can be use throughout?
	
-- 
Inaky

Re: [PATCH 6/8] rfkill: add the WWAN radio type

[Henrique de Moraes Holschuh]

On Fri, 11 Apr 2008, Inaky Perez-Gonzalez wrote:
> On Friday 11 April 2008, Henrique de Moraes Holschuh wrote:
> > Unfortunately, instead of adding a generic Wireless WAN type, a technology-
> > specific type (WiMAX) was added.  That's useless for other WWAN devices,
> > such as EDGE, UMTS, X-RTT and other such radios.
> > 
> > Add a WWAN rfkill type for generic wireless WAN devices.  No keys are added
> > as most devices use KEY_RADIO for WWAN control and need no specific keycode
> > added.
> 
> I know it is easier to complain than to submit code, but at this
> point, shouldn't we make this dynamic? [so that the interested technology
> that provides an rfkill switch registers it?].

I wouldn't have anything against that, but we do need to coalesce the
types when possible, otherwise the "type" notion becomes useless for
rfkill-input.

-- 
  "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 6/8] rfkill: add the WWAN radio type

[Ivo van Doorn]

On Friday 11 April 2008, Henrique de Moraes Holschuh wrote:
> Unfortunately, instead of adding a generic Wireless WAN type, a technology-
> specific type (WiMAX) was added.  That's useless for other WWAN devices,
> such as EDGE, UMTS, X-RTT and other such radios.

Then perhaps we should replace WiMAX with the WWAN type?

> Add a WWAN rfkill type for generic wireless WAN devices.  No keys are added
> as most devices use KEY_RADIO for WWAN control and need no specific keycode
> added.

In the discussion around the WiMAX addition I do remember people wanted
it to have a seperate key code because it was "different technology". Wouldn't that
be the same for all WWAN technologies?
Aka, should the WiMAX keycode be changed to a WWAN keycode in input.h
and then be used for all WWAN rfkill switches?

Ivo

> Signed-off-by: Inaky Perez-Gonzalez <inaky@linux.intel.com>
> Cc: Iñaky Pérez-González <inaky.perez-gonzalez@intel.com>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>
> Cc: John W. Linville <linville@tuxdriver.com>
> Cc: David S. Miller <davem@davemloft.net>
> ---
>  include/linux/rfkill.h    |    2 ++
>  net/rfkill/rfkill-input.c |    5 +++++
>  net/rfkill/rfkill.c       |    3 +++
>  3 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
> index 931d32b..7650517 100644
> --- a/include/linux/rfkill.h
> +++ b/include/linux/rfkill.h
> @@ -34,12 +34,14 @@
>   * RFKILL_TYPE_BLUETOOTH: switch is on a bluetooth device.
>   * RFKILL_TYPE_UWB: switch is on a ultra wideband device.
>   * RFKILL_TYPE_WIMAX: switch is on a WiMAX device.
> + * RFKILL_TYPE_WWAN: switch is on a wireless WAN device.
>   */
>  enum rfkill_type {
>  	RFKILL_TYPE_WLAN ,
>  	RFKILL_TYPE_BLUETOOTH,
>  	RFKILL_TYPE_UWB,
>  	RFKILL_TYPE_WIMAX,
> +	RFKILL_TYPE_WWAN,
>  	RFKILL_TYPE_MAX,
>  };
>  
> diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
> index 7d1e520..675651b 100644
> --- a/net/rfkill/rfkill-input.c
> +++ b/net/rfkill/rfkill-input.c
> @@ -101,6 +101,7 @@ static DEFINE_RFKILL_TASK(rfkill_wlan, RFKILL_TYPE_WLAN);
>  static DEFINE_RFKILL_TASK(rfkill_bt, RFKILL_TYPE_BLUETOOTH);
>  static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);
>  static DEFINE_RFKILL_TASK(rfkill_wimax, RFKILL_TYPE_WIMAX);
> +static DEFINE_RFKILL_TASK(rfkill_wwan, RFKILL_TYPE_WIMAX);
>  
>  static void rfkill_event(struct input_handle *handle, unsigned int type,
>  			unsigned int code, int data)
> @@ -121,6 +122,7 @@ static void rfkill_event(struct input_handle *handle, unsigned int type,
>  			break;
>  		case KEY_RADIO:
>  			/* EVERY radio type */
> +			rfkill_schedule_toggle(&rfkill_wwan);
>  			rfkill_schedule_toggle(&rfkill_wimax);
>  			rfkill_schedule_toggle(&rfkill_uwb);
>  			rfkill_schedule_toggle(&rfkill_bt);
> @@ -133,6 +135,9 @@ static void rfkill_event(struct input_handle *handle, unsigned int type,
>  		switch (code) {
>  		case SW_RADIO:
>  			/* EVERY radio type. data != 0 means radios ON */
> +			rfkill_schedule_set(&rfkill_wwan,
> +					    (data)? RFKILL_STATE_ON:
> +						    RFKILL_STATE_OFF);
>  			rfkill_schedule_set(&rfkill_wimax,
>  					    (data)? RFKILL_STATE_ON:
>  						    RFKILL_STATE_OFF);
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index d149d94..56241a4 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -171,6 +171,9 @@ static ssize_t rfkill_type_show(struct device *dev,
>  	case RFKILL_TYPE_WIMAX:
>  		type = "wimax";
>  		break;
> +	case RFKILL_TYPE_WWAN:
> +		type = "wwan";
> +		break;
>  	default:
>  		BUG();
>  	}

Re: [PATCH 6/8] rfkill: add the WWAN radio type

[Henrique de Moraes Holschuh]

On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> On Friday 11 April 2008, Henrique de Moraes Holschuh wrote:
> > Unfortunately, instead of adding a generic Wireless WAN type, a technology-
> > specific type (WiMAX) was added.  That's useless for other WWAN devices,
> > such as EDGE, UMTS, X-RTT and other such radios.
> 
> Then perhaps we should replace WiMAX with the WWAN type?

And have KEY_WIMAX interact with WWAN, or rename KEY_WIMAX to KEY_WWAN as
well?

I do think it should be OK to do both renames, since it is very unlikely
that a device would have keys for WIMAX and WWAN at the same type.  We
don't even have to rename KEY_WIMAX, we can have KEY_WWAN and KEY_WIMAX map
both to the same keycode.

Inaky?

> > Add a WWAN rfkill type for generic wireless WAN devices.  No keys are added
> > as most devices use KEY_RADIO for WWAN control and need no specific keycode
> > added.
> 
> In the discussion around the WiMAX addition I do remember people wanted
> it to have a seperate key code because it was "different technology". Wouldn't that
> be the same for all WWAN technologies?

IMO, this is an USER INTERFACE part of the kernel.  The user will either
interact with radios one-by-one (and the rfkill class provides this anyway,
even without separate types), or he will want to deal with abstract
concepts: "all radios", "wireless wan", "wireles lan", "personal-space
radios (UWB, BT)"...

I.e. I am not even sure we should have UWB and BT as separate types...  but
naming UWB "Bluetooth" would be wrong, too, so a proper fix there is harder
(breaks stable ABI with userspace).

> Aka, should the WiMAX keycode be changed to a WWAN keycode in input.h
> and then be used for all WWAN rfkill switches?

I'd think so.

We can add a desc field to rfkill with a more human-friendly, not required
to be unique, description of the switch.

 e.g.:  "Intel WiMAX 1234 radio switch"
        "ThinkPad builtin bluetooth switch"

and so on.  It will be far more useful than making the switch type a
technology-granular thing.  And it will be useful for GUIs in userspace.

-- 
  "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 6/8] rfkill: add the WWAN radio type

[Ivo van Doorn]

On Saturday 12 April 2008, Henrique de Moraes Holschuh wrote:
> On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > On Friday 11 April 2008, Henrique de Moraes Holschuh wrote:
> > > Unfortunately, instead of adding a generic Wireless WAN type, a technology-
> > > specific type (WiMAX) was added.  That's useless for other WWAN devices,
> > > such as EDGE, UMTS, X-RTT and other such radios.
> > 
> > Then perhaps we should replace WiMAX with the WWAN type?
> 
> And have KEY_WIMAX interact with WWAN, or rename KEY_WIMAX to KEY_WWAN as
> well?
> 
> I do think it should be OK to do both renames, since it is very unlikely
> that a device would have keys for WIMAX and WWAN at the same type.  We
> don't even have to rename KEY_WIMAX, we can have KEY_WWAN and KEY_WIMAX map
> both to the same keycode.
> 
> Inaky?

I would say rename, having multiple key definitions mapped to the same keycode 
sounds like a bad idea to me.

> > > Add a WWAN rfkill type for generic wireless WAN devices.  No keys are added
> > > as most devices use KEY_RADIO for WWAN control and need no specific keycode
> > > added.
> > 
> > In the discussion around the WiMAX addition I do remember people wanted
> > it to have a seperate key code because it was "different technology". Wouldn't that
> > be the same for all WWAN technologies?
> 
> IMO, this is an USER INTERFACE part of the kernel.  The user will either
> interact with radios one-by-one (and the rfkill class provides this anyway,
> even without separate types), or he will want to deal with abstract
> concepts: "all radios", "wireless wan", "wireles lan", "personal-space
> radios (UWB, BT)"...
> 
> I.e. I am not even sure we should have UWB and BT as separate types...  but
> naming UWB "Bluetooth" would be wrong, too, so a proper fix there is harder
> (breaks stable ABI with userspace).
> 
> > Aka, should the WiMAX keycode be changed to a WWAN keycode in input.h
> > and then be used for all WWAN rfkill switches?
> 
> I'd think so.
> 
> We can add a desc field to rfkill with a more human-friendly, not required
> to be unique, description of the switch.
> 
>  e.g.:  "Intel WiMAX 1234 radio switch"
>         "ThinkPad builtin bluetooth switch"
> 
> and so on.  It will be far more useful than making the switch type a
> technology-granular thing.  And it will be useful for GUIs in userspace.

Sounds good.

Ivo

Re: [PATCH 6/8] rfkill: add the WWAN radio type

[Inaky Perez-Gonzalez]

On Saturday 12 April 2008, Henrique de Moraes Holschuh wrote:
> On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > On Friday 11 April 2008, Henrique de Moraes Holschuh wrote:
> > > Unfortunately, instead of adding a generic Wireless WAN type, a technology-
> > > specific type (WiMAX) was added.  That's useless for other WWAN devices,
> > > such as EDGE, UMTS, X-RTT and other such radios.
> > 
> > Then perhaps we should replace WiMAX with the WWAN type?
> 
> And have KEY_WIMAX interact with WWAN, or rename KEY_WIMAX to KEY_WWAN as
> well?
> 
> I do think it should be OK to do both renames, since it is very unlikely
> that a device would have keys for WIMAX and WWAN at the same type.  We
> don't even have to rename KEY_WIMAX, we can have KEY_WWAN and KEY_WIMAX map
> both to the same keycode.
> 
> Inaky?

I disagree

Most rfkill hw keys are attached to the device. So I can plug wifi, wimax,
3G, UWB and BT dongles (or cards) into a system and they each provide a HW
switch (or should provide).

If each overrides another one, we have a problem.

Is it that unlikely that a device will have 3G and WiMAX support? Not 
really. My laptop might have WWAN built in and then when I travel to some
place where I need WiMAX, I plug a WiMAX USB dongle. Problem. This is just
an example that can be extended to any wireless technology combination.

My point is we should keep it kind of separated, where the guy requesting
the rfkill stuff says "I need a keycode to generate when my hw switch is
operated". And for the rfkill layer to just care about associating that
keycode to that card.

I guess before I didn't have it as clear as I do now, but I am just seeing
more and more that each radio has its own kill switch and then the system
might have a generic one (for example fn+F5 on thinkpads) for killing all
of them at the same time.



-- 
Inaky

Re: [PATCH 6/8] rfkill: add the WWAN radio type

[Henrique de Moraes Holschuh]

On Sat, 12 Apr 2008, Inaky Perez-Gonzalez wrote:
> Most rfkill hw keys are attached to the device. So I can plug wifi, wimax,
> 3G, UWB and BT dongles (or cards) into a system and they each provide a HW
> switch (or should provide).
> 
> If each overrides another one, we have a problem.

Yeah, and we have to avoid such problems.

But let me ask you one thing.  Are we talking of different rfkill
switches, OR are we talking about different KEYs or BUTTONS in the
keyboard/keypad/console/remote control?

Because one thing has very little to do with the other.  No network
device driver shall generate an input event after I am done with the
next patch set...

On the other hand, if you DO have a WiMax, 3G, or EDGE *key* or
*button*, THEN you need a KEY_WIMAX, KEY_3G, KEY_EDGE, or whatever. Even
if there was no rfkill class in existence :-) and that's only if you
would find more than one of them in a keyboard/console/remote
control/keypad, otherwise KEY_WWAN would be enough.  Adding input events
is *expensive*.

The same idea is also true for switches, which must use EV_SW SW_* and
NEVER try to simulate a switch using KEY_* or BUTTON_*.

So, we CAN keep RFKILL_TYPE_WIMAX, and this is decoupled to keeping
KEY_WIMAX.  These are two different decisions, and are not related to
each other in any way.

Do we *have* devices with WiMAX keys or buttons that ALSO have non-WiMAX
WWAN keys or buttons [that support Linux]?  If so, you need KEY_WIMAX
events (and/or BUTTON_WIMAX, depends on the type of event being
generated).  This has nothing to do with rfkill.   The same question and
rationale applies to devices with switches (i.e. stuff that latches to a
state), and SW_* events.

Is it a good idea to have WiMAX as a separate type for rfkill switches?
Probably.  I'd actually say we should have a two-tier approach:

1. switch class:
   WLAN - (wifi stuff and whatever else looks like a WLAN)
   WWAN - (3G, WiMAX, etch)
   personal devices (or whatever the right name for this is): UWB, Bluetooth
   ALL/ANY - "all radios" button/switches/etc

2. switch type:
   802.11abgn(WiFi), 802.16(WiMAX), EDGE/UMTS/X-RTT(Cell), UWB,
   BLUETOOTH, ANY...  this can certainly be granular to the technology
   level.

That would make it easier for rfkill-input to decide on what switches it
should operate when it gets an specific input event.  We would make it
operate on all WWAN-class rf switches for KEY_WWAN (and that would
include WiMAX), and we could make it operate on all switches of a
specific type, e.g. only bluetooth (and not bluetooth+UWB) for
KEY_BLUETOOTH.

> I guess before I didn't have it as clear as I do now, but I am just seeing
> more and more that each radio has its own kill switch and then the system
> might have a generic one (for example fn+F5 on thinkpads) for killing all
> of them at the same time.

Exactly, and we have to enhance rfkill to address that model properly.

My current idea of how it could work is:

1. "Master" switches: those who are to control an entire class of radios
(i.e. all devices of the same type), or all radios (type "any").  The
most typical example is a RO rfkill switch in a laptop or gadget, and
buttons and hot-keys in laptops or gadgets.  Master switches may issue
input events when needed (it usually is).

2. "slave" switches: this is what you usually have on the chipsets and
their drivers (typically, two of them: a hardware one driven by an input
pin in the hardware that the driver can *read* (i.e. it is read-only),
and a software one in the driver code, which is read-write).  They
*never* issue input events.

The big bad thing we need to always keep in mind is that that input pin
in a radio chipset that is exported as a read-only rfkill switch might
be a slave in one gadget/platform, and a master in another
gadget/plaftform.  Let userspace or a platform/gadget-specific platform
driver configure this.  Place all glue related to this in the rfkill
class.

-- 
  "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 6/8] rfkill: add the WWAN radio type

[Ivo van Doorn]

On Sunday 13 April 2008, Henrique de Moraes Holschuh wrote:
> On Sat, 12 Apr 2008, Inaky Perez-Gonzalez wrote:
> > Most rfkill hw keys are attached to the device. So I can plug wifi, wimax,
> > 3G, UWB and BT dongles (or cards) into a system and they each provide a HW
> > switch (or should provide).
> > 
> > If each overrides another one, we have a problem.
> 
> Yeah, and we have to avoid such problems.
> 
> But let me ask you one thing.  Are we talking of different rfkill
> switches, OR are we talking about different KEYs or BUTTONS in the
> keyboard/keypad/console/remote control?
> 
> Because one thing has very little to do with the other.  No network
> device driver shall generate an input event after I am done with the
> next patch set...

Not sure if that will be the desired course of action.
The original rfkill function didn't work with input devices either and just
handled everything in rfkill.
But the input layer people requested the change to use input devices and let
rfkill hook into that, because a rfkill switch/key can be considered an input device.

I've CC'ed Dmitry into this discussion since he was the one who suggested the
input device interaction initialy (and created the rfkill-input module).

> On the other hand, if you DO have a WiMax, 3G, or EDGE *key* or
> *button*, THEN you need a KEY_WIMAX, KEY_3G, KEY_EDGE, or whatever. Even
> if there was no rfkill class in existence :-) and that's only if you
> would find more than one of them in a keyboard/console/remote
> control/keypad, otherwise KEY_WWAN would be enough.  Adding input events
> is *expensive*.
> 
> The same idea is also true for switches, which must use EV_SW SW_* and
> NEVER try to simulate a switch using KEY_* or BUTTON_*.
> 
> So, we CAN keep RFKILL_TYPE_WIMAX, and this is decoupled to keeping
> KEY_WIMAX.  These are two different decisions, and are not related to
> each other in any way.
> 
> Do we *have* devices with WiMAX keys or buttons that ALSO have non-WiMAX
> WWAN keys or buttons [that support Linux]?  If so, you need KEY_WIMAX
> events (and/or BUTTON_WIMAX, depends on the type of event being
> generated).  This has nothing to do with rfkill.   The same question and
> rationale applies to devices with switches (i.e. stuff that latches to a
> state), and SW_* events.
> 
> Is it a good idea to have WiMAX as a separate type for rfkill switches?
> Probably.  I'd actually say we should have a two-tier approach:
> 
> 1. switch class:
>    WLAN - (wifi stuff and whatever else looks like a WLAN)
>    WWAN - (3G, WiMAX, etch)
>    personal devices (or whatever the right name for this is): UWB, Bluetooth
>    ALL/ANY - "all radios" button/switches/etc
> 
> 2. switch type:
>    802.11abgn(WiFi), 802.16(WiMAX), EDGE/UMTS/X-RTT(Cell), UWB,
>    BLUETOOTH, ANY...  this can certainly be granular to the technology
>    level.
> 
> That would make it easier for rfkill-input to decide on what switches it
> should operate when it gets an specific input event.  We would make it
> operate on all WWAN-class rf switches for KEY_WWAN (and that would
> include WiMAX), and we could make it operate on all switches of a
> specific type, e.g. only bluetooth (and not bluetooth+UWB) for
> KEY_BLUETOOTH.
> 
> > I guess before I didn't have it as clear as I do now, but I am just seeing
> > more and more that each radio has its own kill switch and then the system
> > might have a generic one (for example fn+F5 on thinkpads) for killing all
> > of them at the same time.
> 
> Exactly, and we have to enhance rfkill to address that model properly.
> 
> My current idea of how it could work is:
> 
> 1. "Master" switches: those who are to control an entire class of radios
> (i.e. all devices of the same type), or all radios (type "any").  The
> most typical example is a RO rfkill switch in a laptop or gadget, and
> buttons and hot-keys in laptops or gadgets.  Master switches may issue
> input events when needed (it usually is).
> 
> 2. "slave" switches: this is what you usually have on the chipsets and
> their drivers (typically, two of them: a hardware one driven by an input
> pin in the hardware that the driver can *read* (i.e. it is read-only),
> and a software one in the driver code, which is read-write).  They
> *never* issue input events.
> 
> The big bad thing we need to always keep in mind is that that input pin
> in a radio chipset that is exported as a read-only rfkill switch might
> be a slave in one gadget/platform, and a master in another
> gadget/plaftform.  Let userspace or a platform/gadget-specific platform
> driver configure this.  Place all glue related to this in the rfkill
> class.
> 

Re: [PATCH 6/8] rfkill: add the WWAN radio type

[Henrique de Moraes Holschuh]

On Sun, 13 Apr 2008, Ivo van Doorn wrote:
> On Sunday 13 April 2008, Henrique de Moraes Holschuh wrote:
> > On Sat, 12 Apr 2008, Inaky Perez-Gonzalez wrote:
> > > Most rfkill hw keys are attached to the device. So I can plug wifi, wimax,
> > > 3G, UWB and BT dongles (or cards) into a system and they each provide a HW
> > > switch (or should provide).
> > > 
> > > If each overrides another one, we have a problem.
> > 
> > Yeah, and we have to avoid such problems.
> > 
> > But let me ask you one thing.  Are we talking of different rfkill
> > switches, OR are we talking about different KEYs or BUTTONS in the
> > keyboard/keypad/console/remote control?
> > 
> > Because one thing has very little to do with the other.  No network
> > device driver shall generate an input event after I am done with the
> > next patch set...
> 
> Not sure if that will be the desired course of action.
> The original rfkill function didn't work with input devices either and just
> handled everything in rfkill.
> But the input layer people requested the change to use input devices and let
> rfkill hook into that, because a rfkill switch/key can be considered an input device.
> 
> I've CC'ed Dmitry into this discussion since he was the one who suggested the
> input device interaction initialy (and created the rfkill-input module).

In order to avoid a lot of misunderstandings, we better name the
"enable/disable radio apparatus" (the circuit/config register that
causes a radio to disable its RF path) and the "input hardware rf-switch
device" (the thing the user presses/moves) in different ways, at least
when we are talking about them.

Right now both are usually called rfkill switch, and much of the mess
comes from that...

"input hardware rf-switch devices" ARE input devices, belong to the
input layer, and issue input events.

"enable/disable radio apparatus" don't, and the input layer should not
be abused as a "status reporting" feature for those.  We certainly will
benefit from those issuing kernel notifications through a notification
chain for them, though.

And many devices are both at the same time (e.g. a b43 in certain
hardware configurations), and many drivers have both types (e.g.
thinkpad-acpi's hotkey subsystem has "input hardare rf-switch devices"
functions, and thinkpad-acpi's bluetooth firmware handling have
"enable/disable radio apparatus" functions).

-- 
  "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 6/8] rfkill: add the WWAN radio type

[Dmitry Torokhov]

On Sun, Apr 13, 2008 at 03:16:21PM -0300, Henrique de Moraes Holschuh wrote:
> On Sun, 13 Apr 2008, Ivo van Doorn wrote:
> > On Sunday 13 April 2008, Henrique de Moraes Holschuh wrote:
> > > On Sat, 12 Apr 2008, Inaky Perez-Gonzalez wrote:
> > > > Most rfkill hw keys are attached to the device. So I can plug wifi, wimax,
> > > > 3G, UWB and BT dongles (or cards) into a system and they each provide a HW
> > > > switch (or should provide).
> > > > 
> > > > If each overrides another one, we have a problem.
> > > 
> > > Yeah, and we have to avoid such problems.
> > > 
> > > But let me ask you one thing.  Are we talking of different rfkill
> > > switches, OR are we talking about different KEYs or BUTTONS in the
> > > keyboard/keypad/console/remote control?
> > > 
> > > Because one thing has very little to do with the other.  No network
> > > device driver shall generate an input event after I am done with the
> > > next patch set...
> > 
> > Not sure if that will be the desired course of action.
> > The original rfkill function didn't work with input devices either and just
> > handled everything in rfkill.
> > But the input layer people requested the change to use input devices and let
> > rfkill hook into that, because a rfkill switch/key can be considered an input device.
> > 
> > I've CC'ed Dmitry into this discussion since he was the one who suggested the
> > input device interaction initialy (and created the rfkill-input module).
> 
> In order to avoid a lot of misunderstandings, we better name the
> "enable/disable radio apparatus" (the circuit/config register that
> causes a radio to disable its RF path) and the "input hardware rf-switch
> device" (the thing the user presses/moves) in different ways, at least
> when we are talking about them.
> 
> Right now both are usually called rfkill switch, and much of the mess
> comes from that...
> 
> "input hardware rf-switch devices" ARE input devices, belong to the
> input layer, and issue input events.
> 

Exactly. And some times they are completely separated from the RF switch
itself, in the same fashion some keyboards have sleep key that is not
directly wired into ACPI. That was the reason I wanted the radio control
and key/switch/sliter to be separated.

> "enable/disable radio apparatus" don't, and the input layer should not
> be abused as a "status reporting" feature for those.  We certainly will
> benefit from those issuing kernel notifications through a notification
> chain for them, though.
> 
> And many devices are both at the same time (e.g. a b43 in certain
> hardware configurations), and many drivers have both types (e.g.
> thinkpad-acpi's hotkey subsystem has "input hardare rf-switch devices"
> functions, and thinkpad-acpi's bluetooth firmware handling have
> "enable/disable radio apparatus" functions).
> 

-- 
Dmitry

[PATCH 7/8] rfkill: add an "any radio" switch type and functionality

[Henrique de Moraes Holschuh]

Add a RFKILL_TYPE_ANY switch.  This switch can control more than one type
of radio, and it is always subject to toggling by any type of rfkill-input
event.  It is suitable to implement kill-all-radios functionality when
coupled with input event EV_SW SW_RADIO.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
Cc: John W. Linville <linville@tuxdriver.com>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 include/linux/rfkill.h    |    2 ++
 net/rfkill/rfkill-input.c |    9 +++++++++
 net/rfkill/rfkill.c       |    3 +++
 3 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index 7650517..ba8a7e1 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -35,6 +35,7 @@
  * RFKILL_TYPE_UWB: switch is on a ultra wideband device.
  * RFKILL_TYPE_WIMAX: switch is on a WiMAX device.
  * RFKILL_TYPE_WWAN: switch is on a wireless WAN device.
+ * RFKILL_TYPE_ANY: switch kills radios regardless of type.
  */
 enum rfkill_type {
 	RFKILL_TYPE_WLAN ,
@@ -42,6 +43,7 @@ enum rfkill_type {
 	RFKILL_TYPE_UWB,
 	RFKILL_TYPE_WIMAX,
 	RFKILL_TYPE_WWAN,
+	RFKILL_TYPE_ANY,
 	RFKILL_TYPE_MAX,
 };
 
diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
index 675651b..ec9112c 100644
--- a/net/rfkill/rfkill-input.c
+++ b/net/rfkill/rfkill-input.c
@@ -102,6 +102,7 @@ static DEFINE_RFKILL_TASK(rfkill_bt, RFKILL_TYPE_BLUETOOTH);
 static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);
 static DEFINE_RFKILL_TASK(rfkill_wimax, RFKILL_TYPE_WIMAX);
 static DEFINE_RFKILL_TASK(rfkill_wwan, RFKILL_TYPE_WIMAX);
+static DEFINE_RFKILL_TASK(rfkill_any, RFKILL_TYPE_ANY);
 
 static void rfkill_event(struct input_handle *handle, unsigned int type,
 			unsigned int code, int data)
@@ -110,18 +111,23 @@ static void rfkill_event(struct input_handle *handle, unsigned int type,
 		switch (code) {
 		case KEY_WLAN:
 			rfkill_schedule_toggle(&rfkill_wlan);
+			rfkill_schedule_toggle(&rfkill_any);
 			break;
 		case KEY_BLUETOOTH:
 			rfkill_schedule_toggle(&rfkill_bt);
+			rfkill_schedule_toggle(&rfkill_any);
 			break;
 		case KEY_UWB:
 			rfkill_schedule_toggle(&rfkill_uwb);
+			rfkill_schedule_toggle(&rfkill_any);
 			break;
 		case KEY_WIMAX:
 			rfkill_schedule_toggle(&rfkill_wimax);
+			rfkill_schedule_toggle(&rfkill_any);
 			break;
 		case KEY_RADIO:
 			/* EVERY radio type */
+			rfkill_schedule_toggle(&rfkill_any);
 			rfkill_schedule_toggle(&rfkill_wwan);
 			rfkill_schedule_toggle(&rfkill_wimax);
 			rfkill_schedule_toggle(&rfkill_uwb);
@@ -135,6 +141,9 @@ static void rfkill_event(struct input_handle *handle, unsigned int type,
 		switch (code) {
 		case SW_RADIO:
 			/* EVERY radio type. data != 0 means radios ON */
+			rfkill_schedule_set(&rfkill_any,
+					    (data)? RFKILL_STATE_ON:
+						    RFKILL_STATE_OFF);
 			rfkill_schedule_set(&rfkill_wwan,
 					    (data)? RFKILL_STATE_ON:
 						    RFKILL_STATE_OFF);
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 56241a4..9d3bffb 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -174,6 +174,9 @@ static ssize_t rfkill_type_show(struct device *dev,
 	case RFKILL_TYPE_WWAN:
 		type = "wwan";
 		break;
+	case RFKILL_TYPE_ANY:
+		type = "any";
+		break;
 	default:
 		BUG();
 	}
-- 
1.5.4.4

Re: [PATCH 7/8] rfkill: add an "any radio" switch type and functionality

[Pavel Machek]

On Fri 2008-04-11 17:37:23, Henrique de Moraes Holschuh wrote:
> Add a RFKILL_TYPE_ANY switch.  This switch can control more than one type
> of radio, and it is always subject to toggling by any type of rfkill-input
> event.  It is suitable to implement kill-all-radios functionality when
> coupled with input event EV_SW SW_RADIO.

Hmm, how does that work with pcmcia ethernet card plugged in?

How does userspace know which radios this controls?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 7/8] rfkill: add an "any radio" switch type and functionality

[Henrique de Moraes Holschuh]

On Sat, 12 Apr 2008, Pavel Machek wrote:
> On Fri 2008-04-11 17:37:23, Henrique de Moraes Holschuh wrote:
> > Add a RFKILL_TYPE_ANY switch.  This switch can control more than one type
> > of radio, and it is always subject to toggling by any type of rfkill-input
> > event.  It is suitable to implement kill-all-radios functionality when
> > coupled with input event EV_SW SW_RADIO.
> 
> Hmm, how does that work with pcmcia ethernet card plugged in?

I assume you mean PCMCIA WLAN card.

1. The hardware switch can't kill the WLAN card directly (so, unless
software does something, the radio in the PCMCIA card ignores the
switch).

2. But the hardware switch it is a master switch, so it will issue an
input event, which, IF THE USER WANTS IT TO WORK will be either acted
upon by rfkill-input or userspace.

3. rfkill-input or userspace will tell rfkill to switch all WLAN cards
on or off, because of that input event.

4. The WLAN driver bound to the pcmcia WLAN device gets that command and
changes the radio state.

So, from the user PoV, the PCMCIA WLAN card ends up responding to the
hardware switch, which is better behaviour even than what Windows could
do :p

> How does userspace know which radios this controls?

It doesn't.  Not even the hardware knows :) let alone the drivers...

Which is why we shall try to make all master switches of a given type
control all radios of the same type, so you can at least have some idea
of what happens... and then the answer becomes "all radios of the same
type/class", except for those whose drivers didn't implement an rfkill
interface.

-- 
  "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 8/8] rfkill: add parameter to disable radios by default

[Henrique de Moraes Holschuh]

Currently, radios are always enabled when their rfkill interface is
registered.  This is not optimal, the safest state for a radio is to be
offline unless the user turns it on.

Add a module parameter that causes all radios to be disabled when their
rfkill interface is registered.  Add override parameters for each rfkill
switch type as well, just in case the user wants the defaults to be
different for a given radio type.

We don't change the module default, but I'd really recommed doing so in a
future patch.

The parameters are called "default_state" (global), and
"default_<type>_state", where <type> is: "wlan", "bluetooth", "uwb",
"wimax", or "any".  Note that "any" realy is a type and does not mean every
radio type (use the global "default_state" parameter for that.

The precedence order is "most specific first".

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Ivo van Doorn <IvDoorn@gmail.com>
Cc: John W. Linville <linville@tuxdriver.com>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 net/rfkill/rfkill.c |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 58 insertions(+), 1 deletions(-)

diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 9d3bffb..421de8c 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -39,6 +39,34 @@ MODULE_LICENSE("GPL");
 static LIST_HEAD(rfkill_list);	/* list of registered rf switches */
 static DEFINE_MUTEX(rfkill_mutex);
 
+static unsigned int rfkill_default_state = RFKILL_STATE_ON;
+static unsigned int rfkill_default_state_wlan = UINT_MAX;
+static unsigned int rfkill_default_state_bluetooth = UINT_MAX;
+static unsigned int rfkill_default_state_uwb = UINT_MAX;
+static unsigned int rfkill_default_state_wimax = UINT_MAX;
+static unsigned int rfkill_default_state_wwan = UINT_MAX;
+static unsigned int rfkill_default_state_any = UINT_MAX;
+
+module_param_named(default_state, rfkill_default_state, uint, 0444);
+MODULE_PARM_DESC(default_state,
+		 "Default initial state for all radio types, 0 = radio off");
+
+#define RFKILL_RADIO_DEFAULT(__type) \
+	module_param_named(default_##__type##_state, \
+			   rfkill_default_state_##__type, uint, 0444); \
+	MODULE_PARM_DESC(default_##__type##_state, \
+			 "Override initial state for radios of type " \
+			 #__type ", 0 = radio off");
+
+RFKILL_RADIO_DEFAULT(wlan)
+RFKILL_RADIO_DEFAULT(bluetooth)
+RFKILL_RADIO_DEFAULT(uwb)
+RFKILL_RADIO_DEFAULT(wimax)
+RFKILL_RADIO_DEFAULT(wwan)
+RFKILL_RADIO_DEFAULT(any)
+
+#undef RFKILL_RADIO_DEFAULT
+
 static enum rfkill_state rfkill_states[RFKILL_TYPE_MAX];
 
 
@@ -485,6 +513,17 @@ void rfkill_unregister(struct rfkill *rfkill)
 }
 EXPORT_SYMBOL(rfkill_unregister);
 
+static int __init rfkill_init_states(unsigned int state, enum rfkill_type type)
+{
+	if (state == UINT_MAX)
+		return 0;
+	else if (state == RFKILL_STATE_OFF || state == RFKILL_STATE_ON) {
+		rfkill_states[type] = state;
+		return 0;
+	}
+	return 1;
+}
+
 /*
  * Rfkill module initialization/deinitialization.
  */
@@ -493,8 +532,26 @@ static int __init rfkill_init(void)
 	int error;
 	int i;
 
+	if (rfkill_default_state != RFKILL_STATE_OFF &&
+	    rfkill_default_state != RFKILL_STATE_ON)
+		return -EINVAL;
+
 	for (i = 0; i < ARRAY_SIZE(rfkill_states); i++)
-		rfkill_states[i] = RFKILL_STATE_ON;
+		rfkill_states[i] = rfkill_default_state;
+
+	if (rfkill_init_states(rfkill_default_state_wlan,
+				  RFKILL_TYPE_WLAN)
+	    || rfkill_init_states(rfkill_default_state_bluetooth,
+				  RFKILL_TYPE_BLUETOOTH)
+	    || rfkill_init_states(rfkill_default_state_uwb,
+				  RFKILL_TYPE_UWB)
+	    || rfkill_init_states(rfkill_default_state_wimax,
+				  RFKILL_TYPE_WIMAX)
+	    || rfkill_init_states(rfkill_default_state_wwan,
+				  RFKILL_TYPE_WWAN)
+	    || rfkill_init_states(rfkill_default_state_any,
+				  RFKILL_TYPE_ANY))
+		return -EINVAL;
 
 	error = class_register(&rfkill_class);
 	if (error) {
-- 
1.5.4.4

Re: [PATCH 8/8] rfkill: add parameter to disable radios by default

[Ivo van Doorn]

Hi,

> Currently, radios are always enabled when their rfkill interface is
> registered.  This is not optimal, the safest state for a radio is to be
> offline unless the user turns it on.

It defaults to the current state of the switch type.
What perhaps could be done, is that during registration it either reads
the status directly from rfkill to what the device initialized it to,
or call get_status() during registration to determine the new state itself.
That way we prevent series of module parameters and initialize to the state
the driver has indicated..

> Add a module parameter that causes all radios to be disabled when their
> rfkill interface is registered.  Add override parameters for each rfkill
> switch type as well, just in case the user wants the defaults to be
> different for a given radio type.

I am not a big fan of large series of module parameters, so in that aspect
I am not a fan of this patch.

> We don't change the module default, but I'd really recommed doing so in a
> future patch.
> 
> The parameters are called "default_state" (global), and
> "default_<type>_state", where <type> is: "wlan", "bluetooth", "uwb",
> "wimax", or "any".  Note that "any" realy is a type and does not mean every
> radio type (use the global "default_state" parameter for that.
> 
> The precedence order is "most specific first".
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Ivo van Doorn <IvDoorn@gmail.com>
> Cc: John W. Linville <linville@tuxdriver.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>
> ---
>  net/rfkill/rfkill.c |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 58 insertions(+), 1 deletions(-)
> 
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index 9d3bffb..421de8c 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -39,6 +39,34 @@ MODULE_LICENSE("GPL");
>  static LIST_HEAD(rfkill_list);	/* list of registered rf switches */
>  static DEFINE_MUTEX(rfkill_mutex);
>  
> +static unsigned int rfkill_default_state = RFKILL_STATE_ON;
> +static unsigned int rfkill_default_state_wlan = UINT_MAX;
> +static unsigned int rfkill_default_state_bluetooth = UINT_MAX;
> +static unsigned int rfkill_default_state_uwb = UINT_MAX;
> +static unsigned int rfkill_default_state_wimax = UINT_MAX;
> +static unsigned int rfkill_default_state_wwan = UINT_MAX;
> +static unsigned int rfkill_default_state_any = UINT_MAX;
> +
> +module_param_named(default_state, rfkill_default_state, uint, 0444);
> +MODULE_PARM_DESC(default_state,
> +		 "Default initial state for all radio types, 0 = radio off");
> +
> +#define RFKILL_RADIO_DEFAULT(__type) \
> +	module_param_named(default_##__type##_state, \
> +			   rfkill_default_state_##__type, uint, 0444); \
> +	MODULE_PARM_DESC(default_##__type##_state, \
> +			 "Override initial state for radios of type " \
> +			 #__type ", 0 = radio off");
> +
> +RFKILL_RADIO_DEFAULT(wlan)
> +RFKILL_RADIO_DEFAULT(bluetooth)
> +RFKILL_RADIO_DEFAULT(uwb)
> +RFKILL_RADIO_DEFAULT(wimax)
> +RFKILL_RADIO_DEFAULT(wwan)
> +RFKILL_RADIO_DEFAULT(any)
> +
> +#undef RFKILL_RADIO_DEFAULT
> +
>  static enum rfkill_state rfkill_states[RFKILL_TYPE_MAX];
>  
>  
> @@ -485,6 +513,17 @@ void rfkill_unregister(struct rfkill *rfkill)
>  }
>  EXPORT_SYMBOL(rfkill_unregister);
>  
> +static int __init rfkill_init_states(unsigned int state, enum rfkill_type type)
> +{
> +	if (state == UINT_MAX)
> +		return 0;
> +	else if (state == RFKILL_STATE_OFF || state == RFKILL_STATE_ON) {
> +		rfkill_states[type] = state;
> +		return 0;
> +	}
> +	return 1;
> +}
> +
>  /*
>   * Rfkill module initialization/deinitialization.
>   */
> @@ -493,8 +532,26 @@ static int __init rfkill_init(void)
>  	int error;
>  	int i;
>  
> +	if (rfkill_default_state != RFKILL_STATE_OFF &&
> +	    rfkill_default_state != RFKILL_STATE_ON)
> +		return -EINVAL;
> +
>  	for (i = 0; i < ARRAY_SIZE(rfkill_states); i++)
> -		rfkill_states[i] = RFKILL_STATE_ON;
> +		rfkill_states[i] = rfkill_default_state;
> +
> +	if (rfkill_init_states(rfkill_default_state_wlan,
> +				  RFKILL_TYPE_WLAN)
> +	    || rfkill_init_states(rfkill_default_state_bluetooth,
> +				  RFKILL_TYPE_BLUETOOTH)
> +	    || rfkill_init_states(rfkill_default_state_uwb,
> +				  RFKILL_TYPE_UWB)
> +	    || rfkill_init_states(rfkill_default_state_wimax,
> +				  RFKILL_TYPE_WIMAX)
> +	    || rfkill_init_states(rfkill_default_state_wwan,
> +				  RFKILL_TYPE_WWAN)
> +	    || rfkill_init_states(rfkill_default_state_any,
> +				  RFKILL_TYPE_ANY))
> +		return -EINVAL;
>  
>  	error = class_register(&rfkill_class);
>  	if (error) {

Re: [PATCH 8/8] rfkill: add parameter to disable radios by default

[Henrique de Moraes Holschuh]

On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > Currently, radios are always enabled when their rfkill interface is
> > registered.  This is not optimal, the safest state for a radio is to be
> > offline unless the user turns it on.
> 
> It defaults to the current state of the switch type.

Hmm, I better reword that to "Currently, the desired radio state is
hardcoded in rfkill to be initially ON."  Would that be more clear?

The patch changes the initial state of the switch type, the one rfkill
currently is hardcoded to set to "RADIO IS ON" at module load.  I.e. it
just removes the hardcoding by making it possible to set the desired
initial state of all radios at module load time.

This makes rfkill actually a damn good UI for radio switch
initialization, IMHO.  You tell the kernel what is the state you want
for all radios in just one place and rfkill enforces all radios are set
to that state during bootstrap and initial coldplug party.

Drivers loaded later will be set to whatever the global state of their
switch type in rfkill is at the moment.

> What perhaps could be done, is that during registration it either reads
> the status directly from rfkill to what the device initialized it to,

I am *all* for a proper init of rfkill->state to match the real hardware
state.  I have always been, and I complained about that when rfkill
initially landed in mainline.

But I do agree that the current "desired behaviour" of rfkill (i.e. what
it should be doing, barring any bugs): that it will immediately set
newly registered radios to the state their type switch is at, to be a
good idea.

In fact, for write-only radios, you *have* to enforce their initial
state, because it is effectively unknown.  And in order to do that, you
must avoid any code paths that would do stuff like "if (rfk->state !=
desired_state) toggle_radio(desired_state)".

> or call get_status() during registration to determine the new state itself.
> That way we prevent series of module parameters and initialize to the state
> the driver has indicated..

As an user, I do NOT want to have to deal with per-radio-module
parameters in order to set their initial state to ON or OFF.  Yes, it is
what we have right now for some radio devices, and it is extremely
unoptimal.

Keeping track of the radio state (including which initial state it
should be set to) is exactly what something like the rfkill layer is
supposed to be good at: set parameters for an entire class of hardware
in a generic way, in a single place, and in one go.

> > Add a module parameter that causes all radios to be disabled when their
> > rfkill interface is registered.  Add override parameters for each rfkill
> > switch type as well, just in case the user wants the defaults to be
> > different for a given radio type.
> 
> I am not a big fan of large series of module parameters, so in that aspect
> I am not a fan of this patch.

If you think per-radio-type is too much, we just drop that part of the
patch.  It will make it quite smaller, and it will preserve the
functionality that is really needed (let the user tell the kernel to not
try to power up radios by default).

I made it type-aware because I can see an user wanting the long-range
radios off by default, and his personal-device-space radios (UWB,
Bluetooth) on by default.

> > We don't change the module default, but I'd really recommed doing so in a
> > future patch.

In fact, I better ask it now: do you want me to respin the patch with
just one global switch, and make its default to be "radios offline" ?

-- 
  "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 8/8] rfkill: add parameter to disable radios by default

[Ivo van Doorn]

On Saturday 12 April 2008, Henrique de Moraes Holschuh wrote:
> On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > > Currently, radios are always enabled when their rfkill interface is
> > > registered.  This is not optimal, the safest state for a radio is to be
> > > offline unless the user turns it on.
> > 
> > It defaults to the current state of the switch type.
> 
> Hmm, I better reword that to "Currently, the desired radio state is
> hardcoded in rfkill to be initially ON."  Would that be more clear?

No your original comment was clear enough, what I meant is that
the radio of the added switch is set to the state which is currently global
for that particular type.
So when there already is a switch of that type present, then the value
will be set to the state of the first registered switch (either ON or OFF).
When no similar switch is present then it will be set to ON.

> The patch changes the initial state of the switch type, the one rfkill
> currently is hardcoded to set to "RADIO IS ON" at module load.  I.e. it
> just removes the hardcoding by making it possible to set the desired
> initial state of all radios at module load time.
> 
> This makes rfkill actually a damn good UI for radio switch
> initialization, IMHO.  You tell the kernel what is the state you want
> for all radios in just one place and rfkill enforces all radios are set
> to that state during bootstrap and initial coldplug party.
> 
> Drivers loaded later will be set to whatever the global state of their
> switch type in rfkill is at the moment.

This is already implemented, the part that is missing is that for the first
added device it doesn't listen to the state of the hardware switch/button.
That I the issue that should be addressed with this patch:

- rfkill loads, sets initial state to UNDEFINED/UNITIALIZED
- driver with key X loads
- key X is registered to rfkill, driver provides current state in rfkill->state field
- rfkill checks global state with new state
   - if global state is UNDEFINED, then global state = rfkill->state
   - if global state == rfkill->state happy rfkill, nothing to do
   - if global state != rfkill->state change global state, toggle all radios

The state UNDEFINED/UNITIALIZED would in this case be a new state which should
be the default state during initialization.

> > What perhaps could be done, is that during registration it either reads
> > the status directly from rfkill to what the device initialized it to,
> 
> I am *all* for a proper init of rfkill->state to match the real hardware
> state.  I have always been, and I complained about that when rfkill
> initially landed in mainline.
> 
> But I do agree that the current "desired behaviour" of rfkill (i.e. what
> it should be doing, barring any bugs): that it will immediately set
> newly registered radios to the state their type switch is at, to be a
> good idea.
> 
> In fact, for write-only radios, you *have* to enforce their initial
> state, because it is effectively unknown.  And in order to do that, you
> must avoid any code paths that would do stuff like "if (rfk->state !=
> desired_state) toggle_radio(desired_state)".

Ok, for those devices we should set the default state to OFF _unless_
other devices of the same type are already present in which case the
default state should be set to the current global state for that type.

> > or call get_status() during registration to determine the new state itself.
> > That way we prevent series of module parameters and initialize to the state
> > the driver has indicated..
> 
> As an user, I do NOT want to have to deal with per-radio-module
> parameters in order to set their initial state to ON or OFF.  Yes, it is
> what we have right now for some radio devices, and it is extremely
> unoptimal.

Agreed, but having all those module parameters gathered into a single
module isn't a sound option either.

> Keeping track of the radio state (including which initial state it
> should be set to) is exactly what something like the rfkill layer is
> supposed to be good at: set parameters for an entire class of hardware
> in a generic way, in a single place, and in one go.

agreed.

> > > Add a module parameter that causes all radios to be disabled when their
> > > rfkill interface is registered.  Add override parameters for each rfkill
> > > switch type as well, just in case the user wants the defaults to be
> > > different for a given radio type.
> > 
> > I am not a big fan of large series of module parameters, so in that aspect
> > I am not a fan of this patch.
> 
> If you think per-radio-type is too much, we just drop that part of the
> patch.  It will make it quite smaller, and it will preserve the
> functionality that is really needed (let the user tell the kernel to not
> try to power up radios by default).

That and my suggestion above about state checking during rfkill registration
would indeed improve things.

> I made it type-aware because I can see an user wanting the long-range
> radios off by default, and his personal-device-space radios (UWB,
> Bluetooth) on by default.
> 
> > > We don't change the module default, but I'd really recommed doing so in a
> > > future patch.
> 
> In fact, I better ask it now: do you want me to respin the patch with
> just one global switch, and make its default to be "radios offline" ?

If the above is too much to have it included in time for 2.6.26, I would
say the default to offline. But that is personal preference and I won't NACK
either solution. So I let you choose this one. :)

Ivo

Re: [PATCH 8/8] rfkill: add parameter to disable radios by default

[Henrique de Moraes Holschuh]

On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> On Saturday 12 April 2008, Henrique de Moraes Holschuh wrote:
> > On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > > > Currently, radios are always enabled when their rfkill interface is
> > > > registered.  This is not optimal, the safest state for a radio is to be
> > > > offline unless the user turns it on.
> > > 
> > > It defaults to the current state of the switch type.
> > 
> > Hmm, I better reword that to "Currently, the desired radio state is
> > hardcoded in rfkill to be initially ON."  Would that be more clear?
> 
> No your original comment was clear enough, what I meant is that
> the radio of the added switch is set to the state which is currently global
> for that particular type.

Which is exactly what should be done, yes.  So far, I understood the
code correctly.

> So when there already is a switch of that type present, then the value
> will be set to the state of the first registered switch (either ON or OFF).
> When no similar switch is present then it will be set to ON.

Where in the code do we change the *global* state for a switch type,
based on the state of a particular switch of that type?  That's the step
I am missing.

Here's the timeline of how I think (so far, anyway) these whole thing
should work:

1. rfkill class registers with kernel
   - all initial global switch states (there are NO switches registered
     yet) are set to ON (current rfkill) or to either ON or OFF based on
     rfkill module parameters (with the patch).

     These are the "desired radio states for each radio type", and
     are not related to whatever *real* state the hardware is in.  Let's
     call them "global switch state (per switch type)", or "global
     switch states" for short.

     These global switch states are usually manipulated by rfkill-input
     when it traps input events that affect a certain type of switch
     (and we should probably have a way to manipulate these global
     states from userspace as well when rfkill-input is not loaded).

[user interaction to change these switch states MAY or MAY NOT happen
here, it doesn't matter.  If it happens, the switch states CAN be
changed]

2. driver with rfkill support is loaded (coldplug, hotplug, etc)

   2.1 driver allocates an rfkill structure

   2.2 driver sets rfkill->state to the REAL state of the radio in
       the hardware  [optional for write-only devices]  -- i'd prefer
       the API made this mandatory by requesting this state as a
       parameter of the rfkill_register() call.

   2.3 driver registers rfkill interface with the rfkill class

       2.3.1 rfkill class tries to set the radio state
             (using toggle_radio() and updating rfkill->state if that
	     succeeds) to the CURRENT global switch state.

If we have something different than this, it is a bug.  And I am not
clear what we should do in 2.3.1 WHEN the switch is a read-only switch,
but my guess is that we just ignore the failure, as trying to match all
other switches would break badly if there are more than one read-only
switches of the same type, and their states don't match.

> > The patch changes the initial state of the switch type, the one rfkill
> > currently is hardcoded to set to "RADIO IS ON" at module load.  I.e. it
> > just removes the hardcoding by making it possible to set the desired
> > initial state of all radios at module load time.
> > 
> > This makes rfkill actually a damn good UI for radio switch
> > initialization, IMHO.  You tell the kernel what is the state you want
> > for all radios in just one place and rfkill enforces all radios are set
> > to that state during bootstrap and initial coldplug party.
> > 
> > Drivers loaded later will be set to whatever the global state of their
> > switch type in rfkill is at the moment.
> 
> This is already implemented, the part that is missing is that for the first
> added device it doesn't listen to the state of the hardware switch/button.

Why should it?  See below for my comments on centralizing this policy on
the "initial state of radios" on rfkill.

> That I the issue that should be addressed with this patch:
> 
> - rfkill loads, sets initial state to UNDEFINED/UNITIALIZED

Which we don't have :(

> - driver with key X loads

We better make sure not to confuse the layers here.

We have a layer that deals with the switches themselves (rfkill), which
has NOTHING to do with keys at all.  It lets one access the current
switch state, and modify it.  It should also define the policy for what
the initial state of the switch should be, IMO.

Drivers like b43 and thinkpad-acpi are connected to the rfkill layer.

We have a layer that should be OPTIONAL, and that listen to input events
and manipulates the rfkill switches in semanthic groups ("switches for
the same type of hardware").  This is rfkill-input.

No drivers are connected to the rfkill-input layer directly.  When a
driver can report something like EV_KEY KEY_RADIO or EV_SW SW_RADIO (and
the type-specific ones, like KEY_WLAN), it connects directly to the
input layer.

> - key X is registered to rfkill, driver provides current state in rfkill->state field

key X is registered to the INPUT LAYER...  so this doesn't make sense,
or I am getting completely confused.

> - rfkill checks global state with new state
>    - if global state is UNDEFINED, then global state = rfkill->state

IMO, global state should NEVER be undefined.  Setting an initial policy
for radios is a damn good idea.  And if it is to be hardcoded, we
hardcode it to "radios OFF" which is always safe (but I'd just make it a
module parameter).

>    - if global state == rfkill->state happy rfkill, nothing to do

For write-only switches, you ALWAYS have to set them initially.  If you
mean the driver needs to do it to whatever it has done to rfkill->state
BEFORE calling rfkill_register, I am fine with it, but we *NEED* to
document that and revise all drivers to make sure it is correct.

>    - if global state != rfkill->state change global state, toggle all radios
> 
> The state UNDEFINED/UNITIALIZED would in this case be a new state which should
> be the default state during initialization.

I don't mind adding a new UNDEFINED state, it would be very useful for
write-only switches which can be messed with by something other than the
kernel.

But I don't think it is a good idea at all to have "unknown" be allowed
into the GLOBAL state.

If we have an *initial* global state of "unknown", it will confuse
users, as that will make the effective initial state of an entire radio
type change during coldplug depending on the initial state of the first
device of that type registered, which is NOT deterministic at all.

Please let's go with either an OFF *initial* global state (safe), or a
user-seletable *initial* global state (module parameter, and I'd say the
best default would be OFF here too, but AFAIK, it is ON right now).

And there is absolutely no reason at all (as far as I can see, anyway)
to let anything set the global state to "unknown".   If some radio
switch can't follow the global state (e.g. it is read-only), it simply
won't.  If someone overrides the state of the switch (write to state
attribute) to be different from the global state, it will be brought
back in sync the next time the global state changes.

I just thought of a way to let userspace mess with the global states
when it wants to control an entire set of switches (and not just one).
I will send in a patch if you like the idea:  basically, the rfkill
class module also registers a platform driver, and platform devices for
every type of switch.  THOSE devices are used to control the global
state for each type of switch.   This makes rfkill-input functionality
implementable in userspace, which is good.

> > > What perhaps could be done, is that during registration it either reads
> > > the status directly from rfkill to what the device initialized it to,
> > 
> > I am *all* for a proper init of rfkill->state to match the real hardware
> > state.  I have always been, and I complained about that when rfkill
> > initially landed in mainline.
> > 
> > But I do agree that the current "desired behaviour" of rfkill (i.e. what
> > it should be doing, barring any bugs): that it will immediately set
> > newly registered radios to the state their type switch is at, to be a
> > good idea.
> > 
> > In fact, for write-only radios, you *have* to enforce their initial
> > state, because it is effectively unknown.  And in order to do that, you
> > must avoid any code paths that would do stuff like "if (rfk->state !=
> > desired_state) toggle_radio(desired_state)".
> 
> Ok, for those devices we should set the default state to OFF _unless_
> other devices of the same type are already present in which case the
> default state should be set to the current global state for that type.

Or, if we forbit an undefined global state to begin with (see above), we
just set it to the global state (which will be ON or OFF), have less
complex code, AND reduce user confusion...

> > > or call get_status() during registration to determine the new state itself.
> > > That way we prevent series of module parameters and initialize to the state
> > > the driver has indicated..
> > 
> > As an user, I do NOT want to have to deal with per-radio-module
> > parameters in order to set their initial state to ON or OFF.  Yes, it is
> > what we have right now for some radio devices, and it is extremely
> > unoptimal.
> 
> Agreed, but having all those module parameters gathered into a single
> module isn't a sound option either.

Sure it is, if that module is the class that controls all the radio
switches (which rfkill is).   Unless we now make each rfkill radio type
a module of its own, which is quite doable, but IMHO a lot of code
complexity and memory waste for very little gain.

> > Keeping track of the radio state (including which initial state it
> > should be set to) is exactly what something like the rfkill layer is
> > supposed to be good at: set parameters for an entire class of hardware
> > in a generic way, in a single place, and in one go.
> 
> agreed.
> 
> > > > Add a module parameter that causes all radios to be disabled when their
> > > > rfkill interface is registered.  Add override parameters for each rfkill
> > > > switch type as well, just in case the user wants the defaults to be
> > > > different for a given radio type.
> > > 
> > > I am not a big fan of large series of module parameters, so in that aspect
> > > I am not a fan of this patch.
> > 
> > If you think per-radio-type is too much, we just drop that part of the
> > patch.  It will make it quite smaller, and it will preserve the
> > functionality that is really needed (let the user tell the kernel to not
> > try to power up radios by default).
> 
> That and my suggestion above about state checking during rfkill registration
> would indeed improve things.

How about we have a *single* initial global state parameter, but
disallow an "unknown" global state parameter?  Less user confusion, less
module parameters, AND less code complexity.

> > I made it type-aware because I can see an user wanting the long-range
> > radios off by default, and his personal-device-space radios (UWB,
> > Bluetooth) on by default.
> > 
> > > > We don't change the module default, but I'd really recommed doing so in a
> > > > future patch.
> > 
> > In fact, I better ask it now: do you want me to respin the patch with
> > just one global switch, and make its default to be "radios offline" ?
> 
> If the above is too much to have it included in time for 2.6.26, I would
> say the default to offline. But that is personal preference and I won't NACK
> either solution. So I let you choose this one. :)

Ok, I will put my brains and fingers into high gear and come up with a
new patchset, but first I have to look at input-pooldev to see if/how I
can use that for thinkpad-acpi, in order to re-think the read-write
support.

BTW: isn't it high time to add yourself as the RFKILL maintainer to
MAINTAINERS (couldn't find you in there, nor RFKILL for that matter) and
also to document the rfkill class in 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 8/8] rfkill: add parameter to disable radios by default

[Ivo van Doorn]

> > So when there already is a switch of that type present, then the value
> > will be set to the state of the first registered switch (either ON or OFF).
> > When no similar switch is present then it will be set to ON.
> 
> Where in the code do we change the *global* state for a switch type,
> based on the state of a particular switch of that type?  That's the step
> I am missing.

rfkill_switch_all() does exactly that and then calls rfkill_toggle_radio()
for all registered rfkill structures of the changed type.

rfkill_switch_all() is called from rfkill-input

> Here's the timeline of how I think (so far, anyway) these whole thing
> should work:
> 
> 1. rfkill class registers with kernel
>    - all initial global switch states (there are NO switches registered
>      yet) are set to ON (current rfkill) or to either ON or OFF based on
>      rfkill module parameters (with the patch).
> 
>      These are the "desired radio states for each radio type", and
>      are not related to whatever *real* state the hardware is in.  Let's
>      call them "global switch state (per switch type)", or "global
>      switch states" for short.
> 
>      These global switch states are usually manipulated by rfkill-input
>      when it traps input events that affect a certain type of switch
>      (and we should probably have a way to manipulate these global
>      states from userspace as well when rfkill-input is not loaded).

that is provided through sysfs, or some userspace tool that listens
to the input devices which have been registered.

> [user interaction to change these switch states MAY or MAY NOT happen
> here, it doesn't matter.  If it happens, the switch states CAN be
> changed]
> 
> 2. driver with rfkill support is loaded (coldplug, hotplug, etc)
> 
>    2.1 driver allocates an rfkill structure
> 
>    2.2 driver sets rfkill->state to the REAL state of the radio in
>        the hardware  [optional for write-only devices]  -- i'd prefer
>        the API made this mandatory by requesting this state as a
>        parameter of the rfkill_register() call.
> 
>    2.3 driver registers rfkill interface with the rfkill class
> 
>        2.3.1 rfkill class tries to set the radio state
>              (using toggle_radio() and updating rfkill->state if that
> 	     succeeds) to the CURRENT global switch state.
> 
> If we have something different than this, it is a bug.  And I am not
> clear what we should do in 2.3.1 WHEN the switch is a read-only switch,
> but my guess is that we just ignore the failure, as trying to match all
> other switches would break badly if there are more than one read-only
> switches of the same type, and their states don't match.

True but what a user doesn't want is this:

 - wireless key set to "ON"
 - boot kernel
 - press wireless key twice to actually enable the radio

I have a laptop containing 2 read-only keys, one for 802.11 and one for bluetooth,
the global state should be whatever those 2 keys say, otherwise the
status of the various keys can become completely messed up.

I think the chance of having 1 read-only key and multiple read-write/write-only
keys is bigger then having multiple read-only keys.

> > That I the issue that should be addressed with this patch:
> > 
> > - rfkill loads, sets initial state to UNDEFINED/UNITIALIZED
> 
> Which we don't have :(
> 
> > - driver with key X loads
> 
> We better make sure not to confuse the layers here.
> 
> We have a layer that deals with the switches themselves (rfkill), which
> has NOTHING to do with keys at all.  It lets one access the current
> switch state, and modify it.  It should also define the policy for what
> the initial state of the switch should be, IMO.
> 
> Drivers like b43 and thinkpad-acpi are connected to the rfkill layer.
> 
> We have a layer that should be OPTIONAL, and that listen to input events
> and manipulates the rfkill switches in semanthic groups ("switches for
> the same type of hardware").  This is rfkill-input.

right, rfkill-input is optional although I already noticed that at least b43 
makes that module mandatory for some reason.

> No drivers are connected to the rfkill-input layer directly.  When a
> driver can report something like EV_KEY KEY_RADIO or EV_SW SW_RADIO (and
> the type-specific ones, like KEY_WLAN), it connects directly to the
> input layer.

right.

> > - key X is registered to rfkill, driver provides current state in rfkill->state field
> 
> key X is registered to the INPUT LAYER...  so this doesn't make sense,
> or I am getting completely confused.

No sorry. The correct wording would be that:
Driver registered key X to the input layer
Driver registerd rfkill structure (for key X) to rfkill

The initial state of the rfkill structure should be set by the driver during registration
(either by initialization of the field, or as function argument as you suggested).

> > - rfkill checks global state with new state
> >    - if global state is UNDEFINED, then global state = rfkill->state
> 
> IMO, global state should NEVER be undefined.  Setting an initial policy
> for radios is a damn good idea.  And if it is to be hardcoded, we
> hardcode it to "radios OFF" which is always safe (but I'd just make it a
> module parameter).

Ok.

> >    - if global state == rfkill->state happy rfkill, nothing to do
> 
> For write-only switches, you ALWAYS have to set them initially.  If you
> mean the driver needs to do it to whatever it has done to rfkill->state
> BEFORE calling rfkill_register, I am fine with it, but we *NEED* to
> document that and revise all drivers to make sure it is correct.

No letting rfkill handle that would be safer, the driver should be in charge
of determining if toggle_radio() should really do something (in case the interface
hasn't been brought up yet).

> >    - if global state != rfkill->state change global state, toggle all radios
> > 
> > The state UNDEFINED/UNITIALIZED would in this case be a new state which should
> > be the default state during initialization.
> 
> I don't mind adding a new UNDEFINED state, it would be very useful for
> write-only switches which can be messed with by something other than the
> kernel.

Well if we are going to demand the initial state as argument it might not even
be needed. But then the driver should tell rfkill in some way that it is write-only.

> But I don't think it is a good idea at all to have "unknown" be allowed
> into the GLOBAL state.

Ok.

> Please let's go with either an OFF *initial* global state (safe), or a
> user-seletable *initial* global state (module parameter, and I'd say the
> best default would be OFF here too, but AFAIK, it is ON right now).
> 
> And there is absolutely no reason at all (as far as I can see, anyway)
> to let anything set the global state to "unknown".   If some radio
> switch can't follow the global state (e.g. it is read-only), it simply
> won't.  If someone overrides the state of the switch (write to state
> attribute) to be different from the global state, it will be brought
> back in sync the next time the global state changes.
> 
> I just thought of a way to let userspace mess with the global states
> when it wants to control an entire set of switches (and not just one).
> I will send in a patch if you like the idea:  basically, the rfkill
> class module also registers a platform driver, and platform devices for
> every type of switch.  THOSE devices are used to control the global
> state for each type of switch.   This makes rfkill-input functionality
> implementable in userspace, which is good.

Sounds like a good idea.

> > > > or call get_status() during registration to determine the new state itself.
> > > > That way we prevent series of module parameters and initialize to the state
> > > > the driver has indicated..
> > > 
> > > As an user, I do NOT want to have to deal with per-radio-module
> > > parameters in order to set their initial state to ON or OFF.  Yes, it is
> > > what we have right now for some radio devices, and it is extremely
> > > unoptimal.
> > 
> > Agreed, but having all those module parameters gathered into a single
> > module isn't a sound option either.
> 
> Sure it is, if that module is the class that controls all the radio
> switches (which rfkill is).   Unless we now make each rfkill radio type
> a module of its own, which is quite doable, but IMHO a lot of code
> complexity and memory waste for very little gain.

No seperating everything over multiple moduels sounds like a very bad idea.
I would prefer either the OFF default, or a single module parameter.
But again, if others also feel that multiple module parameters should be the way to
go I won't object that much. ;)

> > > > > Add a module parameter that causes all radios to be disabled when their
> > > > > rfkill interface is registered.  Add override parameters for each rfkill
> > > > > switch type as well, just in case the user wants the defaults to be
> > > > > different for a given radio type.
> > > > 
> > > > I am not a big fan of large series of module parameters, so in that aspect
> > > > I am not a fan of this patch.
> > > 
> > > If you think per-radio-type is too much, we just drop that part of the
> > > patch.  It will make it quite smaller, and it will preserve the
> > > functionality that is really needed (let the user tell the kernel to not
> > > try to power up radios by default).
> > 
> > That and my suggestion above about state checking during rfkill registration
> > would indeed improve things.
> 
> How about we have a *single* initial global state parameter, but
> disallow an "unknown" global state parameter?  Less user confusion, less
> module parameters, AND less code complexity.

Ok.

> > > I made it type-aware because I can see an user wanting the long-range
> > > radios off by default, and his personal-device-space radios (UWB,
> > > Bluetooth) on by default.
> > > 
> > > > > We don't change the module default, but I'd really recommed doing so in a
> > > > > future patch.
> > > 
> > > In fact, I better ask it now: do you want me to respin the patch with
> > > just one global switch, and make its default to be "radios offline" ?
> > 
> > If the above is too much to have it included in time for 2.6.26, I would
> > say the default to offline. But that is personal preference and I won't NACK
> > either solution. So I let you choose this one. :)
> 
> Ok, I will put my brains and fingers into high gear and come up with a
> new patchset, but first I have to look at input-pooldev to see if/how I
> can use that for thinkpad-acpi, in order to re-think the read-write
> support.

You can check the implementation in
drivers/net/wireless/rt2x00 or drivers/net/wireless/b43
for an example implementation, both use input-polldev.
I believe only rt2x00 has rfkill-write support while the b43 hardware toggles
the hardware directly and uses rfkill only for notifications.

> BTW: isn't it high time to add yourself as the RFKILL maintainer to
> MAINTAINERS (couldn't find you in there, nor RFKILL for that matter) and

True, I'll do that right away. :)

> also to document the rfkill class in Documentation/ ?

You mean like Documentation/rfkill.txt
It is a bit basic, but I haven't had time to expand it yet.

Ivo

Re: [PATCH 8/8] rfkill: add parameter to disable radios by default

[Henrique de Moraes Holschuh]

On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > > So when there already is a switch of that type present, then the value
> > > will be set to the state of the first registered switch (either ON or OFF).
> > > When no similar switch is present then it will be set to ON.
> > 
> > Where in the code do we change the *global* state for a switch type,
> > based on the state of a particular switch of that type?  That's the step
> > I am missing.
> 
> rfkill_switch_all() does exactly that and then calls rfkill_toggle_radio()
> for all registered rfkill structures of the changed type.
> 
> rfkill_switch_all() is called from rfkill-input

So, we don't mess with the global state when we register the first
device for a type at all, which is how I thought the code worked indeed.

I don't understand what you mean with "So when there already is a switch
of that type present, then the value will be set to the state of the
first registered switch", then.

> > Here's the timeline of how I think (so far, anyway) these whole thing
> > should work:
> > 
> > 1. rfkill class registers with kernel
> >    - all initial global switch states (there are NO switches registered
> >      yet) are set to ON (current rfkill) or to either ON or OFF based on
> >      rfkill module parameters (with the patch).
> > 
> >      These are the "desired radio states for each radio type", and
> >      are not related to whatever *real* state the hardware is in.  Let's
> >      call them "global switch state (per switch type)", or "global
> >      switch states" for short.
> > 
> >      These global switch states are usually manipulated by rfkill-input
> >      when it traps input events that affect a certain type of switch
> >      (and we should probably have a way to manipulate these global
> >      states from userspace as well when rfkill-input is not loaded).
> 
> that is provided through sysfs, or some userspace tool that listens
> to the input devices which have been registered.

Err, where in sysfs can I set the global state of all bluetooth
switches, for example?  I *can* loop all over the switches, find the
ones with a type of bluetooth, and change them.  But that's NOT the same
thing.

Right now one needs rfkill-input loaded, and one has to send an
KEY_BLUETOOTH event from userspace.  In the dark, I should say, because
one can't ask the kernel what the global state is, and any rfkill switch
could have its state different from the global state if someone
interacted with it directly over sysfs.

I will address this in a patch, I think.  It will add a much cleaner way
for userspace to interact with the global states, and it will let the
used chose rfkill-input as an optional lightweight policy for the rfkill
switches and input events, OR to implement a heavy-weight one in
userspace.

> >        2.3.1 rfkill class tries to set the radio state
> >              (using toggle_radio() and updating rfkill->state if that
> > 	     succeeds) to the CURRENT global switch state.
> > 
> > If we have something different than this, it is a bug.  And I am not
> > clear what we should do in 2.3.1 WHEN the switch is a read-only switch,
> > but my guess is that we just ignore the failure, as trying to match all
> > other switches would break badly if there are more than one read-only
> > switches of the same type, and their states don't match.
> 
> True but what a user doesn't want is this:
> 
>  - wireless key set to "ON"
>  - boot kernel
>  - press wireless key twice to actually enable the radio
> 
> I have a laptop containing 2 read-only keys, one for 802.11 and one for bluetooth,
> the global state should be whatever those 2 keys say, otherwise the
> status of the various keys can become completely messed up.
> 
> I think the chance of having 1 read-only key and multiple read-write/write-only
> keys is bigger then having multiple read-only keys.

Ok.  So we assume the read-only ones are more important, and for the
*first* read-only rfkill switch registered for a type, we change the
global state for that type to match the switch state.

For read-only switches of type "any", we do that for all types that have
NO read-only switches registered, I suppose.

Let's call that behaviour a "master" rfkill switch.  Its initial state
is forced on the global state when it is registered, and when it toggles
state, the global state is ALSO toggled.

> > > That I the issue that should be addressed with this patch:
> > > 
> > > - rfkill loads, sets initial state to UNDEFINED/UNITIALIZED
> > 
> > Which we don't have :(
> > 
> > > - driver with key X loads
> > 
> > We better make sure not to confuse the layers here.
> > 
> > We have a layer that deals with the switches themselves (rfkill), which
> > has NOTHING to do with keys at all.  It lets one access the current
> > switch state, and modify it.  It should also define the policy for what
> > the initial state of the switch should be, IMO.
> > 
> > Drivers like b43 and thinkpad-acpi are connected to the rfkill layer.
> > 
> > We have a layer that should be OPTIONAL, and that listen to input events
> > and manipulates the rfkill switches in semanthic groups ("switches for
> > the same type of hardware").  This is rfkill-input.
> 
> right, rfkill-input is optional although I already noticed that at least b43 
> makes that module mandatory for some reason.

We better ask the b43 maintainer about it, then.

> > > - key X is registered to rfkill, driver provides current state in rfkill->state field
> > 
> > key X is registered to the INPUT LAYER...  so this doesn't make sense,
> > or I am getting completely confused.
> 
> No sorry. The correct wording would be that:
> Driver registered key X to the input layer
> Driver registerd rfkill structure (for key X) to rfkill

Ah, ok.

> > >    - if global state == rfkill->state happy rfkill, nothing to do
> > 
> > For write-only switches, you ALWAYS have to set them initially.  If you
> > mean the driver needs to do it to whatever it has done to rfkill->state
> > BEFORE calling rfkill_register, I am fine with it, but we *NEED* to
> > document that and revise all drivers to make sure it is correct.
> 
> No letting rfkill handle that would be safer, the driver should be in charge
> of determining if toggle_radio() should really do something (in case the interface
> hasn't been brought up yet).

So, it is better to make the initial state something drivers have to
provide in the rfkill_register call, IMHO.  Otherwise, people will
forget to initialize that field, and we can't find that out unless we
manually inspect every code that calls rfkill_register.

> > >    - if global state != rfkill->state change global state, toggle all radios
> > > 
> > > The state UNDEFINED/UNITIALIZED would in this case be a new state which should
> > > be the default state during initialization.
> > 
> > I don't mind adding a new UNDEFINED state, it would be very useful for
> > write-only switches which can be messed with by something other than the
> > kernel.
> 
> Well if we are going to demand the initial state as argument it might not even
> be needed. But then the driver should tell rfkill in some way that it is write-only.

Can we just have a bitfield flags field for rfkill devices, and use it
to host such information?  Could be useful to define a read/write switch
as a "master" switch (i.e. one that behaves like the read-only switches
shall in the next round of these patches, and toggle the global state)
for example.

I really have some good uses for such a field, and it has to do with the
global states.  You'll see it in the new patchset, when I finish it.

> > also to document the rfkill class in Documentation/ ?
> 
> You mean like Documentation/rfkill.txt

Drat, I wonder why I didn't find it when I searched for it in latest
mainline.  I will read it now, and update it in any patches I write.

-- 
  "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 8/8] rfkill: add parameter to disable radios by default

[Ivo van Doorn]

On Saturday 12 April 2008, Henrique de Moraes Holschuh wrote:
> On Sat, 12 Apr 2008, Ivo van Doorn wrote:
> > > > So when there already is a switch of that type present, then the value
> > > > will be set to the state of the first registered switch (either ON or OFF).
> > > > When no similar switch is present then it will be set to ON.
> > > 
> > > Where in the code do we change the *global* state for a switch type,
> > > based on the state of a particular switch of that type?  That's the step
> > > I am missing.
> > 
> > rfkill_switch_all() does exactly that and then calls rfkill_toggle_radio()
> > for all registered rfkill structures of the changed type.
> > 
> > rfkill_switch_all() is called from rfkill-input
> 
> So, we don't mess with the global state when we register the first
> device for a type at all, which is how I thought the code worked indeed.
> 
> I don't understand what you mean with "So when there already is a switch
> of that type present, then the value will be set to the state of the
> first registered switch", then.

Consider the following events:

1) rfkill initializes all types to RFKILL_STATE_ON
2) driver X registers rfkill structure
3) input-polldev polls X switches state to RFKILL_STATE_OFF
4) driver Y registers rfkill structure

at that time, what should the status for driver Y be? Should it be changed
to reflect the global state (RFKILL_STATE_OFF) or the state of the driver
(RFKILL_STATE_ON)

The current approach will set Y to RFKILL_STATE_ON, see rfkill_add_switch()

> > > Here's the timeline of how I think (so far, anyway) these whole thing
> > > should work:
> > > 
> > > 1. rfkill class registers with kernel
> > >    - all initial global switch states (there are NO switches registered
> > >      yet) are set to ON (current rfkill) or to either ON or OFF based on
> > >      rfkill module parameters (with the patch).
> > > 
> > >      These are the "desired radio states for each radio type", and
> > >      are not related to whatever *real* state the hardware is in.  Let's
> > >      call them "global switch state (per switch type)", or "global
> > >      switch states" for short.
> > > 
> > >      These global switch states are usually manipulated by rfkill-input
> > >      when it traps input events that affect a certain type of switch
> > >      (and we should probably have a way to manipulate these global
> > >      states from userspace as well when rfkill-input is not loaded).
> > 
> > that is provided through sysfs, or some userspace tool that listens
> > to the input devices which have been registered.
> 
> Err, where in sysfs can I set the global state of all bluetooth
> switches, for example?  I *can* loop all over the switches, find the
> ones with a type of bluetooth, and change them.  But that's NOT the same
> thing.

Ok, I didn't mean the global state for sysfs.
But that is where your previous suggestion comes in to create devices
for each type. This could be done at the moment the first rfkill structure of
that type is added.

> Right now one needs rfkill-input loaded, and one has to send an
> KEY_BLUETOOTH event from userspace.  In the dark, I should say, because
> one can't ask the kernel what the global state is, and any rfkill switch
> could have its state different from the global state if someone
> interacted with it directly over sysfs.

Well all devices of the same type should be in the same state. That is because
during rfkill registration the status of the registrant will be changed to the
current global state.
This does open a hole when the status is changed through sysfs.

> I will address this in a patch, I think.  It will add a much cleaner way
> for userspace to interact with the global states, and it will let the
> used chose rfkill-input as an optional lightweight policy for the rfkill
> switches and input events, OR to implement a heavy-weight one in
> userspace.

That difference has always been the setup for rfkill.
rfkill-input should be used when there is no userspace agent running
that will handle the events. That is why drivers shouldn't depend on rfkill-input
and only work with rfkill.

> > >        2.3.1 rfkill class tries to set the radio state
> > >              (using toggle_radio() and updating rfkill->state if that
> > > 	     succeeds) to the CURRENT global switch state.
> > > 
> > > If we have something different than this, it is a bug.  And I am not
> > > clear what we should do in 2.3.1 WHEN the switch is a read-only switch,
> > > but my guess is that we just ignore the failure, as trying to match all
> > > other switches would break badly if there are more than one read-only
> > > switches of the same type, and their states don't match.
> > 
> > True but what a user doesn't want is this:
> > 
> >  - wireless key set to "ON"
> >  - boot kernel
> >  - press wireless key twice to actually enable the radio
> > 
> > I have a laptop containing 2 read-only keys, one for 802.11 and one for bluetooth,
> > the global state should be whatever those 2 keys say, otherwise the
> > status of the various keys can become completely messed up.
> > 
> > I think the chance of having 1 read-only key and multiple read-write/write-only
> > keys is bigger then having multiple read-only keys.
> 
> Ok.  So we assume the read-only ones are more important, and for the
> *first* read-only rfkill switch registered for a type, we change the
> global state for that type to match the switch state.
> 
> For read-only switches of type "any", we do that for all types that have
> NO read-only switches registered, I suppose.
> 
> Let's call that behaviour a "master" rfkill switch.  Its initial state
> is forced on the global state when it is registered, and when it toggles
> state, the global state is ALSO toggled.

right. I think this would be the correct implementation for all systems
as I seriously doubt any device to have 2 read-only keys for exactly the same
radio type.

> > > > That I the issue that should be addressed with this patch:
> > > > 
> > > > - rfkill loads, sets initial state to UNDEFINED/UNITIALIZED
> > > 
> > > Which we don't have :(
> > > 
> > > > - driver with key X loads
> > > 
> > > We better make sure not to confuse the layers here.
> > > 
> > > We have a layer that deals with the switches themselves (rfkill), which
> > > has NOTHING to do with keys at all.  It lets one access the current
> > > switch state, and modify it.  It should also define the policy for what
> > > the initial state of the switch should be, IMO.
> > > 
> > > Drivers like b43 and thinkpad-acpi are connected to the rfkill layer.
> > > 
> > > We have a layer that should be OPTIONAL, and that listen to input events
> > > and manipulates the rfkill switches in semanthic groups ("switches for
> > > the same type of hardware").  This is rfkill-input.
> > 
> > right, rfkill-input is optional although I already noticed that at least b43 
> > makes that module mandatory for some reason.
> 
> We better ask the b43 maintainer about it, then.

Yes, when the behavior discussed in this thread is implemented to
have rfkill and rfkill-input do the right actions then I'll browse the kernel
tree to look for rfkill implementations and check if the correct approach is taken.

> > > >    - if global state == rfkill->state happy rfkill, nothing to do
> > > 
> > > For write-only switches, you ALWAYS have to set them initially.  If you
> > > mean the driver needs to do it to whatever it has done to rfkill->state
> > > BEFORE calling rfkill_register, I am fine with it, but we *NEED* to
> > > document that and revise all drivers to make sure it is correct.
> > 
> > No letting rfkill handle that would be safer, the driver should be in charge
> > of determining if toggle_radio() should really do something (in case the interface
> > hasn't been brought up yet).
> 
> So, it is better to make the initial state something drivers have to
> provide in the rfkill_register call, IMHO.  Otherwise, people will
> forget to initialize that field, and we can't find that out unless we
> manually inspect every code that calls rfkill_register.

Agreed.

> > > >    - if global state != rfkill->state change global state, toggle all radios
> > > > 
> > > > The state UNDEFINED/UNITIALIZED would in this case be a new state which should
> > > > be the default state during initialization.
> > > 
> > > I don't mind adding a new UNDEFINED state, it would be very useful for
> > > write-only switches which can be messed with by something other than the
> > > kernel.
> > 
> > Well if we are going to demand the initial state as argument it might not even
> > be needed. But then the driver should tell rfkill in some way that it is write-only.
> 
> Can we just have a bitfield flags field for rfkill devices, and use it
> to host such information?  Could be useful to define a read/write switch
> as a "master" switch (i.e. one that behaves like the read-only switches
> shall in the next round of these patches, and toggle the global state)
> for example.

Sounds good to me.

> I really have some good uses for such a field, and it has to do with the
> global states.  You'll see it in the new patchset, when I finish it.

Excellent. Thanks.

> > > also to document the rfkill class in Documentation/ ?
> > 
> > You mean like Documentation/rfkill.txt
> 
> Drat, I wonder why I didn't find it when I searched for it in latest
> mainline.  I will read it now, and update it in any patches I write.

Ok.
When you have the new patches that change the behavior I'll
write an update for the documentation. To make sure driver authors
will have at least a correct reference.

Ivo

Re: [GIT PATCH] rfkill support for r/w and r/o rfkill switches

[Ivo van Doorn]

Hi,

> This patch series (based on v2.6.25-rc8-mm2) has several improvements to
> the rfkill class that I need for thinkpad-acpi, plus two fluff and
> documentation fixes.
> 
> I'd appreciate comments, and if the patches are acceptable, that they are
> sent to mainline early during the 2.6.26 merge window.  That way, I could
> try for a thinkpad-acpi rfkill submission for 2.6.26 as well.
> 
> The thinkpad-acpi work that needs these patches is ready, and can be
> looked at on the thinkpad-acpi git tree (devel branch):
> http://repo.or.cz/w/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git
> 
> Note: new thinkpads have a read-only "type any" radio switch, plus
> read-write WWAN and Bluetooth radio switches.   The embedded WLAN cards
> have WLAN read-write switches (that should be handled by the ipw* and iwl*
> drivers).  Very old thinkpads had ACPI-based WLAN radio switches, but
> thinkpad-acpi does not support those.

The following patches are fine by me, without any additional comments:
[PATCH 1/8] rfkill: clarify meaning of rfkill states
[PATCH 2/8] rfkill: fix minor typo in kernel doc
[PATCH 5/8] rfkill: add read-only rfkill switch support
[PATCH 7/8] rfkill: add an "any radio" switch type and functionality

The following patches are overall fine, but I have some additional comments:
[PATCH 6/8] rfkill: add the WWAN radio type
[PATCH 3/8] rfkill: handle KEY_RADIO and SW_RADIO events

The following patches are _not_ fine with me:
[PATCH 4/8] rfkill: add read-write rfkill switch support
[PATCH 8/8] rfkill: add parameter to disable radios by default

Ivo

Re: [GIT PATCH] rfkill support for r/w and r/o rfkill switches

[John W. Linville]

On Fri, Apr 11, 2008 at 05:37:16PM -0300, Henrique de Moraes Holschuh wrote:
> 
> This patch series (based on v2.6.25-rc8-mm2) has several improvements to
> the rfkill class that I need for thinkpad-acpi, plus two fluff and
> documentation fixes.

Who is handling these rfkill patches?  I think in the past they have
gone through me, but some may have gone directly to Dave M.

If they are coming to me, please collect Ivo's ACKs and resend them
to linux-wireless@vger.kernel.org with me CC'ed as well.

Thanks,

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

Re: [GIT PATCH] rfkill support for r/w and r/o rfkill switches

[Ivo van Doorn]

On Wednesday 16 April 2008, John W. Linville wrote:
> On Fri, Apr 11, 2008 at 05:37:16PM -0300, Henrique de Moraes Holschuh wrote:
> > 
> > This patch series (based on v2.6.25-rc8-mm2) has several improvements to
> > the rfkill class that I need for thinkpad-acpi, plus two fluff and
> > documentation fixes.
> 
> Who is handling these rfkill patches?  I think in the past they have
> gone through me, but some may have gone directly to Dave M.

Yes initial rfkill introduction went through wireless, but subsequent patches
seemed to have gone directly into net-2.6.
I must admit I am not sure what the correct flow for rkfill should be. :S

> If they are coming to me, please collect Ivo's ACKs and resend them
> to linux-wireless@vger.kernel.org with me CC'ed as well.

I am going to setup a rfkill git tree from where rfkill can be managed,
but I still had to look into the correct git tree to branch from.

We should sort out how the rfkill patch flow should go,
directly into net-2.6 or to wireless-2.6/wireless-testing
Although rfkill is a wireless tool, one could argue that linux-wireless
is mostly aimed at the 802.11 protocol, whereas rfkill applies to
irda and bluetooth as well.

Ivo

Re: [GIT PATCH] rfkill support for r/w and r/o rfkill switches

[John W. Linville]

On Wed, Apr 16, 2008 at 09:26:43PM +0200, Ivo van Doorn wrote:
> On Wednesday 16 April 2008, John W. Linville wrote:

> > If they are coming to me, please collect Ivo's ACKs and resend them
> > to linux-wireless@vger.kernel.org with me CC'ed as well.
> 
> I am going to setup a rfkill git tree from where rfkill can be managed,
> but I still had to look into the correct git tree to branch from.
> 
> We should sort out how the rfkill patch flow should go,
> directly into net-2.6 or to wireless-2.6/wireless-testing
> Although rfkill is a wireless tool, one could argue that linux-wireless
> is mostly aimed at the 802.11 protocol, whereas rfkill applies to
> irda and bluetooth as well.

I suppose you could manage it as a separate tree.  Before this round of
patches, that would have seemed like overkill. :-)  My only argument
would be that there are probably more users of rfkill in the wireless
(i.e. wireless LAN) trees than in the Bluetooth, UWB, or WiMAX trees.
But honestly, it is up to you and Dave.  I'm happy to take the patches
in my trees or you guys can arrange something else.

Just let me know if you want me to merge them.  Otherwise, I'll drop
them from my queue.

Thanks,

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

Re: [GIT PATCH] rfkill support for r/w and r/o rfkill switches

[Ivo van Doorn]

On Wednesday 16 April 2008, John W. Linville wrote:
> On Wed, Apr 16, 2008 at 09:26:43PM +0200, Ivo van Doorn wrote:
> > On Wednesday 16 April 2008, John W. Linville wrote:
> 
> > > If they are coming to me, please collect Ivo's ACKs and resend them
> > > to linux-wireless@vger.kernel.org with me CC'ed as well.
> > 
> > I am going to setup a rfkill git tree from where rfkill can be managed,
> > but I still had to look into the correct git tree to branch from.
> > 
> > We should sort out how the rfkill patch flow should go,
> > directly into net-2.6 or to wireless-2.6/wireless-testing
> > Although rfkill is a wireless tool, one could argue that linux-wireless
> > is mostly aimed at the 802.11 protocol, whereas rfkill applies to
> > irda and bluetooth as well.
> 
> I suppose you could manage it as a separate tree.  Before this round of
> patches, that would have seemed like overkill. :-)  My only argument
> would be that there are probably more users of rfkill in the wireless
> (i.e. wireless LAN) trees than in the Bluetooth, UWB, or WiMAX trees.
> But honestly, it is up to you and Dave.  I'm happy to take the patches
> in my trees or you guys can arrange something else.

Dave, do you want the rfkill patches directly, or should they go through
the wireless tree first?

> Just let me know if you want me to merge them.  Otherwise, I'll drop
> them from my queue.

Well the patches from Henrique shouldn't be applied yet,
since he is reworking several of those patches.

Ivo

Re: [GIT PATCH] rfkill support for r/w and r/o rfkill switches

[David Miller]

From: Ivo van Doorn <ivdoorn@gmail.com>
Date: Wed, 16 Apr 2008 22:40:38 +0200

> Dave, do you want the rfkill patches directly, or should they go through
> the wireless tree first?

I'm happy if it goes through the wireless tree.

That way experts like John and the others can review it
from a wireless perspective, and it's one less pull
for me to do :-)