Message-ID: <20120227221831.GF7666@count0.beaverton.ibm.com>
Date:	Mon, 27 Feb 2012 14:18:31 -0800
From:	Matt Helsley <matthltc@...ibm.com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	Matt Helsley <matthltc@...ibm.com>,
	Linux PM list <linux-pm@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Magnus Damm <magnus.damm@...il.com>, markgross@...gnar.org,
	Matthew Garrett <mjg@...hat.com>,
	Greg KH <gregkh@...uxfoundation.org>,
	Arve Hjønnevåg <arve@...roid.com>,
	John Stultz <john.stultz@...aro.org>,
	Brian Swetland <swetland@...gle.com>,
	Neil Brown <neilb@...e.de>,
	Alan Stern <stern@...land.harvard.edu>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: [RFC][PATCH 4/7] Input / PM: Add ioctl to block suspend while
 event queue is not empty

On Sun, Feb 26, 2012 at 09:57:18PM +0100, Rafael J. Wysocki wrote:
> On Friday, February 24, 2012, Matt Helsley wrote:
> > On Wed, Feb 22, 2012 at 12:34:58AM +0100, Rafael J. Wysocki wrote:
> > > From: Arve Hjønnevåg <arve@...roid.com>
> > > 
> > > Add a new ioctl, EVIOCSWAKEUPSRC, to attach a wakeup source object to
> > > an evdev client event queue, such that it will be active whenever the
> > > queue is not empty.  Then, all events in the queue will be regarded
> > > as wakeup events in progress and pm_get_wakeup_count() will block (or
> > > return false if woken up by a signal) until they are removed from the
> > > queue.  In consequence, if the checking of wakeup events is enabled
> > > (e.g. throught the /sys/power/wakeup_count interface), the system
> > > won't be able to go into a sleep state until the queue is empty.
> > > 
> > > This allows user space processes to handle situations in which they
> > > want to do a select() on an evdev descriptor, so they go to sleep
> > > until there are some events to read from the device's queue, and then
> > > they don't want the system to go into a sleep state until all the
> > > events are read (presumably for further processing).  Of course, if
> > > they don't want the system to go into a sleep state _after_ all the
> > > events have been read from the queue, they have to use a separate
> > > mechanism that will prevent the system from doing that and it has
> > > to be activated before reading the first event (that also may be the
> > > last one).
> > 
> > I haven't seen this idea mentioned before but I must admit I haven't
> > been following this thread too closely so apologies (and don't bother
> > rehashing) if it has:
> > 
> > Could you just add this to epoll so that any fd userspace chooses would be
> > capable of doing this without introducing potentially ecclectic ioctl
> > interfaces?
> > 
> > struct epoll_event ev;
> > 
> > epfd = epoll_create1(EPOLL_STAY_AWAKE_SET);
> > ev.data.ptr = foo;
> > epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &ev);
> > 
> > Which could be useful because you can put one epollfd in another's epoll
> > set. Or maybe as an EPOLLKEEPAWAKE flag in the event struct sort of like
> > EPOLLET:
> > 
> > epfd = epoll_create1(0);
> > ev.events = EPOLLIN|EPOLLKEEPAWAKE;
> > epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &ev);
> 
> Do you mean something like the patch below, or something different?

Yeah, this was sort of what I was thinking of. It nicely avoids the
ioctl() bits. I guess my only issue is the fop mimics the epoll
interface -- should it just be an fop to manage the file as a wakeup
source rather than a generic hook into epoll?

Cheers,
	-Matt Helsley

> 
> Rafael
> 
> ---
>  drivers/input/evdev.c     |   55 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/eventpoll.c            |   15 +++++++++++-
>  include/linux/eventpoll.h |    6 +++++
>  include/linux/fs.h        |    1 
>  4 files changed, 76 insertions(+), 1 deletion(-)
> 
> Index: linux/include/linux/fs.h
> ===================================================================
> --- linux.orig/include/linux/fs.h
> +++ linux/include/linux/fs.h
> @@ -1604,6 +1604,7 @@ struct file_operations {
>  	ssize_t (*aio_write) (struct kiocb *, const struct iovec *, unsigned long, loff_t);
>  	int (*readdir) (struct file *, void *, filldir_t);
>  	unsigned int (*poll) (struct file *, struct poll_table_struct *);
> +	void (*epoll_ctl) (struct file *, int, unsigned int);
>  	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
>  	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
>  	int (*mmap) (struct file *, struct vm_area_struct *);
> Index: linux/fs/eventpoll.c
> ===================================================================
> --- linux.orig/fs/eventpoll.c
> +++ linux/fs/eventpoll.c
> @@ -609,6 +609,10 @@ static int ep_remove(struct eventpoll *e
>  	unsigned long flags;
>  	struct file *file = epi->ffd.file;
>  
> +	/* Notify the underlying driver that the polling has completed */
> +	if (file->f_op->epoll_ctl)
> +		file->f_op->epoll_ctl(file, EPOLL_CTL_DEL, epi->event.events);
> +
>  	/*
>  	 * Removes poll wait queue hooks. We _have_ to do this without holding
>  	 * the "ep->lock" otherwise a deadlock might occur. This because of the
> @@ -1094,6 +1098,10 @@ static int ep_insert(struct eventpoll *e
>  	epq.epi = epi;
>  	init_poll_funcptr(&epq.pt, ep_ptable_queue_proc);
>  
> +	/* Notify the underlying driver that we want to poll it */
> +	if (tfile->f_op->epoll_ctl)
> +		tfile->f_op->epoll_ctl(tfile, EPOLL_CTL_ADD, event->events);
> +
>  	/*
>  	 * Attach the item to the poll hooks and get current event bits.
>  	 * We can safely use the file* here because its usage count has
> @@ -1185,6 +1193,7 @@ error_unregister:
>   */
>  static int ep_modify(struct eventpoll *ep, struct epitem *epi, struct epoll_event *event)
>  {
> +	struct file *file = epi->ffd.file;
>  	int pwake = 0;
>  	unsigned int revents;
>  
> @@ -1196,11 +1205,15 @@ static int ep_modify(struct eventpoll *e
>  	epi->event.events = event->events;
>  	epi->event.data = event->data; /* protected by mtx */
>  
> +	/* Notify the underlying driver of the change */
> +	if (file->f_op->epoll_ctl)
> +		file->f_op->epoll_ctl(file, EPOLL_CTL_MOD, event->events);
> +
>  	/*
>  	 * Get current event bits. We can safely use the file* here because
>  	 * its usage count has been increased by the caller of this function.
>  	 */
> -	revents = epi->ffd.file->f_op->poll(epi->ffd.file, NULL);
> +	revents = file->f_op->poll(file, NULL);
>  
>  	/*
>  	 * If the item is "hot" and it is not registered inside the ready
> Index: linux/drivers/input/evdev.c
> ===================================================================
> --- linux.orig/drivers/input/evdev.c
> +++ linux/drivers/input/evdev.c
> @@ -16,6 +16,7 @@
>  #define EVDEV_BUF_PACKETS	8
>  
>  #include <linux/poll.h>
> +#include <linux/eventpoll.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> @@ -43,6 +44,7 @@ struct evdev_client {
>  	unsigned int tail;
>  	unsigned int packet_head; /* [future] position of the first element of next packet */
>  	spinlock_t buffer_lock; /* protects access to buffer, head and tail */
> +	struct wakeup_source *wakeup_source;
>  	struct fasync_struct *fasync;
>  	struct evdev *evdev;
>  	struct list_head node;
> @@ -75,10 +77,12 @@ static void evdev_pass_event(struct evde
>  		client->buffer[client->tail].value = 0;
>  
>  		client->packet_head = client->tail;
> +		__pm_relax(client->wakeup_source);
>  	}
>  
>  	if (event->type == EV_SYN && event->code == SYN_REPORT) {
>  		client->packet_head = client->head;
> +		__pm_stay_awake(client->wakeup_source);
>  		kill_fasync(&client->fasync, SIGIO, POLL_IN);
>  	}
>  
> @@ -255,6 +259,8 @@ static int evdev_release(struct inode *i
>  	mutex_unlock(&evdev->mutex);
>  
>  	evdev_detach_client(evdev, client);
> +	wakeup_source_unregister(client->wakeup_source);
> +
>  	kfree(client);
>  
>  	evdev_close_device(evdev);
> @@ -373,6 +379,8 @@ static int evdev_fetch_next_event(struct
>  	if (have_event) {
>  		*event = client->buffer[client->tail++];
>  		client->tail &= client->bufsize - 1;
> +		if (client->packet_head == client->tail)
> +			__pm_relax(client->wakeup_source);
>  	}
>  
>  	spin_unlock_irq(&client->buffer_lock);
> @@ -433,6 +441,52 @@ static unsigned int evdev_poll(struct fi
>  	return mask;
>  }
>  
> +static void evdev_client_attach_wakeup_source(struct evdev_client *client)
> +{
> +	struct wakeup_source *ws;
> +
> +	ws = wakeup_source_register(dev_name(&client->evdev->dev));
> +	spin_lock_irq(&client->buffer_lock);
> +	client->wakeup_source = ws;
> +	if (client->packet_head != client->tail)
> +		__pm_stay_awake(client->wakeup_source);
> +	spin_unlock_irq(&client->buffer_lock);
> +}
> +
> +static void evdev_client_detach_wakeup_source(struct evdev_client *client)
> +{
> +	struct wakeup_source *ws;
> +
> +	spin_lock_irq(&client->buffer_lock);
> +	ws = client->wakeup_source;
> +	client->wakeup_source = NULL;
> +	spin_unlock_irq(&client->buffer_lock);
> +	wakeup_source_unregister(ws);
> +}
> +
> +static void evdev_epoll_ctl(struct file *file, int op,
> +				    unsigned int events)
> +{
> +	struct evdev_client *client = file->private_data;
> +
> +	switch (op) {
> +	case EPOLL_CTL_ADD:
> +		if ((events & EPOLLWAKEUP) && !client->wakeup_source)
> +			evdev_client_attach_wakeup_source(client);
> +		break;
> +	case EPOLL_CTL_DEL:
> +		if (events & EPOLLWAKEUP)
> +			evdev_client_detach_wakeup_source(client);
> +		break;
> +	case EPOLL_CTL_MOD:
> +		/* 'events' is the new events mask (after the change) */
> +		if ((events & EPOLLWAKEUP) && !client->wakeup_source)
> +			evdev_client_attach_wakeup_source(client);
> +		else if (!(events & EPOLLWAKEUP))
> +			evdev_client_detach_wakeup_source(client);
> +	}
> +}
> +
>  #ifdef CONFIG_COMPAT
>  
>  #define BITS_PER_LONG_COMPAT (sizeof(compat_long_t) * 8)
> @@ -845,6 +899,7 @@ static const struct file_operations evde
>  	.read		= evdev_read,
>  	.write		= evdev_write,
>  	.poll		= evdev_poll,
> +	.epoll_ctl	= evdev_epoll_ctl,
>  	.open		= evdev_open,
>  	.release	= evdev_release,
>  	.unlocked_ioctl	= evdev_ioctl,
> Index: linux/include/linux/eventpoll.h
> ===================================================================
> --- linux.orig/include/linux/eventpoll.h
> +++ linux/include/linux/eventpoll.h
> @@ -26,6 +26,12 @@
>  #define EPOLL_CTL_DEL 2
>  #define EPOLL_CTL_MOD 3
>  
> +/*
> + * Request the handling of system wakeup events so as to prevent automatic
> + * system suspends from happening while those events are being processed.
> + */
> +#define EPOLLWAKEUP (1 << 29)
> +
>  /* Set the One Shot behaviour for the target file descriptor */
>  #define EPOLLONESHOT (1 << 30)
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists