On 20/12/2023 11:37, Sakari Ailus wrote:
> The list of file handles is needed to deliver media events as well as for
> other purposes in the future.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
> drivers/media/mc/mc-device.c | 23 ++++++++++++++++++++++-
> drivers/media/mc/mc-devnode.c | 2 +-
> include/media/media-devnode.h | 4 +++-
> 3 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index 67a39cb63f89..9cc055deeec9 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -45,9 +45,11 @@ static inline void __user *media_get_uptr(__u64 arg)
> return (void __user *)(uintptr_t)arg;
> }
>
> -static int media_device_open(struct file *filp)
> +static int media_device_open(struct media_devnode *devnode, struct file *filp)
> {
> + struct media_device *mdev = to_media_device(devnode);
> struct media_device_fh *fh;
> + unsigned long flags;
>
> fh = kzalloc(sizeof(*fh), GFP_KERNEL);
> if (!fh)
> @@ -55,12 +57,23 @@ static int media_device_open(struct file *filp)
>
> filp->private_data = &fh->fh;
>
> + spin_lock_irqsave(&mdev->fh_list_lock, flags);
The only reason for using the irqsave variant is because we want
to support events in the future, and those can be sent in irq context.
But it is confusing to see this being used here when it is not yet
needed.
At minimum this should be explained in the commit log.
Regards,
Hans
> + list_add(&fh->mdev_list, &mdev->fh_list);
> + spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
> +
> return 0;
> }
>
> static int media_device_close(struct file *filp)
> {
> + struct media_devnode *devnode = media_devnode_data(filp);
> + struct media_device *mdev = to_media_device(devnode);
> struct media_device_fh *fh = media_device_fh(filp);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mdev->fh_list_lock, flags);
> + list_del(&fh->mdev_list);
> + spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
>
> kfree(fh);
>
> @@ -769,11 +782,13 @@ void media_device_init(struct media_device *mdev)
> INIT_LIST_HEAD(&mdev->pads);
> INIT_LIST_HEAD(&mdev->links);
> INIT_LIST_HEAD(&mdev->entity_notify);
> + INIT_LIST_HEAD(&mdev->fh_list);
>
> mutex_init(&mdev->req_queue_mutex);
> mutex_init(&mdev->graph_mutex);
> ida_init(&mdev->entity_internal_idx);
> atomic_set(&mdev->request_id, 0);
> + spin_lock_init(&mdev->fh_list_lock);
>
> mdev->devnode.release = media_device_release;
> media_devnode_init(&mdev->devnode);
> @@ -824,6 +839,8 @@ EXPORT_SYMBOL_GPL(__media_device_register);
>
> void media_device_unregister(struct media_device *mdev)
> {
> + unsigned long flags;
> +
> if (mdev == NULL)
> return;
>
> @@ -834,6 +851,10 @@ void media_device_unregister(struct media_device *mdev)
> }
> mutex_unlock(&mdev->graph_mutex);
>
> + spin_lock_irqsave(&mdev->fh_list_lock, flags);
> + list_del_init(&mdev->fh_list);
> + spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
> +
> device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> dev_dbg(mdev->dev, "Media device unregistering\n");
> media_devnode_unregister(&mdev->devnode);
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 04d376015526..0b5c24828e24 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -171,7 +171,7 @@ static int media_open(struct inode *inode, struct file *filp)
> get_device(&devnode->dev);
> mutex_unlock(&media_devnode_lock);
>
> - ret = devnode->fops->open(filp);
> + ret = devnode->fops->open(devnode, filp);
> if (ret) {
> put_device(&devnode->dev);
> return ret;
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index b0efdde4ffd8..840f7ae852d3 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -21,6 +21,8 @@
> #include <linux/device.h>
> #include <linux/cdev.h>
>
> +struct media_devnode;
> +
> /*
> * Flag to mark the media_devnode struct as registered. Drivers must not touch
> * this flag directly, it will be set and cleared by media_devnode_register and
> @@ -49,7 +51,7 @@ struct media_file_operations {
> __poll_t (*poll) (struct file *, struct poll_table_struct *);
> long (*ioctl) (struct file *, unsigned int, unsigned long);
> long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
> - int (*open) (struct file *);
> + int (*open) (struct media_devnode *, struct file *);
> int (*release) (struct file *);
> };
>
