[RFC] Allow notifications on method execution

[RFC] Allow notifications on method execution

[Matthew Garrett]

Various bits of hardware (some Thinkpads, for example) will execute ACPI 
methods in response to events but won't generate any notifications. It 
would be nice to be able to receive notifications on those updates in 
order to avoid polling. How about something like the following?

diff --git a/drivers/acpi/events/evxface.c b/drivers/acpi/events/evxface.c
index 94a6efe..00a6378 100644
--- a/drivers/acpi/events/evxface.c
+++ b/drivers/acpi/events/evxface.c
@@ -549,6 +549,78 @@ ACPI_EXPORT_SYMBOL(acpi_remove_notify_handler)
 
 /*******************************************************************************
  *
+ * FUNCTION:    acpi_install_method_handler
+ *
+ * PARAMETERS:  Oathname        - The method for which notifies will be handled
+ *              Handler         - Address of the handler
+ *              Context         - Value passed to the handler on each call
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Install a handler for notifies on ACPI method execution
+ *
+ ******************************************************************************/
+
+acpi_status
+acpi_install_method_handler (char *pathname, acpi_method_handler handler,
+			     void *context)
+{
+	struct acpi_namespace_node *node;
+	union acpi_operand_object *method;
+
+	acpi_ns_get_node(ACPI_NS_ALL, pathname, 0, &node);
+	if (!node)
+		return_ACPI_STATUS(AE_BAD_PARAMETER);
+
+	method = acpi_ns_get_attached_object(node);
+
+	if (method->method.notify)
+		return_ACPI_STATUS(AE_ALREADY_EXISTS);
+
+	method->method.notify = handler;
+	method->method.context = context;
+
+	return_ACPI_STATUS(AE_OK);
+}
+ACPI_EXPORT_SYMBOL(acpi_install_method_handler)
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_remove_method_handler
+ *
+ * PARAMETERS:  Pathname        - The method for which notifies will be handled
+ *              Handler         - Address of the handler
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Remove a handler for notifies on ACPI method execution
+ *
+ ******************************************************************************/
+
+acpi_status
+acpi_remove_method_handler (char *pathname, acpi_method_handler handler)
+{
+	struct acpi_namespace_node *node;
+	union acpi_operand_object *method;
+
+	acpi_ns_get_node(ACPI_NS_ALL, pathname, 0, &node);
+	if (!node)
+		return_ACPI_STATUS(AE_BAD_PARAMETER);
+
+	method = acpi_ns_get_attached_object(node);
+
+	if (method->method.notify != handler)
+		return_ACPI_STATUS(AE_BAD_PARAMETER);
+
+	method->method.notify = NULL;
+	method->method.context = NULL;
+
+	return_ACPI_STATUS(AE_OK);
+}
+ACPI_EXPORT_SYMBOL(acpi_remove_method_handler)
+
+/*******************************************************************************
+ *
  * FUNCTION:    acpi_install_gpe_handler
  *
  * PARAMETERS:  gpe_device      - Namespace node for the GPE (NULL for FADT
diff --git a/drivers/acpi/parser/psparse.c b/drivers/acpi/parser/psparse.c
index 15e1702..018c929 100644
--- a/drivers/acpi/parser/psparse.c
+++ b/drivers/acpi/parser/psparse.c
@@ -445,6 +445,7 @@ acpi_status acpi_ps_parse_aml(struct acpi_walk_state *walk_state)
 	struct acpi_thread_state *thread;
 	struct acpi_thread_state *prev_walk_list = acpi_gbl_current_walk_list;
 	struct acpi_walk_state *previous_walk_state;
+	struct acpi_namespace_node *method_node;
 
 	ACPI_FUNCTION_TRACE(ps_parse_aml);
 
@@ -488,6 +489,15 @@ acpi_status acpi_ps_parse_aml(struct acpi_walk_state *walk_state)
 
 	status = AE_OK;
 	while (walk_state) {
+		method_node = walk_state->method_call_node;
+
+		if (method_node) {
+			union acpi_operand_object *method =
+				acpi_ns_get_attached_object(method_node);
+			if (method->method.notify)
+				method->method.notify(method->method.context);
+		}
+
 		if (ACPI_SUCCESS(status)) {
 			/*
 			 * The parse_loop executes AML until the method terminates
diff --git a/include/acpi/acobject.h b/include/acpi/acobject.h
index e9657da..86f6e59 100644
--- a/include/acpi/acobject.h
+++ b/include/acpi/acobject.h
@@ -183,6 +183,8 @@ struct acpi_object_method {
 	u32 aml_length;
 	u8 thread_count;
 	acpi_owner_id owner_id;
+	acpi_method_handler notify;
+	void *context;
 };
 
 /******************************************************************************
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 2c3806e..3ee976a 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -212,6 +212,13 @@ acpi_remove_notify_handler(acpi_handle device,
 			   u32 handler_type, acpi_notify_handler handler);
 
 acpi_status
+acpi_install_method_handler(char *pathname, acpi_method_handler handler,
+			    void *context);
+
+acpi_status
+acpi_remove_method_handler(char *pathname, acpi_method_handler handler);
+
+acpi_status
 acpi_install_address_space_handler(acpi_handle device,
 				   acpi_adr_space_type space_id,
 				   acpi_adr_space_handler handler,
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index dfea2d4..e67bc42 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -768,6 +768,9 @@ void (*acpi_notify_handler) (acpi_handle device, u32 value, void *context);
 typedef
 void (*acpi_object_handler) (acpi_handle object, u32 function, void *data);
 
+typedef
+void (*acpi_method_handler) (void *context);
+
 typedef acpi_status(*acpi_init_handler) (acpi_handle object, u32 function);
 
 #define ACPI_INIT_DEVICE_INI        1


-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC] Allow notifications on method execution

[Henrique de Moraes Holschuh]

On Fri, 20 Jun 2008, Matthew Garrett wrote:
> Various bits of hardware (some Thinkpads, for example) will execute ACPI 
> methods in response to events but won't generate any notifications. It 
> would be nice to be able to receive notifications on those updates in 
> order to avoid polling. How about something like the following?

No comment on the code itself, but I have some about the whole idea.  If I
missed the point, please explain it to me.

Let me start pointing out that I am not really opposed to your idea, but I'd
like to know if it is the best way to go about it, first.

With such ACPI notifies, you could have HAL do some of the stuff kernel
drivers are doing.  So far so good (well, sort of).

But that is NOT necessarily the best, or even the correct way to do it.  You
could use ACPI "traps" sending events to userspace to know brightness
changed, but the proper way to have those events would be for the backlight
class to send uevents for such stuff, instead.

Remember that if ACPI core sends too much data to userspace directly, we
can't easily fix stuff in the drivers to present a more or less sanitized
view of things to userspace.

So I'd really like to know what you need it for?  What sort of hardware
events you'd like to be notified that thinkpad-acpi is not notifying you
about, for example?

Avoiding polling is ALWAYS a good thing.  But being careful about what you
export to userspace is ALWAYS wise as well.  I believe we can do both.

Even if it means we need to go around fixing half-completed sysfs classes
until they're good enough an API for us and userspace.  rfkill is a prime
example of the effort that might be needed.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [RFC] Allow notifications on method execution

[Matthew Garrett]

On Fri, Jun 20, 2008 at 04:39:41PM -0300, Henrique de Moraes Holschuh wrote:

> With such ACPI notifies, you could have HAL do some of the stuff kernel
> drivers are doing.  So far so good (well, sort of).

That's not what I was thinking. Right now, older Thinkpads need polling 
if you want to get some keyboard events. However, the BIOS seems to 
impleemnt this by calling the ACPI CMOS update methods. Rather than 
polling, we can therefore make it event driven. The events won't go out 
to userspace, they'll just call a callback in the driver which then 
generates input events or uevents or whatever.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC] Allow notifications on method execution

[Henrique de Moraes Holschuh]

On Fri, 20 Jun 2008, Matthew Garrett wrote:
> On Fri, Jun 20, 2008 at 04:39:41PM -0300, Henrique de Moraes Holschuh wrote:
> > With such ACPI notifies, you could have HAL do some of the stuff kernel
> > drivers are doing.  So far so good (well, sort of).
> 
> That's not what I was thinking. Right now, older Thinkpads need polling 
> if you want to get some keyboard events. However, the BIOS seems to 
> impleemnt this by calling the ACPI CMOS update methods. Rather than 
> polling, we can therefore make it event driven. The events won't go out 
> to userspace, they'll just call a callback in the driver which then 
> generates input events or uevents or whatever.

Sounds like a good idea, but in order to do that, we just need to let
kernel drivers hook to GPE handlers somehow.  I'd be quite happy to add
support for it to thinkpad-acpi.

I thought your patch did a lot more than this.  If I am wrong about it,
I apologise.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [RFC] Allow notifications on method execution

[Matthew Garrett]

On Fri, Jun 20, 2008 at 07:01:20PM -0300, Henrique de Moraes Holschuh wrote:

> Sounds like a good idea, but in order to do that, we just need to let
> kernel drivers hook to GPE handlers somehow.  I'd be quite happy to add
> support for it to thinkpad-acpi.

I expected that the method name would probably be more consistent, 
whereas the GPE number might change in a way that was difficult to hook. 
There's only three CMOS update methods in the range of Thinkpad BIOSes, 
IIRC?

> I thought your patch did a lot more than this.  If I am wrong about it,
> I apologise.

No problem :)

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC] Allow notifications on method execution

[Henrique de Moraes Holschuh]

On Fri, 20 Jun 2008, Matthew Garrett wrote:
> On Fri, Jun 20, 2008 at 07:01:20PM -0300, Henrique de Moraes Holschuh wrote:
> > Sounds like a good idea, but in order to do that, we just need to let
> > kernel drivers hook to GPE handlers somehow.  I'd be quite happy to add
> > support for it to thinkpad-acpi.
> 
> I expected that the method name would probably be more consistent, 
> whereas the GPE number might change in a way that was difficult to hook. 
> There's only three CMOS update methods in the range of Thinkpad BIOSes, 
> IIRC?

True.  Well, both would work, and I'd have to add this as a whitelist
anyway (which is NOT a problem, as we are talking about old thinkpads
anyway, and there is a finite, non-extremely-huge number of those).

It works marginally better if one traps the GPE, as that looks a lot
more like what one gets in a modern thinkpad instead of what one gets
from the NVRAM (look at the nvram polling code in thinkpad-acpi to
understand what I mean.  Some not so pretty crap was required in there
due to deal with loss of information in the VOLUME/MUTE handling...)

Hotfixing the AML would also allow one to do all of this and more (maybe
hell will freeze over and Len will let us do it :-) ).

Anyway, if we add notifications for normal nodes, please add them for
GPEs too.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [RFC] Allow notifications on method execution

[Matthew Garrett]

On Fri, Jun 20, 2008 at 07:33:44PM -0300, Henrique de Moraes Holschuh wrote:

> It works marginally better if one traps the GPE, as that looks a lot
> more like what one gets in a modern thinkpad instead of what one gets
> from the NVRAM (look at the nvram polling code in thinkpad-acpi to
> understand what I mean.  Some not so pretty crap was required in there
> due to deal with loss of information in the VOLUME/MUTE handling...)

Heh. Yeah, that's true.

> Anyway, if we add notifications for normal nodes, please add them for
> GPEs too.

I'll look into that. With luck it'd let us remove polling from some of 
the other laptop hotkey drivers too.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC] Allow notifications on method execution

[Zhao Yakui]

On Fri, 2008-06-20 at 16:22 +0100, Matthew Garrett wrote:
> Various bits of hardware (some Thinkpads, for example) will execute ACPI 
> methods in response to events but won't generate any notifications. It 
> would be nice to be able to receive notifications on those updates in 
> order to avoid polling. How about something like the following?
Very good idea. From the description the notification can be received
when some ACPI methods are executed. 
But it touches the ACPICA source code. Maybe it is uneasy to accept. 
And I have a question about this.
  Asynchronous or synchronous notification event? It seems that
synchronous notification event is used in the following. If there is too
much to do in the notification function, maybe some mutex will be
blocked for too much time, which will have impact on the ACPI
performance.
 
  Thanks.
> diff --git a/drivers/acpi/events/evxface.c b/drivers/acpi/events/evxface.c
> index 94a6efe..00a6378 100644
> --- a/drivers/acpi/events/evxface.c
> +++ b/drivers/acpi/events/evxface.c
> @@ -549,6 +549,78 @@ ACPI_EXPORT_SYMBOL(acpi_remove_notify_handler)
>  
>  /*******************************************************************************
>   *
> + * FUNCTION:    acpi_install_method_handler
> + *
> + * PARAMETERS:  Oathname        - The method for which notifies will be handled
> + *              Handler         - Address of the handler
> + *              Context         - Value passed to the handler on each call
> + *
> + * RETURN:      Status
> + *
> + * DESCRIPTION: Install a handler for notifies on ACPI method execution
> + *
> + ******************************************************************************/
> +
> +acpi_status
> +acpi_install_method_handler (char *pathname, acpi_method_handler handler,
> +			     void *context)
> +{
> +	struct acpi_namespace_node *node;
> +	union acpi_operand_object *method;
> +
> +	acpi_ns_get_node(ACPI_NS_ALL, pathname, 0, &node);
> +	if (!node)
> +		return_ACPI_STATUS(AE_BAD_PARAMETER);
> +
> +	method = acpi_ns_get_attached_object(node);
> +
> +	if (method->method.notify)
> +		return_ACPI_STATUS(AE_ALREADY_EXISTS);
> +
> +	method->method.notify = handler;
> +	method->method.context = context;
> +
> +	return_ACPI_STATUS(AE_OK);
> +}
> +ACPI_EXPORT_SYMBOL(acpi_install_method_handler)
> +
> +/*******************************************************************************
> + *
> + * FUNCTION:    acpi_remove_method_handler
> + *
> + * PARAMETERS:  Pathname        - The method for which notifies will be handled
> + *              Handler         - Address of the handler
> + *
> + * RETURN:      Status
> + *
> + * DESCRIPTION: Remove a handler for notifies on ACPI method execution
> + *
> + ******************************************************************************/
> +
> +acpi_status
> +acpi_remove_method_handler (char *pathname, acpi_method_handler handler)
> +{
> +	struct acpi_namespace_node *node;
> +	union acpi_operand_object *method;
> +
> +	acpi_ns_get_node(ACPI_NS_ALL, pathname, 0, &node);
> +	if (!node)
> +		return_ACPI_STATUS(AE_BAD_PARAMETER);
> +
> +	method = acpi_ns_get_attached_object(node);
> +
> +	if (method->method.notify != handler)
> +		return_ACPI_STATUS(AE_BAD_PARAMETER);
> +
> +	method->method.notify = NULL;
> +	method->method.context = NULL;
> +
> +	return_ACPI_STATUS(AE_OK);
> +}
> +ACPI_EXPORT_SYMBOL(acpi_remove_method_handler)
> +
> +/*******************************************************************************
> + *
>   * FUNCTION:    acpi_install_gpe_handler
>   *
>   * PARAMETERS:  gpe_device      - Namespace node for the GPE (NULL for FADT
> diff --git a/drivers/acpi/parser/psparse.c b/drivers/acpi/parser/psparse.c
> index 15e1702..018c929 100644
> --- a/drivers/acpi/parser/psparse.c
> +++ b/drivers/acpi/parser/psparse.c
> @@ -445,6 +445,7 @@ acpi_status acpi_ps_parse_aml(struct acpi_walk_state *walk_state)
>  	struct acpi_thread_state *thread;
>  	struct acpi_thread_state *prev_walk_list = acpi_gbl_current_walk_list;
>  	struct acpi_walk_state *previous_walk_state;
> +	struct acpi_namespace_node *method_node;
>  
>  	ACPI_FUNCTION_TRACE(ps_parse_aml);
>  
> @@ -488,6 +489,15 @@ acpi_status acpi_ps_parse_aml(struct acpi_walk_state *walk_state)
>  
>  	status = AE_OK;
>  	while (walk_state) {
> +		method_node = walk_state->method_call_node;
> +
> +		if (method_node) {
> +			union acpi_operand_object *method =
> +				acpi_ns_get_attached_object(method_node);
> +			if (method->method.notify)
> +				method->method.notify(method->method.context);
> +		}
> +
>  		if (ACPI_SUCCESS(status)) {
>  			/*
>  			 * The parse_loop executes AML until the method terminates
> diff --git a/include/acpi/acobject.h b/include/acpi/acobject.h
> index e9657da..86f6e59 100644
> --- a/include/acpi/acobject.h
> +++ b/include/acpi/acobject.h
> @@ -183,6 +183,8 @@ struct acpi_object_method {
>  	u32 aml_length;
>  	u8 thread_count;
>  	acpi_owner_id owner_id;
> +	acpi_method_handler notify;
> +	void *context;
>  };
>  
>  /******************************************************************************
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index 2c3806e..3ee976a 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -212,6 +212,13 @@ acpi_remove_notify_handler(acpi_handle device,
>  			   u32 handler_type, acpi_notify_handler handler);
>  
>  acpi_status
> +acpi_install_method_handler(char *pathname, acpi_method_handler handler,
> +			    void *context);
> +
> +acpi_status
> +acpi_remove_method_handler(char *pathname, acpi_method_handler handler);
> +
> +acpi_status
>  acpi_install_address_space_handler(acpi_handle device,
>  				   acpi_adr_space_type space_id,
>  				   acpi_adr_space_handler handler,
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index dfea2d4..e67bc42 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -768,6 +768,9 @@ void (*acpi_notify_handler) (acpi_handle device, u32 value, void *context);
>  typedef
>  void (*acpi_object_handler) (acpi_handle object, u32 function, void *data);
>  
> +typedef
> +void (*acpi_method_handler) (void *context);
> +
>  typedef acpi_status(*acpi_init_handler) (acpi_handle object, u32 function);
>  
>  #define ACPI_INIT_DEVICE_INI        1
> 
> 

Re: [RFC] Allow notifications on method execution

[Henrique de Moraes Holschuh]

On Mon, 23 Jun 2008, Zhao Yakui wrote:
> On Fri, 2008-06-20 at 16:22 +0100, Matthew Garrett wrote:
> > Various bits of hardware (some Thinkpads, for example) will execute ACPI 
> > methods in response to events but won't generate any notifications. It 
> > would be nice to be able to receive notifications on those updates in 
> > order to avoid polling. How about something like the following?
> Very good idea. From the description the notification can be received
> when some ACPI methods are executed. 
> But it touches the ACPICA source code. Maybe it is uneasy to accept. 
> And I have a question about this.
>   Asynchronous or synchronous notification event? It seems that

I'd do fine with an asynchronous blocking or SCRU (blocking) notify
chain for thinkpad-acpi.  No need for synchronous operation, I'd not be
using this API to hotpatch the AML or anything of the sort.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [RFC] Allow notifications on method execution

[Henrique de Moraes Holschuh]

On Mon, 23 Jun 2008, Henrique de Moraes Holschuh wrote:
> On Mon, 23 Jun 2008, Zhao Yakui wrote:
> > On Fri, 2008-06-20 at 16:22 +0100, Matthew Garrett wrote:
> > > Various bits of hardware (some Thinkpads, for example) will execute ACPI 
> > > methods in response to events but won't generate any notifications. It 
> > > would be nice to be able to receive notifications on those updates in 
> > > order to avoid polling. How about something like the following?
> > Very good idea. From the description the notification can be received
> > when some ACPI methods are executed. 
> > But it touches the ACPICA source code. Maybe it is uneasy to accept. 
> > And I have a question about this.
> >   Asynchronous or synchronous notification event? It seems that
> 
> I'd do fine with an asynchronous blocking or SCRU (blocking) notify
> chain for thinkpad-acpi.  No need for synchronous operation, I'd not be
> using this API to hotpatch the AML or anything of the sort.

But I *really* do think you don't want a table-wide notify chain, something
more fine-grained is better, or it will be a performance hog if too many
drivers use it.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh