What should poll() return when a device is unregistered ? (was "media: Media device node support")

What should poll() return when a device is unregistered ? (was "media: Media device node support")

[Laurent Pinchart]

Hi Hans,

On Monday 22 November 2010 10:08:06 Hans Verkuil wrote:
> On Monday, November 22, 2010 00:35:54 Laurent Pinchart wrote:
> > Hi Jonathan,
> > 
> > I forgot to answer one of your comments.
> > 
> > On Wednesday 17 November 2010 01:31:15 Jonathan Corbet wrote:
> > 
> > [snip]
> > 
> > > > +static unsigned int media_poll(struct file *filp,
> > > > +			       struct poll_table_struct *poll)
> > > > +{
> > > > +	struct media_devnode *mdev = media_devnode_data(filp);
> > > > +
> > > > +	if (!mdev->fops->poll || !media_devnode_is_registered(mdev))
> > > > +		return DEFAULT_POLLMASK;
> > > > +	return mdev->fops->poll(filp, poll);
> > > > +}
> > > 
> > > If it's not registered, I would expect poll() to return an error.
> > 
> > Agreed. I'll return POLLERR | POLLHUP in that case. Is that fine with you
> > ?
> 
> When I looked at this for the core code I decided to just return POLLERR.

I've copied the usbdevfs code which returns POLLERR | POLLHUP when devices are 
disconnected.

> That seemed to be what the majority of other usb drivers do. ALSA returns
> POLLERR | POLLNVAL, by the way, which I think is a poor choice.

Indeed, POLLNVAL has a clear semantics that doesn't apply here.

> This doesn't really seem to be standardized :-(

CC'ing LKML with the question.

POLLERR | POLLHUP and POLLERR won't make a difference to select(), but we 
should still standardize on a poll() return code when devices are unregistered 
and/or - for hot-pluggable devices - disconnected (for V4L devices 
unregistered usually means disconnected) ?

-- 
Regards,

Laurent Pinchart

Re: What should poll() return when a device is unregistered ? (was "media: Media device node support")

[Hans Verkuil]

On Monday, November 22, 2010 11:41:27 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday 22 November 2010 10:08:06 Hans Verkuil wrote:
> > On Monday, November 22, 2010 00:35:54 Laurent Pinchart wrote:
> > > Hi Jonathan,
> > > 
> > > I forgot to answer one of your comments.
> > > 
> > > On Wednesday 17 November 2010 01:31:15 Jonathan Corbet wrote:
> > > 
> > > [snip]
> > > 
> > > > > +static unsigned int media_poll(struct file *filp,
> > > > > +			       struct poll_table_struct *poll)
> > > > > +{
> > > > > +	struct media_devnode *mdev = media_devnode_data(filp);
> > > > > +
> > > > > +	if (!mdev->fops->poll || !media_devnode_is_registered(mdev))
> > > > > +		return DEFAULT_POLLMASK;
> > > > > +	return mdev->fops->poll(filp, poll);
> > > > > +}
> > > > 
> > > > If it's not registered, I would expect poll() to return an error.
> > > 
> > > Agreed. I'll return POLLERR | POLLHUP in that case. Is that fine with you
> > > ?
> > 
> > When I looked at this for the core code I decided to just return POLLERR.
> 
> I've copied the usbdevfs code which returns POLLERR | POLLHUP when devices are 
> disconnected.
> 
> > That seemed to be what the majority of other usb drivers do. ALSA returns
> > POLLERR | POLLNVAL, by the way, which I think is a poor choice.
> 
> Indeed, POLLNVAL has a clear semantics that doesn't apply here.

In fact, POLLNVAL is currently only set by ./sound/core/init.c and by
./drivers/media/video/zoran/zoran_driver.c. In both cases it's rather dubious.

> > This doesn't really seem to be standardized :-(
> 
> CC'ing LKML with the question.
> 
> POLLERR | POLLHUP and POLLERR won't make a difference to select(), but we 
> should still standardize on a poll() return code when devices are unregistered 
> and/or - for hot-pluggable devices - disconnected (for V4L devices 
> unregistered usually means disconnected) ?

Drivers return POLLERR, POLLERR|POLLHUP or POLLHUP in case of a disconnect.
I'm leaning towards POLLHUP as the most appropriate poll return value for a
USB disconnect.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: What should poll() return when a device is unregistered ? (was "media: Media device node support")

[Andy Walls]

On Mon, 2010-11-22 at 12:36 +0100, Hans Verkuil wrote:
> On Monday, November 22, 2010 11:41:27 Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > On Monday 22 November 2010 10:08:06 Hans Verkuil wrote:
> > > On Monday, November 22, 2010 00:35:54 Laurent Pinchart wrote:
> > > > Hi Jonathan,

> > > This doesn't really seem to be standardized :-(
> > 
> > CC'ing LKML with the question.
> > 
> > POLLERR | POLLHUP and POLLERR won't make a difference to select(), but we 
> > should still standardize on a poll() return code when devices are unregistered 
> > and/or - for hot-pluggable devices - disconnected (for V4L devices 
> > unregistered usually means disconnected) ?
> 
> Drivers return POLLERR, POLLERR|POLLHUP or POLLHUP in case of a disconnect.
> I'm leaning towards POLLHUP as the most appropriate poll return value for a
> USB disconnect.

+1 POLLHUP

http://www.opengroup.org/onlinepubs/009695399/functions/poll.html

"POLLHUP
        The device has been disconnected. [...]"


My $0.02 below:
The communication link with the device is closed due to circumstances
that the OS considers normal operation.  The OS has, at some level, shut
down its side of the communication link gracefully as well.

Just because the application or the OS cannot always predict when a USB
disconnect may happen, doesn't mean it is an error.


Regards,
Andy

Re: What should poll() return when a device is unregistered ? (was "media: Media device node support")

[Laurent Pinchart]

Hi Andy,

On Monday 22 November 2010 13:51:37 Andy Walls wrote:
> On Mon, 2010-11-22 at 12:36 +0100, Hans Verkuil wrote:
> > On Monday, November 22, 2010 11:41:27 Laurent Pinchart wrote:
> > > Hi Hans,
> > > 
> > > On Monday 22 November 2010 10:08:06 Hans Verkuil wrote:
> > > > On Monday, November 22, 2010 00:35:54 Laurent Pinchart wrote:
> > > > > Hi Jonathan,
> > > > 
> > > > This doesn't really seem to be standardized :-(
> > > 
> > > CC'ing LKML with the question.
> > > 
> > > POLLERR | POLLHUP and POLLERR won't make a difference to select(), but
> > > we should still standardize on a poll() return code when devices are
> > > unregistered and/or - for hot-pluggable devices - disconnected (for
> > > V4L devices unregistered usually means disconnected) ?
> > 
> > Drivers return POLLERR, POLLERR|POLLHUP or POLLHUP in case of a
> > disconnect. I'm leaning towards POLLHUP as the most appropriate poll
> > return value for a USB disconnect.
> 
> +1 POLLHUP
> 
> http://www.opengroup.org/onlinepubs/009695399/functions/poll.html
> 
> "POLLHUP
>         The device has been disconnected. [...]"
> 
> 
> My $0.02 below:
> The communication link with the device is closed due to circumstances
> that the OS considers normal operation.  The OS has, at some level, shut
> down its side of the communication link gracefully as well.
> 
> Just because the application or the OS cannot always predict when a USB
> disconnect may happen, doesn't mean it is an error.

POLLHUP seems to be the best option standard-wise, but it could be a problem 
for output devices. Applications use the write file descriptors set with 
select() to wait for a buffer to be ready. Unlike POLLERR, POLLHUP only 
reports an event on the read file descriptors set.

-- 
Regards,

Laurent Pinchart

Re: What should poll() return when a device is unregistered ? (was "media: Media device node support")

[Andy Walls]

On Mon, 2010-11-22 at 17:19 +0100, Laurent Pinchart wrote:
> Hi Andy,
> 
> On Monday 22 November 2010 13:51:37 Andy Walls wrote:
> > On Mon, 2010-11-22 at 12:36 +0100, Hans Verkuil wrote:
> > > On Monday, November 22, 2010 11:41:27 Laurent Pinchart wrote:
> > > > Hi Hans,
> > > > 
> > > > On Monday 22 November 2010 10:08:06 Hans Verkuil wrote:
> > > > > On Monday, November 22, 2010 00:35:54 Laurent Pinchart wrote:
> > > > > > Hi Jonathan,
> > > > > 
> > > > > This doesn't really seem to be standardized :-(
> > > > 
> > > > CC'ing LKML with the question.
> > > > 
> > > > POLLERR | POLLHUP and POLLERR won't make a difference to select(), but
> > > > we should still standardize on a poll() return code when devices are
> > > > unregistered and/or - for hot-pluggable devices - disconnected (for
> > > > V4L devices unregistered usually means disconnected) ?
> > > 
> > > Drivers return POLLERR, POLLERR|POLLHUP or POLLHUP in case of a
> > > disconnect. I'm leaning towards POLLHUP as the most appropriate poll
> > > return value for a USB disconnect.
> > 
> > +1 POLLHUP
> > 
> > http://www.opengroup.org/onlinepubs/009695399/functions/poll.html
> > 
> > "POLLHUP
> >         The device has been disconnected. [...]"
> > 
> > 
> > My $0.02 below:
> > The communication link with the device is closed due to circumstances
> > that the OS considers normal operation.  The OS has, at some level, shut
> > down its side of the communication link gracefully as well.
> > 
> > Just because the application or the OS cannot always predict when a USB
> > disconnect may happen, doesn't mean it is an error.
> 
> POLLHUP seems to be the best option standard-wise, but it could be a problem 
> for output devices. Applications use the write file descriptors set with 
> select() to wait for a buffer to be ready. Unlike POLLERR, POLLHUP only 
> reports an event on the read file descriptors set.

After looking at the OpenGroup IEEE Std 1003.1-2004 spec for both
select() and poll(), I can say that select() not indicating an fd (with
POLLHUP status set) is ready for writing is

a. in compliance with the standard

b. likely inspired by the behavior specified for poll() in the standard

c. is overly restrictive given the language in the standard for
select().  select() could return that the fd is writeable, and be in
standards compliance, so long as the next call to an output function on
the fd does not block:

        "A descriptor shall be considered ready for writing when a call
        to an output function with O_NONBLOCK clear would not block,
        whether or not the function would transfer data successfully."
        
d. select() could also indicate an exceptional condition on an fd (with
POLLHUP status set) and still be in standards compliance:

        "[except for sockets], what constitutes an exceptional condition
        is file type-specific."


Despite that research, I'm guessing that the likelihood of consensus for
changing the kernel select()'s behavior for POLLHUP is low.


So I guess one should do something that satisfies both poll() and
select() to induce applications to take the correct action.

POLLHUP|POLLERR, as you had suggested, will probably have the desired
effect:

1. applications that use poll() will be slightly misinformed with the
indication of an error.  However, I'll *speculate* that applications
that have different courses of action for POLLHUP vs. POLLERR, will
prioritize POLLHUP handling over POLLERR handling anyway.

2. applications that use select(), with the fd only in the write_fds
set, will wake up upon the exception condition of a device disconnect
without the fd being in the except_fds set (since that would be useless
anyway in Linux in this case).

For standards compliance -- in at least the case of select() -- the next
call to read(), write(), ioctl( , VIDIOC_DQBUF, ), and ( ,
VIDIOC_QBUF, ) should return EOF or an error with errno set properly.
(If I understand the standard correctly.)

The only trouble will come from applications that use select() and don't
check the return value from the next read(), write(), or ioctl().  I
would hope that such applications are not in widespread use.

Regards,
Andy