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