Hi Laurent,
On 23/02/2024 10:50, Laurent Pinchart wrote:
> Hi Hans,
>
> Thank you for the patch.
>
> On Fri, Feb 23, 2024 at 10:34:32AM +0100, Hans Verkuil wrote:
>> If the media device is unregistered, the read/write/ioctl file ops
>> returned EIO instead of ENODEV. And open returned ENXIO instead of the
>> linux kernel standard of ENODEV.
>
> Are you sure this is right ? Looking at chrdev_open() for instance, it
> returns -ENXIO when it can't find a cdev for the requested minor.
Right, but in this case the cdev is there, but the underlying device has
been removed and no longer exists. Linux returned ENODEV in that case.
'man 2 open' gives an interesting description of ENODEV:
ENODEV pathname refers to a device special file and no corresponding device exists.
(This is a Linux kernel bug; in this situation ENXIO must be returned.)
I think ENXIO is Posix, but in the linux kernel ENODEV is actually used.
Grepping for ENODEV (and looking at some other subsystems) suggests that ENODEV
is pretty standard for this. I think it is the difference between the major/minor
no longer being valid (ENXIO), and that it is still valid, but the underlying
device has gone. For open() that can happen if it is disconnected right after you
managed to enter the open() fop.
Personally I would prefer to have all media subsystems (v4l2/dvb/rc/cec/mc) behave
the same w.r.t. disconnects, and -EIO for read/write/ioctl is really wrong.
That said, if you prefer to stick to ENXIO instead of ENODEV, then I can make
a v2 that just replaces EIO by ENXIO.
Regards,
Hans
> Furthermore, the read() 3 manpage documents the ENXIO error as
>
> ENXIO A request was made of a nonexistent device, or the request
> was outside the capabilities of the device.
>
> while it doesn't document ENODEV at all.
>
>> Change the error code to ENODEV and document this as well in
>> media-func-open.rst.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>> ---
>> .../userspace-api/media/mediactl/media-func-open.rst | 4 ++--
>> drivers/media/mc/mc-devnode.c | 10 +++++-----
>> 2 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/userspace-api/media/mediactl/media-func-open.rst b/Documentation/userspace-api/media/mediactl/media-func-open.rst
>> index 24487cb0a308..8c1c7ebefdb1 100644
>> --- a/Documentation/userspace-api/media/mediactl/media-func-open.rst
>> +++ b/Documentation/userspace-api/media/mediactl/media-func-open.rst
>> @@ -61,5 +61,5 @@ ENFILE
>> ENOMEM
>> Insufficient kernel memory was available.
>>
>> -ENXIO
>> - No device corresponding to this device special file exists.
>> +ENODEV
>> + Device not found or was removed.
>> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
>> index 7f67825c8757..fbf76e1414de 100644
>> --- a/drivers/media/mc/mc-devnode.c
>> +++ b/drivers/media/mc/mc-devnode.c
>> @@ -75,7 +75,7 @@ static ssize_t media_read(struct file *filp, char __user *buf,
>> if (!devnode->fops->read)
>> return -EINVAL;
>> if (!media_devnode_is_registered(devnode))
>> - return -EIO;
>> + return -ENODEV;
>> return devnode->fops->read(filp, buf, sz, off);
>> }
>>
>> @@ -87,7 +87,7 @@ static ssize_t media_write(struct file *filp, const char __user *buf,
>> if (!devnode->fops->write)
>> return -EINVAL;
>> if (!media_devnode_is_registered(devnode))
>> - return -EIO;
>> + return -ENODEV;
>> return devnode->fops->write(filp, buf, sz, off);
>> }
>>
>> @@ -114,7 +114,7 @@ __media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
>> return -ENOTTY;
>>
>> if (!media_devnode_is_registered(devnode))
>> - return -EIO;
>> + return -ENODEV;
>>
>> return ioctl_func(filp, cmd, arg);
>> }
>> @@ -152,11 +152,11 @@ static int media_open(struct inode *inode, struct file *filp)
>> */
>> mutex_lock(&media_devnode_lock);
>> devnode = container_of(inode->i_cdev, struct media_devnode, cdev);
>> - /* return ENXIO if the media device has been removed
>> + /* return ENODEV if the media device has been removed
>> already or if it is not registered anymore. */
>> if (!media_devnode_is_registered(devnode)) {
>> mutex_unlock(&media_devnode_lock);
>> - return -ENXIO;
>> + return -ENODEV;
>> }
>> /* and increase the device refcount */
>> get_device(&devnode->dev);
>
