[PATCH 1/2] input: Add KEY_RFKILL_ALL

[PATCH 1/2] input: Add KEY_RFKILL_ALL

[Matthew Garrett]

Most laptops have keys that are intended to toggle all device state, not
just wifi. These are currently generally mapped to KEY_WLAN. As a result,
rfkill will only kill or enable wifi in response to the key press. This
confuses users and can make it difficult for them to enable bluetooth
and wwan devices.

This patch adds a new keycode, KEY_RFKILL_ALL. It indicates that the
system should toggle the state of all rfkillable devices.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 include/linux/input.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/linux/input.h b/include/linux/input.h
index 8b3bc3e..0eff8ff 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -595,6 +595,8 @@ struct input_absinfo {
 #define KEY_NUMERIC_STAR	0x20a
 #define KEY_NUMERIC_POUND	0x20b
 
+#define KEY_RFKILL_ALL		0x20c
+
 /* We avoid low common keys in module aliases so they don't get huge. */
 #define KEY_MIN_INTERESTING	KEY_MUTE
 #define KEY_MAX			0x2ff
-- 
1.6.2.5

[PATCH 2/2] rfkill: Add support for KEY_RFKILL_ALL

[Matthew Garrett]

Add support for handling KEY_RFKILL_ALL in the rfkill input module. This
simply toggles the state of all rfkill devices.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 net/rfkill/input.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/net/rfkill/input.c b/net/rfkill/input.c
index a7295ad..5517089 100644
--- a/net/rfkill/input.c
+++ b/net/rfkill/input.c
@@ -212,6 +212,12 @@ static void rfkill_event(struct input_handle *handle, unsigned int type,
 		case KEY_WIMAX:
 			rfkill_schedule_toggle(RFKILL_TYPE_WIMAX);
 			break;
+		case KEY_RFKILL_ALL:
+			rfkill_schedule_toggle(RFKILL_TYPE_WLAN);
+			rfkill_schedule_toggle(RFKILL_TYPE_BLUETOOTH);
+			rfkill_schedule_toggle(RFKILL_TYPE_UWB);
+			rfkill_schedule_toggle(RFKILL_TYPE_WIMAX);
+			break;
 		}
 	} else if (type == EV_SW && code == SW_RFKILL_ALL)
 		rfkill_schedule_evsw_rfkillall(data);
@@ -295,6 +301,11 @@ static const struct input_device_id rfkill_ids[] = {
 		.keybit = { [BIT_WORD(KEY_WIMAX)] = BIT_MASK(KEY_WIMAX) },
 	},
 	{
+		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_KEYBIT,
+		.evbit = { BIT_MASK(EV_KEY) },
+		.keybit = { [BIT_WORD(KEY_RFKILL_ALL)] = BIT_MASK(KEY_RFKILL_ALL) },
+	},
+	{
 		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_SWBIT,
 		.evbit = { BIT(EV_SW) },
 		.swbit = { [BIT_WORD(SW_RFKILL_ALL)] = BIT_MASK(SW_RFKILL_ALL) },
-- 
1.6.2.5

Re: [PATCH 2/2] rfkill: Add support for KEY_RFKILL_ALL

[Bastien Nocera]

On Sat, 2009-08-01 at 19:54 +0100, Matthew Garrett wrote:
> 
> +               case KEY_RFKILL_ALL:
> +                       rfkill_schedule_toggle(RFKILL_TYPE_WLAN);
> +                       rfkill_schedule_toggle(RFKILL_TYPE_BLUETOOTH);
> +                       rfkill_schedule_toggle(RFKILL_TYPE_UWB);
> +                       rfkill_schedule_toggle(RFKILL_TYPE_WIMAX);
> +                       break;

RFKILL_TYPE_ALL?

Re: [PATCH 1/2] input: Add KEY_RFKILL_ALL

[Marcel Holtmann]

Hi Matthew,

> Most laptops have keys that are intended to toggle all device state, not
> just wifi. These are currently generally mapped to KEY_WLAN. As a result,
> rfkill will only kill or enable wifi in response to the key press. This
> confuses users and can make it difficult for them to enable bluetooth
> and wwan devices.
> 
> This patch adds a new keycode, KEY_RFKILL_ALL. It indicates that the
> system should toggle the state of all rfkillable devices.

actually I would prefer if we name this key KEY_RFKILL and not put the
policy of ALL into the kernel. Since we wanna get rid of rfkill-input.
That needs to be done in userspace and by users policy. If they wanna
map that key to ALL then that is fine. If they just wanna toggle WiFi,
then that is also fine. If the wanna have to popup some UI, that is also
reasonable.

So just having a KEY_RFKILL is the better way here and policy free. You
can still for the short term have rfkill-input map this to an ALL policy
until we remove that piece and replace it with a proper userspace
solution (which thanks to /dev/rfkill is now possible).

Regards

Marcel

Re: [PATCH 2/2] rfkill: Add support for KEY_RFKILL_ALL

[Marcel Holtmann]

Hi Bastien,

> > +               case KEY_RFKILL_ALL:
> > +                       rfkill_schedule_toggle(RFKILL_TYPE_WLAN);
> > +                       rfkill_schedule_toggle(RFKILL_TYPE_BLUETOOTH);
> > +                       rfkill_schedule_toggle(RFKILL_TYPE_UWB);
> > +                       rfkill_schedule_toggle(RFKILL_TYPE_WIMAX);
> > +                       break;
> 
> RFKILL_TYPE_ALL?

this should be RFKILL_TYPE_ALL indeed. Even if rfkill-input is going to
be removed. For the time being it needs to toggle all. Especially since
the example above already forgets RFKILL_TYPE_WWAN.

Regards

Marcel

Re: [PATCH 1/2] input: Add KEY_RFKILL_ALL

[Matthew Garrett]

On Sat, Aug 01, 2009 at 01:38:53PM -0700, Marcel Holtmann wrote:

> actually I would prefer if we name this key KEY_RFKILL and not put the
> policy of ALL into the kernel. Since we wanna get rid of rfkill-input.
> That needs to be done in userspace and by users policy. If they wanna
> map that key to ALL then that is fine. If they just wanna toggle WiFi,
> then that is also fine. If the wanna have to popup some UI, that is also
> reasonable.

My reasoning was that this conceptually maps to a key that controls all 
of the radios in the system - whether a single press actually kills all 
of them or not is kind of irrelevent (having the simple policy in 
rfkill-input works fine for this, but userspace will obviously want 
something more cunning). My concern with KEY_RFKILL is that it it's not 
obvious that it refers to a specific type of key - ones that purely 
control wifi should still be KEY_WLAN, for instance.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/2] input: Add KEY_RFKILL_ALL

[Marcel Holtmann]

Hi Matthew,

> > actually I would prefer if we name this key KEY_RFKILL and not put the
> > policy of ALL into the kernel. Since we wanna get rid of rfkill-input.
> > That needs to be done in userspace and by users policy. If they wanna
> > map that key to ALL then that is fine. If they just wanna toggle WiFi,
> > then that is also fine. If the wanna have to popup some UI, that is also
> > reasonable.
> 
> My reasoning was that this conceptually maps to a key that controls all 
> of the radios in the system - whether a single press actually kills all 
> of them or not is kind of irrelevent (having the simple policy in 
> rfkill-input works fine for this, but userspace will obviously want 
> something more cunning). My concern with KEY_RFKILL is that it it's not 
> obvious that it refers to a specific type of key - ones that purely 
> control wifi should still be KEY_WLAN, for instance.

actually if the key is clearly hardwired to WLAN, then it should not
even show up as input event at all. This is one of the mis-concepts of
the old RFKILL subsystem. No need to send an input event if the platform
driver is going to rfkill that device anyway.

If a key is clearly labeled as WLAN then it should emit only KEY_WLAN.
And if it is just a generic key with some radio on it like FN-F5 on my
ThinkPad, then it better just emits KEY_RFKILL and we let userspace (or
rfkill-input do the policy).

Remember that in the end it is just a key and whatever the user does
with it is users policy. So in summary it is up to the platform driver
to emit the proper key. For some it might be still KEY_WLAN, for other
it might be KEY_RFKILL. Sounds fair?

Regards

Marcel

Re: [PATCH 1/2] input: Add KEY_RFKILL_ALL

[Matthew Garrett]

On Sat, Aug 01, 2009 at 01:52:22PM -0700, Marcel Holtmann wrote:

> actually if the key is clearly hardwired to WLAN, then it should not
> even show up as input event at all. This is one of the mis-concepts of
> the old RFKILL subsystem. No need to send an input event if the platform
> driver is going to rfkill that device anyway.

There's still a policy decision. Does it kill internal devices, or does 
it kill all attached wlan devices?

> Remember that in the end it is just a key and whatever the user does
> with it is users policy. So in summary it is up to the platform driver
> to emit the proper key. For some it might be still KEY_WLAN, for other
> it might be KEY_RFKILL. Sounds fair?

I agree on the technical side, but not the naming. KEY_WLAN is an 
rfkill-related key, so introducing KEY_RFKILL is potentially confusing. 
KEY_RFKILL_ALL isn't.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/2] input: Add KEY_RFKILL_ALL

[Marcel Holtmann]

Hi Matthew,

> > actually if the key is clearly hardwired to WLAN, then it should not
> > even show up as input event at all. This is one of the mis-concepts of
> > the old RFKILL subsystem. No need to send an input event if the platform
> > driver is going to rfkill that device anyway.
> 
> There's still a policy decision. Does it kill internal devices, or does 
> it kill all attached wlan devices?
> 
> > Remember that in the end it is just a key and whatever the user does
> > with it is users policy. So in summary it is up to the platform driver
> > to emit the proper key. For some it might be still KEY_WLAN, for other
> > it might be KEY_RFKILL. Sounds fair?
> 
> I agree on the technical side, but not the naming. KEY_WLAN is an 
> rfkill-related key, so introducing KEY_RFKILL is potentially confusing. 
> KEY_RFKILL_ALL isn't.

why is it confusing? I don't understanding your argument here. I think
the KEY_RFKILL_ALL is confusing is the user policy then only kills WiFi
devices or toggles between various on/off combinations.

Regards

Marcel

Re: [PATCH 1/2] input: Add KEY_RFKILL_ALL

[Matthew Garrett]

On Sat, Aug 01, 2009 at 02:48:47PM -0700, Marcel Holtmann wrote:

> why is it confusing? I don't understanding your argument here. I think
> the KEY_RFKILL_ALL is confusing is the user policy then only kills WiFi
> devices or toggles between various on/off combinations.

Having it be KEY_RFKILL implies that KEY_WLAN and similar aren't related 
to rfkill. KEY_RFKILL_ALL implies that the key invokes policy on all 
rfkill devices, which is the aim here.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/2] input: Add KEY_RFKILL_ALL

[Marcel Holtmann]

Hi Matthew,

> > why is it confusing? I don't understanding your argument here. I think
> > the KEY_RFKILL_ALL is confusing is the user policy then only kills WiFi
> > devices or toggles between various on/off combinations.
> 
> Having it be KEY_RFKILL implies that KEY_WLAN and similar aren't related 
> to rfkill. KEY_RFKILL_ALL implies that the key invokes policy on all 
> rfkill devices, which is the aim here.

and that is what it should NOT imply. The policy if it applies to all
internal devices, all devices in general or just WiFi or Bluetooth for
example is up to the user.

We are NOT going to have any kind of RFKILL policy in the kernel in the
future. We will remove rfkill-input once we have a proper userspace
solution (aka rfkilld or similar).

Regards

Marcel

Re: [PATCH 1/2] input: Add KEY_RFKILL_ALL

[Matthew Garrett]

On Sat, Aug 01, 2009 at 03:11:25PM -0700, Marcel Holtmann wrote:

> and that is what it should NOT imply. The policy if it applies to all
> internal devices, all devices in general or just WiFi or Bluetooth for
> example is up to the user.
> 
> We are NOT going to have any kind of RFKILL policy in the kernel in the
> future. We will remove rfkill-input once we have a proper userspace
> solution (aka rfkilld or similar).

We seem to be talking at cross purposes here. My objection to the name 
KEY_RFKILL is that it gives no indication what it's meant to be or why 
it's different to KEY_WLAN. KEY_RFKILL implies that it's the only input 
component that an rfkill policy agent needs to listen to, which isn't 
the case.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/2] input: Add KEY_RFKILL_ALL

[Henrique de Moraes Holschuh]

On Sat, 01 Aug 2009, Matthew Garrett wrote:
> This patch adds a new keycode, KEY_RFKILL_ALL. It indicates that the
> system should toggle the state of all rfkillable devices.

What should happen when you press it?  Toggle state, or the same as EV_SW
SW_RFKILL_ALL OFF (i.e. radios off, killing active) ?

-- 
  "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 1/2] input: Add KEY_RFKILL_ALL

[Matthew Garrett]

On Sat, Aug 01, 2009 at 10:05:57PM -0300, Henrique de Moraes Holschuh wrote:
> On Sat, 01 Aug 2009, Matthew Garrett wrote:
> > This patch adds a new keycode, KEY_RFKILL_ALL. It indicates that the
> > system should toggle the state of all rfkillable devices.
> 
> What should happen when you press it?  Toggle state, or the same as EV_SW
> SW_RFKILL_ALL OFF (i.e. radios off, killing active) ?

rfkill-input should toggle, userspace policy can be anything.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/2] input: Add KEY_RFKILL_ALL

[Johannes Berg]

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

On Sat, 2009-08-01 at 23:25 +0100, Matthew Garrett wrote:
> On Sat, Aug 01, 2009 at 03:11:25PM -0700, Marcel Holtmann wrote:
> 
> > and that is what it should NOT imply. The policy if it applies to all
> > internal devices, all devices in general or just WiFi or Bluetooth for
> > example is up to the user.
> > 
> > We are NOT going to have any kind of RFKILL policy in the kernel in the
> > future. We will remove rfkill-input once we have a proper userspace
> > solution (aka rfkilld or similar).
> 
> We seem to be talking at cross purposes here. My objection to the name 
> KEY_RFKILL is that it gives no indication what it's meant to be or why 
> it's different to KEY_WLAN. KEY_RFKILL implies that it's the only input 
> component that an rfkill policy agent needs to listen to, which isn't 
> the case.

Heh, yeah, I can kinda agree with both of you.

We really should've named KEY_WLAN KEY_RFKILL_WLAN instead, etc, and
then both versions would be less ambiguous.

AFAIU, Marcel's argument is that policy might not be "ALL", but cycling
through technology combinations, and your (Matthew's) argument is that
just naming it without the _ALL would imply that KEY_WLAN could be
related to something other than rfkill.

Can we name it KEY_RFKILL, and add a comment to the header file about
what it is? I.e. a "technology-independent RF toggle button" or so?

johannes

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