Re: [PATCH v3] dell-rbtn: Ignore ACPI notifications if device is suspended

[Pali RohÃr]

On Monday 25 April 2016 22:06:11 Gabriele Mazzotta wrote:
> 2016-04-18 14:35 GMT+02:00 Pali RohÃr <pali.rohar@xxxxxxxxx>:
> > On Tuesday 29 March 2016 15:11:35 Rafael J. Wysocki wrote:
> >> On Monday, March 28, 2016 10:33:09 AM Darren Hart wrote:
> >> > On Thu, Mar 24, 2016 at 12:24:56PM +0100, Gabriele Mazzotta wrote:
> >> > > 2016-03-24 10:39 GMT+01:00 Pali RohÃr <pali.rohar@xxxxxxxxx>:
> >> > > > On Monday 21 March 2016 16:13:34 Gabriele Mazzotta wrote:
> >> > > >> 2016-03-21 13:17 GMT+01:00 Pali RohÃr <pali.rohar@xxxxxxxxx>:
> >> > > >> > On Friday 18 March 2016 23:44:23 Gabriele Mazzotta wrote:
> >> > > >> >> +#ifdef CONFIG_PM_SLEEP
> >> > > >> >> +static void ACPI_SYSTEM_XFACE rbtn_acpi_clear_flag(void *context)
> >> > > >> >> +{
> >> > > >> >> +     struct rbtn_data *rbtn_data = context;
> >> > > >> >> +
> >> > > >> >> +     rbtn_data->suspended = false;
> >> > > >> >> +}
> >> > > >> >> +
> >> > > >> >> +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);
> >> > > >> >> +     acpi_status status;
> >> > > >> >> +
> >> > > >> >> +     /*
> >> > > >> >> +      * Clear the flag only after we received the extra
> >> > > >> >> +      * ACPI notification.
> >> > > >> >> +      */
> >> > > >> >> +     status = acpi_os_execute(OSL_NOTIFY_HANDLER,
> >> > > >> >> +                      rbtn_acpi_clear_flag, rbtn_data);
> >> > > >> >> +     if (ACPI_FAILURE(status))
> >> > > >> >> +             rbtn_data->suspended = false;
> >> > > >> >
> >> > > >> > I case when acpi_os_execute success it calls rbtn_acpi_clear_flag,
> >> > > >> > right? And that will set suspended to false. When acpi_os_execute fails,
> >> > > >> > then it set suspended too to false... Then whole acpi_os_execute doing
> >> > > >> > just "barrier" after which suspended flag can be set to false. So I
> >> > > >> > think rbtn_acpi_clear_flag function is not needed here.
> >> > > >> >
> >> > > >> > Cannot you pass NULL or empty function pointer as callback? Or what was
> >> > > >> > reason to do that flag clearing at "two places"?
> >> > > >>
> >> > > >> acpi_os_execute doesn't wait for the callback to be executed, so
> >> > > >> I can't clear the flag from rbtn_resume.
> >> > > >
> >> > > > acpi_os_execute calls callback asynchronously later? Or what exactly do it?
> >> > >
> >> > > In this case, it adds the callback to the kacpi_notify_wq workqueue
> >> > > for deferred execution.
> >> >
> >> > +Rafael for context/advice on the use of acpi_os_execute here.
> >> >
> >> > This is true, but a quick scan through that call path doesn't tell me why we
> >> > would need to call it here instead of just setting rbtn_data->suspended = false.
> >> > The comment suggests waiting for the event, but is that what this is doing? It
> >> > appears to me to be immediately scheduling the function to a work queue, not
> >> > waiting for the event notifier.
> >>
> >> I think this is supposed to work as a barrier.  That is, it will only run after
> >> all events in the queue have been processed.
> >>
> >> I'm not sure if that's necessary, though.
> >>
> >> Thanks,
> >> Rafael
> >>
> >
> > Darren, Gabriele, what is state of this patch? Bug is not still fixed,
> > right?
> 
> Yes, the bug is still there and this patch fixes it.
> 
> Just to make it clear, we need the barrier. Andrei could reproduce
> the bug without it [1], but not with it, as he confirmed in this
> thread [2].
> 
> [1] http://article.gmane.org/gmane.linux.drivers.platform.x86.devel/8001
> [2] http://article.gmane.org/gmane.linux.drivers.platform.x86.devel/8937

Ok, so it means that somebody (who understand ACPI) should review code
and accept it or show what is needed to fix. Plus maybe adds more
comments how that "barrier" works as I was first confused...

Darren, Rafael, can you do review of this patch?

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx