Hi Sakari,
Thank you for the patch.
On Wed, Dec 20, 2023 at 12:36:59PM +0200, Sakari Ailus wrote:
> Assign the media device minor to -1 if it has not been previously
> assigned. There's no dependence to this yes but it will be required by
> patch "media: mc: Implement best effort media device removal safety sans
> refcount" soon.
After a quick look at that patch, I don't see the dependency I'm afraid.
Could you please explain it better ?
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
> drivers/media/mc/mc-devnode.c | 9 ++++++++-
> include/media/media-devnode.h | 2 +-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 717408791a7c..5057c48f8870 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -59,7 +59,8 @@ static void media_devnode_release(struct device *cd)
> {
> struct media_devnode *devnode = to_media_devnode(cd);
>
> - media_devnode_free_minor(devnode->minor);
> + if (devnode->minor != -1)
> + media_devnode_free_minor(devnode->minor);
Should the test be moved to media_devnode_free_minor() ?
>
> /* Release media_devnode and perform other cleanups as needed. */
> if (devnode->release)
> @@ -212,6 +213,7 @@ static const struct file_operations media_devnode_fops = {
> void media_devnode_init(struct media_devnode *devnode)
> {
> device_initialize(&devnode->dev);
> + devnode->minor = -1;
> }
>
> int __must_check media_devnode_register(struct media_devnode *devnode,
> @@ -220,6 +222,9 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
> int minor;
> int ret;
>
> + if (devnode->minor != -1)
> + return -EINVAL;
> +
> /* Part 1: Find a free minor number */
> mutex_lock(&media_devnode_lock);
> minor = find_first_zero_bit(media_devnode_nums, MEDIA_NUM_DEVICES);
> @@ -261,6 +266,8 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
> cdev_add_error:
> media_devnode_free_minor(devnode->minor);
>
> + devnode->minor = -1;
> +
> return ret;
> }
>
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index 6d46c658be21..d050f54f252a 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -60,7 +60,7 @@ struct media_file_operations {
> * @dev: pointer to struct &device containing the media controller device
> * @cdev: struct cdev pointer character device
> * @parent: parent device
> - * @minor: device node minor number
> + * @minor: device node minor number, -1 if unassigned
> * @flags: flags, combination of the ``MEDIA_FLAG_*`` constants
> * @release: release callback called at the end of ``media_devnode_release()``
> * routine at media-device.c.
--
Regards,
Laurent Pinchart
