Hi,
Sorry for the delay reviewing this.
On Mon, 2016-02-08 at 10:41 -0500, João Paulo Rechi Vita wrote:
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.
I've come to the conclusion that the new ops are probably the best
thing to do here.
+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_ACQUIRE, and is only available for one
userspace
+application at a time. Closing the fd or using
RFKILL_OP_AIRPLANE_MODE_RELEASE
+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_CHANGE,
+passing the new value in the 'soft' field of 'struct rfkill_event'.
I don't really see any value in _RELEASE, since an application can just
close the fd? I'd prefer not having the duplicate functionality
and force us to exercise the single code path every time.
quoted hunk
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..9cb999b 100644
--- a/include/uapi/linux/rfkill.h
+++ b/include/uapi/linux/rfkill.h
@@ -67,6 +67,9 @@ enum rfkill_operation { RFKILL_OP_DEL,
RFKILL_OP_CHANGE,
RFKILL_OP_CHANGE_ALL,
+ RFKILL_OP_AIRPLANE_MODE_ACQUIRE,
+ RFKILL_OP_AIRPLANE_MODE_RELEASE,
+ RFKILL_OP_AIRPLANE_MODE_CHANGE,
};
quoted hunk
@@ -1199,7 +1202,7 @@ 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)
+ if (ev.op < RFKILL_OP_CHANGE)
return -EINVAL;
You need to also reject invalid high values, like 27.
mutex_lock(&rfkill_global_mutex);
+ if (ev.op == RFKILL_OP_AIRPLANE_MODE_ACQUIRE) {
+ if (rfkill_apm_owned && !data->is_apm_owner) {
+ count = -EACCES;
+ } else {
+ rfkill_apm_owned = true;
+ data->is_apm_owner = true;
+ }
+ }
+
+ if (ev.op == RFKILL_OP_AIRPLANE_MODE_RELEASE) {
It would probably be better to simply use "switch (ev.op)" and make the
default case do a reject.
if (ev.op == RFKILL_OP_CHANGE_ALL)
rfkill_update_global_state(ev.type, ev.soft);
Also moving the existing code inside the switch, of course.
johannes
On 18 February 2016 at 15:12, Johannes Berg [off-list ref] wrote:
Hi,
Sorry for the delay reviewing this.
No problems!
On Mon, 2016-02-08 at 10:41 -0500, João Paulo Rechi Vita wrote:
quoted
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.
I've come to the conclusion that the new ops are probably the best
thing to do here.
Nice.
quoted
+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_ACQUIRE, and is only available for one
userspace
+application at a time. Closing the fd or using
RFKILL_OP_AIRPLANE_MODE_RELEASE
+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_CHANGE,
+passing the new value in the 'soft' field of 'struct rfkill_event'.
I don't really see any value in _RELEASE, since an application can just
close the fd? I'd prefer not having the duplicate functionality
and force us to exercise the single code path every time.
I actually added this op only for completion, I couldn't think of a
use-case where simply closing the fd wouldn't be enough. I'll remove
it for the next revision.
quoted
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..9cb999b 100644
--- a/include/uapi/linux/rfkill.h
+++ b/include/uapi/linux/rfkill.h
@@ -67,6 +67,9 @@ enum rfkill_operation {
RFKILL_OP_DEL,
RFKILL_OP_CHANGE,
RFKILL_OP_CHANGE_ALL,
+ RFKILL_OP_AIRPLANE_MODE_ACQUIRE,
+ RFKILL_OP_AIRPLANE_MODE_RELEASE,
+ RFKILL_OP_AIRPLANE_MODE_CHANGE,
};
quoted
@@ -1199,7 +1202,7 @@ 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)
+ if (ev.op < RFKILL_OP_CHANGE)
return -EINVAL;
You need to also reject invalid high values, like 27.
Yes, sorry for missing this.
quoted
mutex_lock(&rfkill_global_mutex);
+ if (ev.op == RFKILL_OP_AIRPLANE_MODE_ACQUIRE) {
+ if (rfkill_apm_owned && !data->is_apm_owner) {
+ count = -EACCES;
+ } else {
+ rfkill_apm_owned = true;
+ data->is_apm_owner = true;
+ }
+ }
+
+ if (ev.op == RFKILL_OP_AIRPLANE_MODE_RELEASE) {
It would probably be better to simply use "switch (ev.op)" and make the
default case do a reject.
Sounds better indeed.
quoted
if (ev.op == RFKILL_OP_CHANGE_ALL)
rfkill_update_global_state(ev.type, ev.soft);
Also moving the existing code inside the switch, of course.
Sure.
--
João Paulo Rechi Vita
http://about.me/jprvita