[PATCH] RFKill airplane mode LED trigger

[PATCH] RFKill airplane mode LED trigger

[João Paulo Rechi Vita]

This patch adds a LED trigger to drive the airplane mode LED present in some
modern laptops. It will be used by the asus-wireless platform driver, which is
currently under review on platform-driver-x86@vger.kernel.org (the idea was
mainly approved, but I'm addressing some comments by the maintainers).

João Paulo Rechi Vita (1):
  net/rfkill: Create "airplane mode" LED trigger

 net/rfkill/core.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

-- 
2.5.0

[PATCH] net/rfkill: Create "airplane mode" LED trigger

[João Paulo Rechi Vita]

For platform drivers to be able to correctly drive the "Airplane Mode"
indicative LED there needs to be a RFKill LED trigger tied to the global
state of RFKILL_TYPE_ALL (instead of to a specific RFKill) and that
works in an inverted manner of regular RFKill LED triggers, that is, the
LED is ON when the state is blocked, and OFF otherwise.

This commit implements such a trigger, which will be used by the
asus-wireless x86 platform driver.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 net/rfkill/core.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index b41e9ea..3effc29 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -124,6 +124,26 @@ static bool rfkill_epo_lock_active;
 
 
 #ifdef CONFIG_RFKILL_LEDS
+static void airplane_mode_led_trigger_activate(struct led_classdev *led);
+
+static struct led_trigger airplane_mode_led_trigger = {
+	.name     = "rfkill-airplane-mode",
+	.activate = airplane_mode_led_trigger_activate,
+};
+
+static void airplane_mode_led_trigger_event(void)
+{
+	if (rfkill_global_states[RFKILL_TYPE_ALL].cur & RFKILL_BLOCK_ANY)
+		led_trigger_event(&airplane_mode_led_trigger, LED_FULL);
+	else
+		led_trigger_event(&airplane_mode_led_trigger, LED_OFF);
+}
+
+static void airplane_mode_led_trigger_activate(struct led_classdev *led)
+{
+	airplane_mode_led_trigger_event();
+}
+
 static void rfkill_led_trigger_event(struct rfkill *rfkill)
 {
 	struct led_trigger *trigger;
@@ -175,6 +195,10 @@ static void rfkill_led_trigger_unregister(struct rfkill *rfkill)
 	led_trigger_unregister(&rfkill->led_trigger);
 }
 #else
+static void airplane_mode_led_trigger_event(void)
+{
+}
+
 static void rfkill_led_trigger_event(struct rfkill *rfkill)
 {
 }
@@ -346,6 +370,7 @@ static void __rfkill_switch_all(const enum rfkill_type type, bool blocked)
 
 		for (i = 0; i < NUM_RFKILL_TYPES; i++)
 			rfkill_global_states[i].cur = blocked;
+		airplane_mode_led_trigger_event();
 	} else {
 		rfkill_global_states[type].cur = blocked;
 	}
@@ -1177,6 +1202,7 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
 			enum rfkill_type i;
 			for (i = 0; i < NUM_RFKILL_TYPES; i++)
 				rfkill_global_states[i].cur = ev.soft;
+			airplane_mode_led_trigger_event();
 		} else {
 			rfkill_global_states[ev.type].cur = ev.soft;
 		}
@@ -1293,6 +1319,10 @@ static int __init rfkill_init(void)
 	}
 #endif
 
+#ifdef CONFIG_RFKILL_LEDS
+	led_trigger_register(&airplane_mode_led_trigger);
+#endif
+
  out:
 	return error;
 }
-- 
2.5.0

[PATCH] net/rfkill: Create "airplane mode" LED trigger

[João Paulo Rechi Vita]

For platform drivers to be able to correctly drive the "Airplane Mode"
indicative LED there needs to be a RFKill LED trigger tied to the global
state of RFKILL_TYPE_ALL (instead of to a specific RFKill) and that
works in an inverted manner of regular RFKill LED triggers, that is, the
LED is ON when the state is blocked, and OFF otherwise.

This commit implements such a trigger, which will be used by the
asus-wireless x86 platform driver.

Signed-off-by: João Paulo Rechi Vita <jprvita-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
---
 net/rfkill/core.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index b41e9ea..3effc29 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -124,6 +124,26 @@ static bool rfkill_epo_lock_active;
 
 
 #ifdef CONFIG_RFKILL_LEDS
+static void airplane_mode_led_trigger_activate(struct led_classdev *led);
+
+static struct led_trigger airplane_mode_led_trigger = {
+	.name     = "rfkill-airplane-mode",
+	.activate = airplane_mode_led_trigger_activate,
+};
+
+static void airplane_mode_led_trigger_event(void)
+{
+	if (rfkill_global_states[RFKILL_TYPE_ALL].cur & RFKILL_BLOCK_ANY)
+		led_trigger_event(&airplane_mode_led_trigger, LED_FULL);
+	else
+		led_trigger_event(&airplane_mode_led_trigger, LED_OFF);
+}
+
+static void airplane_mode_led_trigger_activate(struct led_classdev *led)
+{
+	airplane_mode_led_trigger_event();
+}
+
 static void rfkill_led_trigger_event(struct rfkill *rfkill)
 {
 	struct led_trigger *trigger;
@@ -175,6 +195,10 @@ static void rfkill_led_trigger_unregister(struct rfkill *rfkill)
 	led_trigger_unregister(&rfkill->led_trigger);
 }
 #else
+static void airplane_mode_led_trigger_event(void)
+{
+}
+
 static void rfkill_led_trigger_event(struct rfkill *rfkill)
 {
 }
@@ -346,6 +370,7 @@ static void __rfkill_switch_all(const enum rfkill_type type, bool blocked)
 
 		for (i = 0; i < NUM_RFKILL_TYPES; i++)
 			rfkill_global_states[i].cur = blocked;
+		airplane_mode_led_trigger_event();
 	} else {
 		rfkill_global_states[type].cur = blocked;
 	}
@@ -1177,6 +1202,7 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
 			enum rfkill_type i;
 			for (i = 0; i < NUM_RFKILL_TYPES; i++)
 				rfkill_global_states[i].cur = ev.soft;
+			airplane_mode_led_trigger_event();
 		} else {
 			rfkill_global_states[ev.type].cur = ev.soft;
 		}
@@ -1293,6 +1319,10 @@ static int __init rfkill_init(void)
 	}
 #endif
 
+#ifdef CONFIG_RFKILL_LEDS
+	led_trigger_register(&airplane_mode_led_trigger);
+#endif
+
  out:
 	return error;
 }
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger

[João Paulo Rechi Vita]

Hello Johannes,

On 26 December 2015 at 10:05, João Paulo Rechi Vita <jprvita@gmail.com> wrote:
> For platform drivers to be able to correctly drive the "Airplane Mode"
> indicative LED there needs to be a RFKill LED trigger tied to the global
> state of RFKILL_TYPE_ALL (instead of to a specific RFKill) and that
> works in an inverted manner of regular RFKill LED triggers, that is, the
> LED is ON when the state is blocked, and OFF otherwise.
>
> This commit implements such a trigger, which will be used by the
> asus-wireless x86 platform driver.
>

The initial patches of the asus-wireless driver have been queue on the
platform-drivers-x86 tree. Do you have any comments on this one? It
would be great to have it in for 4.5, since the airplane mode LED
management in asus-wireless' pending patches depend on this one.

> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
> ---
>  net/rfkill/core.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index b41e9ea..3effc29 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -124,6 +124,26 @@ static bool rfkill_epo_lock_active;
>
>
>  #ifdef CONFIG_RFKILL_LEDS
> +static void airplane_mode_led_trigger_activate(struct led_classdev *led);
> +
> +static struct led_trigger airplane_mode_led_trigger = {
> +       .name     = "rfkill-airplane-mode",
> +       .activate = airplane_mode_led_trigger_activate,
> +};
> +
> +static void airplane_mode_led_trigger_event(void)
> +{
> +       if (rfkill_global_states[RFKILL_TYPE_ALL].cur & RFKILL_BLOCK_ANY)
> +               led_trigger_event(&airplane_mode_led_trigger, LED_FULL);
> +       else
> +               led_trigger_event(&airplane_mode_led_trigger, LED_OFF);
> +}
> +
> +static void airplane_mode_led_trigger_activate(struct led_classdev *led)
> +{
> +       airplane_mode_led_trigger_event();
> +}
> +
>  static void rfkill_led_trigger_event(struct rfkill *rfkill)
>  {
>         struct led_trigger *trigger;
> @@ -175,6 +195,10 @@ static void rfkill_led_trigger_unregister(struct rfkill *rfkill)
>         led_trigger_unregister(&rfkill->led_trigger);
>  }
>  #else
> +static void airplane_mode_led_trigger_event(void)
> +{
> +}
> +
>  static void rfkill_led_trigger_event(struct rfkill *rfkill)
>  {
>  }
> @@ -346,6 +370,7 @@ static void __rfkill_switch_all(const enum rfkill_type type, bool blocked)
>
>                 for (i = 0; i < NUM_RFKILL_TYPES; i++)
>                         rfkill_global_states[i].cur = blocked;
> +               airplane_mode_led_trigger_event();
>         } else {
>                 rfkill_global_states[type].cur = blocked;
>         }
> @@ -1177,6 +1202,7 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
>                         enum rfkill_type i;
>                         for (i = 0; i < NUM_RFKILL_TYPES; i++)
>                                 rfkill_global_states[i].cur = ev.soft;
> +                       airplane_mode_led_trigger_event();
>                 } else {
>                         rfkill_global_states[ev.type].cur = ev.soft;
>                 }
> @@ -1293,6 +1319,10 @@ static int __init rfkill_init(void)
>         }
>  #endif
>
> +#ifdef CONFIG_RFKILL_LEDS
> +       led_trigger_register(&airplane_mode_led_trigger);
> +#endif
> +
>   out:
>         return error;
>  }
> --
> 2.5.0
>

Thanks and best regards,

--
João Paulo Rechi Vita
http://about.me/jprvita

Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger

[João Paulo Rechi Vita]

Hello Johannes,

On 26 December 2015 at 10:05, João Paulo Rechi Vita <jprvita@gmail.com> wrote:
> For platform drivers to be able to correctly drive the "Airplane Mode"
> indicative LED there needs to be a RFKill LED trigger tied to the global
> state of RFKILL_TYPE_ALL (instead of to a specific RFKill) and that
> works in an inverted manner of regular RFKill LED triggers, that is, the
> LED is ON when the state is blocked, and OFF otherwise.
>
> This commit implements such a trigger, which will be used by the
> asus-wireless x86 platform driver.
>

The initial patches of the asus-wireless driver have been queue on the
platform-drivers-x86 tree. Do you have any comments on this one? It
would be great to have it in for 4.5, since the airplane mode LED
management in asus-wireless' pending patches depend on this one.

> Signed-off-by: João Paulo Rechi Vita <jprvita-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> ---
>  net/rfkill/core.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index b41e9ea..3effc29 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -124,6 +124,26 @@ static bool rfkill_epo_lock_active;
>
>
>  #ifdef CONFIG_RFKILL_LEDS
> +static void airplane_mode_led_trigger_activate(struct led_classdev *led);
> +
> +static struct led_trigger airplane_mode_led_trigger = {
> +       .name     = "rfkill-airplane-mode",
> +       .activate = airplane_mode_led_trigger_activate,
> +};
> +
> +static void airplane_mode_led_trigger_event(void)
> +{
> +       if (rfkill_global_states[RFKILL_TYPE_ALL].cur & RFKILL_BLOCK_ANY)
> +               led_trigger_event(&airplane_mode_led_trigger, LED_FULL);
> +       else
> +               led_trigger_event(&airplane_mode_led_trigger, LED_OFF);
> +}
> +
> +static void airplane_mode_led_trigger_activate(struct led_classdev *led)
> +{
> +       airplane_mode_led_trigger_event();
> +}
> +
>  static void rfkill_led_trigger_event(struct rfkill *rfkill)
>  {
>         struct led_trigger *trigger;
> @@ -175,6 +195,10 @@ static void rfkill_led_trigger_unregister(struct rfkill *rfkill)
>         led_trigger_unregister(&rfkill->led_trigger);
>  }
>  #else
> +static void airplane_mode_led_trigger_event(void)
> +{
> +}
> +
>  static void rfkill_led_trigger_event(struct rfkill *rfkill)
>  {
>  }
> @@ -346,6 +370,7 @@ static void __rfkill_switch_all(const enum rfkill_type type, bool blocked)
>
>                 for (i = 0; i < NUM_RFKILL_TYPES; i++)
>                         rfkill_global_states[i].cur = blocked;
> +               airplane_mode_led_trigger_event();
>         } else {
>                 rfkill_global_states[type].cur = blocked;
>         }
> @@ -1177,6 +1202,7 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
>                         enum rfkill_type i;
>                         for (i = 0; i < NUM_RFKILL_TYPES; i++)
>                                 rfkill_global_states[i].cur = ev.soft;
> +                       airplane_mode_led_trigger_event();
>                 } else {
>                         rfkill_global_states[ev.type].cur = ev.soft;
>                 }
> @@ -1293,6 +1319,10 @@ static int __init rfkill_init(void)
>         }
>  #endif
>
> +#ifdef CONFIG_RFKILL_LEDS
> +       led_trigger_register(&airplane_mode_led_trigger);
> +#endif
> +
>   out:
>         return error;
>  }
> --
> 2.5.0
>

Thanks and best regards,

--
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger

[Marcel Holtmann]

Hi Joao,

> For platform drivers to be able to correctly drive the "Airplane Mode"
> indicative LED there needs to be a RFKill LED trigger tied to the global
> state of RFKILL_TYPE_ALL (instead of to a specific RFKill) and that
> works in an inverted manner of regular RFKill LED triggers, that is, the
> LED is ON when the state is blocked, and OFF otherwise.
> 
> This commit implements such a trigger, which will be used by the
> asus-wireless x86 platform driver.
> 
> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
> ---
> net/rfkill/core.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
> 
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index b41e9ea..3effc29 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -124,6 +124,26 @@ static bool rfkill_epo_lock_active;
> 
> 
> #ifdef CONFIG_RFKILL_LEDS
> +static void airplane_mode_led_trigger_activate(struct led_classdev *led);
> +
> +static struct led_trigger airplane_mode_led_trigger = {
> +	.name     = "rfkill-airplane-mode",
> +	.activate = airplane_mode_led_trigger_activate,
> +};

so I am not convinced the kernel should have the concept of airplane mode at all. It is kinda of a term that keeps changing since airlines do now allow WiFi and Bluetooth short range transmissions during flight. We stayed away from calling it airplane mode since by the nature of it being governed by local regulations it will change over time.

The RFKILL subsystem got away with not labeling it by just saying RFKILL_CHANGE_ALL and if we want a trigger for that action we better find a more general term to describe the fact that all RF devices are shut off.

Keep in mind that even with airplane mode on, you can re-activate Bluetooth and WiFi these days. So while you are in airplane mode, then RFKILL switches for these two technologies can be taken back off. If we wanted to model that in the kernel we would be putting policy in the kernel and I think that is a bad idea.

That is pretty much the main reason why ConnMan never tried to push the information about flight mode back into the kernel. It is not a policy that the kernel can determine in the first place.

Regards

Marcel

Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger

[Johannes Berg]

On Tue, 2016-01-05 at 22:55 -0800, Marcel Holtmann wrote:
> 
> so I am not convinced the kernel should have the concept of airplane
> mode at all.

[snip long story]

This is true, but that doesn't mean the patch is bad, just the naming
could be different.

I think the patch could name this "rfkill-all" (or so) instead, and
replace all the "airplane_mode" identifiers as well.

Then the driver can still default to "rfkill-all" trigger, or a
suitably interested userspace could remove the trigger and manage the
LED state itself.

Then again - if I think about that more - perhaps the kernel *should*
have a concept of airplane mode, just one that's not necessarily tied
to the "rfkill_all" setting, but could be controlled by userspace. That
way, userspace wouldn't have to know about the LED, just about the
airplane mode indicator (for which rfkill would probably be an
appropriate place)

Two comments on the patch itself:

> +#ifdef CONFIG_RFKILL_LEDS
> +       led_trigger_register(&airplane_mode_led_trigger);
> +#endif

Everything else uses inlines to avoid ifdefs, you can do the same here.
Also, error handling seems necessary.

johannes

Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger

[João Paulo Rechi Vita]

+ Darren Hart and platform-drivers-x86 (sorry, I was trying to avoid
too much cross-posting and ended up leaving interested parties out).

On 6 Jan 2016 05:32, "Johannes Berg" <johannes@sipsolutions.net> wrote:
>
> On Tue, 2016-01-05 at 22:55 -0800, Marcel Holtmann wrote:
> >
> > so I am not convinced the kernel should have the concept of airplane
> > mode at all.
>
> [snip long story]
>
> This is true, but that doesn't mean the patch is bad, just the naming
> could be different.
>
> I think the patch could name this "rfkill-all" (or so) instead, and
> replace all the "airplane_mode" identifiers as well.
>

I agree "airplane mode" is not the best name here and I can change in
an updated version.

> Then the driver can still default to "rfkill-all" trigger, or a
> suitably interested userspace could remove the trigger and manage the
> LED state itself.
>

Trying to answer both Marcel's and your comments, I think we should
rather have a trigger which fires when the global state of
RFKILL_TYPE_ALL changes, instead of tied to the op
RFKILL_OP_CHANGE_ALL. Then we should also update the global states on
every set block operation instead of only on RFKILL_OP_CHANGE_ALL.
This part does not look like policy to me (please correct me if I'm
wrong).

The platform driver can default this trigger and have the LED reflect
the global state of RFKILL_TYPE_ALL. This indeed looks like policy,
but mostly because the physical LED label is an airplane icon, what
the LED will be representing is "all radios are off". If that is not
acceptable in the kernel, I can expose the LED to userspace instead
and different userspaces can decide when to trigger it (in which case
we don't need this patch at all). But considering this is a laptop
platform driver, the only way I can see this being used is having the
LED lit when all the radios are off, and unlit otherwise (maybe I'm
being a bit short-visioned).

In any case, I think we should update the global states on every set
block operation, to have them consistent with the individual states. I
can provide a patch for that.

> Then again - if I think about that more - perhaps the kernel *should*
> have a concept of airplane mode, just one that's not necessarily tied
> to the "rfkill_all" setting, but could be controlled by userspace. That
> way, userspace wouldn't have to know about the LED, just about the
> airplane mode indicator (for which rfkill would probably be an
> appropriate place)
>

If I'm following this correctly, your suggestion is to have an
"airplane mode indicator" switch in the RFKill subsystem, which would
be driven by userspace and will be tied to a trigger that could be
used by platform drivers to drive physical airplane mode LEDs and
similar. That's an interesting idea and probably better than expecting
userspaces to know about platform details like LED presence. If this
is feasible looks like a nice generic solution for this problem.

Please let me know what you guys think is the best solution so I can work on it.

> Two comments on the patch itself:
>
> > +#ifdef CONFIG_RFKILL_LEDS
> > +       led_trigger_register(&amp;airplane_mode_led_trigger);
> > +#endif
>
> Everything else uses inlines to avoid ifdefs, you can do the same here.
> Also, error handling seems necessary.
>

Ok, I can fix this if there is an updated version of this patch.

--
João Paulo Rechi Vita
http://about.me/jprvita

Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger

[Johannes Berg]

On Thu, 2016-01-07 at 11:19 -0500, João Paulo Rechi Vita wrote:

> Trying to answer both Marcel's and your comments, I think we should
> rather have a trigger which fires when the global state of
> RFKILL_TYPE_ALL changes, instead of tied to the op
> RFKILL_OP_CHANGE_ALL. Then we should also update the global states on
> every set block operation instead of only on RFKILL_OP_CHANGE_ALL.
> This part does not look like policy to me (please correct me if I'm
> wrong).

Yeah, this seems fine. I'm not sure really quite what the difference
between the state and OP_CHANGE_ALL would be, but using the state does
seem reasonable to me. I also agree it's not really policy. In a sense
though, one might argue that it encourages the wrong policy.

> The platform driver can default this trigger and have the LED reflect
> the global state of RFKILL_TYPE_ALL. This indeed looks like policy,
> but mostly because the physical LED label is an airplane icon, what
> the LED will be representing is "all radios are off". If that is not
> acceptable in the kernel, I can expose the LED to userspace instead
> and different userspaces can decide when to trigger it (in which case
> we don't need this patch at all). But considering this is a laptop
> platform driver, the only way I can see this being used is having the
> LED lit when all the radios are off, and unlit otherwise (maybe I'm
> being a bit short-visioned).

As Marcel said, the question is whether or not a physical LED with an
airplane icon really should be lit when all the radios are off, or
should instead be lit when the system is in an "airplane safe" mode.
Consider, for example, an Android phone: You can quite easily display
both the WiFi connection icon and the airplane icon.

> If I'm following this correctly, your suggestion is to have an
> "airplane mode indicator" switch in the RFKill subsystem, which would
> be driven by userspace and will be tied to a trigger that could be
> used by platform drivers to drive physical airplane mode LEDs and
> similar. That's an interesting idea and probably better than
> expecting
> userspaces to know about platform details like LED presence. If this
> is feasible looks like a nice generic solution for this problem.

Mostly correct, yes. I'd argue that it should come with the following
semantics:
 * default to the state of all as you described, so that without any
   userspace you still get some kind of sane default behaviour
 * allow only a single userspace owner, and require that owner to
   toggle it as required, to avoid multiple userspace applications
   stepping on each others' toes. This could be implemented by making
   this a new /dev/rfkill command, and requiring the fd to be held
   open while controlling the airplane mode state.

This would be the most generic solution, starting with the default
behaviour you implemented but allowing userspace to implement its own
airplane mode semantics without having to know about the platform LEDs
etc.

We could even do that in two stages, with your (updated) patch as the
first stage.

I would want to see some interest from userspace (e.g. Marcel for
connman) though before implementing that.


johannes

Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger

[João Paulo Rechi Vita]

Hello, Johannes.

On 7 January 2016 at 16:01, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2016-01-07 at 11:19 -0500, João Paulo Rechi Vita wrote:
>
>> Trying to answer both Marcel's and your comments, I think we should
>> rather have a trigger which fires when the global state of
>> RFKILL_TYPE_ALL changes, instead of tied to the op
>> RFKILL_OP_CHANGE_ALL. Then we should also update the global states on
>> every set block operation instead of only on RFKILL_OP_CHANGE_ALL.
>> This part does not look like policy to me (please correct me if I'm
>> wrong).
>
> Yeah, this seems fine. I'm not sure really quite what the difference
> between the state and OP_CHANGE_ALL would be, but using the state does
> seem reasonable to me. I also agree it's not really policy. In a sense
> though, one might argue that it encourages the wrong policy.
>

There was a misunderstanding of these semantics on my side, despite
this being documented in Documentation/rfkill.txt. Now I've realized
the semantic difference between 1."having all the individual switches
of a certain type blocked at a certain moment" and 2."blocking all
switches of a certain type": on the 1st situation, each switch was
individually blocked in different moments, and by chance a certain
type had all its registered switches blocked, while on the 2nd, all
switches of a certain type were blocked altogether using
RFKILL_OP_CHANGE_ALL. Only on the 2nd situation we want to update the
default state for hotplugged devices, and that's why
rfkill_global_states[type] is not updated when individual switches are
driven, even if we read situation 1. Then there is actually no
difference between the state value and the operation, and there is
nothing to be fixed on an individual switch change. Sorry for the
confusion.

I'm going to add a note about this behavior to
include/uapi/linux/rfkill.h as well.

>> The platform driver can default this trigger and have the LED reflect
>> the global state of RFKILL_TYPE_ALL. This indeed looks like policy,
>> but mostly because the physical LED label is an airplane icon, what
>> the LED will be representing is "all radios are off". If that is not
>> acceptable in the kernel, I can expose the LED to userspace instead
>> and different userspaces can decide when to trigger it (in which case
>> we don't need this patch at all). But considering this is a laptop
>> platform driver, the only way I can see this being used is having the
>> LED lit when all the radios are off, and unlit otherwise (maybe I'm
>> being a bit short-visioned).
>
> As Marcel said, the question is whether or not a physical LED with an
> airplane icon really should be lit when all the radios are off, or
> should instead be lit when the system is in an "airplane safe" mode.
> Consider, for example, an Android phone: You can quite easily display
> both the WiFi connection icon and the airplane icon.
>

Yes. I think we're actually saying the same thing with different words here.

>> If I'm following this correctly, your suggestion is to have an
>> "airplane mode indicator" switch in the RFKill subsystem, which would
>> be driven by userspace and will be tied to a trigger that could be
>> used by platform drivers to drive physical airplane mode LEDs and
>> similar. That's an interesting idea and probably better than
>> expecting
>> userspaces to know about platform details like LED presence. If this
>> is feasible looks like a nice generic solution for this problem.
>
> Mostly correct, yes. I'd argue that it should come with the following
> semantics:
>  * default to the state of all as you described, so that without any
>    userspace you still get some kind of sane default behaviour

Agreed, and we would still be in an "airplane safe" mode, but maybe a
too conservative one.

>  * allow only a single userspace owner, and require that owner to
>    toggle it as required, to avoid multiple userspace applications
>    stepping on each others' toes. This could be implemented by making
>    this a new /dev/rfkill command, and requiring the fd to be held
>    open while controlling the airplane mode state.
>

And when the fd gets released we would fallback to default, right? So
that would avoid two userpace apps trying to control it at the same
time, but not one after the other (which is a good thing). As I
understand it also implies we would not expose this control point
through sysfs.

> This would be the most generic solution, starting with the default
> behaviour you implemented but allowing userspace to implement its own
> airplane mode semantics without having to know about the platform LEDs
> etc.
>
> We could even do that in two stages, with your (updated) patch as the
> first stage.
>

Great, I already fixed the previous comments and started working on a
prototype of the second stage, but then a naming question came to my
mind. They way I'm designing it is to have only one trigger and change
its behavior when the "airplane mode indicator" is under userspace
control (basically changing when to call led_trigger_event() to fire
the trigger). In this case it probably makes sense to call the trigger
something like "rfkill-airplane_mode", as before, because it will fire
when we're in an "airplane safe" mode, controlled by userspace or with
a fallback default behavior.

Another option would be to have two separate triggers,
"rfkill-airplane_mode" controlled by userspace, and "rfkill-all"
implementing the default behavior, and change which trigger is set to
each LED *from the trigger implementation side* in rfkill. Unless I'm
missing something, I don't think this second approach is possible.

So the question is, if we go with the 1st approach, would it be fine
to call the trigger "rkill-airplane_mode", even for the first stage
when only the default behavior is implemented, or should I rename it
(which implies renaming an API to other drivers) during the 2nd stage
implementation? I think sticking with one name from the beginning is
better, but since it goes against previous feedback, I decided to ask
first.

> I would want to see some interest from userspace (e.g. Marcel for
> connman) though before implementing that.
>

I'll send patches for this soon.

Thanks for the feedback,

--
João Paulo Rechi Vita
http://about.me/jprvita

Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger

[João Paulo Rechi Vita]

Hello, Johannes.

On 7 January 2016 at 16:01, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2016-01-07 at 11:19 -0500, João Paulo Rechi Vita wrote:
>
>> Trying to answer both Marcel's and your comments, I think we should
>> rather have a trigger which fires when the global state of
>> RFKILL_TYPE_ALL changes, instead of tied to the op
>> RFKILL_OP_CHANGE_ALL. Then we should also update the global states on
>> every set block operation instead of only on RFKILL_OP_CHANGE_ALL.
>> This part does not look like policy to me (please correct me if I'm
>> wrong).
>
> Yeah, this seems fine. I'm not sure really quite what the difference
> between the state and OP_CHANGE_ALL would be, but using the state does
> seem reasonable to me. I also agree it's not really policy. In a sense
> though, one might argue that it encourages the wrong policy.
>

There was a misunderstanding of these semantics on my side, despite
this being documented in Documentation/rfkill.txt. Now I've realized
the semantic difference between 1."having all the individual switches
of a certain type blocked at a certain moment" and 2."blocking all
switches of a certain type": on the 1st situation, each switch was
individually blocked in different moments, and by chance a certain
type had all its registered switches blocked, while on the 2nd, all
switches of a certain type were blocked altogether using
RFKILL_OP_CHANGE_ALL. Only on the 2nd situation we want to update the
default state for hotplugged devices, and that's why
rfkill_global_states[type] is not updated when individual switches are
driven, even if we read situation 1. Then there is actually no
difference between the state value and the operation, and there is
nothing to be fixed on an individual switch change. Sorry for the
confusion.

I'm going to add a note about this behavior to
include/uapi/linux/rfkill.h as well.

>> The platform driver can default this trigger and have the LED reflect
>> the global state of RFKILL_TYPE_ALL. This indeed looks like policy,
>> but mostly because the physical LED label is an airplane icon, what
>> the LED will be representing is "all radios are off". If that is not
>> acceptable in the kernel, I can expose the LED to userspace instead
>> and different userspaces can decide when to trigger it (in which case
>> we don't need this patch at all). But considering this is a laptop
>> platform driver, the only way I can see this being used is having the
>> LED lit when all the radios are off, and unlit otherwise (maybe I'm
>> being a bit short-visioned).
>
> As Marcel said, the question is whether or not a physical LED with an
> airplane icon really should be lit when all the radios are off, or
> should instead be lit when the system is in an "airplane safe" mode.
> Consider, for example, an Android phone: You can quite easily display
> both the WiFi connection icon and the airplane icon.
>

Yes. I think we're actually saying the same thing with different words here.

>> If I'm following this correctly, your suggestion is to have an
>> "airplane mode indicator" switch in the RFKill subsystem, which would
>> be driven by userspace and will be tied to a trigger that could be
>> used by platform drivers to drive physical airplane mode LEDs and
>> similar. That's an interesting idea and probably better than
>> expecting
>> userspaces to know about platform details like LED presence. If this
>> is feasible looks like a nice generic solution for this problem.
>
> Mostly correct, yes. I'd argue that it should come with the following
> semantics:
>  * default to the state of all as you described, so that without any
>    userspace you still get some kind of sane default behaviour

Agreed, and we would still be in an "airplane safe" mode, but maybe a
too conservative one.

>  * allow only a single userspace owner, and require that owner to
>    toggle it as required, to avoid multiple userspace applications
>    stepping on each others' toes. This could be implemented by making
>    this a new /dev/rfkill command, and requiring the fd to be held
>    open while controlling the airplane mode state.
>

And when the fd gets released we would fallback to default, right? So
that would avoid two userpace apps trying to control it at the same
time, but not one after the other (which is a good thing). As I
understand it also implies we would not expose this control point
through sysfs.

> This would be the most generic solution, starting with the default
> behaviour you implemented but allowing userspace to implement its own
> airplane mode semantics without having to know about the platform LEDs
> etc.
>
> We could even do that in two stages, with your (updated) patch as the
> first stage.
>

Great, I already fixed the previous comments and started working on a
prototype of the second stage, but then a naming question came to my
mind. They way I'm designing it is to have only one trigger and change
its behavior when the "airplane mode indicator" is under userspace
control (basically changing when to call led_trigger_event() to fire
the trigger). In this case it probably makes sense to call the trigger
something like "rfkill-airplane_mode", as before, because it will fire
when we're in an "airplane safe" mode, controlled by userspace or with
a fallback default behavior.

Another option would be to have two separate triggers,
"rfkill-airplane_mode" controlled by userspace, and "rfkill-all"
implementing the default behavior, and change which trigger is set to
each LED *from the trigger implementation side* in rfkill. Unless I'm
missing something, I don't think this second approach is possible.

So the question is, if we go with the 1st approach, would it be fine
to call the trigger "rkill-airplane_mode", even for the first stage
when only the default behavior is implemented, or should I rename it
(which implies renaming an API to other drivers) during the 2nd stage
implementation? I think sticking with one name from the beginning is
better, but since it goes against previous feedback, I decided to ask
first.

> I would want to see some interest from userspace (e.g. Marcel for
> connman) though before implementing that.
>

I'll send patches for this soon.

Thanks for the feedback,

Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger

[Johannes Berg]

Hi,

> There was a misunderstanding of these semantics on my side, despite
> this being documented in Documentation/rfkill.txt. Now I've realized
> the semantic difference between 1."having all the individual switches
> of a certain type blocked at a certain moment" and 2."blocking all
> switches of a certain type": on the 1st situation, each switch was
> individually blocked in different moments, and by chance a certain
> type had all its registered switches blocked, while on the 2nd, all
> switches of a certain type were blocked altogether using
> RFKILL_OP_CHANGE_ALL. Only on the 2nd situation we want to update the
> default state for hotplugged devices, and that's why
> rfkill_global_states[type] is not updated when individual switches
> are
> driven, even if we read situation 1. Then there is actually no
> difference between the state value and the operation, and there is
> nothing to be fixed on an individual switch change. Sorry for the
> confusion.

Ok, glad you have that cleared up because I'm not sure I understand
what you were confused about :)

> I'm going to add a note about this behavior to
> include/uapi/linux/rfkill.h as well.

If it helps the next person, sure!

> >  * allow only a single userspace owner, and require that owner to
> >    toggle it as required, to avoid multiple userspace applications
> >    stepping on each others' toes. This could be implemented by
> > making
> >    this a new /dev/rfkill command, and requiring the fd to be held
> >    open while controlling the airplane mode state.
> > 
> 
> And when the fd gets released we would fallback to default, right?

Yeah, I guess so. Something has to be known to be controlling it, and
two applications can't really do it at the same time.

>  So
> that would avoid two userpace apps trying to control it at the same
> time, but not one after the other (which is a good thing). As I
> understand it also implies we would not expose this control point
> through sysfs.

Yes, and I agree, sysfs wouldn't make a lot of sense for this (since
you can't track ownership that way.)

> Great, I already fixed the previous comments and started working on a
> prototype of the second stage, but then a naming question came to my
> mind. They way I'm designing it is to have only one trigger and
> change
> its behavior when the "airplane mode indicator" is under userspace
> control (basically changing when to call led_trigger_event() to fire
> the trigger). In this case it probably makes sense to call the
> trigger
> something like "rfkill-airplane_mode", as before, because it will
> fire when we're in an "airplane safe" mode, controlled by userspace
> or with a fallback default behavior.

Sure.

> Another option would be to have two separate triggers,
> "rfkill-airplane_mode" controlled by userspace, and "rfkill-all"
> implementing the default behavior, and change which trigger is set to
> each LED *from the trigger implementation side* in rfkill. Unless I'm
> missing something, I don't think this second approach is possible.

Yes, this doesn't make sense.

> So the question is, if we go with the 1st approach, would it be fine
> to call the trigger "rkill-airplane_mode", even for the first stage
> when only the default behavior is implemented, or should I rename it
> (which implies renaming an API to other drivers) during the 2nd stage
> implementation? I think sticking with one name from the beginning is
> better, but since it goes against previous feedback, I decided to ask
> first.

Indeed, I think it's better. I wasn't previously considering (much) the
possibility of enhancing the framework :)

Thanks for your work on this - I see also your patches already, will go
over them soon; I'm working part-time on parental leave right now, so things take a bit longer than usual.

johannes

Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger

[João Paulo Rechi Vita]

Hello Johannes,

On 19 January 2016 at 15:08, Johannes Berg <johannes@sipsolutions.net> wrote:
> Hi,
>
>> There was a misunderstanding of these semantics on my side, despite
>> this being documented in Documentation/rfkill.txt. Now I've realized
>> the semantic difference between 1."having all the individual switches
>> of a certain type blocked at a certain moment" and 2."blocking all
>> switches of a certain type": on the 1st situation, each switch was
>> individually blocked in different moments, and by chance a certain
>> type had all its registered switches blocked, while on the 2nd, all
>> switches of a certain type were blocked altogether using
>> RFKILL_OP_CHANGE_ALL. Only on the 2nd situation we want to update the
>> default state for hotplugged devices, and that's why
>> rfkill_global_states[type] is not updated when individual switches
>> are
>> driven, even if we read situation 1. Then there is actually no
>> difference between the state value and the operation, and there is
>> nothing to be fixed on an individual switch change. Sorry for the
>> confusion.
>
> Ok, glad you have that cleared up because I'm not sure I understand
> what you were confused about :)
>
>> I'm going to add a note about this behavior to
>> include/uapi/linux/rfkill.h as well.
>
> If it helps the next person, sure!
>
>> >  * allow only a single userspace owner, and require that owner to
>> >    toggle it as required, to avoid multiple userspace applications
>> >    stepping on each others' toes. This could be implemented by
>> > making
>> >    this a new /dev/rfkill command, and requiring the fd to be held
>> >    open while controlling the airplane mode state.
>> >
>>
>> And when the fd gets released we would fallback to default, right?
>
> Yeah, I guess so. Something has to be known to be controlling it, and
> two applications can't really do it at the same time.
>

Ack.

>>  So
>> that would avoid two userpace apps trying to control it at the same
>> time, but not one after the other (which is a good thing). As I
>> understand it also implies we would not expose this control point
>> through sysfs.
>
> Yes, and I agree, sysfs wouldn't make a lot of sense for this (since
> you can't track ownership that way.)
>

Yes, that's what I thought.

>> Great, I already fixed the previous comments and started working on a
>> prototype of the second stage, but then a naming question came to my
>> mind. They way I'm designing it is to have only one trigger and
>> change
>> its behavior when the "airplane mode indicator" is under userspace
>> control (basically changing when to call led_trigger_event() to fire
>> the trigger). In this case it probably makes sense to call the
>> trigger
>> something like "rfkill-airplane_mode", as before, because it will
>> fire when we're in an "airplane safe" mode, controlled by userspace
>> or with a fallback default behavior.
>
> Sure.
>

Ack.

>> Another option would be to have two separate triggers,
>> "rfkill-airplane_mode" controlled by userspace, and "rfkill-all"
>> implementing the default behavior, and change which trigger is set to
>> each LED *from the trigger implementation side* in rfkill. Unless I'm
>> missing something, I don't think this second approach is possible.
>
> Yes, this doesn't make sense.
>

Ok, good to know :)

>> So the question is, if we go with the 1st approach, would it be fine
>> to call the trigger "rkill-airplane_mode", even for the first stage
>> when only the default behavior is implemented, or should I rename it
>> (which implies renaming an API to other drivers) during the 2nd stage
>> implementation? I think sticking with one name from the beginning is
>> better, but since it goes against previous feedback, I decided to ask
>> first.
>
> Indeed, I think it's better. I wasn't previously considering (much) the
> possibility of enhancing the framework :)
>
> Thanks for your work on this - I see also your patches already, will go
> over them soon; I'm working part-time on parental leave right now, so things take a bit longer than usual.
>

Thanks for the clarifications! I'll clean-up the patches that actually
implement this (the ones I've send are only general
fixes/improvements) and send them in the next days.

Don't worry about delays, enjoy your parental leave!

--
João Paulo Rechi Vita
http://about.me/jprvita

Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger

[João Paulo Rechi Vita]

Hello Johannes,

On 19 January 2016 at 15:08, Johannes Berg <johannes@sipsolutions.net> wrote:
> Hi,
>
>> There was a misunderstanding of these semantics on my side, despite
>> this being documented in Documentation/rfkill.txt. Now I've realized
>> the semantic difference between 1."having all the individual switches
>> of a certain type blocked at a certain moment" and 2."blocking all
>> switches of a certain type": on the 1st situation, each switch was
>> individually blocked in different moments, and by chance a certain
>> type had all its registered switches blocked, while on the 2nd, all
>> switches of a certain type were blocked altogether using
>> RFKILL_OP_CHANGE_ALL. Only on the 2nd situation we want to update the
>> default state for hotplugged devices, and that's why
>> rfkill_global_states[type] is not updated when individual switches
>> are
>> driven, even if we read situation 1. Then there is actually no
>> difference between the state value and the operation, and there is
>> nothing to be fixed on an individual switch change. Sorry for the
>> confusion.
>
> Ok, glad you have that cleared up because I'm not sure I understand
> what you were confused about :)
>
>> I'm going to add a note about this behavior to
>> include/uapi/linux/rfkill.h as well.
>
> If it helps the next person, sure!
>
>> >  * allow only a single userspace owner, and require that owner to
>> >    toggle it as required, to avoid multiple userspace applications
>> >    stepping on each others' toes. This could be implemented by
>> > making
>> >    this a new /dev/rfkill command, and requiring the fd to be held
>> >    open while controlling the airplane mode state.
>> >
>>
>> And when the fd gets released we would fallback to default, right?
>
> Yeah, I guess so. Something has to be known to be controlling it, and
> two applications can't really do it at the same time.
>

Ack.

>>  So
>> that would avoid two userpace apps trying to control it at the same
>> time, but not one after the other (which is a good thing). As I
>> understand it also implies we would not expose this control point
>> through sysfs.
>
> Yes, and I agree, sysfs wouldn't make a lot of sense for this (since
> you can't track ownership that way.)
>

Yes, that's what I thought.

>> Great, I already fixed the previous comments and started working on a
>> prototype of the second stage, but then a naming question came to my
>> mind. They way I'm designing it is to have only one trigger and
>> change
>> its behavior when the "airplane mode indicator" is under userspace
>> control (basically changing when to call led_trigger_event() to fire
>> the trigger). In this case it probably makes sense to call the
>> trigger
>> something like "rfkill-airplane_mode", as before, because it will
>> fire when we're in an "airplane safe" mode, controlled by userspace
>> or with a fallback default behavior.
>
> Sure.
>

Ack.

>> Another option would be to have two separate triggers,
>> "rfkill-airplane_mode" controlled by userspace, and "rfkill-all"
>> implementing the default behavior, and change which trigger is set to
>> each LED *from the trigger implementation side* in rfkill. Unless I'm
>> missing something, I don't think this second approach is possible.
>
> Yes, this doesn't make sense.
>

Ok, good to know :)

>> So the question is, if we go with the 1st approach, would it be fine
>> to call the trigger "rkill-airplane_mode", even for the first stage
>> when only the default behavior is implemented, or should I rename it
>> (which implies renaming an API to other drivers) during the 2nd stage
>> implementation? I think sticking with one name from the beginning is
>> better, but since it goes against previous feedback, I decided to ask
>> first.
>
> Indeed, I think it's better. I wasn't previously considering (much) the
> possibility of enhancing the framework :)
>
> Thanks for your work on this - I see also your patches already, will go
> over them soon; I'm working part-time on parental leave right now, so things take a bit longer than usual.
>

Thanks for the clarifications! I'll clean-up the patches that actually
implement this (the ones I've send are only general
fixes/improvements) and send them in the next days.

Don't worry about delays, enjoy your parental leave!