| Message ID | 1462199948-6424-2-git-send-email-jprvita@endlessm.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi! > 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. If userspace wants to control the manually, it can do just that -- control it manually. There should not be a need to "override the default policy". Pavel
On Wed, 2016-05-04 at 09:29 +0200, Pavel Machek wrote: > > If userspace wants to control the manually, it can do just that -- > control it manually. There should not be a need to "override the > default policy". I'm still not buying this. In the original situation, without these patches, userspace has to have a list of all LEDs that are supposed to indicate airplane mode. With this patch only (without patch 2/3), userspace can look up the default trigger, but then has to change it, causing the necessary information to be lost immediately when you actually use it - that also seems like a bad idea. With the patches, the userspace that cares can also concentrate on something it already *does* - i.e. dealing with the rfkill API - and let the rest of the situation be sorted out in itself. Now, if the LED subsystem had a really good way of specifying LED intent, and it was widely used, and rfkill didn't already concern itself with the rfkill status of all devices ... yeah maybe this wouldn't be needed. As it stands, I still think this is the best way forward. johannes
On Thu 2016-05-12 11:32:52, Johannes Berg wrote: > On Wed, 2016-05-04 at 09:29 +0200, Pavel Machek wrote: > > > > If userspace wants to control the manually, it can do just that -- > > control it manually. There should not be a need to "override the > > default policy". > > I'm still not buying this. > > In the original situation, without these patches, userspace has to have > a list of all LEDs that are supposed to indicate airplane mode. Well, that's situation for many LEDs. > With this patch only (without patch 2/3), userspace can look up the > default trigger, but then has to change it, causing the necessary > information to be lost immediately when you actually use it - that also > seems like a bad idea. We should not store "what kind of led this is" in a trigger. LED subsystem seems to use suffix of LED name to do that. So if we standartize, lets say "::rfkill" suffix for this, it should work and follow existing practice. > Now, if the LED subsystem had a really good way of specifying LED > intent, and it was widely used, and rfkill didn't already concern > itself with the rfkill status of all devices ... yeah maybe this > wouldn't be needed. As it stands, I still think this is the best way > forward. There is one -- suffix in the LED name. Pavel
On Thu, 2016-05-19 at 09:16 +0200, Pavel Machek wrote: > > In the original situation, without these patches, userspace has to > > have a list of all LEDs that are supposed to indicate airplane > > mode. > Well, that's situation for many LEDs. That doesn't make it a *good* situation though. > > With this patch only (without patch 2/3), userspace can look up the > > default trigger, but then has to change it, causing the necessary > > information to be lost immediately when you actually use it - that > > also seems like a bad idea. > We should not store "what kind of led this is" in a trigger. That's pretty much what I'm arguing though. > LED > subsystem seems to use suffix of LED name to do that. So if we > standartize, lets say "::rfkill" suffix for this, it should work and > follow existing practice. [...] > There is one -- suffix in the LED name. I don't really think that's a good way, and it doesn't seem to be used universally, but I suppose it's good enough. João, that means you should send a patch to add the ::rfkill suffix. And Pavel should send a patch to document the practice and the existing suffixes with their meaning ;-) johannes
On 9 June 2016 at 08:43, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2016-05-19 at 09:16 +0200, Pavel Machek wrote: > (...) >> LED >> subsystem seems to use suffix of LED name to do that. So if we >> standartize, lets say "::rfkill" suffix for this, it should work and >> follow existing practice. > [...] >> There is one -- suffix in the LED name. > > I don't really think that's a good way, and it doesn't seem to be used > universally, but I suppose it's good enough. > The main practical drawback of this approach IMO is that we can't guarantee that userspace processes will not step on each other's toes trying to control the LED concurrently. But I guess that is something that userspace will have to solve for now, I rather get this moving without the trigger than not moving at all. > João, that means you should send a patch to add the ::rfkill suffix. > IMO "airplane" (or maybe "airplane-mode") is a better suffix, as it reflects the label on the machine's chassis. I'll name it "asus-wireless::airplane" and send this through platform-drivers-x86, as this is now contained in the platform-drivers-x86 subsystem. Thanks Johannes for your patience and help designing and reviewing the rfkill changes, even if not all of them made it through in the end. And thanks everyone else involved for the feedback. Best regards, -- João Paulo Rechi Vita http://about.me/jprvita
Hi! > > João, that means you should send a patch to add the ::rfkill suffix. > > > > IMO "airplane" (or maybe "airplane-mode") is a better suffix, as it > reflects the label on the machine's chassis. I'll name it > "asus-wireless::airplane" and send this through platform-drivers-x86, > as this is now contained in the platform-drivers-x86 subsystem. Thanks > Johannes for your patience and help designing and reviewing the rfkill > changes, even if not all of them made it through in the end. And > thanks everyone else involved for the feedback. Actually, I'd do '::rfkill', for consistency with other places in /sys. /sys/devices/platform/thinkpad_acpi/rfkill/rfkill1/name /sys/class/rfkill /sys/module/rfkill Thanks for patience, Pavel
On 13 June 2016 at 15:00, Pavel Machek <pavel@ucw.cz> wrote: > Hi! > >> > João, that means you should send a patch to add the ::rfkill suffix. >> > >> >> IMO "airplane" (or maybe "airplane-mode") is a better suffix, as it >> reflects the label on the machine's chassis. I'll name it >> "asus-wireless::airplane" and send this through platform-drivers-x86, >> as this is now contained in the platform-drivers-x86 subsystem. Thanks >> Johannes for your patience and help designing and reviewing the rfkill >> changes, even if not all of them made it through in the end. And >> thanks everyone else involved for the feedback. > > Actually, I'd do '::rfkill', for consistency with other places in > /sys. > > /sys/devices/platform/thinkpad_acpi/rfkill/rfkill1/name > /sys/class/rfkill > /sys/module/rfkill > If we use "rfkill" as a suffix, how do you expect userspace to be able to differentiate between a LED that indicates airplane-mode (LED ON when all radios are OFF) and a LED that indicates the state of a specific radio like WiFi or Bluetooth (LED ON when that specific radio is ON)? If we're going this route we should provide meaningful information here. -- João Paulo Rechi Vita http://about.me/jprvita
On Mon 2016-06-13 15:59:35, João Paulo Rechi Vita wrote: > On 13 June 2016 at 15:00, Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > > > >> > João, that means you should send a patch to add the ::rfkill suffix. > >> > > >> > >> IMO "airplane" (or maybe "airplane-mode") is a better suffix, as it > >> reflects the label on the machine's chassis. I'll name it > >> "asus-wireless::airplane" and send this through platform-drivers-x86, > >> as this is now contained in the platform-drivers-x86 subsystem. Thanks > >> Johannes for your patience and help designing and reviewing the rfkill > >> changes, even if not all of them made it through in the end. And > >> thanks everyone else involved for the feedback. > > > > Actually, I'd do '::rfkill', for consistency with other places in > > /sys. > > > > /sys/devices/platform/thinkpad_acpi/rfkill/rfkill1/name > > /sys/class/rfkill > > /sys/module/rfkill > > > > If we use "rfkill" as a suffix, how do you expect userspace to be able > to differentiate between a LED that indicates airplane-mode (LED ON > when all radios are OFF) and a LED that indicates the state of a > specific radio like WiFi or Bluetooth (LED ON when that specific radio > is ON)? If we're going this route we should provide meaningful > information here. '::airplane' has same problem, no? If you want to distinguish that, maybe you can do '::rfkill' for everything vs '::rfkill-wifi' for wifi-only and '::rfkill-bt' for bluetooth... Best regards, Pavel
On 13 June 2016 at 17:01, Pavel Machek <pavel@ucw.cz> wrote: > On Mon 2016-06-13 15:59:35, João Paulo Rechi Vita wrote: >> On 13 June 2016 at 15:00, Pavel Machek <pavel@ucw.cz> wrote: >> > Hi! >> > >> >> > João, that means you should send a patch to add the ::rfkill suffix. >> >> > >> >> >> >> IMO "airplane" (or maybe "airplane-mode") is a better suffix, as it >> >> reflects the label on the machine's chassis. I'll name it >> >> "asus-wireless::airplane" and send this through platform-drivers-x86, >> >> as this is now contained in the platform-drivers-x86 subsystem. Thanks >> >> Johannes for your patience and help designing and reviewing the rfkill >> >> changes, even if not all of them made it through in the end. And >> >> thanks everyone else involved for the feedback. >> > >> > Actually, I'd do '::rfkill', for consistency with other places in >> > /sys. >> > >> > /sys/devices/platform/thinkpad_acpi/rfkill/rfkill1/name >> > /sys/class/rfkill >> > /sys/module/rfkill >> > >> >> If we use "rfkill" as a suffix, how do you expect userspace to be able >> to differentiate between a LED that indicates airplane-mode (LED ON >> when all radios are OFF) and a LED that indicates the state of a >> specific radio like WiFi or Bluetooth (LED ON when that specific radio >> is ON)? If we're going this route we should provide meaningful >> information here. > > '::airplane' has same problem, no? > No, because in this case we would not use "airplane" as a suffix for a LED associated with an individual radio. > If you want to distinguish that, maybe you can do '::rfkill' for > everything vs '::rfkill-wifi' for wifi-only and '::rfkill-bt' for > bluetooth... > The problem here is that the "rfkill" name is already associated with individual rfkill switches under /sys/class/rfkill, /sys/devices/platform/*/rfkill etc, so I think we're better off distinguishing "airplane" vs "wifi" vs "bluetooth" etc, to avoid confusion. -- João Paulo Rechi Vita http://about.me/jprvita
On Mon 2016-06-13 17:10:02, João Paulo Rechi Vita wrote: > On 13 June 2016 at 17:01, Pavel Machek <pavel@ucw.cz> wrote: > > On Mon 2016-06-13 15:59:35, João Paulo Rechi Vita wrote: > >> On 13 June 2016 at 15:00, Pavel Machek <pavel@ucw.cz> wrote: > >> > Hi! > >> > > >> >> > João, that means you should send a patch to add the ::rfkill suffix. > >> >> > > >> >> > >> >> IMO "airplane" (or maybe "airplane-mode") is a better suffix, as it > >> >> reflects the label on the machine's chassis. I'll name it > >> >> "asus-wireless::airplane" and send this through platform-drivers-x86, > >> >> as this is now contained in the platform-drivers-x86 subsystem. Thanks > >> >> Johannes for your patience and help designing and reviewing the rfkill > >> >> changes, even if not all of them made it through in the end. And > >> >> thanks everyone else involved for the feedback. > >> > > >> > Actually, I'd do '::rfkill', for consistency with other places in > >> > /sys. > >> > > >> > /sys/devices/platform/thinkpad_acpi/rfkill/rfkill1/name > >> > /sys/class/rfkill > >> > /sys/module/rfkill > >> > > >> > >> If we use "rfkill" as a suffix, how do you expect userspace to be able > >> to differentiate between a LED that indicates airplane-mode (LED ON > >> when all radios are OFF) and a LED that indicates the state of a > >> specific radio like WiFi or Bluetooth (LED ON when that specific radio > >> is ON)? If we're going this route we should provide meaningful > >> information here. > > > > '::airplane' has same problem, no? > > > > No, because in this case we would not use "airplane" as a suffix for a > LED associated with an individual radio. > > > If you want to distinguish that, maybe you can do '::rfkill' for > > everything vs '::rfkill-wifi' for wifi-only and '::rfkill-bt' for > > bluetooth... > > > > The problem here is that the "rfkill" name is already associated with > individual rfkill switches under /sys/class/rfkill, > /sys/devices/platform/*/rfkill etc, so I think we're better off > distinguishing "airplane" vs "wifi" vs "bluetooth" etc, to avoid > confusion. (Actually, "::wifi" is super confusing, I'd expect that to be a led that blinks when wifi is active.) Well... "airplane" is quite confusing. I'd we use "rfkill" for disabling radios, and I believe we should stick with that. But small problem might be polarity. You may need both "::rfkill" and "::not-rfkill". Best regards, Pavel
On Mon, 2016-06-13 at 23:21 +0200, Pavel Machek wrote: > > (Actually, "::wifi" is super confusing, I'd expect that to be a led > that blinks when wifi is active.) Agree with that, yeah, that'd be confusing. > Well... "airplane" is quite confusing. I'd we use "rfkill" for > disabling radios, and I believe we should stick with that. > > But small problem might be polarity. You may need both "::rfkill" and > "::not-rfkill". I'd argue that "airplane" matches better what you're likely to find on the chassis of the system. In either case I think the suffixes should be documented, for both future kernel and application developers. johannes
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 884027f..9adf95e 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 @@ -1262,15 +1300,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; } @@ -1279,6 +1324,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 @@ -1295,5 +1341,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);