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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/