Moving to rfkill component.

Created attachment 349063 [details] [review]
Inhibit kernel handling of RFKILL key events

Both the kernel and GNOME handle RFKILL related key events causing the
RFKILL state to be toggled multiple times. It is possible to tell the
kernel not to handle these events using an ioctl. This patch will
prevent kernel handling of these events while a GNOME session is active.

Created attachment 349067 [details] [review]
Inhibit kernel handling of RFKILL key events

Both the kernel and GNOME handle RFKILL related key events causing the
RFKILL state to be toggled multiple times. It is possible to tell the
kernel not to handle these events using an ioctl. This patch will
prevent kernel handling of these events while a GNOME session is active.


This version also cleans up the FD in finalize so that no resources
are leaked.

Note that I have *not* double checked that all keys are handled in user space. So before merging, we need to ensure support for the following evdev events:
 * KEY_WLAN
 * KEY_BLUETOOTH
 * KEY_UWB
 * KEY_WIMAX
 * KEY_RFKILL
and in particular also:
 * SW_RFKILL_ALL

Review of attachment 349067 [details] [review]:

::: plugins/rfkill/gsd-rfkill-manager.c
@@ +732,3 @@
                                                   on_mm_proxy_gotten, g_object_ref (manager));
 
+        manager->priv->session = G_DBUS_PROXY (gnome_settings_bus_get_session_proxy ());

We're using similar code in 3 different plugins. Would definitely be worth adding a helper in gnome-settings-daemon/gnome-settings-bus.[ch].

::: plugins/rfkill/rfkill-glib.c
@@ +453,3 @@
+
+void
+cc_rfkill_glib_set_kernel_noinput (CcRfkillGlib        *rfkill,

This needs to return a boolean and a GError. There are patches for additional error handling in this code in bugzilla if you want to rebase on top of that.

@@ +561,3 @@
+	g_object_class_install_property (object_class,
+					 PROP_KERNEL_NOINPUT,
+					 g_param_spec_string ("kernel-noinput",

A string? Looks like you didn't test this patch, as that can't possibly have worked.

Yeah, didn't test all the property getters/setters as the code doesn't actually use it currentyl. At the same time, if you want error handling, then I would probably just drop the property entirely and just have a function to block/unblock but no getter/setter and such.


That said, much more important question. Do you have a good idea on how to handle an rfkill switch? (i.e. SW_RFKILL_ALL)

It looks like for the LID we are going through upowerd currently. As I don't think we have infrastructure to handle these kind of events elsewhere it might make sense to do the same thing for the rfkill switch (for now).

Bastien, do you maybe have a better suggestion?

(In reply to Benjamin Berg from comment #6)
> Yeah, didn't test all the property getters/setters as the code doesn't
> actually use it currentyl. At the same time, if you want error handling,
> then I would probably just drop the property entirely and just have a
> function to block/unblock but no getter/setter and such.

I'd turn it on, always, and wrap it in an #ifdef, so we can easily disable it for testing.

> That said, much more important question. Do you have a good idea on how to
> handle an rfkill switch? (i.e. SW_RFKILL_ALL)

What would generate those events? A keyboard key (if so, which one), or a physical switch that's not a hardware rfkill?

> It looks like for the LID we are going through upowerd currently. As I don't
> think we have infrastructure to handle these kind of events elsewhere it
> might make sense to do the same thing for the rfkill switch (for now).
> 
> Bastien, do you maybe have a better suggestion?

libinput recently gained support for switches. We might need to have mutter forward that event to us if it's indeed required.

(In reply to Bastien Nocera from comment #7)
> (In reply to Benjamin Berg from comment #6)
> > Yeah, didn't test all the property getters/setters as the code doesn't
> > actually use it currentyl. At the same time, if you want error handling,
> > then I would probably just drop the property entirely and just have a
> > function to block/unblock but no getter/setter and such.
> 
> I'd turn it on, always, and wrap it in an #ifdef, so we can easily disable
> it for testing.

Ehm, it *has* to be turned off when we loose control of the evdev devices (i.e. the session is not active anymore). Otherwise RFKILL stops working as soon as e.g. GDM is in control to allow logging in as another user.

Now, I didn't care about error handling really because the only way an error is thrown is if we have an ancient (>6 years) kernel or the kernel input is not compiled in.

> > That said, much more important question. Do you have a good idea on how to
> > handle an rfkill switch? (i.e. SW_RFKILL_ALL)
> 
> What would generate those events? A keyboard key (if so, which one), or a
> physical switch that's not a hardware rfkill?

I *think* (from reading the kernel code) the "hardware" switch on the X220 for example would stop working with this patch applied currently. I haven't gotten around to testing this yet.

> > It looks like for the LID we are going through upowerd currently. As I don't
> > think we have infrastructure to handle these kind of events elsewhere it
> > might make sense to do the same thing for the rfkill switch (for now).
> > 
> > Bastien, do you maybe have a better suggestion?
> 
> libinput recently gained support for switches. We might need to have mutter
> forward that event to us if it's indeed required.

Two things to check here (I simply don't know much about input handling currently):
 * What happens when the switch happens while the session is inactive?
   (i.e. does libinput repoll the switch state?)
 * Does this work on X11?

(In reply to Benjamin Berg from comment #8)
> (In reply to Bastien Nocera from comment #7)
> > (In reply to Benjamin Berg from comment #6)
> > > Yeah, didn't test all the property getters/setters as the code doesn't
> > > actually use it currentyl. At the same time, if you want error handling,
> > > then I would probably just drop the property entirely and just have a
> > > function to block/unblock but no getter/setter and such.
> > 
> > I'd turn it on, always, and wrap it in an #ifdef, so we can easily disable
> > it for testing.
> 
> Ehm, it *has* to be turned off when we loose control of the evdev devices
> (i.e. the session is not active anymore). Otherwise RFKILL stops working as
> soon as e.g. GDM is in control to allow logging in as another user.

Of course... A property would be better, because once you've added the "SessionIsActive" to gnome-settings-bus.[ch], the codegen'ed object should have a property for "SessionIsActive" as well. So g_object_bind_property() between the D-Bus object and the Rfkill one, and it's nicely sorted.

> Now, I didn't care about error handling really because the only way an error
> is thrown is if we have an ancient (>6 years) kernel or the kernel input is
> not compiled in.

That's fine, but I'd rather the error message be printed from the gsd-rfkill-manager than in this code.

> > > That said, much more important question. Do you have a good idea on how to
> > > handle an rfkill switch? (i.e. SW_RFKILL_ALL)
> > 
> > What would generate those events? A keyboard key (if so, which one), or a
> > physical switch that's not a hardware rfkill?
> 
> I *think* (from reading the kernel code) the "hardware" switch on the X220
> for example would stop working with this patch applied currently. I haven't
> gotten around to testing this yet.

"hardware" indeed...

> > > It looks like for the LID we are going through upowerd currently. As I don't
> > > think we have infrastructure to handle these kind of events elsewhere it
> > > might make sense to do the same thing for the rfkill switch (for now).
> > > 
> > > Bastien, do you maybe have a better suggestion?
> > 
> > libinput recently gained support for switches. We might need to have mutter
> > forward that event to us if it's indeed required.
> 
> Two things to check here (I simply don't know much about input handling
> currently):
>  * What happens when the switch happens while the session is inactive?
>    (i.e. does libinput repoll the switch state?)

The session that's active will likely end up handling it, if it's a GNOME one. If there's no session active, then we're not running, and the kernel will sort it out.

>  * Does this work on X11?

No, but the easy way is to not use this feature at all in X11. No ioctl, no OSD, and rfkill-input keeps on working as it did before my original patches.

We could show the OSD if the rfkill state change was not done by GNOME itself.

That would give us the following steps:
 1) Create patch to turn GNOME handling of rfkill on/off at runtime and turn off by default. If turned on, disable the kernel handling.
 2) Look into showing the OSD if the rfkill action was not triggered by GNOME.
 3) Add infrastructure to correctly handle the RFKILL_ALL switch through libinput/mutter/gnome-shell.
 4) Hook up everything in GSD and turn rfkill handling back on if the switch handling is available (this will only happen on X11)

I have opened https://bugs.freedesktop.org/show_bug.cgi?id=100551 for the libinput part.


We can backport 1 and 2 to fix the user facing bug that it is currently not easily possible to turn of Airplane mode again.

(In reply to Benjamin Berg from comment #10)
> We could show the OSD if the rfkill state change was not done by GNOME
> itself.
> 
> That would give us the following steps:
>  1) Create patch to turn GNOME handling of rfkill on/off at runtime and turn
> off by default. If turned on, disable the kernel handling.
>  2) Look into showing the OSD if the rfkill action was not triggered by
> GNOME.

This is where your proposal falls down. Changing one rfkill device might have an impact on another one. You can't easily know whether the change is the consequence of a user action or not.

We can't show the OSD if we're not also handling the input.

>  3) Add infrastructure to correctly handle the RFKILL_ALL switch through
> libinput/mutter/gnome-shell.
>  4) Hook up everything in GSD and turn rfkill handling back on if the switch
> handling is available (this will only happen on X11)
> 
> I have opened https://bugs.freedesktop.org/show_bug.cgi?id=100551 for the
> libinput part.
> 
> 
> We can backport 1 and 2 to fix the user facing bug that it is currently not
> easily possible to turn of Airplane mode again.

The way I'd fix this code:
- add SessionIsActive access to the gnome-settings-bus.[ch] code
- use this new code in the 2 plugins that already use this functionality
- add property for handling rfkill input events in rfkill-glib
- add all the code, but disable the binding for stable branches, and on X11
- add rfkill switch support for libinput/mutter/gnome-shell in such a way that g-s-d can access it
- add code to handle all those separate events in g-s-d

I am tempted to just create a patch that disables all RFKILL handling in GNOME again. As I see it, it has been broken since it was first merged in 3.20 and it seems that under some configurations we will keep it disabled even in the future.

Disabling might create two regressions overall:
 * OSD is gone, but I still think it might be possible to create some heuristic
 * WLAN key doesn't toggle rfkill on all devices (it shouldn't be WLAN in the first place …)

Other than that, I agree with the general strategy.

Created attachment 350079 [details] [review]
Alright, here is a new patchset which retains the OSD and also the way that

GNOME handles the different rfkill buttons (i.e. doing an rfkill on everything
instead of only wlan).

The new property to block the kernel input is not yet used. At this point I think
we can get away with only handling all the key events (and ignoring switch events)
as the switch events are generally only used as an additional notification while
the platform drivers already do a hard block.

However, we do not currently handle the following keys:
 * WIMAX
 * RFKILL

These keys are missing througout the input stack and we are unlikely to be
able to ever handle them on X11. If these keys can be handled by GSD it
should set the BlockKernelInputHandling property on the Rfkill manager and
take over all control from the kernel.

I'll open a new bug for the part of actually inhibiting the kernel handling.

Created attachment 350080 [details] [review]
rfkill: Expose API to inhibit kernel handling of input events

The new BlockKernelInputHandling property on the Rfkill manager can be
used to disable kernel handling of input events. Kernel handling will
only be disabled while the session is active as it is assumed that input
events cannot be handled by other parts of the stack otherwise.

Created attachment 350081 [details] [review]
rfkill: Add ToggleAirplaneMode method call which returns the new state

When called this function also gets the information whether the kernel
may have seen the same input event and might be acting on it. In the
event that the kernel may have been acting on the same event it will not
return immediately but instead make sure that there are no race
conditions.
This works by considering events up to 50ms before the call and 100ms
after the call to coincide with the same input event and only ensuring
the expected rfkill state if kernel and userspace agree or after the
timeout has passed.

Note that the above heuristic will only be used when the kernel handling
is not inhibited. This should be the case in the future once GSD handles
all the rfkill related button events. Unfortunately that requires adding
new buttons to the input layer which is likely to only work on wayland.

Created attachment 350082 [details] [review]
media-keys: Use the new rfkill toggle action to prevent race conditions

To prevent race conditions with the kernel and userspace handling input
events a new ToggleAirplaneMode method has been added to the rfkill
manager. Use this instead of setting the property and show the OSD
according to the new state reported by the function.

Hi,

I just encountered this problem on a Dell Venue 11 Pro I've been playing with.

So I've been looking at your patches, I've been trying to come up with a nicer solution but I really cannot think of something better.

So I've given your patches a test spin. 50 / 100 ms is way too short a timeout I had to quadrupple it to 200 ms / 400 ms with that change your patches fix things to work reliable for me (the Venue 11 Pro model I've is the i5-4300y model, so not exactly slow).

I've also reviewed the patches, taking a good look at the logic / algorithm and that looks good, +1 from me for that. I've not done any coding style checks as I'm not that familiar with the gnome coding style.

Regards,

Hans

Review of attachment 350079 [details] [review]:

::: plugins/rfkill/rfkill-glib.c
@@ +459,3 @@
+
+	/* Nothing to do if the states already match. */
+	if (noinput == (rfkill->noinput_fd >= 0))

if (cc_rfkill_glib_get_kernel_noinput (rfkill) == noinput)
...

@@ +462,3 @@
+		return;
+
+	if (!noinput && (rfkill->noinput_fd >= 0)) {

And again. Maybe you want to use a macro to handle this without the "overhead" of the type check.

@@ +469,3 @@
+	}
+
+	if (noinput && (rfkill->noinput_fd < 0)) {

And here (!...get_noinput())

@@ +475,3 @@
+		if (fd < 0) {
+			if (errno == EACCES)
+				g_warning ("Could not open RFKILL control device, please verify your installation");

Do you want to print the errno in a g_debug() otherwise?

@@ +481,3 @@
+		res = ioctl (fd, RFKILL_IOCTL_NOINPUT, (long) 0);
+		if (res != 0) {
+			g_warning ("Could not disable kernel handling of RFKILL related keys.");

Print the errno as well.

@@ +486,3 @@
+		}
+
+		g_debug("Opened rfkill noinput FD.");

Space before the parenthesis.

@@ +566,3 @@
+					 PROP_KERNEL_NOINPUT,
+					 g_param_spec_boolean ("kernel-noinput",
+							       "Kernel Noinput",

This is supposed to be a short string in English. "Noinput" is not a word :)

Review of attachment 350080 [details] [review]:

::: plugins/rfkill/gsd-rfkill-manager.c
@@ +103,3 @@
 "    <property name='BluetoothHardwareAirplaneMode' type='b' access='read'/>"
 "    <property name='BluetoothHasAirplaneMode' type='b' access='read'/>"
+"    <property name='BlockKernelInputHandling' type='b' access='readwrite'/>"

I don't understand (yet, maybe?) why this needs to be external API. Please add this in another patch. I believe that the rkfill handler should always inhibit the input handling from the kernel unless it's not the active session.

Review of attachment 350081 [details] [review]:

> When called this function also gets the information whether the kernel
> may have seen the same input event and might be acting on it.

In which cases would we actually get that? This looks awfully complicated...

Review of attachment 350082 [details] [review]:

> To prevent race conditions with the kernel and userspace handling input
> events

Why would there be races? I don't understand that. You'd need to lose the control of the session at the same time as the button has been pressed, I'm not sure we should be too bothered about what happens in those cases.

Review of attachment 350080 [details] [review]:

::: plugins/rfkill/gsd-rfkill-manager.c
@@ +103,3 @@
 "    <property name='BluetoothHardwareAirplaneMode' type='b' access='read'/>"
 "    <property name='BluetoothHasAirplaneMode' type='b' access='read'/>"
+"    <property name='BlockKernelInputHandling' type='b' access='readwrite'/>"

After discussion on IRC, this needs to be removed. The kernel needs to be told not to handle rfkill keypresses. If there are blocker bugs (on top of my head, in xkeyboard-config and mutter), then we should reference them in this bug.

(In reply to Bastien Nocera from comment #22)
> Review of attachment 350080 [details] [review] [review]:
> 
> ::: plugins/rfkill/gsd-rfkill-manager.c
> @@ +103,3 @@
>  "    <property name='BluetoothHardwareAirplaneMode' type='b'
> access='read'/>"
>  "    <property name='BluetoothHasAirplaneMode' type='b' access='read'/>"
> +"    <property name='BlockKernelInputHandling' type='b'
> access='readwrite'/>"
> 
> After discussion on IRC, this needs to be removed. The kernel needs to be
> told not to handle rfkill keypresses. If there are blocker bugs (on top of
> my head, in xkeyboard-config and mutter), then we should reference them in
> this bug.

I already opened bug #760517 to keep track of the required changes and blocker bugs. By now this bug is actually more about fixing the current situation without causing different regressions. The aim here is to:

Fix current bugs:
 * Disabling airplane mode does not work.

Not introducing regressions:
 * Retain the OSD
 * Do a proper airplane mode action on machines with currently broken keybinding mapping in the kernel (i.e. thinkpads binding to KEY_WLAN)
 * Retain rfkill action on machines that bind to KEY_RFKILL

And also:
 * Provide a path forward including a transition period to properly solve this issue throughout the stack.

I could leave out this (currently unused) DBus API for now and we might not need it should we map KEY_RFKILL to Xf86UWB or similar. But we still need most of the heuristics and stuff in the immediate future.

(In reply to Benjamin Berg from comment #23)
<snip>
> Not introducing regressions:
>  * Retain the OSD

Keeping the OSD is not, and should not be on the list of things to keep.

That OSD is nice to have and easy to keep with the workaround to not hit either of the other two regressions.

To spell this out more, the choices are:

1. Disabling kernel handling now:
   - Keeps OSD
   - Breaks key handling on laptop models other than thinkpads

2. Keep as is:
   - Impossible to disable airplane mode again using buttons

3. Disable GNOME handling:
   - Breaks thinkpads as kernel only disables wifi (missing bluetooth)
   - Breaks OSD (no feedback at all under some conditions)

4. This workaround:
   - Retains OSD
   - Ensures proper RFKILL function on thinkpads (i.e. toggling bluetooth)
   - Heuristic only taken if absolutely necessary (currently always)
   - Can easily be removed once the full stack supports everything
     (which may or may not be the case on X11 depending on the keysym
     mapping)

(In reply to Benjamin Berg from comment #26)
> To spell this out more, the choices are:
> 
> 1. Disabling kernel handling now:
>    - Keeps OSD
>    - Breaks key handling on laptop models other than thinkpads
> 
> 2. Keep as is:
>    - Impossible to disable airplane mode again using buttons
> 
> 3. Disable GNOME handling:
>    - Breaks thinkpads as kernel only disables wifi (missing bluetooth)
>    - Breaks OSD (no feedback at all under some conditions)
> 
> 4. This workaround:
>    - Retains OSD
>    - Ensures proper RFKILL function on thinkpads (i.e. toggling bluetooth)
>    - Heuristic only taken if absolutely necessary (currently always)
>    - Can easily be removed once the full stack supports everything
>      (which may or may not be the case on X11 depending on the keysym
>      mapping)

+1 to the above summary that is how I see things too, as said I've hit the same gnome-bug (on a Dell Venue Pro 11) and the current situation is quite bad. When I was looking into this, this weekend and last Monday I tried to come up with cleaner alternatives and they basically all suck.

One additional option I've thought about would be:

5. Detect if unhandled keys are present (iterate over all input devices, check if they can report KEY_RFKILL, KEY_WIMAX or SW_RFKILL_ALL) and then:
5.1 If no unhandled keys are present disable kernel rfkill input handling (aka 1.)
5.2 If unhandled keys are present disable gnome handling (aka 3.)

This might be cleaner, esp. with an eye to the future because then on wayland we can simply make the "unhandled-keys-present?" check always return false.

AFAICT this basically offers the same end-result as 4. since by their nature unhandled keys are ignored by gnome already as it does not see them, without the event races the 3th patch is trying to work around. Note that currently all input devices generating trouble-some events are non hotpluggable, so we would need to do the check only once.

Created attachment 351058 [details] [review]
media-keys: Disable rfkill keys handling

If we handle rfkill input keys without telling rfkill, we end up
handling those keys in gnome-settings-daemon, as well as in the kernel's
rfkill-input driver. As handling all the different keys would require
more work than possible in a stable release, we'll let rfkill-input
handle this, and lose the OSD for now.

Comment on attachment 351058 [details] [review]
media-keys: Disable rfkill keys handling

Attachment 351058 [details] pushed as 6ab16d1 - media-keys: Disable rfkill keys handling

Pushed to gnome-3-22 and gnome-3-24, making the support go back to
its GNOME 3.18 level.

Created attachment 351059 [details] [review]
rfkill: Add property to Rfkill helper to inhibit kernel handling

The kernel will handle input events by default which is causing race
conditions and can for example make it impossible to disable airplane
mode again. Add API to allow disabling the kernel handler.

Comment on attachment 351059 [details] [review]
rfkill: Add property to Rfkill helper to inhibit kernel handling

Attachment 351059 [details] pushed as 3810072 - rfkill: Add property to Rfkill helper to inhibit kernel handling

Created attachment 351060 [details] [review]
rfkill: Inhibit rfkill-input's rfkill keys handling

Disable the kernel's handling of rfkill input events when the
session is active, so that multiple logins of GNOME, or use of any
sessions without this handling can fall back onto the kernel's support.

Attachment 351060 [details] pushed as ccceec4 - rfkill: Inhibit rfkill-input's rfkill keys handling

With those 2 patches merged in master, the only fixes required would now be in:
-  xkeyboard-config: pass KEY_RFKILL and KEY_WIMAX through as XF86WLAN and XF86UWB, respectively
- mutter: handle multiple keycodes that correspond to one keysym

I don't know how to handle SW_RFKILL_ALL though, as I don't seem to have any hardware that uses it. Ideas welcome.

If all the patches for support haven't landed when 3.26 is coming around, then we'll disable the handling in media-keys (same code as in gnome-3-24 and earlier), and revert ccceec40f8302973af47d64846d4c6ea12a938af.

Hi,

(In reply to Bastien Nocera from comment #32)
> Created attachment 351060 [details] [review] [review]
> rfkill: Inhibit rfkill-input's rfkill keys handling
> 
> Disable the kernel's handling of rfkill input events when the
> session is active, so that multiple logins of GNOME, or use of any
> sessions without this handling can fall back onto the kernel's support.

This will break the RFKILL hotkey on the keyboard of many modern non Lenovo laptops:

[hans@shalem linux]$ ack -l KEY_RFKILL drivers/platform/x86
drivers/platform/x86/toshiba_acpi.c
drivers/platform/x86/asus-wireless.c
drivers/platform/x86/asus-laptop.c
drivers/platform/x86/hp-wireless.c
drivers/platform/x86/intel-hid.c
drivers/platform/x86/asus-nb-wmi.c
drivers/platform/x86/dell-wmi.c
drivers/platform/x86/dell-rbtn.c
drivers/platform/x86/fujitsu-laptop.c

So from my pov NACK for this fix. I agree that this is the direction to take, but for now you really need to guard the g_object_bind_property call with an "if (rfkill_has_unhandled_rfkill_keys())" call, which does as I described as option 5. in comment 27.

Ugh, I see you already pushed this? Why? Both Benjamin and I have indicated that this will break things on a ton of laptops. You're fixing laptops using KEY_WLAN at the expense of all laptops which use KEY_RFKILL which has now effectively just become a dead-key while before it would correctly turn on/off all radios (but not trigger an OSD as gnome would not see the event). Gnome should really not be claiming to handle rfkill input events on devices with a KEY_RFKILL key as that is simply not true.

You have until GNOME 3.26 is released to get the fixes into xkeyboard-config and mutter. If you're interested in doing this another way, then, please, feel free to start maintaining this code, because I'm at my tether's end.

I've asked at least 5 times for the support to be implemented in a specific way, and every single time I've had patches that did not. If you're interested in unmaintainable patches like the ones that were attached to this bug, then be my guest.

Hi,

(In reply to Bastien Nocera from comment #34)
> With those 2 patches merged in master, the only fixes required would now be
> in:
> -  xkeyboard-config: pass KEY_RFKILL and KEY_WIMAX through as XF86WLAN and
> XF86UWB, respectively

Except that that is impossible to do under Xorg due to the max supported scancode being 8 bits so we cannot map new codes.

Hmm double checking this might just work with an updated xkb config, the scancodes have an offset of 8 for some ancient reason and KEY_RFKILL is 247 which + 8 is 255, which should work (I'm not sure why no-one has mapped it yet).

So we may luck out here. I will take a look at this sometime the coming week.

> - mutter: handle multiple keycodes that correspond to one keysym
> 
> I don't know how to handle SW_RFKILL_ALL though, as I don't seem to have any
> hardware that uses it. Ideas welcome.

It is used only by the thinkpad-acpi driver which says it is used on the T60 at least, but Benjamin checked the thinkpad-acpi code and it will also hardblock the radios when the switch is thrown so we do not really need to worry about it.

Regards,

Hans

(In reply to Hans de Goede from comment #37)
> Hi,
> 
> (In reply to Bastien Nocera from comment #34)
> > With those 2 patches merged in master, the only fixes required would now be
> > in:
> > -  xkeyboard-config: pass KEY_RFKILL and KEY_WIMAX through as XF86WLAN and
> > XF86UWB, respectively
> 
> Except that that is impossible to do under Xorg due to the max supported
> scancode being 8 bits so we cannot map new codes.
> 
> Hmm double checking this might just work with an updated xkb config, the
> scancodes have an offset of 8 for some ancient reason and KEY_RFKILL is 247
> which + 8 is 255, which should work (I'm not sure why no-one has mapped it
> yet).
> 
> So we may luck out here. I will take a look at this sometime the coming week.

If the keycode is too high there are still 2 options available:
- only handle the rfkill keys on wayland (meaning adding a gtk+ dep to the rfkill plugin)
- remap the keys in udev to generate KEY_WLAN (not sure whether that would have an impact on rfkill-input's handling if not inhibited)

> > - mutter: handle multiple keycodes that correspond to one keysym
> > 
> > I don't know how to handle SW_RFKILL_ALL though, as I don't seem to have any
> > hardware that uses it. Ideas welcome.
> 
> It is used only by the thinkpad-acpi driver which says it is used on the T60
> at least, but Benjamin checked the thinkpad-acpi code and it will also
> hardblock the radios when the switch is thrown so we do not really need to
> worry about it.

If/when libinput gains generic switch support, we could use this to show an OSD. In the meanwhile, we're not losing anything compared to the pre-3.18 state of affairs.

(In reply to Bastien Nocera from comment #38)
> (In reply to Hans de Goede from comment #37)
> > Hmm double checking this might just work with an updated xkb config, the
> > scancodes have an offset of 8 for some ancient reason and KEY_RFKILL is 247
> > which + 8 is 255, which should work (I'm not sure why no-one has mapped it
> > yet).
> > 
> > So we may luck out here. I will take a look at this sometime the coming week.
> 
> If the keycode is too high there are still 2 options available:
> - only handle the rfkill keys on wayland (meaning adding a gtk+ dep to the
> rfkill plugin)
> - remap the keys in udev to generate KEY_WLAN (not sure whether that would
> have an impact on rfkill-input's handling if not inhibited)

That will only work if the evdev driver in question actually supports remapping and a lot of the platform/x86 drivers do not.

(In reply to Bastien Nocera from comment #38)
> If/when libinput gains generic switch support, we could use this to show an
> OSD.

see https://bugs.freedesktop.org/show_bug.cgi?id=100551, currently waiting for you guys

(In reply to Bastien Nocera from comment #36)
> You have until GNOME 3.26 is released to get the fixes into xkeyboard-config
> and mutter. If you're interested in doing this another way, then, please,
> feel free to start maintaining this code, because I'm at my tether's end.

Also a NACK from me for the patch as it was committed to master. Not because it is wrong, but because it either needs proper dependencies (which don't exist yet) or a guard.

I am curious. Which parts of the code base does that offer cover?

> I've asked at least 5 times for the support to be implemented in a specific
> way, and every single time I've had patches that did not. If you're
> interested in unmaintainable patches like the ones that were attached to
> this bug, then be my guest.

And I have considered doing that multiple times. But each time I started to look into different and simpler solutions I ended up realising that it would cause regressions to a lot of existing users. Regressions that I personally do not find acceptable during a stable release cycle. In particular as this feature has been shipping for multiple releases now, even if it has been somewhat broken all along.

Hi All,

Ok I've been working on updating various bits and pieces to make KEY_RFKILL work and I can confirm that it works under X11 too, so once we've all the fixes for KEY_RFKILL upstream we're good to go with this bug.

I've decided to use a new keysym for the RFKILL key, so that we can differentiate between KEY_WLAN and KEY_RFKILL in the future (if we want to), as a bonus this means no mutter changes are necessary

This involves several steps:
1) Get the new keysym define accepted upstream:
https://patchwork.freedesktop.org/patch/155279/
2) Sync libkxbcommon with these changes:
https://github.com/jwrdegoede/libxkbcommon/commit/570fcdfd8107f824a398190056436612efa442d1
3) Rebuild libX11 against a new xproto package with the added keysyms
4) Update xkeyboard-config with mappings for the new keysyms (patch to be written)
5) Update g-s-d with XF86Rfkill keybinding:
https://github.com/jwrdegoede/gnome-settings-daemon/commit/5df52e6c6d6bfc132141879d1485840dc77608c8

With all the other steps depending on 1. as that finalizes the keysym name. I've commit rights for xproto, so as soon as I've an ack I'll push 1. and start working on the other bits.

Regards,

Hans

> Ok I've been working on updating various bits and pieces to make KEY_RFKILL
> work and I can confirm that it works under X11 too, so once we've all the
> fixes for KEY_RFKILL upstream we're good to go with this bug.
> 
> I've decided to use a new keysym for the RFKILL key, so that we can
> differentiate between KEY_WLAN and KEY_RFKILL in the future (if we want to),
> as a bonus this means no mutter changes are necessary
> 
> This involves several steps:
> 1) Get the new keysym define accepted upstream:
> https://patchwork.freedesktop.org/patch/155279/
> 2) Sync libkxbcommon with these changes:
> https://github.com/jwrdegoede/libxkbcommon/commit/
> 570fcdfd8107f824a398190056436612efa442d1
> 3) Rebuild libX11 against a new xproto package with the added keysyms
> 4) Update xkeyboard-config with mappings for the new keysyms (patch to be
> written)
> 5) Update g-s-d with XF86Rfkill keybinding:

I've just finished all steps, except for step 5. Step 5 is being tracked in bug 781505, so there is nothing left to track in this bug and this bug can be closed.

Note I've a Fedora 26 update with all the changes from step 1-4 above in there here:
https://bodhi.fedoraproject.org/updates/FEDORA-2017-d3b4fda341

This is currently lacking the libX11 rebuild, that has been done too, but I need to add it to the update once it it unlocked for editing.

Not really gnome, but I feel it is better to discuss this here then in RH bugzilla, given that F26 will thus have all the necessary bits, I think we should consider backporting the proper fix (rather then the disabling of the feature) for F26 ?

(In reply to Hans de Goede from comment #45)
> Not really gnome, but I feel it is better to discuss this here then in RH
> bugzilla, given that F26 will thus have all the necessary bits, I think we
> should consider backporting the proper fix (rather then the disabling of the
> feature) for F26 ?

I don't want to introduce new dependencies in a released version of GNOME, especially ones as cutting edge as the patches above (which haven't all landed, correct?).

Backporting to F26 is fine, if you can make sure that all the bits are in place.

Hi,

(In reply to Bastien Nocera from comment #46)
> (In reply to Hans de Goede from comment #45)
> > Not really gnome, but I feel it is better to discuss this here then in RH
> > bugzilla, given that F26 will thus have all the necessary bits, I think we
> > should consider backporting the proper fix (rather then the disabling of the
> > feature) for F26 ?
> 
> I don't want to introduce new dependencies in a released version of GNOME,

Understood.

> especially ones as cutting edge as the patches above (which haven't all
> landed, correct?).

The only thing not upstream yet are the xkeyboard-fixes, I've a Reviewed-by for those now, so I can push them, but in another mail you said you had a question about those so I'm holding of on pushing those for now.

> Backporting to F26 is fine, if you can make sure that all the bits are in place.

Ok, all the necessary bits are already in F26, so I will coordinaye backporting these with Benjamin and Kalev.

Regards,

Hans

*** Bug 771421 has been marked as a duplicate of this bug. ***

*** Bug 767696 has been marked as a duplicate of this bug. ***

seems like we're done here, closing