[PATCHv2 00/10] RFKill airplane-mode indicator

[PATCHv2 00/10] RFKill airplane-mode indicator

[João Paulo Rechi Vita]

This series implements an airplane-mode indicator LED trigger, which can be
used by platform drivers. The default policy have have airplane-mode set when
all the radios known by RFKill are OFF, and unset otherwise. This policy can be
overwritten by one single userspace application at a time using the operations
_AIRPLANE_MODE_INDICATOR_ACQUIRE and _AIRPLANE_MODE_INDICATOR_CHANGE. When the
airplane-mode indicator state changes, userspace gets notifications through the
RFKill control misc device (/dev/rfkill).

The series also contains a few general fixes and improvements to the subsystem.

Additionally, I have a couple of patches to have this feature supported by the
userspace tool 'rfkill' [1]. Should I use a different subject prefix to help
separate those from kernel patches in linux-wireless?

[1] https://wireless.wiki.kernel.org/en/users/documentation/rfkill

João Paulo Rechi Vita (10):
  rfkill: Improve documentation language
  rfkill: Remove extra blank line
  rfkill: Point to the correct deprecated doc location
  rfkill: Move "state" sysfs file back to stable
  rfkill: Factor rfkill_global_states[].cur assignments
  rfkill: Add documentation about LED triggers
  rfkill: Create "rfkill-airplane-mode" LED trigger
  rfkill: Use switch to demux userspace operations
  rfkill: Userspace control for airplane mode
  rfkill: Notify userspace of airplane-mode state changes

 Documentation/ABI/obsolete/sysfs-class-rfkill |  20 ----
 Documentation/ABI/stable/sysfs-class-rfkill   |  27 ++++-
 Documentation/rfkill.txt                      |  17 +++
 include/uapi/linux/rfkill.h                   |   6 +
 net/rfkill/core.c                             | 162 ++++++++++++++++++++------
 5 files changed, 173 insertions(+), 59 deletions(-)
 delete mode 100644 Documentation/ABI/obsolete/sysfs-class-rfkill

-- 
2.5.0

[PATCHv2 01/10] rfkill: Improve documentation language

[João Paulo Rechi Vita]

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 net/rfkill/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index a805831..ffbc375 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -282,8 +282,8 @@ static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
 	spin_lock_irqsave(&rfkill->lock, flags);
 	if (err) {
 		/*
-		 * Failed -- reset status to _prev, this may be different
-		 * from what set set _PREV to earlier in this function
+		 * Failed -- reset status to _PREV, which may be different
+		 * from what we have set _PREV to earlier in this function
 		 * if rfkill_set_sw_state was invoked.
 		 */
 		if (rfkill->state & RFKILL_BLOCK_SW_PREV)
-- 
2.5.0

[PATCHv2 02/10] rfkill: Remove extra blank line

[João Paulo Rechi Vita]

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 net/rfkill/core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index ffbc375..56d79cb 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -455,7 +455,6 @@ bool rfkill_get_global_sw_state(const enum rfkill_type type)
 }
 #endif
 
-
 bool rfkill_set_hw_state(struct rfkill *rfkill, bool blocked)
 {
 	unsigned long flags;
-- 
2.5.0

[PATCHv2 03/10] rfkill: Point to the correct deprecated doc location

[João Paulo Rechi Vita]

The "claim" sysfs interface has been removed, so its documentation now
lives in the "removed" folder.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 Documentation/ABI/stable/sysfs-class-rfkill | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-class-rfkill b/Documentation/ABI/stable/sysfs-class-rfkill
index 097f522..e51571e 100644
--- a/Documentation/ABI/stable/sysfs-class-rfkill
+++ b/Documentation/ABI/stable/sysfs-class-rfkill
@@ -2,8 +2,10 @@ rfkill - radio frequency (RF) connector kill switch support
 
 For details to this subsystem look at Documentation/rfkill.txt.
 
-For the deprecated /sys/class/rfkill/*/state and
-/sys/class/rfkill/*/claim knobs of this interface look in
+For the deprecated /sys/class/rfkill/*/claim knobs of this interface look in
+Documentation/ABI/removed/sysfs-class-rfkill.
+
+For the deprecated /sys/class/rfkill/*/state knobs of this interface look in
 Documentation/ABI/obsolete/sysfs-class-rfkill.
 
 What: 		/sys/class/rfkill
-- 
2.5.0

[PATCHv2 04/10] rfkill: Move "state" sysfs file back to stable

[João Paulo Rechi Vita]

There is still quite a bit of code using this interface, so we can't
just remove it. Hopefully it will be possible in the future, but since
its scheduled removal date is past 2 years already, we are better having
the documentation reflecting the current state of things.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 Documentation/ABI/obsolete/sysfs-class-rfkill | 20 --------------------
 Documentation/ABI/stable/sysfs-class-rfkill   | 25 ++++++++++++++++++++++---
 2 files changed, 22 insertions(+), 23 deletions(-)
 delete mode 100644 Documentation/ABI/obsolete/sysfs-class-rfkill

diff --git a/Documentation/ABI/obsolete/sysfs-class-rfkill b/Documentation/ABI/obsolete/sysfs-class-rfkill
deleted file mode 100644
index e736d14..0000000
--- a/Documentation/ABI/obsolete/sysfs-class-rfkill
+++ /dev/null
@@ -1,20 +0,0 @@
-rfkill - radio frequency (RF) connector kill switch support
-
-For details to this subsystem look at Documentation/rfkill.txt.
-
-What:		/sys/class/rfkill/rfkill[0-9]+/state
-Date:		09-Jul-2007
-KernelVersion	v2.6.22
-Contact:	linux-wireless@vger.kernel.org
-Description: 	Current state of the transmitter.
-		This file is deprecated and scheduled to be removed in 2014,
-		because its not possible to express the 'soft and hard block'
-		state of the rfkill driver.
-Values: 	A numeric value.
-		0: RFKILL_STATE_SOFT_BLOCKED
-			transmitter is turned off by software
-		1: RFKILL_STATE_UNBLOCKED
-			transmitter is (potentially) active
-		2: RFKILL_STATE_HARD_BLOCKED
-			transmitter is forced off by something outside of
-			the driver's control.
diff --git a/Documentation/ABI/stable/sysfs-class-rfkill b/Documentation/ABI/stable/sysfs-class-rfkill
index e51571e..e1ba4a1 100644
--- a/Documentation/ABI/stable/sysfs-class-rfkill
+++ b/Documentation/ABI/stable/sysfs-class-rfkill
@@ -5,9 +5,6 @@ For details to this subsystem look at Documentation/rfkill.txt.
 For the deprecated /sys/class/rfkill/*/claim knobs of this interface look in
 Documentation/ABI/removed/sysfs-class-rfkill.
 
-For the deprecated /sys/class/rfkill/*/state knobs of this interface look in
-Documentation/ABI/obsolete/sysfs-class-rfkill.
-
 What: 		/sys/class/rfkill
 Date:		09-Jul-2007
 KernelVersion:	v2.6.22
@@ -44,6 +41,28 @@ Values: 	A numeric value.
 		1: true
 
 
+What:		/sys/class/rfkill/rfkill[0-9]+/state
+Date:		09-Jul-2007
+KernelVersion	v2.6.22
+Contact:	linux-wireless@vger.kernel.org
+Description: 	Current state of the transmitter.
+		This file was scheduled to be removed in 2014, but due to its
+		large number of users it will be sticking around for a bit
+		longer. Despite it being marked as stabe, the newer "hard" and
+		"soft" interfaces should be preffered, since it is not possible
+		to express the 'soft and hard block' state of the rfkill driver
+		through this interface. There will likely be another attempt to
+		remove it in the future.
+Values: 	A numeric value.
+		0: RFKILL_STATE_SOFT_BLOCKED
+			transmitter is turned off by software
+		1: RFKILL_STATE_UNBLOCKED
+			transmitter is (potentially) active
+		2: RFKILL_STATE_HARD_BLOCKED
+			transmitter is forced off by something outside of
+			the driver's control.
+
+
 What:		/sys/class/rfkill/rfkill[0-9]+/hard
 Date:		12-March-2010
 KernelVersion	v2.6.34
-- 
2.5.0

[PATCHv2 05/10] rfkill: Factor rfkill_global_states[].cur assignments

[João Paulo Rechi Vita]

Factor all assignments to rfkill_global_states[].cur into a single
function rfkill_update_global_state().

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 net/rfkill/core.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 56d79cb..8b96869 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -302,6 +302,19 @@ static void rfkill_set_block(struct rfkill *rfkill, bool blocked)
 		rfkill_event(rfkill);
 }
 
+static void rfkill_update_global_state(enum rfkill_type type, bool blocked)
+{
+	int i;
+
+	if (type != RFKILL_TYPE_ALL) {
+		rfkill_global_states[type].cur = blocked;
+		return;
+	}
+
+	for (i = 0; i < NUM_RFKILL_TYPES; i++)
+		rfkill_global_states[i].cur = blocked;
+}
+
 #ifdef CONFIG_RFKILL_INPUT
 static atomic_t rfkill_input_disabled = ATOMIC_INIT(0);
 
@@ -319,15 +332,7 @@ static void __rfkill_switch_all(const enum rfkill_type type, bool blocked)
 {
 	struct rfkill *rfkill;
 
-	if (type == RFKILL_TYPE_ALL) {
-		int i;
-
-		for (i = 0; i < NUM_RFKILL_TYPES; i++)
-			rfkill_global_states[i].cur = blocked;
-	} else {
-		rfkill_global_states[type].cur = blocked;
-	}
-
+	rfkill_update_global_state(type, blocked);
 	list_for_each_entry(rfkill, &rfkill_list, node) {
 		if (rfkill->type != type && type != RFKILL_TYPE_ALL)
 			continue;
@@ -1164,15 +1169,8 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
 
 	mutex_lock(&rfkill_global_mutex);
 
-	if (ev.op == RFKILL_OP_CHANGE_ALL) {
-		if (ev.type == RFKILL_TYPE_ALL) {
-			enum rfkill_type i;
-			for (i = 0; i < NUM_RFKILL_TYPES; i++)
-				rfkill_global_states[i].cur = ev.soft;
-		} else {
-			rfkill_global_states[ev.type].cur = ev.soft;
-		}
-	}
+	if (ev.op == RFKILL_OP_CHANGE_ALL)
+		rfkill_update_global_state(ev.type, ev.soft);
 
 	list_for_each_entry(rfkill, &rfkill_list, node) {
 		if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
@@ -1261,10 +1259,8 @@ static struct miscdevice rfkill_miscdev = {
 static int __init rfkill_init(void)
 {
 	int error;
-	int i;
 
-	for (i = 0; i < NUM_RFKILL_TYPES; i++)
-		rfkill_global_states[i].cur = !rfkill_default_state;
+	rfkill_update_global_state(RFKILL_TYPE_ALL, !rfkill_default_state);
 
 	error = class_register(&rfkill_class);
 	if (error)
-- 
2.5.0

[PATCHv2 06/10] rfkill: Add documentation about LED triggers

[João Paulo Rechi Vita]

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 Documentation/rfkill.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
index 2ee6ef9..1f0c270 100644
--- a/Documentation/rfkill.txt
+++ b/Documentation/rfkill.txt
@@ -83,6 +83,8 @@ rfkill drivers that control devices that can be hard-blocked unless they also
 assign the poll_hw_block() callback (then the rfkill core will poll the
 device). Don't do this unless you cannot get the event in any other way.
 
+RFKill provides per-switch LED triggers, which can be used to drive LEDs
+according to the switch state (LED_FULL when blocked, LED_OFF otherwise).
 
 
 5. Userspace support
-- 
2.5.0

[PATCHv2 07/10] rfkill: Create "rfkill-airplane-mode" LED trigger

[João Paulo Rechi Vita]

This creates a new LED trigger to be used by platform drivers as a
default trigger for airplane-mode indicator LEDs.

By default this trigger will fire when RFKILL_OP_CHANGE_ALL is called
for all types (RFKILL_TYPE_ALL), setting the LED brightness to LED_FULL
when the changing the state to blocked, and to LED_OFF when the changing
the state to unblocked. In the future there will be a mechanism for
userspace to override the default policy, so it can implement its own.

This trigger will be used by the asus-wireless x86 platform driver.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 Documentation/rfkill.txt |  2 ++
 net/rfkill/core.c        | 49 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
index 1f0c270..b13025a 100644
--- a/Documentation/rfkill.txt
+++ b/Documentation/rfkill.txt
@@ -85,6 +85,8 @@ device). Don't do this unless you cannot get the event in any other way.
 
 RFKill provides per-switch LED triggers, which can be used to drive LEDs
 according to the switch state (LED_FULL when blocked, LED_OFF otherwise).
+An airplane-mode indicator LED trigger is also available, which triggers
+LED_FULL when all radios known by RFKill are blocked, and LED_OFF otherwise.
 
 
 5. Userspace support
diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 8b96869..50b538b 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -126,6 +126,30 @@ static bool rfkill_epo_lock_active;
 
 
 #ifdef CONFIG_RFKILL_LEDS
+static struct led_trigger rfkill_apm_led_trigger;
+
+static void rfkill_apm_led_trigger_event(bool state)
+{
+	led_trigger_event(&rfkill_apm_led_trigger, state ? LED_FULL : LED_OFF);
+}
+
+static void rfkill_apm_led_trigger_activate(struct led_classdev *led)
+{
+	rfkill_apm_led_trigger_event(!rfkill_default_state);
+}
+
+static int rfkill_apm_led_trigger_register(void)
+{
+	rfkill_apm_led_trigger.name = "rfkill-airplane-mode";
+	rfkill_apm_led_trigger.activate = rfkill_apm_led_trigger_activate;
+	return led_trigger_register(&rfkill_apm_led_trigger);
+}
+
+static void rfkill_apm_led_trigger_unregister(void)
+{
+	led_trigger_unregister(&rfkill_apm_led_trigger);
+}
+
 static void rfkill_led_trigger_event(struct rfkill *rfkill)
 {
 	struct led_trigger *trigger;
@@ -177,6 +201,19 @@ static void rfkill_led_trigger_unregister(struct rfkill *rfkill)
 	led_trigger_unregister(&rfkill->led_trigger);
 }
 #else
+static void rfkill_apm_led_trigger_event(bool state)
+{
+}
+
+static int rfkill_apm_led_trigger_register(void)
+{
+	return 0;
+}
+
+static void rfkill_apm_led_trigger_unregister(void)
+{
+}
+
 static void rfkill_led_trigger_event(struct rfkill *rfkill)
 {
 }
@@ -313,6 +350,7 @@ static void rfkill_update_global_state(enum rfkill_type type, bool blocked)
 
 	for (i = 0; i < NUM_RFKILL_TYPES; i++)
 		rfkill_global_states[i].cur = blocked;
+	rfkill_apm_led_trigger_event(blocked);
 }
 
 #ifdef CONFIG_RFKILL_INPUT
@@ -1260,15 +1298,22 @@ static int __init rfkill_init(void)
 {
 	int error;
 
+	error = rfkill_apm_led_trigger_register();
+	if (error)
+		goto out;
+
 	rfkill_update_global_state(RFKILL_TYPE_ALL, !rfkill_default_state);
 
 	error = class_register(&rfkill_class);
-	if (error)
+	if (error) {
+		rfkill_apm_led_trigger_unregister();
 		goto out;
+	}
 
 	error = misc_register(&rfkill_miscdev);
 	if (error) {
 		class_unregister(&rfkill_class);
+		rfkill_apm_led_trigger_unregister();
 		goto out;
 	}
 
@@ -1277,6 +1322,7 @@ static int __init rfkill_init(void)
 	if (error) {
 		misc_deregister(&rfkill_miscdev);
 		class_unregister(&rfkill_class);
+		rfkill_apm_led_trigger_unregister();
 		goto out;
 	}
 #endif
@@ -1293,5 +1339,6 @@ static void __exit rfkill_exit(void)
 #endif
 	misc_deregister(&rfkill_miscdev);
 	class_unregister(&rfkill_class);
+	rfkill_apm_led_trigger_unregister();
 }
 module_exit(rfkill_exit);
-- 
2.5.0

[PATCHv2 08/10] rfkill: Use switch to demux userspace operations

[João Paulo Rechi Vita]

Using a switch to handle different ev.op values in rfkill_fop_write()
makes the code easier to extend, as out-of-range values can always be
handled by the default case.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 net/rfkill/core.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 50b538b..04d7fa1 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -1185,6 +1185,7 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
 {
 	struct rfkill *rfkill;
 	struct rfkill_event ev;
+	int ret = 0;
 
 	/* we don't need the 'hard' variable but accept it */
 	if (count < RFKILL_EVENT_SIZE_V1 - 1)
@@ -1199,29 +1200,32 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
 	if (copy_from_user(&ev, buf, count))
 		return -EFAULT;
 
-	if (ev.op != RFKILL_OP_CHANGE && ev.op != RFKILL_OP_CHANGE_ALL)
-		return -EINVAL;
-
 	if (ev.type >= NUM_RFKILL_TYPES)
 		return -EINVAL;
 
 	mutex_lock(&rfkill_global_mutex);
 
-	if (ev.op == RFKILL_OP_CHANGE_ALL)
+	switch (ev.op) {
+	case RFKILL_OP_CHANGE_ALL:
 		rfkill_update_global_state(ev.type, ev.soft);
-
-	list_for_each_entry(rfkill, &rfkill_list, node) {
-		if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
-			continue;
-
-		if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL)
-			continue;
-
-		rfkill_set_block(rfkill, ev.soft);
+		list_for_each_entry(rfkill, &rfkill_list, node)
+			if (rfkill->type == ev.type ||
+			    ev.type == RFKILL_TYPE_ALL)
+				rfkill_set_block(rfkill, ev.soft);
+		break;
+	case RFKILL_OP_CHANGE:
+		list_for_each_entry(rfkill, &rfkill_list, node)
+			if (rfkill->idx == ev.idx && rfkill->type == ev.type)
+				rfkill_set_block(rfkill, ev.soft);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
 	}
+
 	mutex_unlock(&rfkill_global_mutex);
 
-	return count;
+	return ret ? ret : count;
 }
 
 static int rfkill_fop_release(struct inode *inode, struct file *file)
-- 
2.5.0

[PATCHv2 09/10] rfkill: Userspace control for airplane mode

[João Paulo Rechi Vita]

Provide an interface for the airplane-mode indicator be controlled from
userspace. User has to first acquire the control through
RFKILL_OP_AIRPLANE_MODE_ACQUIRE and keep the fd open for the whole time
it wants to be in control of the indicator. Closing the fd or using
RFKILL_OP_AIRPLANE_MODE_RELEASE restores the default policy.

To change state of the indicator, the RFKILL_OP_AIRPLANE_MODE_CHANGE
operation is used, passing the value on "struct rfkill_event.soft". If
the caller has not acquired the airplane-mode control beforehand, the
operation fails.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 Documentation/rfkill.txt    | 10 ++++++++++
 include/uapi/linux/rfkill.h |  6 ++++++
 net/rfkill/core.c           | 35 +++++++++++++++++++++++++++++++++--
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
index b13025a..9dbe3fc 100644
--- a/Documentation/rfkill.txt
+++ b/Documentation/rfkill.txt
@@ -87,6 +87,7 @@ RFKill provides per-switch LED triggers, which can be used to drive LEDs
 according to the switch state (LED_FULL when blocked, LED_OFF otherwise).
 An airplane-mode indicator LED trigger is also available, which triggers
 LED_FULL when all radios known by RFKill are blocked, and LED_OFF otherwise.
+The airplane-mode indicator LED trigger policy can be overridden by userspace.
 
 
 5. Userspace support
@@ -123,5 +124,14 @@ RFKILL_TYPE
 The contents of these variables corresponds to the "name", "state" and
 "type" sysfs files explained above.
 
+Userspace can also override the default airplane-mode indicator policy through
+/dev/rfkill. Control of the airplane mode indicator has to be acquired first,
+using RFKILL_OP_AIRPLANE_MODE_INDICATOR_ACQUIRE, and is only available for one
+userspace application at a time. Closing the fd reverts the airplane-mode
+indicator back to the default kernel policy and makes it available for other
+applications to take control. Changes to the airplane-mode indicator state can
+be made using RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE, passing the new value
+in the 'soft' field of 'struct rfkill_event'.
+
 
 For further details consult Documentation/ABI/stable/sysfs-class-rfkill.
diff --git a/include/uapi/linux/rfkill.h b/include/uapi/linux/rfkill.h
index 2e00dce..36e0770 100644
--- a/include/uapi/linux/rfkill.h
+++ b/include/uapi/linux/rfkill.h
@@ -61,12 +61,18 @@ enum rfkill_type {
  * @RFKILL_OP_CHANGE_ALL: userspace changes all devices (of a type, or all)
  *	into a state, also updating the default state used for devices that
  *	are hot-plugged later.
+ * @RFKILL_OP_AIRPLANE_MODE_INDICATOR_ACQUIRE: userspace acquires control of
+ * 	the airplane-mode indicator.
+ * @RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE: userspace changes the
+ * 	airplane-mode indicator state.
  */
 enum rfkill_operation {
 	RFKILL_OP_ADD = 0,
 	RFKILL_OP_DEL,
 	RFKILL_OP_CHANGE,
 	RFKILL_OP_CHANGE_ALL,
+	RFKILL_OP_AIRPLANE_MODE_INDICATOR_ACQUIRE,
+	RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE,
 };
 
 /**
diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 04d7fa1..8ea8b73 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -89,6 +89,7 @@ struct rfkill_data {
 	struct mutex		mtx;
 	wait_queue_head_t	read_wait;
 	bool			input_handler;
+	bool			is_apm_owner;
 };
 
 
@@ -123,7 +124,7 @@ static struct {
 } rfkill_global_states[NUM_RFKILL_TYPES];
 
 static bool rfkill_epo_lock_active;
-
+static bool rfkill_apm_owned;
 
 #ifdef CONFIG_RFKILL_LEDS
 static struct led_trigger rfkill_apm_led_trigger;
@@ -350,7 +351,8 @@ static void rfkill_update_global_state(enum rfkill_type type, bool blocked)
 
 	for (i = 0; i < NUM_RFKILL_TYPES; i++)
 		rfkill_global_states[i].cur = blocked;
-	rfkill_apm_led_trigger_event(blocked);
+	if (!rfkill_apm_owned)
+		rfkill_apm_led_trigger_event(blocked);
 }
 
 #ifdef CONFIG_RFKILL_INPUT
@@ -1180,9 +1182,23 @@ static ssize_t rfkill_fop_read(struct file *file, char __user *buf,
 	return ret;
 }
 
+static int rfkill_airplane_mode_release(struct rfkill_data *data)
+{
+	bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur;
+
+	if (rfkill_apm_owned && data->is_apm_owner) {
+		rfkill_apm_owned = false;
+		data->is_apm_owner = false;
+		rfkill_apm_led_trigger_event(state);
+		return 0;
+	}
+	return -EACCES;
+}
+
 static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
 				size_t count, loff_t *pos)
 {
+	struct rfkill_data *data = file->private_data;
 	struct rfkill *rfkill;
 	struct rfkill_event ev;
 	int ret = 0;
@@ -1218,6 +1234,20 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
 			if (rfkill->idx == ev.idx && rfkill->type == ev.type)
 				rfkill_set_block(rfkill, ev.soft);
 		break;
+	case RFKILL_OP_AIRPLANE_MODE_INDICATOR_ACQUIRE:
+		if (rfkill_apm_owned && !data->is_apm_owner) {
+			ret = -EACCES;
+		} else {
+			rfkill_apm_owned = true;
+			data->is_apm_owner = true;
+		}
+		break;
+	case RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE:
+		if (rfkill_apm_owned && data->is_apm_owner)
+			rfkill_apm_led_trigger_event(ev.soft);
+		else
+			ret = -EACCES;
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -1234,6 +1264,7 @@ static int rfkill_fop_release(struct inode *inode, struct file *file)
 	struct rfkill_int_event *ev, *tmp;
 
 	mutex_lock(&rfkill_global_mutex);
+	rfkill_airplane_mode_release(data);
 	list_del(&data->list);
 	mutex_unlock(&rfkill_global_mutex);
 
-- 
2.5.0

[PATCHv2 10/10] rfkill: Notify userspace of airplane-mode state changes

[João Paulo Rechi Vita]

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 Documentation/rfkill.txt    |  3 +++
 include/uapi/linux/rfkill.h |  4 ++--
 net/rfkill/core.c           | 13 +++++++++++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
index 9dbe3fc..588b4bf 100644
--- a/Documentation/rfkill.txt
+++ b/Documentation/rfkill.txt
@@ -133,5 +133,8 @@ applications to take control. Changes to the airplane-mode indicator state can
 be made using RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE, passing the new value
 in the 'soft' field of 'struct rfkill_event'.
 
+This same API is also used to provide userspace with notifications of changes
+to airplane-mode indicator state.
+
 
 For further details consult Documentation/ABI/stable/sysfs-class-rfkill.
diff --git a/include/uapi/linux/rfkill.h b/include/uapi/linux/rfkill.h
index 36e0770..2ccb02f 100644
--- a/include/uapi/linux/rfkill.h
+++ b/include/uapi/linux/rfkill.h
@@ -63,8 +63,8 @@ enum rfkill_type {
  *	are hot-plugged later.
  * @RFKILL_OP_AIRPLANE_MODE_INDICATOR_ACQUIRE: userspace acquires control of
  * 	the airplane-mode indicator.
- * @RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE: userspace changes the
- * 	airplane-mode indicator state.
+ * @RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE: the airplane-mode indicator state
+ * 	changed -- userspace changes the airplane-mode indicator state.
  */
 enum rfkill_operation {
 	RFKILL_OP_ADD = 0,
diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 8ea8b73..c59fd1d 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -131,7 +131,20 @@ static struct led_trigger rfkill_apm_led_trigger;
 
 static void rfkill_apm_led_trigger_event(bool state)
 {
+	struct rfkill_data *data;
+	struct rfkill_int_event *ev;
+
 	led_trigger_event(&rfkill_apm_led_trigger, state ? LED_FULL : LED_OFF);
+
+	list_for_each_entry(data, &rfkill_fds, list) {
+		ev = kzalloc(sizeof(*ev), GFP_KERNEL);
+		if (!ev)
+			continue;
+		ev->ev.op = RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE;
+		ev->ev.soft = state;
+		list_add_tail(&ev->list, &data->events);
+		wake_up_interruptible(&data->read_wait);
+	}
 }
 
 static void rfkill_apm_led_trigger_activate(struct led_classdev *led)
-- 
2.5.0

Re: [PATCHv2 00/10] RFKill airplane-mode indicator

[Dan Williams]

On Mon, 2016-02-22 at 11:36 -0500, João Paulo Rechi Vita wrote:
> This series implements an airplane-mode indicator LED trigger, which
> can be
> used by platform drivers. The default policy have have airplane-mode
> set when
> all the radios known by RFKill are OFF, and unset otherwise. This
> policy can be
> overwritten by one single userspace application at a time using the
> operations
> _AIRPLANE_MODE_INDICATOR_ACQUIRE and _AIRPLANE_MODE_INDICATOR_CHANGE.
> 
Double-check your commit messages on some of these patches; they didn't
get updated to add INDICATOR.

Dan

> When the
> airplane-mode indicator state changes, userspace gets notifications
> through the
> RFKill control misc device (/dev/rfkill).
> 
> The series also contains a few general fixes and improvements to the
> subsystem.
> 
> Additionally, I have a couple of patches to have this feature
> supported by the
> userspace tool 'rfkill' [1]. Should I use a different subject prefix
> to help
> separate those from kernel patches in linux-wireless?
> 
> [1] https://wireless.wiki.kernel.org/en/users/documentation/rfkill
> 
> João Paulo Rechi Vita (10):
>   rfkill: Improve documentation language
>   rfkill: Remove extra blank line
>   rfkill: Point to the correct deprecated doc location
>   rfkill: Move "state" sysfs file back to stable
>   rfkill: Factor rfkill_global_states[].cur assignments
>   rfkill: Add documentation about LED triggers
>   rfkill: Create "rfkill-airplane-mode" LED trigger
>   rfkill: Use switch to demux userspace operations
>   rfkill: Userspace control for airplane mode
>   rfkill: Notify userspace of airplane-mode state changes
> 
>  Documentation/ABI/obsolete/sysfs-class-rfkill |  20 ----
>  Documentation/ABI/stable/sysfs-class-rfkill   |  27 ++++-
>  Documentation/rfkill.txt                      |  17 +++
>  include/uapi/linux/rfkill.h                   |   6 +
>  net/rfkill/core.c                             | 162
> ++++++++++++++++++++------
>  5 files changed, 173 insertions(+), 59 deletions(-)
>  delete mode 100644 Documentation/ABI/obsolete/sysfs-class-rfkill
> 

[PATCHv3] rfkill: Userspace control for airplane mode

[João Paulo Rechi Vita]

Provide an interface for the airplane-mode indicator be controlled from
userspace. User has to first acquire the control through
RFKILL_OP_AIRPLANE_MODE_INDICATOR_ACQUIRE and keep the fd open for the
whole time it wants to be in control of the indicator. Closing the fd
restores the default policy.

To change state of the indicator, the
RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE operation is used, passing the
value on "struct rfkill_event.soft". If the caller has not acquired the
airplane-mode control beforehand, the operation fails.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 Documentation/rfkill.txt    | 10 ++++++++++
 include/uapi/linux/rfkill.h |  6 ++++++
 net/rfkill/core.c           | 35 +++++++++++++++++++++++++++++++++--
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt
index b13025a..9dbe3fc 100644
--- a/Documentation/rfkill.txt
+++ b/Documentation/rfkill.txt
@@ -87,6 +87,7 @@ RFKill provides per-switch LED triggers, which can be used to drive LEDs
 according to the switch state (LED_FULL when blocked, LED_OFF otherwise).
 An airplane-mode indicator LED trigger is also available, which triggers
 LED_FULL when all radios known by RFKill are blocked, and LED_OFF otherwise.
+The airplane-mode indicator LED trigger policy can be overridden by userspace.
 
 
 5. Userspace support
@@ -123,5 +124,14 @@ RFKILL_TYPE
 The contents of these variables corresponds to the "name", "state" and
 "type" sysfs files explained above.
 
+Userspace can also override the default airplane-mode indicator policy through
+/dev/rfkill. Control of the airplane mode indicator has to be acquired first,
+using RFKILL_OP_AIRPLANE_MODE_INDICATOR_ACQUIRE, and is only available for one
+userspace application at a time. Closing the fd reverts the airplane-mode
+indicator back to the default kernel policy and makes it available for other
+applications to take control. Changes to the airplane-mode indicator state can
+be made using RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE, passing the new value
+in the 'soft' field of 'struct rfkill_event'.
+
 
 For further details consult Documentation/ABI/stable/sysfs-class-rfkill.
diff --git a/include/uapi/linux/rfkill.h b/include/uapi/linux/rfkill.h
index 2e00dce..36e0770 100644
--- a/include/uapi/linux/rfkill.h
+++ b/include/uapi/linux/rfkill.h
@@ -61,12 +61,18 @@ enum rfkill_type {
  * @RFKILL_OP_CHANGE_ALL: userspace changes all devices (of a type, or all)
  *	into a state, also updating the default state used for devices that
  *	are hot-plugged later.
+ * @RFKILL_OP_AIRPLANE_MODE_INDICATOR_ACQUIRE: userspace acquires control of
+ * 	the airplane-mode indicator.
+ * @RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE: userspace changes the
+ * 	airplane-mode indicator state.
  */
 enum rfkill_operation {
 	RFKILL_OP_ADD = 0,
 	RFKILL_OP_DEL,
 	RFKILL_OP_CHANGE,
 	RFKILL_OP_CHANGE_ALL,
+	RFKILL_OP_AIRPLANE_MODE_INDICATOR_ACQUIRE,
+	RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE,
 };
 
 /**
diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 04d7fa1..8ea8b73 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -89,6 +89,7 @@ struct rfkill_data {
 	struct mutex		mtx;
 	wait_queue_head_t	read_wait;
 	bool			input_handler;
+	bool			is_apm_owner;
 };
 
 
@@ -123,7 +124,7 @@ static struct {
 } rfkill_global_states[NUM_RFKILL_TYPES];
 
 static bool rfkill_epo_lock_active;
-
+static bool rfkill_apm_owned;
 
 #ifdef CONFIG_RFKILL_LEDS
 static struct led_trigger rfkill_apm_led_trigger;
@@ -350,7 +351,8 @@ static void rfkill_update_global_state(enum rfkill_type type, bool blocked)
 
 	for (i = 0; i < NUM_RFKILL_TYPES; i++)
 		rfkill_global_states[i].cur = blocked;
-	rfkill_apm_led_trigger_event(blocked);
+	if (!rfkill_apm_owned)
+		rfkill_apm_led_trigger_event(blocked);
 }
 
 #ifdef CONFIG_RFKILL_INPUT
@@ -1180,9 +1182,23 @@ static ssize_t rfkill_fop_read(struct file *file, char __user *buf,
 	return ret;
 }
 
+static int rfkill_airplane_mode_release(struct rfkill_data *data)
+{
+	bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur;
+
+	if (rfkill_apm_owned && data->is_apm_owner) {
+		rfkill_apm_owned = false;
+		data->is_apm_owner = false;
+		rfkill_apm_led_trigger_event(state);
+		return 0;
+	}
+	return -EACCES;
+}
+
 static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
 				size_t count, loff_t *pos)
 {
+	struct rfkill_data *data = file->private_data;
 	struct rfkill *rfkill;
 	struct rfkill_event ev;
 	int ret = 0;
@@ -1218,6 +1234,20 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
 			if (rfkill->idx == ev.idx && rfkill->type == ev.type)
 				rfkill_set_block(rfkill, ev.soft);
 		break;
+	case RFKILL_OP_AIRPLANE_MODE_INDICATOR_ACQUIRE:
+		if (rfkill_apm_owned && !data->is_apm_owner) {
+			ret = -EACCES;
+		} else {
+			rfkill_apm_owned = true;
+			data->is_apm_owner = true;
+		}
+		break;
+	case RFKILL_OP_AIRPLANE_MODE_INDICATOR_CHANGE:
+		if (rfkill_apm_owned && data->is_apm_owner)
+			rfkill_apm_led_trigger_event(ev.soft);
+		else
+			ret = -EACCES;
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -1234,6 +1264,7 @@ static int rfkill_fop_release(struct inode *inode, struct file *file)
 	struct rfkill_int_event *ev, *tmp;
 
 	mutex_lock(&rfkill_global_mutex);
+	rfkill_airplane_mode_release(data);
 	list_del(&data->list);
 	mutex_unlock(&rfkill_global_mutex);
 
-- 
2.5.0

Re: [PATCHv2 00/10] RFKill airplane-mode indicator

[João Paulo Rechi Vita]

On 22 February 2016 at 12:00, Dan Williams <dcbw@redhat.com> wrote:
> On Mon, 2016-02-22 at 11:36 -0500, João Paulo Rechi Vita wrote:
>> This series implements an airplane-mode indicator LED trigger, which
>> can be
>> used by platform drivers. The default policy have have airplane-mode
>> set when
>> all the radios known by RFKill are OFF, and unset otherwise. This
>> policy can be
>> overwritten by one single userspace application at a time using the
>> operations
>> _AIRPLANE_MODE_INDICATOR_ACQUIRE and _AIRPLANE_MODE_INDICATOR_CHANGE.
>>
> Double-check your commit messages on some of these patches; they didn't
> get updated to add INDICATOR.
>

Thanks for catching this, Dan. I've sent an updated version fixing
this problem in reply to the patch where this slept through (9/10).

--
João Paulo Rechi Vita
http://about.me/jprvita

Re: [PATCHv2 04/10] rfkill: Move "state" sysfs file back to stable

[Pavel Machek]

On Mon 2016-02-22 11:36:35, João Paulo Rechi Vita wrote:
> There is still quite a bit of code using this interface, so we can't
> just remove it. Hopefully it will be possible in the future, but since
> its scheduled removal date is past 2 years already, we are better having
> the documentation reflecting the current state of things.

Umm. If you think it is still obsolete (and you do), you should keep
it in place.

> -rfkill - radio frequency (RF) connector kill switch support
> -
> -For details to this subsystem look at Documentation/rfkill.txt.
> -
> -What:		/sys/class/rfkill/rfkill[0-9]+/state
> -Date:		09-Jul-2007
> -KernelVersion	v2.6.22
> -Contact:	linux-wireless@vger.kernel.org
> -Description: 	Current state of the transmitter.
> -		This file is deprecated and scheduled to be removed in
> -2014,

Change it to "and may be removed,"
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv2 09/10] rfkill: Userspace control for airplane mode

[Pavel Machek]

Hi!

[BTW you should probably Cc people responsible for LED subsystem...]

> Provide an interface for the airplane-mode indicator be controlled from
> userspace. User has to first acquire the control through
> RFKILL_OP_AIRPLANE_MODE_ACQUIRE and keep the fd open for the whole time
> it wants to be in control of the indicator. Closing the fd or using
> RFKILL_OP_AIRPLANE_MODE_RELEASE restores the default policy.
> 
> To change state of the indicator, the RFKILL_OP_AIRPLANE_MODE_CHANGE
> operation is used, passing the value on "struct rfkill_event.soft". If
> the caller has not acquired the airplane-mode control beforehand, the
> operation fails.

So... you add LED trigger to display the state of the airplane
mode. Ok, why not.

But now you add an way to override it? Why? If someone wants to change
the led state, he can just change trigger to none, and then control
the LED manually...

BTW what happens when the device contains both radios controlled by
kernel (wifi, bluetooth) and radios controlled by userspace (GSM
modem)?

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

Re: [PATCHv2 09/10] rfkill: Userspace control for airplane mode

[Johannes Berg]

On Tue, 2016-02-23 at 21:45 +0100, Pavel Machek wrote:

> So... you add LED trigger to display the state of the airplane
> mode. Ok, why not.

Yes. However, consider that "the airplane mode" isn't a well-defined
concept; some systems may want to light up that LED even when wifi is
still enabled, since you're nowadays quite likely to be allowed to use
wifi (but not enable e.g. LTE) while in-flight.

> But now you add an way to override it? Why? If someone wants to
> change
> the led state, he can just change trigger to none, and then control
> the LED manually...

Yes, but now you've forced every application that wants to deal with
this to know about every single LED that might be used with this
trigger... that won't work for some generic userland tool that might
want to implement an "airplane-mode policy".

> BTW what happens when the device contains both radios controlled by
> kernel (wifi, bluetooth) and radios controlled by userspace (GSM
> modem)?

You'd better have the userspace to control the LED :)

johannes

Re: [PATCHv2 09/10] rfkill: Userspace control for airplane mode

[Pavel Machek]

On Tue 2016-02-23 21:55:14, Johannes Berg wrote:
> On Tue, 2016-02-23 at 21:45 +0100, Pavel Machek wrote:
> 
> > So... you add LED trigger to display the state of the airplane
> > mode. Ok, why not.
> 
> Yes. However, consider that "the airplane mode" isn't a well-defined
> concept; some systems may want to light up that LED even when wifi is
> still enabled, since you're nowadays quite likely to be allowed to use
> wifi (but not enable e.g. LTE) while in-flight.

Well "the airplane mode" is well defined. It means no intentional
transmitting at radio frequencies.

The fact that you are allowed to use WIFI on certain flights does not
change anything.

> > But now you add an way to override it? Why? If someone wants to
> > change
> > the led state, he can just change trigger to none, and then control
> > the LED manually...
> 
> Yes, but now you've forced every application that wants to deal with
> this to know about every single LED that might be used with this
> trigger... that won't work for some generic userland tool that might
> want to implement an "airplane-mode policy".

I see that "airplane mode" trigger might be a tiny bit
useful... dunno, for a LED near the airplane mode switch, when vendor
replaced hardware toggle with a key. But policy should have nothing to
do with that. If you argue additional "policy daemon" is needed for
that... forget it, that's overdesigned.

(Besides, finding all LEDs with given trigger is trivial
operation. Besides... there should never be more than one).

> > BTW what happens when the device contains both radios controlled by
> > kernel (wifi, bluetooth) and radios controlled by userspace (GSM
> > modem)?
> 
> You'd better have the userspace to control the LED :)

Yes, so lets forget that and no additional triggers? Good ;-).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv2 09/10] rfkill: Userspace control for airplane mode

[Johannes Berg]

On Tue, 2016-02-23 at 22:45 +0100, Pavel Machek wrote:

> Well "the airplane mode" is well defined. It means no intentional
> transmitting at radio frequencies.
> 
> The fact that you are allowed to use WIFI on certain flights does not
> change anything.

Nope, not that simple. Pick up any (contemporary) smartphone and watch
what happens with the airplane mode indicator (the little airplane
icon) when you enable wifi after enabling airplane mode. It stays
there.

Clearly the same logic could then apply to an actual LED (on a system
that's less size-constrained, e.g. a laptop or tablet.)

Thus, the display of "in airplane mode" *does* have policy, and clearly
there's precedent for not disabling the icon or LED when wifi is
enabled, but the kernel shouldn't really impose that. Now, the kernel
has a "safe" default which does what you thought was the "well defined"
airplane mode, but at the same time it's obviously not good enough.

And evidently, the Asus system that this was originally proposed for
has such an LED.

> I see that "airplane mode" trigger might be a tiny bit
> useful... dunno, for a LED near the airplane mode switch, when vendor
> replaced hardware toggle with a key. But policy should have nothing
> to do with that. If you argue additional "policy daemon" is needed
> for that... forget it, that's overdesigned.

See above.

> (Besides, finding all LEDs with given trigger is trivial
> operation. Besides... there should never be more than one).

That *might* actually work. But once a tool has detached the trigger
the information is gone; and tools would have to do that to control the
LED, making recovery from any kind of error difficult.

In any case, I've applied those patches already.

As far as the LED subsystem is concerned, all of this is just another
trigger anyway.

johannes

custom ioctl-based interface to control LED in networking (was Re: [PATCHv2 09/10] rfkill: Userspace control for airplane mode)

[Pavel Machek]

On Wed 2016-02-24 10:01:49, Johannes Berg wrote:
> On Tue, 2016-02-23 at 22:45 +0100, Pavel Machek wrote:
> 
> > Well "the airplane mode" is well defined. It means no intentional
> > transmitting at radio frequencies.
> > 
> > The fact that you are allowed to use WIFI on certain flights does not
> > change anything.
> 
> Nope, not that simple. Pick up any (contemporary) smartphone and watch
> what happens with the airplane mode indicator (the little airplane
> icon) when you enable wifi after enabling airplane mode. It stays
> there.
> 
> Clearly the same logic could then apply to an actual LED (on a system
> that's less size-constrained, e.g. a laptop or tablet.)
> 
> Thus, the display of "in airplane mode" *does* have policy, and clearly
> there's precedent for not disabling the icon or LED when wifi is
> enabled, but the kernel shouldn't really impose that. Now, the kernel
> has a "safe" default which does what you thought was the "well defined"
> airplane mode, but at the same time it's obviously not good enough.

If you want different trigger, implement different trigger. If you
want to indicate all but wifi, implement all but wifi, and then
userspace can select it by writing trigger name. If you want complete
userspace control, fine, but we have standard interface and it is not
ioctl.

> > (Besides, finding all LEDs with given trigger is trivial
> > operation. Besides... there should never be more than one).
> 
> That *might* actually work. But once a tool has detached the trigger
> the information is gone; and tools would have to do that to control the
> LED, making recovery from any kind of error difficult.

If you allow userspace to control the LED, well, then the LED may no
longer display the "airplane mode" status. Debugging "wrong trigger is
set" will certainly be less tricky than figuring out that "airplane
mode trigger" is actually "no trigger" based on some obscure ioctls.

> In any case, I've applied those patches already.

Well, you can still revert them before it becomes ABI. David does
not have to pull from you and Linus does not have to pull from
Linus. Besides, the series really should have been Cc-ed to LED
people, too.

Having custom, ioctl-based interface to control the LED is just too
ugly. (And I did not see it documented, which should be another reason
not to merge it.)  

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

Re: custom ioctl-based interface to control LED in networking (was Re: [PATCHv2 09/10] rfkill: Userspace control for airplane mode)

[Johannes Berg]

On Wed, 2016-02-24 at 11:46 +0100, Pavel Machek wrote:

> If you want different trigger, implement different trigger. If you
> want to indicate all but wifi, implement all but wifi, and then
> userspace can select it by writing trigger name. 

This is still mostly a strawman, since userspace cannot have a database
of LEDs that indicate airplane mode.

I'm sure you'd also not like to see 2**7 triggers implemented in rfkill
to cover all the possibilities.

> If you want complete
> userspace control, fine, but we have standard interface and it is not
> ioctl.

The "standard interface" is usable if you really just want to driver a
single LED and you know which one.

I think you're looking at this the wrong way, focusing too much on the
LED aspect.

Really what you have here is a concept of "airplane mode", and that
concept is specific to the rfkill subsystem. This happens to affect
mostly an LED trigger, today, but as a concept it's something that
*should* be managed within the rfkill subsystem.

> Besides, the series really should have been Cc-ed to LED
> people, too.

That's simply unreasonable, you're essentially saying that any user of
any kernel infrastructure should be Cc'ed to the implementer of that
infrastructure... 9/10 patches in this series aren't even LED specific,
only the *previous* patch, the one that tied the "airplane mode"
concept to an LED trigger in a very standard way had anything to do
with LED triggers at all.

johannes

Re: custom ioctl-based interface to control LED in networking (was Re: [PATCHv2 09/10] rfkill: Userspace control for airplane mode)

[Pavel Machek]

On Wed 2016-02-24 12:01:37, Johannes Berg wrote:
> On Wed, 2016-02-24 at 11:46 +0100, Pavel Machek wrote:
> 
> > If you want different trigger, implement different trigger. If you
> > want to indicate all but wifi, implement all but wifi, and then
> > userspace can select it by writing trigger name. 
> 
> This is still mostly a strawman, since userspace cannot have a database
> of LEDs that indicate airplane mode.

Why would it need to? It could look at default triggers for the led if
it really wanted to.

> I'm sure you'd also not like to see 2**7 triggers implemented in rfkill
> to cover all the possibilities.

Are all the possibilities useful? I assumed about 2 are. If you need
2**7 of them, LED triggers can have parameters.

> > If you want complete
> > userspace control, fine, but we have standard interface and it is not
> > ioctl.
> 
> The "standard interface" is usable if you really just want to driver a
> single LED and you know which one.
> 
> I think you're looking at this the wrong way, focusing too much on the
> LED aspect.
> 
> Really what you have here is a concept of "airplane mode", and that
> concept is specific to the rfkill subsystem. This happens to affect
> mostly an LED trigger, today, but as a concept it's something that
> *should* be managed within the rfkill subsystem.

How is that concept used outside the LEDs? What semantics does
"airplane mode" have? You tried to explain "airplane mode" is not well
defined up in this thread...

> > Besides, the series really should have been Cc-ed to LED
> > people, too.
> 
> That's simply unreasonable, you're essentially saying that any user of
> any kernel infrastructure should be Cc'ed to the implementer of that
> infrastructure... 9/10 patches in this series aren't even LED
> > specific,

I'm not saying that. I'm saying that LED maintainers should be Cced,
to keep the interfaces consistent.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: custom ioctl-based interface to control LED in networking (was Re: [PATCHv2 09/10] rfkill: Userspace control for airplane mode)

[Johannes Berg]

On Wed, 2016-02-24 at 13:14 +0100, Pavel Machek wrote:
> 
> Why would it need to? It could look at default triggers for the led
> if it really wanted to.

And then it needs to change them; if anything goes wrong error recovery
is practically impossible since the trigger information is lost
forever.

> > I'm sure you'd also not like to see 2**7 triggers implemented in
> > rfkill
> > to cover all the possibilities.
> 
> Are all the possibilities useful? I assumed about 2 are. If you need
> 2**7 of them, LED triggers can have parameters.

It's not my position to decide which combinations some system
integrator or userspace developer might find useful.

Even when we add parameters to a trigger (I don't see a generic way to
do that, but please do enlighten me), you're now ignoring the issue of
the userspace controlled GSM modem...

> > Really what you have here is a concept of "airplane mode", and that
> > concept is specific to the rfkill subsystem. This happens to affect
> > mostly an LED trigger, today, but as a concept it's something that
> > *should* be managed within the rfkill subsystem.
> 
> How is that concept used outside the LEDs? What semantics does
> "airplane mode" have? You tried to explain "airplane mode" is not
> well defined up in this thread...

I'd say it's well-defined for any given set of system software, so if
e.g. NetworkManager decides to define it one way, and connman another
way, there's a definition but the kernel need not interfere with it.

> > > Besides, the series really should have been Cc-ed to LED
> > > people, too.
> > 
> > That's simply unreasonable, you're essentially saying that any user
> > of any kernel infrastructure should be Cc'ed to the implementer of
> > that infrastructure... 9/10 patches in this series aren't even LED
> > specific,
> 
> I'm not saying that. I'm saying that LED maintainers should be Cced,
> to keep the interfaces consistent.

I pretty much have to read it that way, since the LED API is in no way
impacted by these changes. Here's a new trigger, with some magic inner
working. No impact on the LED API.

johannes

Re: custom ioctl-based interface to control LED in networking (was Re: [PATCHv2 09/10] rfkill: Userspace control for airplane mode)

[Pavel Machek]

On Wed 2016-02-24 14:31:33, Johannes Berg wrote:
> On Wed, 2016-02-24 at 13:14 +0100, Pavel Machek wrote:
> > 
> > Why would it need to? It could look at default triggers for the led
> > if it really wanted to.
> 
> And then it needs to change them; if anything goes wrong error recovery
> is practically impossible since the trigger information is lost
> forever.

Well, conventional way to solve this is to simply name the led
"acer::airplane"... that way it is persistent. We already do that for
display/keyboard backlights.

> It's not my position to decide which combinations some system
> integrator or userspace developer might find useful.
> 
> Even when we add parameters to a trigger (I don't see a generic way to
> do that, but please do enlighten me), you're now ignoring the issue of
> the userspace controlled GSM modem...

Take a look at the blinking triggers.

> > > Really what you have here is a concept of "airplane mode", and that
> > > concept is specific to the rfkill subsystem. This happens to affect
> > > mostly an LED trigger, today, but as a concept it's something that
> > > *should* be managed within the rfkill subsystem.
> > 
> > How is that concept used outside the LEDs? What semantics does
> > "airplane mode" have? You tried to explain "airplane mode" is not
> > well defined up in this thread...
> 
> I'd say it's well-defined for any given set of system software, so if
> e.g. NetworkManager decides to define it one way, and connman another
> way, there's a definition but the kernel need not interfere with it.

So... the LED changes meaning during boot? That's surely not a nice
solution.

So... you rather store bit in a kernel with unclear semantics,
explaining "oh I need to leave the flexibility to userland"? Sorry,
that's just ugly.

> > I'm not saying that. I'm saying that LED maintainers should be Cced,
> > to keep the interfaces consistent.
> 
> I pretty much have to read it that way, since the LED API is in no way
> impacted by these changes. Here's a new trigger, with some magic inner
> working. No impact on the LED API.

New LED trigger and new ioctl for LED control... not matching how LEDs
are normally controlled.

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

Re: [PATCHv2 08/10] rfkill: Use switch to demux userspace operations

[Jouni Malinen]

On Mon, Feb 22, 2016 at 11:36:39AM -0500, João Paulo Rechi Vita wrote:
> Using a switch to handle different ev.op values in rfkill_fop_write()
> makes the code easier to extend, as out-of-range values can always be
> handled by the default case.

This breaks rfkill.. There are automated test scripts for testing this
area (and most of Wi-Fi for that matter. It would be nice if these were
used for changes before they get contributed upstream..

http://buildbot.w1.fi/hwsim/

This specific commit broke all the rfkill_* test cases because of
following:

> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> @@ -1199,29 +1200,32 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
> -	list_for_each_entry(rfkill, &rfkill_list, node) {
> -		if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
> -			continue;
> -
> -		if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL)
> -			continue;

Note that RFKILL_TYPE_ALL here..

> +		list_for_each_entry(rfkill, &rfkill_list, node)
> +			if (rfkill->type == ev.type ||
> +			    ev.type == RFKILL_TYPE_ALL)
> +				rfkill_set_block(rfkill, ev.soft);

It was included for RFKILL_OP_CHANGE_ALL.

> +	case RFKILL_OP_CHANGE:
> +		list_for_each_entry(rfkill, &rfkill_list, node)
> +			if (rfkill->idx == ev.idx && rfkill->type == ev.type)
> +				rfkill_set_block(rfkill, ev.soft);

but not for RFKILL_OP_CHANGE..

This needs following to work:


diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 59ff92d..c4bbd19 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -1239,7 +1239,9 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
 		break;
 	case RFKILL_OP_CHANGE:
 		list_for_each_entry(rfkill, &rfkill_list, node)
-			if (rfkill->idx == ev.idx && rfkill->type == ev.type)
+			if (rfkill->idx == ev.idx &&
+			    (rfkill->type == ev.type ||
+			     ev.type == RFKILL_TYPE_ALL))
 				rfkill_set_block(rfkill, ev.soft);
 		ret = 0;
 		break;
 
-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCHv2 08/10] rfkill: Use switch to demux userspace operations

[João Paulo Rechi Vita]

Hello Jouni,

On 26 February 2016 at 12:59, Jouni Malinen <j@w1.fi> wrote:
> On Mon, Feb 22, 2016 at 11:36:39AM -0500, João Paulo Rechi Vita wrote:
>> Using a switch to handle different ev.op values in rfkill_fop_write()
>> makes the code easier to extend, as out-of-range values can always be
>> handled by the default case.
>
> This breaks rfkill.. There are automated test scripts for testing this
> area (and most of Wi-Fi for that matter. It would be nice if these were
> used for changes before they get contributed upstream..
>
> http://buildbot.w1.fi/hwsim/
>

Thanks for pointing that out, I haven't heard of this tool before.
I'll give it a try before my next submission.

> This specific commit broke all the rfkill_* test cases because of
> following:
>
>> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
>> @@ -1199,29 +1200,32 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
>> -     list_for_each_entry(rfkill, &rfkill_list, node) {
>> -             if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
>> -                     continue;
>> -
>> -             if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL)
>> -                     continue;
>
> Note that RFKILL_TYPE_ALL here..
>
>> +             list_for_each_entry(rfkill, &rfkill_list, node)
>> +                     if (rfkill->type == ev.type ||
>> +                         ev.type == RFKILL_TYPE_ALL)
>> +                             rfkill_set_block(rfkill, ev.soft);
>
> It was included for RFKILL_OP_CHANGE_ALL.
>
>> +     case RFKILL_OP_CHANGE:
>> +             list_for_each_entry(rfkill, &rfkill_list, node)
>> +                     if (rfkill->idx == ev.idx && rfkill->type == ev.type)
>> +                             rfkill_set_block(rfkill, ev.soft);
>
> but not for RFKILL_OP_CHANGE..
>
> This needs following to work:
>
>
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index 59ff92d..c4bbd19 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -1239,7 +1239,9 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
>                 break;
>         case RFKILL_OP_CHANGE:
>                 list_for_each_entry(rfkill, &rfkill_list, node)
> -                       if (rfkill->idx == ev.idx && rfkill->type == ev.type)
> +                       if (rfkill->idx == ev.idx &&
> +                           (rfkill->type == ev.type ||
> +                            ev.type == RFKILL_TYPE_ALL))
>                                 rfkill_set_block(rfkill, ev.soft);
>                 ret = 0;
>                 break;
>

I agree there is a difference in the logic here, thanks for taking the
time to point it out so clearly, and sorry for missing this. But AFAIU
userspace should not call RFKILL_OP_CHANGE with ev.type ==
RFKILL_TYPE_ALL, as RFKILL_OP_CHANGE is intended to be used to
block/unblock one RFKill switch, and it is not possible to create a
RFKill switch with type == RFKILL_TYPE_ALL (rfkill_alloc() would
return NULL).

I tried to look into the source code of the test suite you pointed,
but couldn't easily figure out how it ends up with that combination.
Could you please explain (or point me in the code) how is that a valid
operation? If I'm not missing anything, we should probably return
EINVAL in this case.

Regards,

--
João Paulo Rechi Vita
http://about.me/jprvita

Re: [PATCHv2 08/10] rfkill: Use switch to demux userspace operations

[Jouni Malinen]

On Mon, Feb 29, 2016 at 05:30:20PM -0500, João Paulo Rechi Vita wrote:

> I agree there is a difference in the logic here, thanks for taking the
> time to point it out so clearly, and sorry for missing this. But AFAIU
> userspace should not call RFKILL_OP_CHANGE with ev.type ==
> RFKILL_TYPE_ALL, as RFKILL_OP_CHANGE is intended to be used to
> block/unblock one RFKill switch, and it is not possible to create a
> RFKill switch with type == RFKILL_TYPE_ALL (rfkill_alloc() would
> return NULL).

Interesting. Maybe Johannes can comment on that part since I think he
wrote the code that interacts with kernel for the rfkill test cases.

> I tried to look into the source code of the test suite you pointed,
> but couldn't easily figure out how it ends up with that combination.
> Could you please explain (or point me in the code) how is that a valid
> operation? If I'm not missing anything, we should probably return
> EINVAL in this case.

These specific failures were shown for the test cases in this file:
http://w1.fi/cgit/hostap/tree/tests/hwsim/test_rfkill.py

The interaction with kernel is done using this code:
http://w1.fi/cgit/hostap/tree/tests/hwsim/rfkill.py

It does indeed look like TYPE_ALL is used here (the block() and
unblock() implementations). If this is incorrect, we can certainly
change the script since I'd assume this is not used for anything else
than the hwsim test cases (or well who knows, it is available out there,
so if someone needs python code to do rfkill operations..).
 
-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCHv2 08/10] rfkill: Use switch to demux userspace operations

[Johannes Berg]

On Tue, 2016-03-01 at 00:39 +0200, Jouni Malinen wrote:

> > I agree there is a difference in the logic here,

Gah. I thought I'd reviewed the logic and made sure there's no
difference ... :)

> >  thanks for taking the
> > time to point it out so clearly, and sorry for missing this. But AFAIU
> > userspace should not call RFKILL_OP_CHANGE with ev.type ==
> > RFKILL_TYPE_ALL, as RFKILL_OP_CHANGE is intended to be used to
> > block/unblock one RFKill switch, and it is not possible to create a
> > RFKill switch with type == RFKILL_TYPE_ALL (rfkill_alloc() would
> > return NULL).

> Interesting. Maybe Johannes can comment on that part since I think he
> wrote the code that interacts with kernel for the rfkill test cases.

So first of all, it seems that this argument is invalid since we can't break the ABI/API here; although perhaps if it's only a test case ...

Oh. It took me a while, but I see now. The original intent (I think)
was that with RFKILL_OP_CHANGE, the type would be ignored entirely. It
seems that the (my) original intent wouldn't have been to force
userspace to specify *both* the index and the type, but instead do

OP_CHANGE_ALL -> use type (possibly TYPE_ALL, ignoring idx)
OP_CHANGE     -> use idx (ignoring type)


The original code implemented it as follows:

                if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
                        continue;

-> check the idx only for OP_CHANGE

                if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL)
                        continue;

-> check the type, allowing _ALL

Now, all userspace that I found sets the ev.type field to TYPE_ALL all
the time; and it had to given these checks.

e.g. from rfkill.py:

# idx, type, op, soft, hard
_event_struct = '@IBBBB'

[...]

    def block(self):
        rfk = open('/dev/rfkill', 'w')
        s = struct.pack(_event_struct, self.idx, TYPE_ALL, _OP_CHANGE, 1, 0)
        rfk.write(s)
        rfk.close()


This check, originally, probably should've been

                if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL &&
                    ev.op != RFKILL_OP_CHANGE)
                        continue;

to ignore the type entirely.

I'm fine with Jouni's change, preserving the original behaviour of
requiring TYPE_ALL or the correct type, but I'm tempted to simply
remove the type check entirely.

Thoughts?

johannes

Re: [PATCHv2 08/10] rfkill: Use switch to demux userspace operations

[João Paulo Rechi Vita]

On 1 March 2016 at 08:43, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2016-03-01 at 00:39 +0200, Jouni Malinen wrote:
>
>> > I agree there is a difference in the logic here,
>
> Gah. I thought I'd reviewed the logic and made sure there's no
> difference ... :)
>
>> >  thanks for taking the
>> > time to point it out so clearly, and sorry for missing this. But AFAIU
>> > userspace should not call RFKILL_OP_CHANGE with ev.type ==
>> > RFKILL_TYPE_ALL, as RFKILL_OP_CHANGE is intended to be used to
>> > block/unblock one RFKill switch, and it is not possible to create a
>> > RFKill switch with type == RFKILL_TYPE_ALL (rfkill_alloc() would
>> > return NULL).
>
>> Interesting. Maybe Johannes can comment on that part since I think he
>> wrote the code that interacts with kernel for the rfkill test cases.
>
> So first of all, it seems that this argument is invalid since we can't break the ABI/API here; although perhaps if it's only a test case ...
>

Yep, that's an important point (not breaking the API/ABI).

> Oh. It took me a while, but I see now. The original intent (I think)
> was that with RFKILL_OP_CHANGE, the type would be ignored entirely. It
> seems that the (my) original intent wouldn't have been to force
> userspace to specify *both* the index and the type, but instead do
>
> OP_CHANGE_ALL -> use type (possibly TYPE_ALL, ignoring idx)
> OP_CHANGE     -> use idx (ignoring type)
>
>
> The original code implemented it as follows:
>
>                 if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
>                         continue;
>
> -> check the idx only for OP_CHANGE
>
>                 if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL)
>                         continue;
>
> -> check the type, allowing _ALL
>
> Now, all userspace that I found sets the ev.type field to TYPE_ALL all
> the time; and it had to given these checks.
>
> e.g. from rfkill.py:
>
> # idx, type, op, soft, hard
> _event_struct = '@IBBBB'
>
> [...]
>
>     def block(self):
>         rfk = open('/dev/rfkill', 'w')
>         s = struct.pack(_event_struct, self.idx, TYPE_ALL, _OP_CHANGE, 1, 0)
>         rfk.write(s)
>         rfk.close()
>
>
> This check, originally, probably should've been
>
>                 if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL &&
>                     ev.op != RFKILL_OP_CHANGE)
>                         continue;
>
> to ignore the type entirely.
>
> I'm fine with Jouni's change, preserving the original behaviour of
> requiring TYPE_ALL or the correct type, but I'm tempted to simply
> remove the type check entirely.
>
> Thoughts?
>

I think this patch should keep the original logic, as this is supposed
to be a refactor only. If we decide to remove the check, we should to
it in a separate patch, to make it clear for someone looking at the
history later.

I'm fine with removing the type check (in a separate patch), but I
don't see much gain in doing so.

--
João Paulo Rechi Vita
http://about.me/jprvita

Re: [PATCHv2 08/10] rfkill: Use switch to demux userspace operations

[João Paulo Rechi Vita]

Hello Johannes,

On 1 March 2016 at 11:15, João Paulo Rechi Vita <jprvita@gmail.com> wrote:
> On 1 March 2016 at 08:43, Johannes Berg <johannes@sipsolutions.net> wrote:
>>
>> I'm fine with Jouni's change, preserving the original behaviour of
>> requiring TYPE_ALL or the correct type, but I'm tempted to simply
>> remove the type check entirely.
>>
>> Thoughts?
>>
>
> I think this patch should keep the original logic, as this is supposed
> to be a refactor only. If we decide to remove the check, we should to
> it in a separate patch, to make it clear for someone looking at the
> history later.
>
> I'm fine with removing the type check (in a separate patch), but I
> don't see much gain in doing so.
>

I just saw you picked this patch with Jouni's fix, thanks!

--
João Paulo Rechi Vita
http://about.me/jprvita