| Message ID | 1454946096-9752-9-git-send-email-jprvita@endlessm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
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. > > 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. I'd like to clarify a bit, so tell me if I'm correct or not. Using RFKILL_OP_AIRPLANE_MODE_CHANGE does not actually change any device state. It's just an indicator with no relationship to any of the registered rfkill switches, right? I wonder if setting RFKILL_OP_AIRPLANE_MODE_CHANGE(true) shouldn't also softblock all switches, otherwise you can set airplane mode all day long with RFKILL_OP_AIRPLANE_MODE_CHANGE and it doesn't actually enable airplane mode at all? Dan > Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> > --- > Documentation/rfkill.txt | 10 ++++++++++ > include/uapi/linux/rfkill.h | 3 +++ > net/rfkill/core.c | 47 > ++++++++++++++++++++++++++++++++++++++++++--- > 3 files changed, 57 insertions(+), 3 deletions(-) > > diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt > index b13025a..aa6e014 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_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'. > + > > 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, > }; > > /** > diff --git a/net/rfkill/core.c b/net/rfkill/core.c > index fb11547..8067701 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 > @@ -1183,6 +1185,7 @@ static ssize_t rfkill_fop_read(struct file > *file, char __user *buf, > 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; > > @@ -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; > > if (ev.type >= NUM_RFKILL_TYPES) > @@ -1207,6 +1210,34 @@ static ssize_t rfkill_fop_write(struct file > *file, const char __user *buf, > > 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) { > + if (rfkill_apm_owned && !data->is_apm_owner) { > + count = -EACCES; > + } else { > + bool state = > rfkill_global_states[RFKILL_TYPE_ALL].cur; > + > + rfkill_apm_owned = false; > + data->is_apm_owner = false; > + rfkill_apm_led_trigger_event(state); > + } > + } > + > + if (ev.op == RFKILL_OP_AIRPLANE_MODE_CHANGE) { > + if (rfkill_apm_owned && data->is_apm_owner) > + rfkill_apm_led_trigger_event(ev.soft); > + else > + count = -EACCES; > + } > + > if (ev.op == RFKILL_OP_CHANGE_ALL) > rfkill_update_global_state(ev.type, ev.soft); > > @@ -1230,7 +1261,17 @@ static int rfkill_fop_release(struct inode > *inode, struct file *file) > struct rfkill_int_event *ev, *tmp; > > mutex_lock(&rfkill_global_mutex); > + > + if (data->is_apm_owner) { > + bool state = > rfkill_global_states[RFKILL_TYPE_ALL].cur; > + > + rfkill_apm_owned = false; > + data->is_apm_owner = false; > + rfkill_apm_led_trigger_event(state); > + } > + > list_del(&data->list); > + > mutex_unlock(&rfkill_global_mutex); > > mutex_destroy(&data->mtx); -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Joa Paulo, > 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. as explained in an earlier response, the concept Airplane Mode seems to be imposing policy into the kernel. Do we really want have this as a kernel exposed API. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-02-08 17:20, Marcel Holtmann wrote: > as explained in an earlier response, the concept Airplane Mode seems > to be imposing policy into the kernel. Do we really want have this as > a kernel exposed API. This patch is the one that *solves* that problem ... please read it more carefully? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8 February 2016 at 11:11, Dan Williams <dcbw@redhat.com> wrote: > 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. >> >> 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. > > I'd like to clarify a bit, so tell me if I'm correct or not. Using > RFKILL_OP_AIRPLANE_MODE_CHANGE does not actually change any device > state. It's just an indicator with no relationship to any of the > registered rfkill switches, right? > Yes, that's correct, and it is the intended behavior. > I wonder if setting RFKILL_OP_AIRPLANE_MODE_CHANGE(true) shouldn't also > softblock all switches, otherwise you can set airplane mode all day > long with RFKILL_OP_AIRPLANE_MODE_CHANGE and it doesn't actually enable > airplane mode at all? > The idea behind it is to just provide the mechanism for userspace to decide what exactly being in airplane mode means, so it would set/unset the indicator in the right moment. -- João Paulo Rechi Vita http://about.me/jprvita -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Marcel, On 8 February 2016 at 11:20, Marcel Holtmann <marcel@holtmann.org> wrote: > Hi Joa Paulo, > >> 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. > > as explained in an earlier response, the concept Airplane Mode seems to be imposing policy into the kernel. Do we really want have this as a kernel exposed API. > On that very same thread we decided to keep using the "airplane mode" name both for when having the default policy of "set it when all radios are off" or when allowing userspace to override the default. Please see the following message from Johannes http://www.spinics.net/lists/linux-wireless/msg146069.html and let me know if that covers your concern. Thanks! -- João Paulo Rechi Vita http://about.me/jprvita -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi João, On Tue, Feb 9, 2016 at 2:41 AM, João Paulo Rechi Vita <jprvita@gmail.com> 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. > > 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 | 3 +++ > net/rfkill/core.c | 47 ++++++++++++++++++++++++++++++++++++++++++--- > 3 files changed, 57 insertions(+), 3 deletions(-) > > diff --git a/net/rfkill/core.c b/net/rfkill/core.c > index fb11547..8067701 100644 > --- a/net/rfkill/core.c > +++ b/net/rfkill/core.c > @@ -1207,6 +1210,34 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf, > > 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) { > + if (rfkill_apm_owned && !data->is_apm_owner) { Are you sure this is correct? In the case that airplane mode isn't owned, the rfkill_apm_led_trigger_event() call will be a noop, so we should arguably not be calling it. Also, should we just fail silently if we're not the owner? I.e. what does userspace learn from this op failing and is that useful? > + count = -EACCES; > + } else { > + bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur; > + > + rfkill_apm_owned = false; > + data->is_apm_owner = false; > + rfkill_apm_led_trigger_event(state); > + } > + } > + > + if (ev.op == RFKILL_OP_AIRPLANE_MODE_CHANGE) { > + if (rfkill_apm_owned && data->is_apm_owner) > + rfkill_apm_led_trigger_event(ev.soft); > + else > + count = -EACCES; > + } > + > if (ev.op == RFKILL_OP_CHANGE_ALL) > rfkill_update_global_state(ev.type, ev.soft); > > @@ -1230,7 +1261,17 @@ static int rfkill_fop_release(struct inode *inode, struct file *file) > struct rfkill_int_event *ev, *tmp; > > mutex_lock(&rfkill_global_mutex); > + > + if (data->is_apm_owner) { > + bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur; > + > + rfkill_apm_owned = false; > + data->is_apm_owner = false; > + rfkill_apm_led_trigger_event(state); Also, this code is duplicated from the _RELEASE op above. Would it make sense to factor it out into a separate function? > + } > + > list_del(&data->list); > + (extra line) > mutex_unlock(&rfkill_global_mutex); > > mutex_destroy(&data->mtx); Thanks,
On 8 February 2016 at 17:53, Julian Calaby <julian.calaby@gmail.com> wrote: >> + if (ev.op == RFKILL_OP_AIRPLANE_MODE_RELEASE) { >> + if (rfkill_apm_owned && !data->is_apm_owner) { > > Are you sure this is correct? > > In the case that airplane mode isn't owned, the > rfkill_apm_led_trigger_event() call will be a noop, so we should > arguably not be calling it. > Ok, I'm changing the check to be consistent with _CHANGE, so the call only succeeds if (rfkill_apm_owned && data->is_apm_owner), and return an error otherwise. > Also, should we just fail silently if we're not the owner? I.e. what > does userspace learn from this op failing and is that useful? > I think it is better to return an error every time userspace is trying to call an operation that it was not supposed to call at a certain state (without acquiring control of the airplane-mode indicator). If a program has a logic error that makes it call _RELEASE without calling _ACQUIRE first, it's easier for the programmer to spot the problem if we return an error here. >> + count = -EACCES; >> + } else { >> + bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur; >> + >> + rfkill_apm_owned = false; >> + data->is_apm_owner = false; >> + rfkill_apm_led_trigger_event(state); >> + } >> + } >> + >> + if (ev.op == RFKILL_OP_AIRPLANE_MODE_CHANGE) { >> + if (rfkill_apm_owned && data->is_apm_owner) >> + rfkill_apm_led_trigger_event(ev.soft); >> + else >> + count = -EACCES; >> + } >> + >> if (ev.op == RFKILL_OP_CHANGE_ALL) >> rfkill_update_global_state(ev.type, ev.soft); >> >> @@ -1230,7 +1261,17 @@ static int rfkill_fop_release(struct inode *inode, struct file *file) >> struct rfkill_int_event *ev, *tmp; >> >> mutex_lock(&rfkill_global_mutex); >> + >> + if (data->is_apm_owner) { >> + bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur; >> + >> + rfkill_apm_owned = false; >> + data->is_apm_owner = false; >> + rfkill_apm_led_trigger_event(state); > > Also, this code is duplicated from the _RELEASE op above. Would it > make sense to factor it out into a separate function? > Yes, makes sense. This also made me notice I was assigning a negative value to a size_t variable (count). >> + } >> + >> list_del(&data->list); >> + > > (extra line) > After factoring out the _RELEASE code it looks better without this additional line. >> mutex_unlock(&rfkill_global_mutex); >> >> mutex_destroy(&data->mtx); > > Thanks, > Thanks for the review, Julian. I'm sending an updated version. -- João Paulo Rechi Vita http://about.me/jprvita -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2016-02-08 at 10:11 -0600, Dan Williams wrote: > I'd like to clarify a bit, so tell me if I'm correct or not. Using > RFKILL_OP_AIRPLANE_MODE_CHANGE does not actually change any device > state. It's just an indicator with no relationship to any of the > registered rfkill switches, right? Yes. I'm not sure I'm totally happy with this userspace API. > I wonder if setting RFKILL_OP_AIRPLANE_MODE_CHANGE(true) shouldn't > also > softblock all switches, otherwise you can set airplane mode all day > long with RFKILL_OP_AIRPLANE_MODE_CHANGE and it doesn't actually > enable > airplane mode at all? No, this is kinda the point that you could implement your policy in userspace now. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-02-10 at 17:07 +0100, Johannes Berg wrote: > On Mon, 2016-02-08 at 10:11 -0600, Dan Williams wrote: > > I'd like to clarify a bit, so tell me if I'm correct or not. Using > > RFKILL_OP_AIRPLANE_MODE_CHANGE does not actually change any device > > state. It's just an indicator with no relationship to any of the > > registered rfkill switches, right? > > Yes. I'm not sure I'm totally happy with this userspace API. > > > I wonder if setting RFKILL_OP_AIRPLANE_MODE_CHANGE(true) shouldn't > > also > > softblock all switches, otherwise you can set airplane mode all day > > long with RFKILL_OP_AIRPLANE_MODE_CHANGE and it doesn't actually > > enable > > airplane mode at all? > > No, this is kinda the point that you could implement your policy in > userspace now. Yeah, I get that now. It's just that to me, something called "AIRPLANE_MODE_CHANGE" seems like it should actually change airplane mode on/off, which implies killing radios. I wouldn't have had the problem if it was named AIRPLANE_MODE_INDICATOR_CHANGE, which makes it clear this is simply an indicator and has no actual effect on anything other than a LED. Dan -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-02-10 17:53, Dan Williams wrote: > Yeah, I get that now. It's just that to me, something called > "AIRPLANE_MODE_CHANGE" seems like it should actually change airplane > mode on/off, which implies killing radios. I wouldn't have had the > problem if it was named AIRPLANE_MODE_INDICATOR_CHANGE, which makes it > clear this is simply an indicator and has no actual effect on anything > other than a LED. Yeah, I agree. I'm not even sure that it's a good idea to subsume this into the regular operations in the API, although that's obviously the easiest extension. I'll need to think about that a bit more with the code at hand though. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 February 2016 at 12:12, Johannes Berg <johannes@sipsolutions.net> wrote: > On 2016-02-10 17:53, Dan Williams wrote: >> >> Yeah, I get that now. It's just that to me, something called >> "AIRPLANE_MODE_CHANGE" seems like it should actually change airplane >> mode on/off, which implies killing radios. I wouldn't have had the >> problem if it was named AIRPLANE_MODE_INDICATOR_CHANGE, which makes it >> clear this is simply an indicator and has no actual effect on anything >> other than a LED. > I also agree the AIRPLANE_MODE_INDICATOR_* prefix makes things more clear here. Thanks for the feedback, Dan! > > Yeah, I agree. I'm not even sure that it's a good idea to subsume this into > the regular operations in the API, although that's obviously the easiest > extension. I'll need to think about that a bit more with the code at hand > though. > Initially I have thought about creating and additional RFKill switch for that, but I think it would be a bit hackish since we would have to treat it specially in sysfs attributes and probably other places, and userspace would also need a special case when going through the list of RFKill switches in the system. The proposed solution has equivalent semantics to an RFKill switch, is backward-compatible (users would only ignore the unknown operations and evens -- although gsd has a "default:" case to abort execution on an unknown event) and does not extend the RFKill event struct. One alternative would be to move the control point to a separate device, like /dev/rfkill-airplane-mode, but I'm not sure this is a very elegant approach. Anyway, I'm open to work on changes to the API, but it would be great if you could at least pick or reject the non-polemical patches of the series, so I don't need to carry them anymore. -- João Paulo Rechi Vita http://about.me/jprvita -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18 February 2016 at 15:12, Johannes Berg <johannes@sipsolutions.net> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/rfkill.txt b/Documentation/rfkill.txt index b13025a..aa6e014 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_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'. + 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, }; /** diff --git a/net/rfkill/core.c b/net/rfkill/core.c index fb11547..8067701 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 @@ -1183,6 +1185,7 @@ static ssize_t rfkill_fop_read(struct file *file, char __user *buf, 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; @@ -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; if (ev.type >= NUM_RFKILL_TYPES) @@ -1207,6 +1210,34 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf, 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) { + if (rfkill_apm_owned && !data->is_apm_owner) { + count = -EACCES; + } else { + bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur; + + rfkill_apm_owned = false; + data->is_apm_owner = false; + rfkill_apm_led_trigger_event(state); + } + } + + if (ev.op == RFKILL_OP_AIRPLANE_MODE_CHANGE) { + if (rfkill_apm_owned && data->is_apm_owner) + rfkill_apm_led_trigger_event(ev.soft); + else + count = -EACCES; + } + if (ev.op == RFKILL_OP_CHANGE_ALL) rfkill_update_global_state(ev.type, ev.soft); @@ -1230,7 +1261,17 @@ static int rfkill_fop_release(struct inode *inode, struct file *file) struct rfkill_int_event *ev, *tmp; mutex_lock(&rfkill_global_mutex); + + if (data->is_apm_owner) { + bool state = rfkill_global_states[RFKILL_TYPE_ALL].cur; + + rfkill_apm_owned = false; + data->is_apm_owner = false; + rfkill_apm_led_trigger_event(state); + } + list_del(&data->list); + mutex_unlock(&rfkill_global_mutex); mutex_destroy(&data->mtx);
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 | 3 +++ net/rfkill/core.c | 47 ++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 57 insertions(+), 3 deletions(-)