RFKill airplane-mode indicator

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

Signed-off-by: João Paulo Rechi Vita <redacted>
---
 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

Signed-off-by: João Paulo Rechi Vita <redacted>
---
 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
 

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

Signed-off-by: João Paulo Rechi Vita <redacted>
---
 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

Signed-off-by: João Paulo Rechi Vita <redacted>
---
 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

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

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

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

Signed-off-by: João Paulo Rechi Vita <redacted>
---
 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

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

Signed-off-by: João Paulo Rechi Vita <redacted>
---
 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

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

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

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

On 22 February 2016 at 12:00, Dan Williams [off-list ref] wrote:

On Mon, 2016-02-22 at 11:36 -0500, João Paulo Rechi Vita wrote:

quoted

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

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

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

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

On Tue 2016-02-23 21:55:14, Johannes Berg wrote:

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

quoted

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.

quoted

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).

quoted

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

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

On Wed 2016-02-24 10:01:49, Johannes Berg wrote:

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

quoted

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.

quoted

(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

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

On Wed 2016-02-24 12:01:37, Johannes Berg wrote:

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

quoted

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.

quoted

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

quoted

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

quoted

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

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.

quoted

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

quoted

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.

quoted

quoted

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

On Wed 2016-02-24 14:31:33, Johannes Berg wrote:

On Wed, 2016-02-24 at 13:14 +0100, Pavel Machek wrote:

quoted

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.

quoted

quoted

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.

quoted

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

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:

Note that RFKILL_TYPE_ALL here..

It was included for RFKILL_OP_CHANGE_ALL.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello Jouni,

On 26 February 2016 at 12:59, Jouni Malinen [off-list ref] wrote:

On Mon, Feb 22, 2016 at 11:36:39AM -0500, João Paulo Rechi Vita wrote:

quoted

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.

7 more quoted lines

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

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

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

quoted

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 ... :)

quoted

 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()

16 more lines

On 1 March 2016 at 08:43, Johannes Berg [off-list ref] wrote:

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

quoted

quoted

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 ... :)

quoted

quoted

 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).

quoted

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

12 more quoted lines

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

Hello Johannes,

On 1 March 2016 at 11:15, João Paulo Rechi Vita [off-list ref] wrote:

On 1 March 2016 at 08:43, Johannes Berg [off-list ref] wrote:

quoted

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