Re: [PATCH v3 updated 3/3] ACPI / PMIC: AXP288: support virtual GPIO in ACPI table

From: Rafael J. Wysocki
Date: Tue Nov 25 2014 - 15:13:11 EST

Next message: Louis Langholtz: "PATCH: avoid possible integer overflow with cmp_range() in kernel/range.c"
Previous message: Rafael J. Wysocki: "Re: [PATCH 1/2] ASoC: rt5677: Add ACPI device probing"
In reply to: Aaron Lu: "Re: [PATCH v3 updated 3/3] ACPI / PMIC: AXP288: support virtual GPIO in ACPI table"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tuesday, November 25, 2014 08:01:38 PM Aaron Lu wrote:
> On 11/24/2014 11:19 PM, Rafael J. Wysocki wrote:
> > On Monday, November 24, 2014 05:32:33 PM Aaron Lu wrote:
> >> On 11/24/2014 09:06 AM, Rafael J. Wysocki wrote:
> >>> On Friday, November 21, 2014 03:11:51 PM Aaron Lu wrote:
> >>>> + if (!result) {
> >>>> + status = acpi_install_address_space_handler(
> >>>> + ACPI_HANDLE(parent), ACPI_ADR_SPACE_GPIO,
> >>>> + intel_xpower_pmic_gpio_handler, NULL, NULL);
> >>>
> >>> So here we have a problem, because we can't unregister the opregion handler
> >>> registered above if this fails. Not nice.
> >>
> >> I'm not correct in the cover letter, the actual problem with operation
> >> region remove is with module unload, it happens like this:
> >>
> >> The acpi_remove_address_space_handler doesn't wait for the current
> >> running handler to return, so if we call
> >> acpi_remove_address_space_handler in a module's exit function, the
> >> handler's memory will be freed and the running handler will access to
> >> a no longer valid memory place.
> >>
> >> So as long as we can make sure the handler will not go away from the
> >> memory, we are safe.
> >
> > This only means that address space handlers cannot be removed from kernel
> > modules.
>
> If the module can not be unloaded(no module exit call), then we should
> be safe.
>
> >
> > Lv was trying to add a wrapper for that some time ago, maybe it's a good
> > idea to revive that?
>
> Is it this one? If it is, I'll test it and then add the unload
> functionality to the PMIC drivers.

Well, you need to wait for the patch below to be applied to upstream ACPICA
and transfered to Linux from there.

> From: Lv Zheng <lv.zheng@xxxxxxxxx>
> Date: Tue, 25 Nov 2014 15:42:44 +0800
> Subject: [PATCH] ACPICA: Events: Add invocation protection for operation region callbacks.
>
> This patch adds invocation protection around operation region callbacks to
> offer a module safe environment for OSPM provided address space handlers.
>
> It is likely that OSPM where ModuleDevice is supported will implement
> specific address space handlers in the modules. Thus the unloading of
> the handlers' owner modules may lead to program crash around the invocation
> if the handler callbacks are invoked without protection. Since the
> handler callbacks are invoked inside of ACPICA, it is better to implement
> invocation protection inside of ACPICA.
> As address space handlers are expected to be executed in parallel, it is
> not a good choice to implement protection using locks. This patch uses
> reference counting based protection mechanism. When OSPM calls
> acpi_remove_address_space_handler(), the function waits until all invocations
> exit to ensure no invocation can happen after the unloading of the modules.
>
> Note that this might be a workaround and not tested, better solution could
> be implemented to tune the design of the namespace objects. Lv Zheng.
>
> Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx>
> ---
> drivers/acpi/acpica/acevents.h | 9 ++++
> drivers/acpi/acpica/acobject.h | 1 +
> drivers/acpi/acpica/evhandler.c | 117 ++++++++++++++++++++++++++++++++++++++++
> drivers/acpi/acpica/evregion.c | 11 +++-
> drivers/acpi/acpica/evxfregn.c | 9 ++++
> include/acpi/actypes.h | 2 +
> 6 files changed, 147 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/acpica/acevents.h b/drivers/acpi/acpica/acevents.h
> index 7a7811a9fc26..3020ac4ab7a8 100644
> --- a/drivers/acpi/acpica/acevents.h
> +++ b/drivers/acpi/acpica/acevents.h
> @@ -175,6 +175,15 @@ acpi_ev_install_space_handler(struct acpi_namespace_node *node,
> acpi_adr_space_handler handler,
> acpi_adr_space_setup setup, void *context);
>
> +acpi_status
> +acpi_ev_get_space_handler(union acpi_operand_object *handler_desc);
> +
> +void acpi_ev_put_space_handler(union acpi_operand_object *handler_desc);
> +
> +void acpi_ev_flush_space_handler(union acpi_operand_object *handler_desc);
> +
> +s16 acpi_ev_space_handler_count(union acpi_operand_object *handler_desc);
> +
> /*
> * evregion - Operation region support
> */
> diff --git a/drivers/acpi/acpica/acobject.h b/drivers/acpi/acpica/acobject.h
> index 8abb393dafab..a719d9733e6b 100644
> --- a/drivers/acpi/acpica/acobject.h
> +++ b/drivers/acpi/acpica/acobject.h
> @@ -304,6 +304,7 @@ struct acpi_object_notify_handler {
>
> struct acpi_object_addr_handler {
> ACPI_OBJECT_COMMON_HEADER u8 space_id;
> + s16 invocation_count;
> u8 handler_flags;
> acpi_adr_space_handler handler;
> struct acpi_namespace_node *node; /* Parent device */
> diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c
> index 78ac29351c9e..b27cc8e0507f 100644
> --- a/drivers/acpi/acpica/evhandler.c
> +++ b/drivers/acpi/acpica/evhandler.c
> @@ -499,6 +499,7 @@ acpi_ev_install_space_handler(struct acpi_namespace_node * node,
> handler_obj->address_space.space_id = (u8)space_id;
> handler_obj->address_space.handler_flags = flags;
> handler_obj->address_space.region_list = NULL;
> + handler_obj->address_space.invocation_count = 0;
> handler_obj->address_space.node = node;
> handler_obj->address_space.handler = handler;
> handler_obj->address_space.context = context;
> @@ -534,3 +535,119 @@ acpi_ev_install_space_handler(struct acpi_namespace_node * node,
> unlock_and_exit:
> return_ACPI_STATUS(status);
> }
> +
> +/*******************************************************************************
> + *
> + * FUNCTION: acpi_ev_flush_space_handler
> + *
> + * PARAMETERS: handler_desc - Address space object
> + * (ACPI_TYPE_LOCAL_ADDRESS_HANDLER)
> + *
> + * RETURN: None.
> + *
> + * DESCRIPTION: Flush the reference of the given address space handler object.
> + *
> + ******************************************************************************/
> +
> +void acpi_ev_flush_space_handler(union acpi_operand_object *handler_desc)
> +{
> + acpi_cpu_flags lock_flags;
> +
> + ACPI_FUNCTION_TRACE_PTR(acpi_ev_flush_space_handler, handler_desc);
> +
> + if (handler_desc && handler_desc->address_space.invocation_count >= 0) {
> + lock_flags =
> + acpi_os_acquire_lock(acpi_gbl_reference_count_lock);
> + handler_desc->address_space.invocation_count |= ACPI_INT16_MIN;
> + acpi_os_release_lock(acpi_gbl_reference_count_lock, lock_flags);
> + }
> +
> + return_VOID;
> +}
> +
> +/*******************************************************************************
> + *
> + * FUNCTION: acpi_ev_get_space_handler
> + *
> + * PARAMETERS: handler_desc - Address space object
> + * (ACPI_TYPE_LOCAL_ADDRESS_HANDLER)
> + *
> + * RETURN: Status.
> + *
> + * DESCRIPTION: Acquire an reference of the given address space handler object.
> + *
> + ******************************************************************************/
> +
> +acpi_status acpi_ev_get_space_handler(union acpi_operand_object *handler_desc)
> +{
> + acpi_cpu_flags lock_flags;
> +
> + ACPI_FUNCTION_TRACE_PTR(acpi_ev_get_space_handler, handler_desc);
> +
> + if (handler_desc && handler_desc->address_space.invocation_count >= 0) {
> + lock_flags =
> + acpi_os_acquire_lock(acpi_gbl_reference_count_lock);
> + handler_desc->address_space.invocation_count++;
> + acpi_os_release_lock(acpi_gbl_reference_count_lock, lock_flags);
> + return_ACPI_STATUS(AE_OK);
> + }
> +
> + return_ACPI_STATUS(AE_ERROR);
> +}
> +
> +/*******************************************************************************
> + *
> + * FUNCTION: acpi_ev_put_space_handler
> + *
> + * PARAMETERS: handler_desc - Address space object
> + * (ACPI_TYPE_LOCAL_ADDRESS_HANDLER)
> + *
> + * RETURN: None.
> + *
> + * DESCRIPTION: Release an reference of the given address space handler object.
> + *
> + ******************************************************************************/
> +
> +void acpi_ev_put_space_handler(union acpi_operand_object *handler_desc)
> +{
> + acpi_cpu_flags lock_flags;
> +
> + ACPI_FUNCTION_TRACE_PTR(acpi_ev_put_space_handler, handler_desc);
> +
> + if (handler_desc) {
> + lock_flags =
> + acpi_os_acquire_lock(acpi_gbl_reference_count_lock);
> + handler_desc->address_space.invocation_count--;
> + acpi_os_release_lock(acpi_gbl_reference_count_lock, lock_flags);
> + }
> +
> + return_VOID;
> +}
> +
> +/*******************************************************************************
> + *
> + * FUNCTION: acpi_ev_space_handler_count
> + *
> + * PARAMETERS: handler_desc - Address space object
> + * (ACPI_TYPE_LOCAL_ADDRESS_HANDLER)
> + *
> + * RETURN: Invocation count of the handler.
> + *
> + * DESCRIPTION: Get the reference of the given address space handler object.
> + *
> + ******************************************************************************/
> +
> +s16 acpi_ev_space_handler_count(union acpi_operand_object *handler_desc)
> +{
> + s16 count = 0;
> + acpi_cpu_flags lock_flags;
> +
> + if (handler_desc) {
> + lock_flags =
> + acpi_os_acquire_lock(acpi_gbl_reference_count_lock);
> + count = handler_desc->address_space.invocation_count;
> + acpi_os_release_lock(acpi_gbl_reference_count_lock, lock_flags);
> + }
> +
> + return (count);
> +}
> diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c
> index 8eb8575e8c16..dcdd779257d0 100644
> --- a/drivers/acpi/acpica/evregion.c
> +++ b/drivers/acpi/acpica/evregion.c
> @@ -165,6 +165,10 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
> return_ACPI_STATUS(AE_NOT_EXIST);
> }
>
> + status = acpi_ev_get_space_handler(handler_desc);
> + if (ACPI_FAILURE(status)) {
> + return_ACPI_STATUS(AE_NOT_EXIST);
> + }
> context = handler_desc->address_space.context;
>
> /*
> @@ -185,7 +189,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
> region_obj,
> acpi_ut_get_region_name(region_obj->region.
> space_id)));
> - return_ACPI_STATUS(AE_NOT_EXIST);
> + status = AE_NOT_EXIST;
> + goto error_exit;
> }
>
> /*
> @@ -210,7 +215,7 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
> acpi_ut_get_region_name(region_obj->
> region.
> space_id)));
> - return_ACPI_STATUS(status);
> + goto error_exit;
> }
>
> /* Region initialization may have been completed by region_setup */
> @@ -306,6 +311,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
> acpi_ex_enter_interpreter();
> }
>
> +error_exit:
> + acpi_ev_put_space_handler(handler_desc);
> return_ACPI_STATUS(status);
> }
>
> diff --git a/drivers/acpi/acpica/evxfregn.c b/drivers/acpi/acpica/evxfregn.c
> index 2d6f187939c7..c8c8538aae40 100644
> --- a/drivers/acpi/acpica/evxfregn.c
> +++ b/drivers/acpi/acpica/evxfregn.c
> @@ -266,6 +266,15 @@ acpi_remove_address_space_handler(acpi_handle device,
>
> *last_obj_ptr = handler_obj->address_space.next;
>
> + /* Wait for handlers to exit */
> +
> + acpi_ev_flush_space_handler(handler_obj);
> + while (acpi_ev_space_handler_count(handler_obj) != ACPI_INT16_MIN) {
> + (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
> + acpi_os_sleep((u64)10);
> + (void)acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
> + }
> +
> /* Now we can delete the handler object */
>
> acpi_ut_remove_reference(handler_obj);
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index 7000e66f768e..a341a9a8157f 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -65,6 +65,8 @@
> #define ACPI_UINT16_MAX (u16)(~((u16) 0)) /* 0xFFFF */
> #define ACPI_UINT32_MAX (u32)(~((u32) 0)) /* 0xFFFFFFFF */
> #define ACPI_UINT64_MAX (u64)(~((u64) 0)) /* 0xFFFFFFFFFFFFFFFF */
> +#define ACPI_INT16_MAX ((s16)(ACPI_UINT16_MAX>>1))
> +#define ACPI_INT16_MIN ((s16)(-ACPI_INT16_MAX - 1))
> #define ACPI_ASCII_MAX 0x7F
>
> /*
>

--
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Louis Langholtz: "PATCH: avoid possible integer overflow with cmp_range() in kernel/range.c"
Previous message: Rafael J. Wysocki: "Re: [PATCH 1/2] ASoC: rt5677: Add ACPI device probing"
In reply to: Aaron Lu: "Re: [PATCH v3 updated 3/3] ACPI / PMIC: AXP288: support virtual GPIO in ACPI table"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]