Message-ID: <1691764.Wi9xRTfTU8@vostro.rjw.lan>
Date:	Sat, 15 Nov 2014 00:34:05 +0100
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Lv Zheng <lv.zheng@...el.com>
Cc:	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Len Brown <len.brown@...el.com>, Lv Zheng <zetalog@...il.com>,
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
	Len Brown <lenb@...nel.org>,
	Robert Moore <robert.moore@...el.com>,
	Corey Minyard <minyard@....org>, devel@...ica.org,
	openipmi-developer@...ts.sourceforge.net
Subject: Re: [PATCH] ACPI/OSL: Add IRQ handler flushing support in the OSL.

On Wednesday, November 05, 2014 03:06:13 PM Lv Zheng wrote:
> It is possible that a GPE handler or a fixed event handler still accessed
> after removing the handlers by invoking acpi_remove_gpe_handler() or
> acpi_remove_fixed_event_handler(), this possibility can crash OPSM after a
> module removal. In the Linux kernel, though all other GPE drivers are not
> modules, since the IPMI_SI (ipmi_si_intf.c) can be compiled as a module, we
> still need to consider a solution for this issue when the driver switches
> to ACPI_GPE_RAW_HANDLER mode in order to invoke GPE APIs.
> 
> ACPICA expects acpi_os_wait_events_complete() to be invoked after GPE
> disabling so that OSPM can ensure all running GPE handlers have exitted.
> But currently acpi_os_wait_events_complete() can only flush _Lxx/_Exx
> evaluation work queue and this philosophy cannot work for drivers that have
> installed a dedicated GPE handler.
> 
> The only way to protect a callback is to perform some state holders
> (reference count, state machine) before invoking the callback. Then this
> issue can only be fixed by the following means:
> 1. Flush GPE in ACPICA before invoking the GPE handler. But currently,
>    there is no such implementation in acpi_ev_gpe_dispatch().
> 2. Flush GPE in ACPICA OSL before invoking the SCI handler. But currently,
>    there is no such implementation in acpi_irq().
> 3. Flush IRQ in OSPM IRQ layer before invoking the IRQ handler. In Linus
>    kernel, this can be done by synchronize_irq().
> 4. Flush scheduling in OSPM vector entry layer before invoking the vector.
>    In Linux, this can be done by synchronize_sched().
> 
> Since ACPICA expects the GPE handlers to be flushed by the ACPICA OSL or
> the GPE drivers. If it is implemented by the GPE driver, we should see
> synchronize_irq()/synchronize_sched() invoked in such drivers. If it is
> implemented by the ACPICA OSL, ACPICA currently provides
> acpi_os_wait_events_complete() hook to achieve this. After the following
> commit:
>   Commit: 69c841b6dd8313c9a673246cc0e2535174272cab
>   Author: Lv Zheng <lv.zheng@...el.com>
>   Subject: ACPICA: Update use of acpi_os_wait_events_complete interface.
> The OSL acpi_os_wait_events_complete() is invoked after a GPE handler is
> removed from acpi_remove_gpe_handler() or a fixed event handler is removed
> from acpi_remove_fixed_event_handler(). Thus it is possible to implement
> GPE handler flushing using this ACPICA OSL now. So the solution 1 is
> currently not taken into account.
> 
> By examining the IPMI_SI driver, we noticed that the IPMI_SI driver:
> 1. Uses free_irq() to flush non GPE based IRQ handlers, in free_irq(),
>    synchronize_irq() is invoked, and
> 2. Uses acpi_remove_gpe_handler() to flush GPE based IRQ handlers, for such
>    IRQ handlers, there is no synchronize_irq() invoked.
> Since there isn't synchronize_sched() implemented for this driver, from the
> driver's perspective, acpi_remove_gpe_handler() should have properly
> flushed the GPE handlers for it. Since the driver doesn't invoke
> synchronize_irq(), the solution 3 is not what the drivers expect.
> 
> This patch implements solution 2. But since given the fact that the GPE is
> managed inside of ACPICA, and implementing the GPE flushing requires to
> implement the whole GPE management code again in the OSL, instead of
> flushing GPE, this patch flushes IRQ in acpi_os_wait_events_complete(). The
> flushing could last longer than expected as though the target GPE/fixed
> event that is removed can be fastly flushed, other GPEs/fix events can still
> be issued during the flushing period.
> 
> This patch fixes this issue by invoking synchronize_hardirq() in
> acpi_os_wait_events_complete(). The reason why we don't invoke
> synchronize_irq() is: currently ACPICA is not threaded IRQ capable and the
> only difference between synchronize_irq() and synchronize_hardirq() is
> synchronize_irq() also flushes threaded IRQ handlers. Thus using
> synchronize_hardirq() can help to reduce the overall synchronization time
> for the current ACPICA implementation.
> 
> Signed-off-by: Lv Zheng <lv.zheng@...el.com>
> Cc: Rafael J. Wysocki <rjw@...ysocki.net>
> Cc: Len Brown <lenb@...nel.org>
> Cc: Robert Moore <robert.moore@...el.com>
> Cc: Corey Minyard <minyard@....org>
> Cc: linux-acpi@...r.kernel.org
> Cc: devel@...ica.org
> Cc: openipmi-developer@...ts.sourceforge.net

Queued up for 3.19, thanks!

> ---
>  drivers/acpi/osl.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 9964f70..2524196 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -1188,6 +1188,12 @@ EXPORT_SYMBOL(acpi_os_execute);
>  
>  void acpi_os_wait_events_complete(void)
>  {
> +	/*
> +	 * Make sure the GPE handler or the fixed event handler is not used
> +	 * on another CPU after removal.
> +	 */
> +	if (acpi_irq_handler)
> +		synchronize_hardirq(acpi_gbl_FADT.sci_interrupt);
>  	flush_workqueue(kacpid_wq);
>  	flush_workqueue(kacpi_notify_wq);
>  }
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists