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

[Kay Sievers]

On Wed, Nov 10, 2010 at 07:33, Nicolas Pitre
<nicolas.pitre@xxxxxxxxxxxxx> wrote:
> On Wed, 10 Nov 2010, Kay Sievers wrote:
>
>> On Wed, Nov 10, 2010 at 02:26, Nicolas Pitre
>> <nicolas.pitre@xxxxxxxxxxxxx> wrote:
>> > On Wed, 10 Nov 2010, Kay Sievers wrote:
>> >
>> >> On Fri, Oct 22, 2010 at 20:21, Greg Kroah-Hartman <gregkh@xxxxxxx> wrote:
>> >> > From: Nicolas Pitre <nico@xxxxxxxxxxx>
>> >> >
>> >> > 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@xxxxxxxxxxxxx>
> ---
> 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;
> Â}

Looks good to me.

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