On Mon, Feb 05, 2024 at 04:32:44PM +0100, Hans Verkuil wrote:
> On 05/02/2024 16:16, Laurent Pinchart wrote:
> > On Mon, Feb 05, 2024 at 04:11:43PM +0100, Hans Verkuil wrote:
> >> 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.
> >
> > Even in that case, would media_device_open() ever be called from
> > interrupt context ? spin_lock_irqsave() is only needed if you don't know
> > which context the function can be called from. If we know we'll be
> > called from interruptible context only, you can use spin_lock_irq()
> > instead.
>
> Someone can call open() while at the same time the kernel sends a
> media event from interrupt context. Such an event function will walk
> over the fh_list. The irqsave here is meant to ensure that no event
> interrupt can run while we add our fh to the fh list.
You don't need spin_lock_irqsave() for that, spin_lock_irq() is enough.
In your interrupt handler, you need spin_lock() only.
spin_lock_irqsave() is for places that can be called both from
interruptible and non-interruptible contexts.
> But without interrupts that access fh_list the irqsave variant is
> confusing.
>
> >> 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.
> >>
> >>> + 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 *);
> >>> };
> >>>
--
Regards,
Laurent Pinchart
