2016-03-11 10:45 GMT+01:00 Pali Rohár <pali.rohar@xxxxxxxxx>:
> On Thursday 07 January 2016 23:35:29 Pali Rohár wrote:
>> On Tuesday 22 December 2015 01:20:30 Rafael J. Wysocki wrote:
>> > On Monday, December 21, 2015 04:34:58 PM Gabriele Mazzotta 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.
>> >
>> > ACPI notifications are delivered asynchronously to drivers, so you'd
>> > need to flush kacpi_notify_wq in .resume(). That would make the driver
>> > wait for things it really doesn't need to wait for, so it would not be
>> > super-optimal.
>> >
>> > > 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.
>> >
>> > I'd try something like setting the flag from .suspend() and then adding
>> > a work item to clear it to kacpi_notify_wq from .resume(). Then you'll
>> > know that all of the pending notifications have been processed before
>> > your flag is cleared.
>> >
>> > That would require a new helper for adding work items to kacpi_notify_wq
>> > from drivers, but it shouldn't be too difficult to create one.
>> >
>> > Thanks,
>> > Rafael
>> >
>>
>> Hi all! Is there any progress or new version of this patch?
>>
>
> Gabriele, Darren, Alex... was this problem fixed? Or what is current state?
As far as I know, there was no progress. I'm now going to try what
Rafael suggested and see what I can do.
> --
> Pali Rohár
> pali.rohar@xxxxxxxxx
--
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
