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