Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1302422 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger Marcel Holtmann <marcel@holtmann.org> - 2016-01-06 08:00 +0100
    Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger Johannes Berg <johannes@sipsolutions.net> - 2016-01-06 11:40 +0100
      Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger João Paulo Rechi Vita <jprvita@gmail.com> - 2016-01-07 17:30 +0100
        Re: [PATCH] net/rfkill: Create "airplane mode" LED trigger Johannes Berg <johannes@sipsolutions.net> - 2016-01-07 22:10 +0100

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1302544

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303718

+ 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303913

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web