After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 760517 - rfkill: disable rfkill-input when we have the seat
rfkill: disable rfkill-input when we have the seat
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: rfkill
3.19.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
: 767696 771421 (view as bug list)
Depends on: 781505
Blocks:
 
 
Reported: 2016-01-12 11:46 UTC by Bastien Nocera
Modified: 2017-05-24 14:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Inhibit kernel handling of RFKILL key events (7.77 KB, patch)
2017-03-31 16:05 UTC, Benjamin Berg
none Details | Review
Inhibit kernel handling of RFKILL key events (8.04 KB, patch)
2017-03-31 17:40 UTC, Benjamin Berg
none Details | Review
Alright, here is a new patchset which retains the OSD and also the way that (5.38 KB, patch)
2017-04-19 15:45 UTC, Benjamin Berg
needs-work Details | Review
rfkill: Expose API to inhibit kernel handling of input events (4.62 KB, patch)
2017-04-19 15:45 UTC, Benjamin Berg
needs-work Details | Review
rfkill: Add ToggleAirplaneMode method call which returns the new state (14.94 KB, patch)
2017-04-19 15:45 UTC, Benjamin Berg
reviewed Details | Review
media-keys: Use the new rfkill toggle action to prevent race conditions (5.37 KB, patch)
2017-04-19 15:45 UTC, Benjamin Berg
reviewed Details | Review
media-keys: Disable rfkill keys handling (1.64 KB, patch)
2017-05-04 14:34 UTC, Bastien Nocera
committed Details | Review
rfkill: Add property to Rfkill helper to inhibit kernel handling (5.68 KB, patch)
2017-05-04 14:53 UTC, Bastien Nocera
committed Details | Review
rfkill: Inhibit rfkill-input's rfkill keys handling (2.23 KB, patch)
2017-05-04 15:09 UTC, Bastien Nocera
committed Details | Review

Description Bastien Nocera 2016-01-12 11:46:37 UTC
If CONFIG_RFKILL_INPUT is enabled, the rfkill core in the kernel will handle the keypresses, and stop us from receiving them, and acting up on them.

We should disable the in-kernel feature by doing an ioctl of RFKILL_IOC_NOINPUT when we have the seat.

There however doesn't seem to be a way to re-enable it when we lose the seat.
Comment 1 Bastien Nocera 2016-04-14 14:53:48 UTC
Moving to rfkill component.
Comment 2 Benjamin Berg 2017-03-31 16:05:26 UTC
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.
Comment 3 Benjamin Berg 2017-03-31 17:40:19 UTC
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.
Comment 4 Benjamin Berg 2017-03-31 19:11:13 UTC
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
Comment 5 Bastien Nocera 2017-04-03 09:43:15 UTC
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.
Comment 6 Benjamin Berg 2017-04-03 13:28:35 UTC
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?
Comment 7 Bastien Nocera 2017-04-03 13:36:59 UTC
(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.
Comment 8 Benjamin Berg 2017-04-03 14:24:32 UTC
(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?
Comment 9 Bastien Nocera 2017-04-04 08:46:29 UTC
(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.
Comment 10 Benjamin Berg 2017-04-04 09:48:14 UTC
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.
Comment 11 Bastien Nocera 2017-04-04 10:03:35 UTC
(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
Comment 12 Benjamin Berg 2017-04-04 18:46:27 UTC
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.
Comment 13 Benjamin Berg 2017-04-19 15:45:08 UTC
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.
Comment 14 Benjamin Berg 2017-04-19 15:45:29 UTC
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.
Comment 15 Benjamin Berg 2017-04-19 15:45:47 UTC
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.
Comment 16 Benjamin Berg 2017-04-19 15:45:59 UTC
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.
Comment 17 Hans de Goede 2017-05-01 13:06:21 UTC
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
Comment 18 Bastien Nocera 2017-05-04 11:40:16 UTC
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 :)
Comment 19 Bastien Nocera 2017-05-04 11:45:15 UTC
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.
Comment 20 Bastien Nocera 2017-05-04 11:50:18 UTC
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...
Comment 21 Bastien Nocera 2017-05-04 11:52:19 UTC
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.
Comment 22 Bastien Nocera 2017-05-04 12:32:06 UTC
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.
Comment 23 Benjamin Berg 2017-05-04 12:59:03 UTC
(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.
Comment 24 Bastien Nocera 2017-05-04 13:01:31 UTC
(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.
Comment 25 Benjamin Berg 2017-05-04 13:07:07 UTC
That OSD is nice to have and easy to keep with the workaround to not hit either of the other two regressions.
Comment 26 Benjamin Berg 2017-05-04 13:26:43 UTC
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)
Comment 27 Hans de Goede 2017-05-04 14:27:05 UTC
(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.
Comment 28 Bastien Nocera 2017-05-04 14:34:45 UTC
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 29 Bastien Nocera 2017-05-04 14:37:06 UTC
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.
Comment 30 Bastien Nocera 2017-05-04 14:53:56 UTC
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 31 Bastien Nocera 2017-05-04 14:54:30 UTC
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
Comment 32 Bastien Nocera 2017-05-04 15:09:10 UTC
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.
Comment 33 Bastien Nocera 2017-05-04 15:09:52 UTC
Attachment 351060 [details] pushed as ccceec4 - rfkill: Inhibit rfkill-input's rfkill keys handling
Comment 34 Bastien Nocera 2017-05-04 15:17:07 UTC
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.
Comment 35 Hans de Goede 2017-05-04 15:17:59 UTC
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.
Comment 36 Bastien Nocera 2017-05-04 15:22:55 UTC
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.
Comment 37 Hans de Goede 2017-05-04 15:54:43 UTC
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
Comment 38 Bastien Nocera 2017-05-04 16:10:48 UTC
(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.
Comment 39 Hans de Goede 2017-05-04 17:14:14 UTC
(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.
Comment 40 Peter Hutterer 2017-05-04 21:48:32 UTC
(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
Comment 41 Benjamin Berg 2017-05-05 11:58:42 UTC
(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.
Comment 42 Hans de Goede 2017-05-10 15:03:43 UTC
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
Comment 43 Hans de Goede 2017-05-12 14:24:22 UTC
> 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.
Comment 44 Hans de Goede 2017-05-12 14:28:42 UTC
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.
Comment 45 Hans de Goede 2017-05-12 14:30:01 UTC
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 ?
Comment 46 Bastien Nocera 2017-05-15 05:53:49 UTC
(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.
Comment 47 Hans de Goede 2017-05-15 06:43:12 UTC
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
Comment 48 Rui Matos 2017-05-19 09:00:19 UTC
*** Bug 771421 has been marked as a duplicate of this bug. ***
Comment 49 Rui Matos 2017-05-19 09:02:38 UTC
*** Bug 767696 has been marked as a duplicate of this bug. ***
Comment 50 Rui Matos 2017-05-24 14:04:23 UTC
seems like we're done here, closing