| Message ID | 54746FA2.9030408@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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@intel.com> > 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@intel.com> > --- > 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 > > /* >
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 /*