linux-kernel - Re: [PATCH 29/49] vcs: add poll/fasync support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 10 Nov 2010 01:33:12 -0500 (EST)
From:	Nicolas Pitre <nicolas.pitre@...onical.com>
To:	Kay Sievers <kay.sievers@...y.org>
cc:	Greg Kroah-Hartman <gregkh@...e.de>,
	lkml <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 29/49] vcs: add poll/fasync support

On Wed, 10 Nov 2010, Kay Sievers wrote:

> On Wed, Nov 10, 2010 at 02:26, Nicolas Pitre
> <nicolas.pitre@...onical.com> wrote:
> > On Wed, 10 Nov 2010, Kay Sievers wrote:
> >
> >> On Fri, Oct 22, 2010 at 20:21, Greg Kroah-Hartman <gregkh@...e.de> wrote:
> >> > From: Nicolas Pitre <nico@...xnic.net>
> >> >
> >> > The /dev/vcs* devices are used, amongst other things, by accessibility
> >> > applications such as BRLTTY to display the screen content onto refreshable
> >> > braille displays.  Currently this is performed by constantly reading from
> >> > /dev/vcsa0 whether or not the screen content has changed.  Given the
> >> > default braille refresh rate of 25 times per second, this easily qualifies
> >> > as the biggest source of wake-up events preventing laptops from entering
> >> > deeper power saving states.
> >> >
> >> > To avoid this periodic polling, let's add support for select()/poll() and
> >> > SIGIO with the /dev/vcs* devices.  The implemented semantic is to report
> >> > data availability whenever the corresponding vt has seen some update after
> >> > the last read() operation.  The application still has to lseek() back
> >> > as usual in order to read() the new data.
> >>
> >> Shouldn't it raise POLLPRI/POLLERR then, when it's not about new data
> >> to read? We do this for several files in the kernel where we just want
> >> to wakup someone, but the pretty well-defined semantics of poll()
> >> don't apply.
> >
> > Hmmm...  Maybe POLLPRI, but POLLERR makes no sense.
> 
> I have no opinion about that, but it's what others do:
>   http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=31b07093c44a7a442394d44423e21d783f5523b8
>   http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=4508a7a734b111b8b7e39986237d84acb1168dd0
> 
> The POLLIN should definitely go, it's POSIX defined. What we do here
> is more like an error that wakes up than a stream data to read. :)

OK.  What about this then?
----- >8
Subject: [PATCH] vcs: make proper usage of the poll flags

Kay Sievers pointed out that usage of POLLIN is well defined by POSIX,
and the current usage here doesn't follow that definition.  So let's
duplicate the same semantics as implemented by sysfs_poll() instead.

Signed-off-by: Nicolas Pitre <nicolas.pitre@...onical.com>
---
diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index 273ab44..a93856e 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -553,12 +553,12 @@ static unsigned int
 vcs_poll(struct file *file, poll_table *wait)
 {
 	struct vcs_poll_data *poll = vcs_poll_data_get(file);
-	int ret = 0;
+	int ret = DEFAULT_POLLMASK|POLLERR|POLLPRI;
 
 	if (poll) {
 		poll_wait(file, &poll->waitq, wait);
-		if (!poll->seen_last_update)
-			ret = POLLIN | POLLRDNORM;
+		if (poll->seen_last_update)
+			ret = DEFAULT_POLLMASK; 
 	}
 	return ret;
 }