On Mon, Dec 21, 2015 at 11:34 PM, Gabriele Mazzotta
<gabriele.mzt@xxxxxxxxx> wrote:
> 2015-12-20 17:21 GMT+01:00 Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>:
>> On Friday, December 18, 2015 04:12:08 PM Darren Hart wrote:
>>> On Fri, Nov 20, 2015 at 03:44:25PM +0100, Pali Rohár wrote:
>>> > On Friday 23 October 2015 20:03:19 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").
>>> >
>>> > Gabriele, are you going to send this patch?
>>> >
>>> > I think that patch should be OK as it drop events when device is in
>>> > suspend state (when it should not receive events)...
>>> >
>>> > Darren, what do you think about it?
>>> >
>>>
>>> Sorry, this one has been difficult for me to track, but it's clearly an issue,
>>> and new systems are experiencing it as well.
>>>
>>> I'd like to get Rafael's opinion on disabling .notify ACPI function while
>>> suspended.
>>>
>>> +Rafael
>>
>> This by far wouldn't be enough, because drivers may install ACPI notify
>> handlers by themselves and those are hooked up directly into the ACPICA
>> code.
>>
>> Besides, some drivers may actually want to receive those events while
>> the system is suspending or resuming, so I think this has to be addressed
>> on a per-driver basis.
>
> Hi,
>
> sorry, but I'm not sure I understood everything, so I'll try to
> briefly describe the problem and its current solution.
>
> Currently dell-rbtn listens for the notifications sent to an ACPI
> device and for notification sends an input event to userspace.
>
> The problem we have is that some BIOSes send an extra notification
> on resume and therefore we send an extra input event.
>
> What we want to do is to ignore this ACPI notification without
> affecting the systems whose BIOS does not send this extra
> notification. We know that not all of them send this notification.
>
> What I noticed is that the extra notification is issued by the _WAK
> method and always arrives before dell-rbtn has been resumed, so
> what I did is to add a flag that is set by the suspend and resume
> callbacks of the device driver.
Sorry I screw up my mail filter and I wasn't aware of this thread until now.
BIOS sends this additional ACPI event for the systems with hardware
switch so a driver can update its state; therefore this is done only
once and therefore ignoring the ACPI event sent to dell rbtn once
after resume is sufficient.
I actually tried a solution similar to Gabriele's patch above (one
with rbtn_suspend and rbtn_resume) a while ago and it works fine.
If there is a conclusion and there is a patch to be tested, I am happy
to test it on wider range (I should be able to find 5+ Dell systems
that runs on dell-rbtn).
>
> What we were wondering is whether this would be enough or we
> need to do something different, e.g. ignore all the notifications that
> arrived X seconds after the execution of the resume callback.
>
> Thanks,
> Gabriele
>
>>> Has Dell been involved here?
>>
>> Not that I know of.
>>
>> Thanks,
>> Rafael
>>
--
Cheers,
Alex Hung
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
