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
--
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
