Re: [PATCH] media: mc: return ENODEV instead of EIO/ENXIO when dev is, unregistered

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.
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@xs4all.nl>
> ---
>  .../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);

-- 
Regards,

Laurent Pinchart