Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Darren Hart <dvhart@infradead.org>]

On Fri, Oct 23, 2015 at 08:03:19PM +0200, Gabriele Mazzotta wrote:
> On 23/10/2015 13:14, Pali Rohár wrote:
> >On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
> >>>In my opinion it is better to ignore user key press after resume, if it
> >>>fix our problem. Better as false-positive event.
> >>
> >>The following appears to work really well. The notification arrives
> >>before rbtn_resume() has been executed, so the extra event is ignored.
> >>
> >>diff --git a/drivers/platform/x86/dell-rbtn.c
> >>b/drivers/platform/x86/dell-rbtn.c
> >>index cd410e3..1d64b72 100644
> >>--- a/drivers/platform/x86/dell-rbtn.c
> >>+++ b/drivers/platform/x86/dell-rbtn.c
> >>@@ -28,6 +28,7 @@ struct rbtn_data {
> >>  	enum rbtn_type type;
> >>  	struct rfkill *rfkill;
> >>  	struct input_dev *input_dev;
> >>+	bool suspended;
> >>  };
> >>
> >>
> >>@@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
> >>  	{ "", 0 },
> >>  };
> >>
> >>+#ifdef CONFIG_PM_SLEEP
> >>+static int rbtn_suspend(struct device *dev)
> >>+{
> >>+	struct acpi_device *device = to_acpi_device(dev);
> >>+	struct rbtn_data *rbtn_data = acpi_driver_data(device);
> >>+
> >>+	rbtn_data->suspended = true;
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static int rbtn_resume(struct device *dev)
> >>+{
> >>+	struct acpi_device *device = to_acpi_device(dev);
> >>+	struct rbtn_data *rbtn_data = acpi_driver_data(device);
> >>+
> >>+	rbtn_data->suspended = false;
> >>+
> >>+	return 0;
> >>+}
> >>+#endif
> >>+static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
> >>+
> >>  static struct acpi_driver rbtn_driver = {
> >>  	.name = "dell-rbtn",
> >>  	.ids = rbtn_ids,
> >>+	.drv.pm = &rbtn_pm_ops,
> >>  	.ops = {
> >>  		.add = rbtn_add,
> >>  		.remove = rbtn_remove,
> >>@@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
> >>event)
> >>  {
> >>  	struct rbtn_data *rbtn_data = device->driver_data;
> >>
> >>+	if (rbtn_data->suspended)
> >>+		return;
> >>+
> >>  	if (event != 0x80) {
> >>  		dev_info(&device->dev, "Received unknown event (0x%x)\n",
> >>  			 event);
> >>
> >
> >Great, but is not there a better way to turn off .notify ACPI function
> >when that ACPI device is suspended?
> >
> >Is not this ACPI device driver bug that it allows to call .notify method
> >even if device is suspended?
> 
> I was surprised this worked, I was assuming that nothing could run
> before the resume callback, but I was wrong. I think it makes sense to
> treat ACPI devices in a special way, but I really don't know, we need
> someone more knowledgeable to answer these questions. However, while I
> was trying to figure things out, I stumbled upon the following:
> e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend events").
> 

My understanding here though is that laptops with a Fn+RBTN key which changes
the state of the radio in firmware should be handled via the rfkill interface
rather than the inpu interface - so while this patch may appear to work, it's
using the input interface to copy the rfkill interface/state.

The proper solution, if I'm understanding this correctly - and apologies if not,
some of this is new territory for me as well - would be for this device to be
detected as firmware controlled (what we refer to as a SLIDER in the code -
which needs to be renamed IMHO).

Let's sort out this point, then we can pull in Rafael to make sure this is how
to best deal with the spurious event on resume.

-- 
Darren Hart
Intel Open Source Technology Center