# Re: \[PATCH 8/9\] rfkill: Userspace control for airplane mode - List: linux-api - 2 messages, 2 authors - Page: https://ratatoskr.run/linux-api/2016/02/4688841/t ## Re: \[PATCH 8/9\] rfkill: Userspace control for airplane mode - From: Johannes Berg \ - Date: 2016-02-18 20:12:55 - Message: https://ratatoskr.run/linux-api/2016/02/4688841.md ```text 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. >  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, >  };   > @@ -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 ``` ## Re: \[PATCH 8/9\] rfkill: Userspace control for airplane mode - From: João Paulo Rechi Vita \ - Date: 2016-02-22 16:12:09 - Message: https://ratatoskr.run/linux-api/2016/02/4690854.md ```text On 18 February 2016 at 15:12, Johannes Berg [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: >> 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. >> +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. >> 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, >> }; > >> @@ -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. >> 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. >> 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 ```