[PATCH 0/2] Deliver ACPI events upon subscription and implement multiple readers for /proc/acpi/event

[PATCH 0/2] Deliver ACPI events upon subscription and implement multiple readers for /proc/acpi/event

[Mattia Dongili]

Hello.

This patch series revamps an old submission[1] from Dmitry Torokhov that
adds support for multiple readers on /proc/acpi/event.

Due to the code reorganization happened in the last 1.5 years I
re-implemented it a little differently introducing a subscription
mechanism to receive events (it might be used in different context
also). driver/acpi/event.c uses it to register a new listener for each
process opening the /proc/acpi/event file.

I was also wondering if this could fit the recent discussion on
"Untangling the sleep hotkey mess"[2] to allow event deliver through
the input subsystem (yes, a proposed solution was the other way around
but this might still be a viable implementation).

[1]: http://marc.theaimsgroup.com/?l=acpi4linux&m=109333066714671&w=2
[2]: sorry, can't find the beginning of the thread
     http://marc.theaimsgroup.com/?l=linux-acpi&m=113676899930391&w=2

Dmitry, I don't know how to apply the signed-off thing correctly here,
in the meantime:

Signed-off-by: Mattia Dongili <malattia@linux.it>

[PATCH 1/2] Implement ACPI event delivery upon subscription.

[Mattia Dongili]

[PATCH 1/2] Implement ACPI event delivery upon subscription.

Drivers can register their listener to receive events instead of
using a single event queue.
Limit the number of pending events to 32 so if a reader is stuck
it won't consume all memory.

Signed-off-by: Mattia Dongili <malattia@linux.it>


---

 drivers/acpi/bus.c      |   92 ++++++++++++++++++++---------------------------
 include/acpi/acpi_bus.h |   10 +++++
 2 files changed, 50 insertions(+), 52 deletions(-)

0933a109f7da68a80a9a4137c486db17f8bfb9b6
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 606f873..c4d5412 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -280,91 +280,79 @@ EXPORT_SYMBOL(acpi_bus_set_power);
    -------------------------------------------------------------------------- */
 
 static DEFINE_SPINLOCK(acpi_bus_event_lock);
-
-LIST_HEAD(acpi_bus_event_list);
+LIST_HEAD(acpi_event_listener_list);
 DECLARE_WAIT_QUEUE_HEAD(acpi_bus_event_queue);
 
-extern int event_is_open;
-
-int acpi_bus_generate_event(struct acpi_device *device, u8 type, int data)
+int acpi_bus_register_event_listener(struct acpi_event_listener *listener)
 {
-	struct acpi_bus_event *event = NULL;
 	unsigned long flags = 0;
 
-	ACPI_FUNCTION_TRACE("acpi_bus_generate_event");
+	ACPI_FUNCTION_TRACE("acpi_bus_add_listener");
 
-	if (!device)
+	if (!listener)
 		return_VALUE(-EINVAL);
 
-	/* drop event on the floor if no one's listening */
-	if (!event_is_open)
-		return_VALUE(0);
-
-	event = kmalloc(sizeof(struct acpi_bus_event), GFP_ATOMIC);
-	if (!event)
-		return_VALUE(-ENOMEM);
-
-	strcpy(event->device_class, device->pnp.device_class);
-	strcpy(event->bus_id, device->pnp.bus_id);
-	event->type = type;
-	event->data = data;
-
 	spin_lock_irqsave(&acpi_bus_event_lock, flags);
-	list_add_tail(&event->node, &acpi_bus_event_list);
+	list_add_tail(&listener->node, &acpi_event_listener_list);
 	spin_unlock_irqrestore(&acpi_bus_event_lock, flags);
 
-	wake_up_interruptible(&acpi_bus_event_queue);
-
 	return_VALUE(0);
 }
 
-EXPORT_SYMBOL(acpi_bus_generate_event);
+EXPORT_SYMBOL(acpi_bus_register_event_listener);
 
-int acpi_bus_receive_event(struct acpi_bus_event *event)
+int acpi_bus_unregister_event_listener(struct acpi_event_listener *listener)
 {
 	unsigned long flags = 0;
-	struct acpi_bus_event *entry = NULL;
 
-	DECLARE_WAITQUEUE(wait, current);
+	ACPI_FUNCTION_TRACE("acpi_bus_remove_listener");
 
-	ACPI_FUNCTION_TRACE("acpi_bus_receive_event");
-
-	if (!event)
+	if (!listener)
 		return_VALUE(-EINVAL);
 
-	if (list_empty(&acpi_bus_event_list)) {
+	spin_lock_irqsave(&acpi_bus_event_lock, flags);
+	list_del(&listener->node);
+	spin_unlock_irqrestore(&acpi_bus_event_lock, flags);
 
-		set_current_state(TASK_INTERRUPTIBLE);
-		add_wait_queue(&acpi_bus_event_queue, &wait);
+	return_VALUE(0);
+}
 
-		if (list_empty(&acpi_bus_event_list))
-			schedule();
+EXPORT_SYMBOL(acpi_bus_unregister_event_listener);
 
-		remove_wait_queue(&acpi_bus_event_queue, &wait);
-		set_current_state(TASK_RUNNING);
+int acpi_bus_generate_event(struct acpi_device *device, u8 type, int data)
+{
+	struct acpi_bus_event *event;
+	struct acpi_event_listener *listener = NULL;
+	unsigned long flags = 0;
 
-		if (signal_pending(current))
-			return_VALUE(-ERESTARTSYS);
-	}
+	ACPI_FUNCTION_TRACE("acpi_bus_generate_event");
 
-	spin_lock_irqsave(&acpi_bus_event_lock, flags);
-	entry =
-	    list_entry(acpi_bus_event_list.next, struct acpi_bus_event, node);
-	if (entry)
-		list_del(&entry->node);
-	spin_unlock_irqrestore(&acpi_bus_event_lock, flags);
+	if (!device)
+		return_VALUE(-EINVAL);
 
-	if (!entry)
-		return_VALUE(-ENODEV);
+	spin_lock_irqsave(&acpi_bus_event_lock, flags);
+	/* drop event on the floor if no one's listening */
+	if (list_empty(&acpi_event_listener_list)) {
+		spin_unlock_irqrestore(&acpi_bus_event_lock, flags);
+		return_VALUE(0);
+	}
 
-	memcpy(event, entry, sizeof(struct acpi_bus_event));
+	list_for_each_entry(listener, &acpi_event_listener_list, node) {
+		event = &listener->buffer[listener->head];
+		listener->head = (listener->head + 1) & (ACPI_EVENT_BUFFER_SIZE - 1);
+		strcpy(event->device_class, device->pnp.device_class);
+		strcpy(event->bus_id, device->pnp.bus_id);
+		event->type = type;
+		event->data = data;
+	}
+	spin_unlock_irqrestore(&acpi_bus_event_lock, flags);
 
-	kfree(entry);
+	wake_up_interruptible(&acpi_bus_event_queue);
 
 	return_VALUE(0);
 }
 
-EXPORT_SYMBOL(acpi_bus_receive_event);
+EXPORT_SYMBOL(acpi_bus_generate_event);
 
 /* --------------------------------------------------------------------------
                              Notification Handling
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 0b54e9a..4ae8d07 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -313,6 +313,14 @@ struct acpi_bus_event {
 	u32 data;
 };
 
+#define ACPI_EVENT_BUFFER_SIZE 32
+struct acpi_event_listener {
+	struct acpi_bus_event	buffer[ACPI_EVENT_BUFFER_SIZE];
+	int			head;
+	int			tail;
+	struct list_head	node;
+};
+
 extern struct subsystem acpi_subsys;
 
 /*
@@ -324,6 +332,8 @@ void acpi_bus_data_handler(acpi_handle h
 int acpi_bus_get_status(struct acpi_device *device);
 int acpi_bus_get_power(acpi_handle handle, int *state);
 int acpi_bus_set_power(acpi_handle handle, int state);
+int acpi_bus_register_event_listener(struct acpi_event_listener *listener);
+int acpi_bus_unregister_event_listener(struct acpi_event_listener *listener);
 int acpi_bus_generate_event(struct acpi_device *device, u8 type, int data);
 int acpi_bus_receive_event(struct acpi_bus_event *event);
 int acpi_bus_register_driver(struct acpi_driver *driver);
-- 
1.1.GIT

[PATCH 2/2] Support multiple readers for /proc/acpi/event.

[Mattia Dongili]

[PATCH 2/2] Support multiple readers for /proc/acpi/event.

Each reader registers itself as an ACPI Event listener to get
notified of events.

Signed-off-by: Mattia Dongili <malattia@linux.it>


---

 drivers/acpi/event.c |   83 +++++++++++++++++++++++++++-----------------------
 1 files changed, 45 insertions(+), 38 deletions(-)

05c7c439c5c09f3f690144e79824b24329121e91
diff --git a/drivers/acpi/event.c b/drivers/acpi/event.c
index 2dbb1b0..ffb6835 100644
--- a/drivers/acpi/event.c
+++ b/drivers/acpi/event.c
@@ -15,87 +15,93 @@
 #define _COMPONENT		ACPI_SYSTEM_COMPONENT
 ACPI_MODULE_NAME("event")
 
+struct acpi_event_reader {
+	char	str[ACPI_MAX_STRING];
+	int	chars_remaining;
+	char	*ptr;
+	struct	acpi_event_listener listener;
+};
+
 /* Global vars for handling event proc entry */
-static DEFINE_SPINLOCK(acpi_system_event_lock);
-int event_is_open = 0;
-extern struct list_head acpi_bus_event_list;
 extern wait_queue_head_t acpi_bus_event_queue;
 
 static int acpi_system_open_event(struct inode *inode, struct file *file)
 {
-	spin_lock_irq(&acpi_system_event_lock);
+	struct acpi_event_reader *reader =
+		kzalloc(sizeof(struct acpi_event_reader), GFP_KERNEL);
 
-	if (event_is_open)
-		goto out_busy;
+	if (!reader)
+		return -ENOMEM;
 
-	event_is_open = 1;
+	acpi_bus_register_event_listener(&reader->listener);
+	file->private_data = reader;
 
-	spin_unlock_irq(&acpi_system_event_lock);
 	return 0;
-
-      out_busy:
-	spin_unlock_irq(&acpi_system_event_lock);
-	return -EBUSY;
 }
 
 static ssize_t
 acpi_system_read_event(struct file *file, char __user * buffer, size_t count,
 		       loff_t * ppos)
 {
-	int result = 0;
-	struct acpi_bus_event event;
-	static char str[ACPI_MAX_STRING];
-	static int chars_remaining = 0;
-	static char *ptr;
+	int				result;
+	struct acpi_bus_event		*event;
+	struct acpi_event_reader	*reader = file->private_data;
+	struct acpi_event_listener	*listener = &reader->listener;
 
 	ACPI_FUNCTION_TRACE("acpi_system_read_event");
 
-	if (!chars_remaining) {
-		memset(&event, 0, sizeof(struct acpi_bus_event));
+	if (!reader->chars_remaining) {
 
-		if ((file->f_flags & O_NONBLOCK)
-		    && (list_empty(&acpi_bus_event_list)))
+		if (listener->head == listener->tail &&
+				(file->f_flags & O_NONBLOCK)) {
 			return_VALUE(-EAGAIN);
+		}
 
-		result = acpi_bus_receive_event(&event);
+ 		result = wait_event_interruptible(acpi_bus_event_queue,
+						  listener->head != listener->tail);
 		if (result)
 			return_VALUE(result);
 
-		chars_remaining = sprintf(str, "%s %s %08x %08x\n",
-					  event.device_class ? event.
-					  device_class : "<unknown>",
-					  event.bus_id ? event.
-					  bus_id : "<unknown>", event.type,
-					  event.data);
-		ptr = str;
+		event = &listener->buffer[listener->tail];
+		listener->tail = (listener->tail + 1) & (ACPI_EVENT_BUFFER_SIZE - 1);
+
+		reader->chars_remaining = sprintf(reader->str, "%s %s %08x %08x\n",
+					  event->device_class ?
+						event->device_class : "<unknown>",
+					  event->bus_id ?  event->bus_id : "<unknown>",
+					  event->type, event->data);
+		reader->ptr = reader->str;
 	}
 
-	if (chars_remaining < count) {
-		count = chars_remaining;
+	if (reader->chars_remaining < count) {
+		count = reader->chars_remaining;
 	}
 
-	if (copy_to_user(buffer, ptr, count))
+	if (copy_to_user(buffer, reader->ptr, count))
 		return_VALUE(-EFAULT);
 
 	*ppos += count;
-	chars_remaining -= count;
-	ptr += count;
+	reader->chars_remaining -= count;
+	reader->ptr += count;
 
 	return_VALUE(count);
 }
 
 static int acpi_system_close_event(struct inode *inode, struct file *file)
 {
-	spin_lock_irq(&acpi_system_event_lock);
-	event_is_open = 0;
-	spin_unlock_irq(&acpi_system_event_lock);
+	struct acpi_event_reader *reader = file->private_data;
+
+	if (!acpi_bus_unregister_event_listener(&reader->listener))
+		kfree(reader);
+
 	return 0;
 }
 
 static unsigned int acpi_system_poll_event(struct file *file, poll_table * wait)
 {
+	struct acpi_event_reader *reader = file->private_data;
 	poll_wait(file, &acpi_bus_event_queue, wait);
-	if (!list_empty(&acpi_bus_event_list))
+	if (reader->listener.head != reader->listener.tail)
 		return POLLIN | POLLRDNORM;
 	return 0;
 }
@@ -131,3 +137,4 @@ static int __init acpi_event_init(void)
 }
 
 subsys_initcall(acpi_event_init);
+
-- 
1.1.GIT

RE: [PATCH 0/2] Deliver ACPI events upon subscription and implement multiple readers for /proc/acpi/event

[Brown, Len]

What's the problem with opening a socket to the user-space acpid --
the way multiple readers work today?

thanks,
-Len

Re: [PATCH 0/2] Deliver ACPI events upon subscription and implement multiple readers for /proc/acpi/event

[Dmitry Torokhov]

On Tuesday 24 January 2006 20:17, Brown, Len wrote:
> What's the problem with opening a socket to the user-space acpid --
> the way multiple readers work today?
>

- one does not want to use current implementation of acpid?
- one does not want to depend on having acpid running before
  starting snooping acpi events?
- because allowing multiple readers is a "right thing to do"?

... just to name a few advantages.

-- 
Dmitry

Re: [PATCH 0/2] Deliver ACPI events upon subscription and implement multiple readers for /proc/acpi/event

[Mattia Dongili]

On Wed, January 25, 2006 2:46 am, Dmitry Torokhov said:
> On Tuesday 24 January 2006 20:17, Brown, Len wrote:
>> What's the problem with opening a socket to the user-space acpid --
>> the way multiple readers work today?
>>
>
> - one does not want to use current implementation of acpid?
> - one does not want to depend on having acpid running before
>   starting snooping acpi events?
> - because allowing multiple readers is a "right thing to do"?

One more:
- because acpid disappearing and reappearing at random times causes
useless complexity and troubles to its socket readers?

-- 
mattia
:wq!

RE: [PATCH 0/2] Deliver ACPI events upon subscription and implement multiple readers for /proc/acpi/event

[Brown, Len]

 
>On Tuesday 24 January 2006 20:17, Brown, Len wrote:
>> What's the problem with opening a socket to the user-space acpid --
>> the way multiple readers work today?
>>
>
>- one does not want to use current implementation of acpid?

What is better?

>- one does not want to depend on having acpid running before
>  starting snooping acpi events?

For example?

>- because allowing multiple readers is a "right thing to do"?

This is not self-evident, can you give some examples?

The argument against, I suppose, is simply why to add code
to the kernel when the same feature is already working
in user-space?

The other argument against is why enhance an interface when
perhaps we should instead consider replacing it altogether...

>> Dmitry, I don't know how to apply the signed-off thing correctly here,

If Dmitry is the original author, then the proper format is to put

From: Dmitry Torokhov <dtor_core@ameritech.net>

at start of the body of the message where it can over-ride the e-mail author.
This is documented in Documentation/SubmittingPatches.

thanks,
-Len

ps. I'm not trying to be difficult about this, but would like to
give this (recurring) proposal the attention it deserves and either
accept it or reject it once and for all.

Re: [PATCH 0/2] Deliver ACPI events upon subscription and implement multiple readers for /proc/acpi/event

[Dmitry Torokhov]

On Wednesday 25 January 2006 00:09, Brown, Len wrote:
> 
> >On Tuesday 24 January 2006 20:17, Brown, Len wrote:
> >> What's the problem with opening a socket to the user-space acpid --
> >> the way multiple readers work today?
> >>
> >
> >- one does not want to use current implementation of acpid?
> 
> What is better?
> 

Probably nothing at the moment. It does not mean that every program
out there must mimic "legacy" acpid to provice concurrent access
to ACPI events.

> >- one does not want to depend on having acpid running before
> >  starting snooping acpi events?
> 
> For example?

You must ensure that nothing gets to /proc/acpi/events before acpid,
otherwise it won't be able to access it screwing up your system
state.

> 
> >- because allowing multiple readers is a "right thing to do"?
> 
> This is not self-evident, can you give some examples?

"I am trying to read from /proc/acpi/events and there is nothing there"
types of questions on various mailing lists come to my mind.

> 
> The argument against, I suppose, is simply why to add code
> to the kernel when the same feature is already working
> in user-space?
> 

If you remember original code (without registering multiple in-kernel
listeners) I think it was more compact that what is in the kernel
at the moment. Also IIRC current in kernel code may start filling memory
if acpid stops reading events for some reason.

For the record I think multiple listeners are not needed since the only
possible user is input layer and I firmly believe that ACPI should report
keys/buttons using input layer natively so userspace can get uniform
notification of "Sleep" button being pressed no matter whther that button
is controlled by ACPI or it is just another key on USB keyboard.

> The other argument against is why enhance an interface when
> perhaps we should instead consider replacing it altogether...
> 
> >> Dmitry, I don't know how to apply the signed-off thing correctly here,
> 
> If Dmitry is the original author, then the proper format is to put
> 
> From: Dmitry Torokhov <dtor_core@ameritech.net>
> 
> at start of the body of the message where it can over-ride the e-mail author.
> This is documented in Documentation/SubmittingPatches.
>

I'd say it is only valid if original patch is mostly intact. If it was
heavily reworked/enhanced I think mentioning the original author in
description is fine but authorship belongs the person who reworked the
code. Just IMHO.

-- 
Dmitry

Re: [PATCH 0/2] Deliver ACPI events upon subscription and implement multiple readers for /proc/acpi/event

[Mattia Dongili]

On Wed, January 25, 2006 6:23 am, Dmitry Torokhov said:
> On Wednesday 25 January 2006 00:09, Brown, Len wrote:
[...]
> If you remember original code (without registering multiple in-kernel
> listeners) I think it was more compact that what is in the kernel
> at the moment. Also IIRC current in kernel code may start filling memory
> if acpid stops reading events for some reason.
>
> For the record I think multiple listeners are not needed since the only
> possible user is input layer and I firmly believe that ACPI should report
> keys/buttons using input layer natively so userspace can get uniform
> notification of "Sleep" button being pressed no matter whther that button
> is controlled by ACPI or it is just another key on USB keyboard.

I have no problem changing the implementation to avoid useless stuff if
the patch is going to be considered for acceptance.

>> The other argument against is why enhance an interface when
>> perhaps we should instead consider replacing it altogether...

The same thing has been said at Dmitry's submission (08/2004) and
/proc/acpi/event is still there. :)
Also, removing /proc/acpi/event will take a very long time.
Anyway what would you suggest (if you have already some plan)? I'd like to
try implementing it.

-- 
mattia
:wq!

Re: [PATCH 0/2] Deliver ACPI events upon subscription and implement multiple readers for /proc/acpi/event

[Thomas Renninger]

Mattia Dongili wrote:
> On Wed, January 25, 2006 6:23 am, Dmitry Torokhov said:
>> On Wednesday 25 January 2006 00:09, Brown, Len wrote:
> [...]
 > I have no problem changing the implementation to avoid useless stuff if
> the patch is going to be considered for acceptance.
> 
>>> The other argument against is why enhance an interface when
>>> perhaps we should instead consider replacing it altogether...
> 
> The same thing has been said at Dmitry's submission (08/2004) and
> /proc/acpi/event is still there. :)
> Also, removing /proc/acpi/event will take a very long time.
> Anyway what would you suggest (if you have already some plan)? I'd like to
> try implementing it.

What about the dbus-kernel interface?
Acpi events are probably one of the things that should
make use of this one sooner or later?

However I have no idea in which state dbus is on kernel side.

   Thomas