[PATCH 1/2] acpi: Provide interface for driver notification on method call

[PATCH 1/2] acpi: Provide interface for driver notification on method call

[Matthew Garrett]

It's sometimes useful for a driver to receive notifications in response
to an ACPI event even if there's no explicit notification in the bytecode.
Add an interface to allow drivers to register for callbacks when a given
method is executed.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/acpica/acobject.h |    2 +
 drivers/acpi/acpica/dsmethod.c |    5 ++
 drivers/acpi/acpica/evxface.c  |  103 ++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpixf.h          |    7 +++
 include/acpi/actypes.h         |    2 +
 5 files changed, 119 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/acpica/acobject.h b/drivers/acpi/acpica/acobject.h
index cde18ea..b46b23c 100644
--- a/drivers/acpi/acpica/acobject.h
+++ b/drivers/acpi/acpica/acobject.h
@@ -188,6 +188,8 @@ struct acpi_object_method {
 	u32 aml_length;
 	u8 thread_count;
 	acpi_owner_id owner_id;
+	acpi_osd_exec_callback notify;
+	void *context;
 };
 
 /******************************************************************************
diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c
index 7210392..a453cda 100644
--- a/drivers/acpi/acpica/dsmethod.c
+++ b/drivers/acpi/acpica/dsmethod.c
@@ -420,6 +420,11 @@ acpi_ds_call_control_method(struct acpi_thread_state *thread,
 		}
 	}
 
+	if (obj_desc->method.notify) {
+		acpi_os_execute(OSL_NOTIFY_HANDLER, obj_desc->method.notify,
+				obj_desc->method.context);
+	}
+
 	return_ACPI_STATUS(status);
 
       cleanup:
diff --git a/drivers/acpi/acpica/evxface.c b/drivers/acpi/acpica/evxface.c
index b407579..af81832 100644
--- a/drivers/acpi/acpica/evxface.c
+++ b/drivers/acpi/acpica/evxface.c
@@ -653,6 +653,109 @@ 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 (acpi_handle handle, acpi_method_handler handler,
+			     void *context)
+{
+	struct acpi_namespace_node *node;
+	union acpi_operand_object *method;
+	acpi_status status;
+
+	if (!handle || !handler)
+		return_ACPI_STATUS(AE_BAD_PARAMETER);
+
+	status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
+
+	if (ACPI_FAILURE(status))
+		return_ACPI_STATUS(status);
+
+	node = acpi_ns_validate_handle(handle);
+
+	if (!node) {
+		status = AE_BAD_PARAMETER;
+		goto unlock_and_exit;
+	}
+
+	method = acpi_ns_get_attached_object(node);
+
+	if (method->method.notify) {
+		status = AE_ALREADY_EXISTS;
+		goto unlock_and_exit;
+	}
+
+	method->method.notify = handler;
+	method->method.context = context;
+
+unlock_and_exit:
+	acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
+	return_ACPI_STATUS(status);
+}
+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 (acpi_handle handle, acpi_method_handler handler)
+{
+	struct acpi_namespace_node *node;
+	union acpi_operand_object *method;
+	acpi_status status;
+
+	if (!handle || !handler)
+		return_ACPI_STATUS(AE_BAD_PARAMETER);
+
+	status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
+	if (ACPI_FAILURE(status))
+		return_ACPI_STATUS(status);
+
+	node = acpi_ns_validate_handle(handle);
+
+	if (!node) {
+		status = AE_BAD_PARAMETER;
+		goto unlock_and_exit;
+	}
+
+	method = acpi_ns_get_attached_object(node);
+
+	if (method->method.notify != handler) {
+		status = AE_BAD_PARAMETER;
+		goto unlock_and_exit;
+	}
+
+	method->method.notify = NULL;
+	method->method.context = NULL;
+
+unlock_and_exit:
+	acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
+	return_ACPI_STATUS(status);
+}
+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/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 4447a04..21e7646 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -240,6 +240,13 @@ acpi_remove_notify_handler(acpi_handle device,
 			   u32 handler_type, acpi_notify_handler handler);
 
 acpi_status
+acpi_install_method_handler(acpi_handle handle, acpi_method_handler handler,
+			    void *context);
+
+acpi_status
+acpi_remove_method_handler(acpi_handle handle, 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 3f08e64..bf2fcf8 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -916,6 +916,8 @@ void (*acpi_notify_handler) (acpi_handle device, u32 value, void *context);
 typedef
 void (*acpi_object_handler) (acpi_handle object, 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
-- 
1.7.0.1

[PATCH 2/2] thinkpad_acpi: Hook volume events

[Matthew Garrett]

Not all Thinkpads generate events for volume hotkeys, so hook into the
CMOS update and generate events from there without polling. This should
let Pulseaudio do something sensible with the mute state.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 63290b3..c8c4f2b 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3083,6 +3083,15 @@ static const struct tpacpi_quirk tpacpi_hotkey_qtable[] __initconst = {
 	TPACPI_Q_IBM('1', 'D', TPACPI_HK_Q_INIMASK), /* X22, X23, X24 */
 };
 
+static void volume_alsa_notify_change(void);
+static int volume_check_status(void);
+
+static void cmos_notifier(void *context)
+{
+	if (volume_check_status())
+		volume_alsa_notify_change();
+}
+
 static int __init hotkey_init(struct ibm_init_struct *iibm)
 {
 	/* Requirements for changing the default keymaps:
@@ -4939,11 +4948,16 @@ static int __init cmos_init(struct ibm_init_struct *iibm)
 	if (res)
 		return res;
 
+	if (cmos_handle)
+		acpi_install_method_handler(cmos_handle, cmos_notifier,
+					    iibm->data);
+
 	return (cmos_handle)? 0 : 1;
 }
 
 static void cmos_exit(void)
 {
+	acpi_remove_method_handler(cmos_handle, cmos_notifier);
 	device_remove_file(&tpacpi_pdev->dev, &dev_attr_cmos_command);
 }
 
@@ -6458,6 +6472,7 @@ static enum tpacpi_volume_access_mode volume_mode =
 
 static enum tpacpi_volume_capabilities volume_capabilities;
 static int volume_control_allowed;
+static u8 volume_state;
 
 /*
  * Used to syncronize writers to TP_EC_AUDIO and
@@ -6544,6 +6559,21 @@ static int volume_set_status(const u8 status)
 	return volume_set_status_ec(status);
 }
 
+static int volume_check_status()
+{
+	u8 new_state;
+	int ret = 0;
+
+	volume_get_status(&new_state);
+
+	if (new_state != volume_state) {
+		ret = 1;
+		volume_state = new_state;
+	}
+
+	return ret;
+}
+
 /* returns < 0 on error, 0 on no change, 1 on change */
 static int __volume_set_mute_ec(const bool mute)
 {
@@ -6931,6 +6961,8 @@ static int __init volume_init(struct ibm_init_struct *iibm)
 		return rc;
 	}
 
+	volume_get_status(&volume_state);
+
 	printk(TPACPI_INFO
 		"Console audio control enabled, mode: %s\n",
 		(volume_control_allowed) ?
-- 
1.7.0.1

Re: [PATCH 2/2] thinkpad_acpi: Hook volume events

[Henrique de Moraes Holschuh]

On Mon, 03 May 2010, Matthew Garrett wrote:
> Not all Thinkpads generate events for volume hotkeys, so hook into the
> CMOS update and generate events from there without polling. This should
> let Pulseaudio do something sensible with the mute state.

The idea is sound, but it would be probably cleaner to do it only on those
thinkpads which need it.  I don't like the idea of handling duplicate
notifications, and the logic depends entirely on nothing messing with the
mixer state behind our backs, I'd prefer to rely on it just when strictly
required.

Also, UCMS is called for a number of reasons.  It would be best to filter
based on the UCMS parameter before doing an expensive PC CMOS RAM read or EC
read.  We will be dealing with a finite number of thinkpads anyway.

On the same note, we need to be called *after* the UCMS method completes,
otherwise, we will have to schedule delayed work.  I didn't study your ACPI
patch enough to know whether this happens, but that detail is not clear from
the description.

-- 
  "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: [PATCH 2/2] thinkpad_acpi: Hook volume events

[Matthew Garrett]

On Wed, May 05, 2010 at 08:30:05AM -0300, Henrique de Moraes Holschuh wrote:

> The idea is sound, but it would be probably cleaner to do it only on those
> thinkpads which need it.  I don't like the idea of handling duplicate
> notifications, and the logic depends entirely on nothing messing with the
> mixer state behind our backs, I'd prefer to rely on it just when strictly
> required.

In this case the notification is purely at the ALSA level, so there's no 
harm caused by duplicate notifications.

> Also, UCMS is called for a number of reasons.  It would be best to filter
> based on the UCMS parameter before doing an expensive PC CMOS RAM read or EC
> read.  We will be dealing with a finite number of thinkpads anyway.

Passing the parameter is... difficult. That would involve rather more 
surgery to the ACPI core, since the parameters are core-internal objects 
at this point. It's an obvious feature though, so I'll look into it some 
more.

> On the same note, we need to be called *after* the UCMS method completes,
> otherwise, we will have to schedule delayed work.  I didn't study your ACPI
> patch enough to know whether this happens, but that detail is not clear from
> the description.

The call is made after executing the original method, yes.

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

Re: [PATCH 2/2] thinkpad_acpi: Hook volume events

[Henrique de Moraes Holschuh]

On Wed, 05 May 2010, Matthew Garrett wrote:
> On Wed, May 05, 2010 at 08:30:05AM -0300, Henrique de Moraes Holschuh wrote:
> > The idea is sound, but it would be probably cleaner to do it only on those
> > thinkpads which need it.  I don't like the idea of handling duplicate
> > notifications, and the logic depends entirely on nothing messing with the
> > mixer state behind our backs, I'd prefer to rely on it just when strictly
> > required.
> 
> In this case the notification is purely at the ALSA level, so there's no 
> harm caused by duplicate notifications.

Reading that stuff ain't cheap.  Either a locked trip through the LPC bus
for the CMOS nvram, or ACPI EC I/O (which is even worse, I fear.  That EC is
also hooked to LPC, and there's a lot more code involved).

I really would rather not enable it on boxes where it isn't required.  And
we know which ones are those: IBM thinkpads where the all-available-hotkeys
mask has the bits for the volume hotkeys set.

> > Also, UCMS is called for a number of reasons.  It would be best to filter
> > based on the UCMS parameter before doing an expensive PC CMOS RAM read or EC
> > read.  We will be dealing with a finite number of thinkpads anyway.
> 
> Passing the parameter is... difficult. That would involve rather more 
> surgery to the ACPI core, since the parameters are core-internal objects 
> at this point. It's an obvious feature though, so I'll look into it some 
> more.

It would be very very useful, though.

-- 
  "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: [PATCH 2/2] thinkpad_acpi: Hook volume events

[Matthew Garrett]

On Thu, May 06, 2010 at 12:02:39AM -0300, Henrique de Moraes Holschuh wrote:

> Reading that stuff ain't cheap.  Either a locked trip through the LPC bus
> for the CMOS nvram, or ACPI EC I/O (which is even worse, I fear.  That EC is
> also hooked to LPC, and there's a lot more code involved).
> 
> I really would rather not enable it on boxes where it isn't required.  And
> we know which ones are those: IBM thinkpads where the all-available-hotkeys
> mask has the bits for the volume hotkeys set.

Ok, that's easy enough to do. I'll rework and send that.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

------------------------------------------------------------------------------