| Message ID | 1456159001-20307-10-git-send-email-jprvita@endlessm.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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: > > > So... you add LED trigger to display the state of the airplane > > mode. Ok, why not. > > Yes. However, consider that "the airplane mode" isn't a well-defined > concept; some systems may want to light up that LED even when wifi is > still enabled, since you're nowadays quite likely to be allowed to use > wifi (but not enable e.g. LTE) while in-flight. Well "the airplane mode" is well defined. It means no intentional transmitting at radio frequencies. The fact that you are allowed to use WIFI on certain flights does not change anything. > > But now you add an way to override it? Why? If someone wants to > > change > > the led state, he can just change trigger to none, and then control > > the LED manually... > > Yes, but now you've forced every application that wants to deal with > this to know about every single LED that might be used with this > trigger... that won't work for some generic userland tool that might > want to implement an "airplane-mode policy". I see that "airplane mode" trigger might be a tiny bit useful... dunno, for a LED near the airplane mode switch, when vendor replaced hardware toggle with a key. But policy should have nothing to do with that. If you argue additional "policy daemon" is needed for that... forget it, that's overdesigned. (Besides, finding all LEDs with given trigger is trivial operation. Besides... there should never be more than one). > > BTW what happens when the device contains both radios controlled by > > kernel (wifi, bluetooth) and radios controlled by userspace (GSM > > modem)? > > You'd better have the userspace to control the LED :) Yes, so lets forget that and no additional triggers? Good ;-). Pavel
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: > > > Well "the airplane mode" is well defined. It means no intentional > > transmitting at radio frequencies. > > > > The fact that you are allowed to use WIFI on certain flights does not > > change anything. > > Nope, not that simple. Pick up any (contemporary) smartphone and watch > what happens with the airplane mode indicator (the little airplane > icon) when you enable wifi after enabling airplane mode. It stays > there. > > Clearly the same logic could then apply to an actual LED (on a system > that's less size-constrained, e.g. a laptop or tablet.) > > Thus, the display of "in airplane mode" *does* have policy, and clearly > there's precedent for not disabling the icon or LED when wifi is > enabled, but the kernel shouldn't really impose that. Now, the kernel > has a "safe" default which does what you thought was the "well defined" > airplane mode, but at the same time it's obviously not good enough. If you want different trigger, implement different trigger. If you want to indicate all but wifi, implement all but wifi, and then userspace can select it by writing trigger name. If you want complete userspace control, fine, but we have standard interface and it is not ioctl. > > (Besides, finding all LEDs with given trigger is trivial > > operation. Besides... there should never be more than one). > > That *might* actually work. But once a tool has detached the trigger > the information is gone; and tools would have to do that to control the > LED, making recovery from any kind of error difficult. If you allow userspace to control the LED, well, then the LED may no longer display the "airplane mode" status. Debugging "wrong trigger is set" will certainly be less tricky than figuring out that "airplane mode trigger" is actually "no trigger" based on some obscure ioctls. > In any case, I've applied those patches already. Well, you can still revert them before it becomes ABI. David does not have to pull from you and Linus does not have to pull from Linus. Besides, the series really should have been Cc-ed to LED people, too. Having custom, ioctl-based interface to control the LED is just too ugly. (And I did not see it documented, which should be another reason not to merge it.) Pavel
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: > > > If you want different trigger, implement different trigger. If you > > want to indicate all but wifi, implement all but wifi, and then > > userspace can select it by writing trigger name. > > This is still mostly a strawman, since userspace cannot have a database > of LEDs that indicate airplane mode. Why would it need to? It could look at default triggers for the led if it really wanted to. > I'm sure you'd also not like to see 2**7 triggers implemented in rfkill > to cover all the possibilities. Are all the possibilities useful? I assumed about 2 are. If you need 2**7 of them, LED triggers can have parameters. > > If you want complete > > userspace control, fine, but we have standard interface and it is not > > ioctl. > > The "standard interface" is usable if you really just want to driver a > single LED and you know which one. > > I think you're looking at this the wrong way, focusing too much on the > LED aspect. > > Really what you have here is a concept of "airplane mode", and that > concept is specific to the rfkill subsystem. This happens to affect > mostly an LED trigger, today, but as a concept it's something that > *should* be managed within the rfkill subsystem. How is that concept used outside the LEDs? What semantics does "airplane mode" have? You tried to explain "airplane mode" is not well defined up in this thread... > > Besides, the series really should have been Cc-ed to LED > > people, too. > > That's simply unreasonable, you're essentially saying that any user of > any kernel infrastructure should be Cc'ed to the implementer of that > infrastructure... 9/10 patches in this series aren't even LED > > specific, I'm not saying that. I'm saying that LED maintainers should be Cced, to keep the interfaces consistent. Pavel
On Wed, 2016-02-24 at 13:14 +0100, Pavel Machek wrote: > > Why would it need to? It could look at default triggers for the led > if it really wanted to. And then it needs to change them; if anything goes wrong error recovery is practically impossible since the trigger information is lost forever. > > I'm sure you'd also not like to see 2**7 triggers implemented in > > rfkill > > to cover all the possibilities. > > Are all the possibilities useful? I assumed about 2 are. If you need > 2**7 of them, LED triggers can have parameters. It's not my position to decide which combinations some system integrator or userspace developer might find useful. Even when we add parameters to a trigger (I don't see a generic way to do that, but please do enlighten me), you're now ignoring the issue of the userspace controlled GSM modem... > > Really what you have here is a concept of "airplane mode", and that > > concept is specific to the rfkill subsystem. This happens to affect > > mostly an LED trigger, today, but as a concept it's something that > > *should* be managed within the rfkill subsystem. > > How is that concept used outside the LEDs? What semantics does > "airplane mode" have? You tried to explain "airplane mode" is not > well defined up in this thread... I'd say it's well-defined for any given set of system software, so if e.g. NetworkManager decides to define it one way, and connman another way, there's a definition but the kernel need not interfere with it. > > > Besides, the series really should have been Cc-ed to LED > > > people, too. > > > > That's simply unreasonable, you're essentially saying that any user > > of any kernel infrastructure should be Cc'ed to the implementer of > > that infrastructure... 9/10 patches in this series aren't even LED > > specific, > > I'm not saying that. I'm saying that LED maintainers should be Cced, > to keep the interfaces consistent. I pretty much have to read it that way, since the LED API is in no way impacted by these changes. Here's a new trigger, with some magic inner working. No impact on the LED API. johannes
On Wed 2016-02-24 14:31:33, Johannes Berg wrote: > On Wed, 2016-02-24 at 13:14 +0100, Pavel Machek wrote: > > > > Why would it need to? It could look at default triggers for the led > > if it really wanted to. > > And then it needs to change them; if anything goes wrong error recovery > is practically impossible since the trigger information is lost > forever. Well, conventional way to solve this is to simply name the led "acer::airplane"... that way it is persistent. We already do that for display/keyboard backlights. > It's not my position to decide which combinations some system > integrator or userspace developer might find useful. > > Even when we add parameters to a trigger (I don't see a generic way to > do that, but please do enlighten me), you're now ignoring the issue of > the userspace controlled GSM modem... Take a look at the blinking triggers. > > > Really what you have here is a concept of "airplane mode", and that > > > concept is specific to the rfkill subsystem. This happens to affect > > > mostly an LED trigger, today, but as a concept it's something that > > > *should* be managed within the rfkill subsystem. > > > > How is that concept used outside the LEDs? What semantics does > > "airplane mode" have? You tried to explain "airplane mode" is not > > well defined up in this thread... > > I'd say it's well-defined for any given set of system software, so if > e.g. NetworkManager decides to define it one way, and connman another > way, there's a definition but the kernel need not interfere with it. So... the LED changes meaning during boot? That's surely not a nice solution. So... you rather store bit in a kernel with unclear semantics, explaining "oh I need to leave the flexibility to userland"? Sorry, that's just ugly. > > I'm not saying that. I'm saying that LED maintainers should be Cced, > > to keep the interfaces consistent. > > I pretty much have to read it that way, since the LED API is in no way > impacted by these changes. Here's a new trigger, with some magic inner > working. No impact on the LED API. New LED trigger and new ioctl for LED control... not matching how LEDs are normally controlled. Best regards, Pavel
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);
Provide an interface for the airplane-mode indicator be controlled from userspace. User has to first acquire the control through RFKILL_OP_AIRPLANE_MODE_ACQUIRE and keep the fd open for the whole time it wants to be in control of the indicator. Closing the fd or using RFKILL_OP_AIRPLANE_MODE_RELEASE restores the default policy. To change state of the indicator, the RFKILL_OP_AIRPLANE_MODE_CHANGE operation is used, passing the value on "struct rfkill_event.soft". If the caller has not acquired the airplane-mode control beforehand, the operation fails. Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> --- Documentation/rfkill.txt | 10 ++++++++++ include/uapi/linux/rfkill.h | 6 ++++++ net/rfkill/core.c | 35 +++++++++++++++++++++++++++++++++-- 3 files changed, 49 insertions(+), 2 deletions(-)