| Message ID | 20231220103713.113386-27-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Media device lifetime management | expand |
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@linux.intel.com> > --- > 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 *); > }; >
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@linux.intel.com> > > --- > > 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. > 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 *); > > }; > >
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@linux.intel.com> >>> --- >>> 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. But without interrupts that access fh_list the irqsave variant is confusing. Regards, Hans > >> 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 *); >>> }; >>> >
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@linux.intel.com> > >>> --- > >>> 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 *); > >>> }; > >>>
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); + 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 *); };
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@linux.intel.com> --- 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(-)