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

[Darren Hart]

On Tue, May 24, 2016 at 09:09:38AM +0200, Pali RohÃr wrote:
> On Tuesday 24 May 2016 06:48:41 Andrei Borzenkov wrote:
> > 24.05.2016 02:03, Gabriele Mazzotta ÐÐÑÐÑ:
> > > On 24/05/2016 00:22, Pali RohÃr wrote:

...

> > > +#ifdef CONFIG_PM_SLEEP
> > > +static void ACPI_SYSTEM_XFACE rbtn_acpi_clear_flag(void *context)
> 
> I would rename this function to rbtn_clear_suspended_flag.
> 

...

> > > +	/*
> > > +	 * Upon resume, some BIOSes autonomously send an ACPI notification

You can drop "autonomously", it reads a bit awkwardly, and doesn't add any
information.

> > > +	 * that triggers an unwanted input event. In order to ignore it,
> > > +	 * we use a flag that we set at suspend and clear once we have
> > > +	 * received the extra notification. Since ACPI notifications are
> > > +	 * delivered asynchronously to drivers, we clear the flag from the
> > > +	 * workqueue used to deliver the notifications. This should be enough
> > > +	 * to guarantee that the flag is cleared only after we received the
> > > +	 * extra notification, if any.
> > > +	 */
> > 
> > "guarantee" is rather strong word here. We really do not know anything
> > how and when these notifications are generated by firmware, so can only
> > hope. But otherwise this explains what this patch intends to do (so that
> > even me finally understood it :)
> 
> Yes, thats better.
> 
> > > +	status = acpi_os_execute(OSL_NOTIFY_HANDLER,
> > > +			 rbtn_acpi_clear_flag, rbtn_data);
> > > +	if (ACPI_FAILURE(status))
> > > +		rbtn_data->suspended = false;
> 
> And here rbtn_clear_suspended_flag(rbtn_data) call instead direct
> assignment.
> 

I'm dropping this from the queue, and awaiting an updated version with the
requested changes (these from Pali, and the issue raised about "guarantee" being
too strong).

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center