| Message ID | 20231220103713.113386-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Media device lifetime management | expand |
On 20/12/2023 11:36, 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 yes -> yet > patch "media: mc: Implement best effort media device removal safety sans > refcount" soon. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> After fixing that somewhat confusing typo above: Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > 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); > > /* 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.
On 20/12/2023 11:37, Sakari Ailus wrote: > The video device depends on the existence of its media device --- if there > is one. Acquire a reference to it. > > Note that when the media device release callback is used, then the V4L2 > device release callback is ignored and a warning is issued if both are > set. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-dev.c | 51 ++++++++++++++++++++---------- > 1 file changed, 34 insertions(+), 17 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > index d13954bd31fd..c1e4995eaf5c 100644 > --- a/drivers/media/v4l2-core/v4l2-dev.c > +++ b/drivers/media/v4l2-core/v4l2-dev.c > @@ -176,6 +176,11 @@ static void v4l2_device_release(struct device *cd) > { > struct video_device *vdev = to_video_device(cd); > struct v4l2_device *v4l2_dev = vdev->v4l2_dev; > + bool v4l2_dev_has_release = v4l2_dev->release; > +#ifdef CONFIG_MEDIA_CONTROLLER > + struct media_device *mdev = v4l2_dev->mdev; > + bool mdev_has_release = mdev && mdev->ops && mdev->ops->release; > +#endif > > mutex_lock(&videodev_lock); > if (WARN_ON(video_devices[vdev->minor] != vdev)) { > @@ -198,8 +203,8 @@ static void v4l2_device_release(struct device *cd) > > mutex_unlock(&videodev_lock); > > -#if defined(CONFIG_MEDIA_CONTROLLER) > - if (v4l2_dev->mdev && vdev->vfl_dir != VFL_DIR_M2M) { > +#ifdef CONFIG_MEDIA_CONTROLLER > + if (mdev && vdev->vfl_dir != VFL_DIR_M2M) { > /* Remove interfaces and interface links */ > media_devnode_remove(vdev->intf_devnode); > if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN) > @@ -207,23 +212,31 @@ static void v4l2_device_release(struct device *cd) > } > #endif > > - /* Do not call v4l2_device_put if there is no release callback set. > - * Drivers that have no v4l2_device release callback might free the > - * v4l2_dev instance in the video_device release callback below, so we > - * must perform this check here. > - * > - * TODO: In the long run all drivers that use v4l2_device should use the > - * v4l2_device release callback. This check will then be unnecessary. > - */ > - if (v4l2_dev->release == NULL) > - v4l2_dev = NULL; > - > /* Release video_device and perform other > cleanups as needed. */ > vdev->release(vdev); > > - /* Decrease v4l2_device refcount */ > - if (v4l2_dev) > +#ifdef CONFIG_MEDIA_CONTROLLER > + if (mdev) > + media_device_put(mdev); > + > + /* > + * Generally both struct media_device and struct v4l2_device are > + * embedded in the same driver's context struct so having a release > + * callback in both is a bug. > + */ > + WARN_ON(v4l2_dev_has_release && mdev_has_release); How about: if (WARN_ON(v4l2_dev_has_release && mdev_has_release)) v4l2_dev_has_release = false; > +#endif > + > + /* > + * Decrease v4l2_device refcount, but only if the media device doesn't > + * have a release callback. > + */ > + if (v4l2_dev_has_release > +#ifdef CONFIG_MEDIA_CONTROLLER > + && !mdev_has_release > +#endif > + ) Then this change is no longer needed. General question: do we have drivers today that set both release functions? Because that would now cause a WARN in the kernel log with this patch. > v4l2_device_put(v4l2_dev); > } > > @@ -792,11 +805,14 @@ static int video_register_media_controller(struct video_device *vdev) > u32 intf_type; > int ret; > > - /* Memory-to-memory devices are more complex and use > + /* > + * Memory-to-memory devices are more complex and use > * their own function to register its mc entities. > */ > - if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M) > + if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M) { > + media_device_get(vdev->v4l2_dev->mdev); > return 0; > + } > > vdev->entity.obj_type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE; > vdev->entity.function = MEDIA_ENT_F_UNKNOWN; > @@ -875,6 +891,7 @@ static int video_register_media_controller(struct video_device *vdev) > > /* FIXME: how to create the other interface links? */ > > + media_device_get(vdev->v4l2_dev->mdev); > #endif > return 0; > } Regards, Hans
On 20/12/2023 11:37, Sakari Ailus wrote: > Call devm_request_irq() before registering the async notifier, as otherwise > it would be possible to use the device before the interrupts could be > deliveted to the driver. deliveted -> delivered Isn't this a regular fix? Ditto for the previous patch (20/29). I'd just queue this up in the next PR. Regards, Hans > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index da82d09b46ab..3222ec5b8345 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -1789,11 +1789,6 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > > v4l2_async_nf_init(&cio2->notifier, &cio2->v4l2_dev); > > - /* Register notifier for subdevices we care */ > - r = cio2_parse_firmware(cio2); > - if (r) > - goto fail_clean_notifier; > - > r = devm_request_irq(dev, pci_dev->irq, cio2_irq, IRQF_SHARED, > CIO2_NAME, cio2); > if (r) { > @@ -1801,6 +1796,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > goto fail_clean_notifier; > } > > + /* Register notifier for subdevices we care */ > + r = cio2_parse_firmware(cio2); > + if (r) > + goto fail_clean_notifier; > + > pm_runtime_put_noidle(dev); > pm_runtime_allow(dev); >
On 20/12/2023 11:37, Sakari Ailus wrote: > Release all the resources when the media device is related, moving away > form the struct v4l2_device used for that purpose. form -> from > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c > index af127476e920..3e59f8c256c7 100644 > --- a/drivers/media/test-drivers/vimc/vimc-core.c > +++ b/drivers/media/test-drivers/vimc/vimc-core.c > @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) > return 0; > } > > -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > +static void vimc_mdev_release(struct media_device *mdev) > { > struct vimc_device *vimc = > - container_of(v4l2_dev, struct vimc_device, v4l2_dev); > + container_of_const(mdev, struct vimc_device, mdev); Why this change? > > vimc_release_subdevs(vimc); > - media_device_cleanup(&vimc->mdev); > kfree(vimc->ent_devs); > kfree(vimc); > } > @@ -336,6 +335,10 @@ static int vimc_register_devices(struct vimc_device *vimc) > return ret; > } > > +static const struct media_device_ops vimc_mdev_ops = { > + .release = vimc_mdev_release, > +}; > + > static int vimc_probe(struct platform_device *pdev) > { > const struct font_desc *font = find_font("VGA8x16"); > @@ -369,12 +372,12 @@ static int vimc_probe(struct platform_device *pdev) > snprintf(vimc->mdev.bus_info, sizeof(vimc->mdev.bus_info), > "platform:%s", VIMC_PDEV_NAME); > vimc->mdev.dev = &pdev->dev; > + vimc->mdev.ops = &vimc_mdev_ops; > media_device_init(&vimc->mdev); > > ret = vimc_register_devices(vimc); > if (ret) { > - media_device_cleanup(&vimc->mdev); > - kfree(vimc); > + media_device_put(&vimc->mdev); > return ret; > } > /* > @@ -382,7 +385,6 @@ static int vimc_probe(struct platform_device *pdev) > * if the registration fails, we release directly from probe > */ > > - vimc->v4l2_dev.release = vimc_v4l2_dev_release; > platform_set_drvdata(pdev, vimc); > return 0; > } > @@ -397,6 +399,7 @@ static void vimc_remove(struct platform_device *pdev) > media_device_unregister(&vimc->mdev); > v4l2_device_unregister(&vimc->v4l2_dev); > v4l2_device_put(&vimc->v4l2_dev); > + media_device_put(&vimc->mdev); > } > > static void vimc_dev_release(struct device *dev) Regards, Hans
On 20/12/2023 11:37, Sakari Ailus wrote: > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > The media devnode core associates devnodes with files by storing the > devnode pointer in the file structure private_data field. In order to > allow tracking of per-file-handle data introduce a new media devnode > file handle structure that stores the devnode pointer, and store a > pointer to that structure in the file private_data field. > > Users of the media devnode code (the only existing user being > media_device) are responsible for managing their own subclass of the > media_devnode_fh structure. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Prepare struct media_device_fh to be used for maintaining file handle list > to avoid shuffling things here and there right after. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/mc/mc-device.c | 14 +++++++++++++- > drivers/media/mc/mc-devnode.c | 20 +++++++++----------- > include/media/media-device.h | 7 +++++++ > include/media/media-devnode.h | 18 +++++++++++++++++- > include/media/media-fh.h | 32 ++++++++++++++++++++++++++++++++ > 5 files changed, 78 insertions(+), 13 deletions(-) > create mode 100644 include/media/media-fh.h > > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c > index 10426c2796b6..67a39cb63f89 100644 > --- a/drivers/media/mc/mc-device.c > +++ b/drivers/media/mc/mc-device.c > @@ -22,6 +22,7 @@ > #include <media/media-device.h> > #include <media/media-devnode.h> > #include <media/media-entity.h> > +#include <media/media-fh.h> > #include <media/media-request.h> > > #ifdef CONFIG_MEDIA_CONTROLLER > @@ -35,7 +36,6 @@ > #define MEDIA_ENT_SUBTYPE_MASK 0x0000ffff > #define MEDIA_ENT_T_DEVNODE_UNKNOWN (MEDIA_ENT_F_OLD_BASE | \ > MEDIA_ENT_SUBTYPE_MASK) > - > /* ----------------------------------------------------------------------------- > * Userspace API > */ > @@ -47,11 +47,23 @@ static inline void __user *media_get_uptr(__u64 arg) > > static int media_device_open(struct file *filp) > { > + struct media_device_fh *fh; > + > + fh = kzalloc(sizeof(*fh), GFP_KERNEL); > + if (!fh) > + return -ENOMEM; > + > + filp->private_data = &fh->fh; > + > return 0; > } > > static int media_device_close(struct file *filp) > { > + struct media_device_fh *fh = media_device_fh(filp); > + > + kfree(fh); > + > return 0; > } > > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c > index 4ea05e42dafb..04d376015526 100644 > --- a/drivers/media/mc/mc-devnode.c > +++ b/drivers/media/mc/mc-devnode.c > @@ -150,6 +150,7 @@ static long media_compat_ioctl(struct file *filp, unsigned int cmd, > static int media_open(struct inode *inode, struct file *filp) > { > struct media_devnode *devnode; > + struct media_devnode_fh *fh; > int ret; > > /* Check if the media device is available. This needs to be done with > @@ -170,17 +171,15 @@ static int media_open(struct inode *inode, struct file *filp) > get_device(&devnode->dev); > mutex_unlock(&media_devnode_lock); > > - filp->private_data = devnode; > - > - if (devnode->fops->open) { > - ret = devnode->fops->open(filp); > - if (ret) { > - put_device(&devnode->dev); > - filp->private_data = NULL; > - return ret; > - } > + ret = devnode->fops->open(filp); > + if (ret) { > + put_device(&devnode->dev); > + return ret; > } > > + fh = filp->private_data; > + fh->devnode = devnode; > + > return 0; > } > > @@ -189,8 +188,7 @@ static int media_release(struct inode *inode, struct file *filp) > { > struct media_devnode *devnode = media_devnode_data(filp); > > - if (devnode->fops->release) > - devnode->fops->release(filp); > + devnode->fops->release(filp); > > filp->private_data = NULL; > > diff --git a/include/media/media-device.h b/include/media/media-device.h > index 98e1892f1b51..83b8ea44463d 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -110,6 +110,10 @@ struct media_device_ops { > * other operations that stop or start streaming. > * @request_id: Used to generate unique request IDs > * > + * @fh_list: List of file handles in the media device > + * (struct media_device_fh.mdev_list). > + * @fh_list_lock: Serialise access to fh_list list. > + * > * This structure represents an abstract high-level media device. It allows easy > * access to entities and provides basic media device-level support. The > * structure can be allocated directly or embedded in a larger structure. > @@ -182,6 +186,9 @@ struct media_device { > > struct mutex req_queue_mutex; > atomic_t request_id; > + > + struct list_head fh_list; > + spinlock_t fh_list_lock; > }; > > /* We don't need to include usb.h here */ > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h > index d050f54f252a..b0efdde4ffd8 100644 > --- a/include/media/media-devnode.h > +++ b/include/media/media-devnode.h > @@ -53,6 +53,20 @@ struct media_file_operations { > int (*release) (struct file *); > }; > > +/** > + * struct media_devnode_fh - Media device node file handle > + * @devnode: pointer to the media device node > + * > + * This structure serves as a base for per-file-handle data storage. Media > + * device node users embed media_devnode_fh in their custom file handle data > + * structures and store the media_devnode_fh in the file private_data in order > + * to let the media device node core locate the media_devnode corresponding to a > + * file handle. > + */ > +struct media_devnode_fh { > + struct media_devnode *devnode; > +}; > + > /** > * struct media_devnode - Media device node > * @media_dev: pointer to struct &media_device > @@ -137,7 +151,9 @@ void media_devnode_unregister(struct media_devnode *devnode); > */ > static inline struct media_devnode *media_devnode_data(struct file *filp) > { > - return filp->private_data; > + struct media_devnode_fh *fh = filp->private_data; > + > + return fh->devnode; > } > > /** > diff --git a/include/media/media-fh.h b/include/media/media-fh.h > new file mode 100644 > index 000000000000..6f00744b81d6 > --- /dev/null > +++ b/include/media/media-fh.h > @@ -0,0 +1,32 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Media device file handle > + * > + * Copyright (C) 2019--2023 Intel Corporation > + */ > + > +#ifndef MEDIA_FH_H > +#define MEDIA_FH_H > + > +#include <linux/list.h> > +#include <linux/file.h> > + > +#include <media/media-devnode.h> > + > +/** > + * struct media_device_fh - File handle specific information on MC > + * > + * @fh: The media device file handle > + * @mdev_list: This file handle in media device's list of file handles > + */ > +struct media_device_fh { > + struct media_devnode_fh fh; > + struct list_head mdev_list; > +}; > + > +static inline struct media_device_fh *media_device_fh(struct file *filp) > +{ > + return container_of(filp->private_data, struct media_device_fh, fh); > +} > + > +#endif /* MEDIA_FH_H */
Hi Sakari, Thank you for the patch. On Wed, Dec 20, 2023 at 12:36:52PM +0200, Sakari Ailus wrote: > The release callback is only used to print a debug message. Drop it. (It > will be re-introduced later in a different form.) > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/mc/mc-device.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c > index d4553a3705f5..c0ea08a8fc31 100644 > --- a/drivers/media/mc/mc-device.c > +++ b/drivers/media/mc/mc-device.c > @@ -566,11 +566,6 @@ static DEVICE_ATTR_RO(model); > * Registration/unregistration > */ > > -static void media_device_release(struct media_devnode *devnode) > -{ > - dev_dbg(devnode->parent, "Media device released\n"); > -} > - > static void __media_device_unregister_entity(struct media_entity *entity) > { > struct media_device *mdev = entity->graph_obj.mdev; > @@ -718,7 +713,6 @@ int __must_check __media_device_register(struct media_device *mdev, > /* Register the device node. */ > mdev->devnode.fops = &media_device_fops; > mdev->devnode.parent = mdev->dev; > - mdev->devnode.release = media_device_release; > > /* Set version 0 to indicate user-space that the graph is static */ > mdev->topology_version = 0;
Hi Sakari, Thank you for the patch. On Wed, Dec 20, 2023 at 12:36:53PM +0200, Sakari Ailus wrote: > cdev_device_del() is the right function to remove a device when > cdev_device_add() succeeds. If it does not, however, put_device() needs to > be used instead. Fix this. Where's the put_device() call ? > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/mc/mc-devnode.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c > index ce93ab9be676..7e22938dfd81 100644 > --- a/drivers/media/mc/mc-devnode.c > +++ b/drivers/media/mc/mc-devnode.c > @@ -254,7 +254,6 @@ int __must_check media_devnode_register(struct media_devnode *devnode, > > cdev_add_error: > mutex_lock(&media_devnode_lock); > - cdev_device_del(&devnode->cdev, &devnode->dev); > clear_bit(devnode->minor, media_devnode_nums); > mutex_unlock(&media_devnode_lock); >
Hi Sakari, Thank you for the patch. On Wed, Dec 20, 2023 at 12:36:55PM +0200, Sakari Ailus wrote: > As registering a device node of an entity belonging to a media device Did you mean s/As registering/Registering/ ? > will require a reference to the struct device. Taking that reference is > only possible once the device has been initialised, which took place only s/took/takes/ > when it was registered. Split this in two, and initialise the device when s/was/is/ > the media device is allocated. > > Don't distribute the effects of these changes yet. Add media_device_get() > and media_device_put() first. Don't propagate the effects of these changes to drivers yet, we want to expose media_device refcounting with media_device_get() and media_device_put() functions first. I'm not sure that's exactly what you meant though. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/mc/mc-device.c | 18 +++++++++++++----- > drivers/media/mc/mc-devnode.c | 17 ++++++++++------- > include/media/media-devnode.h | 19 ++++++++++++++----- > 3 files changed, 37 insertions(+), 17 deletions(-) > > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c > index c0ea08a8fc31..ebf037cd5f4a 100644 > --- a/drivers/media/mc/mc-device.c > +++ b/drivers/media/mc/mc-device.c > @@ -717,19 +717,26 @@ int __must_check __media_device_register(struct media_device *mdev, > /* Set version 0 to indicate user-space that the graph is static */ > mdev->topology_version = 0; > > + media_devnode_init(&mdev->devnode); > + > ret = media_devnode_register(&mdev->devnode, owner); > if (ret < 0) > - return ret; > + goto out_put; > > ret = device_create_file(&mdev->devnode.dev, &dev_attr_model); > - if (ret < 0) { > - media_devnode_unregister(&mdev->devnode); > - return ret; > - } > + if (ret < 0) > + goto out_unregister; > > dev_dbg(mdev->dev, "Media device registered\n"); > > return 0; > + > +out_unregister: I would name the labels err_unregister and err_put. > + media_devnode_unregister(&mdev->devnode); > +out_put: > + put_device(&mdev->devnode.dev); > + > + return ret; > } > EXPORT_SYMBOL_GPL(__media_device_register); > > @@ -803,6 +810,7 @@ void media_device_unregister(struct media_device *mdev) > device_remove_file(&mdev->devnode.dev, &dev_attr_model); > dev_dbg(mdev->dev, "Media device unregistering\n"); > media_devnode_unregister(&mdev->devnode); > + put_device(&mdev->devnode.dev); > } > EXPORT_SYMBOL_GPL(media_device_unregister); > > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c > index 8bc7450ac144..7b17419050fb 100644 > --- a/drivers/media/mc/mc-devnode.c > +++ b/drivers/media/mc/mc-devnode.c > @@ -204,6 +204,11 @@ static const struct file_operations media_devnode_fops = { > .llseek = no_llseek, > }; > > +void media_devnode_init(struct media_devnode *devnode) > +{ > + device_initialize(&devnode->dev); > +} > + > int __must_check media_devnode_register(struct media_devnode *devnode, > struct module *owner) > { > @@ -235,7 +240,6 @@ int __must_check media_devnode_register(struct media_devnode *devnode, > if (devnode->parent) > devnode->dev.parent = devnode->parent; > dev_set_name(&devnode->dev, "media%d", devnode->minor); > - device_initialize(&devnode->dev); > > /* Part 3: Add the media and character devices */ > ret = cdev_device_add(&devnode->cdev, &devnode->dev); > @@ -267,14 +271,13 @@ void media_devnode_unregister(struct media_devnode *devnode) > clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags); > mutex_unlock(&media_devnode_lock); > > - cdev_del(&devnode->cdev); > - device_unregister(&devnode->dev); > + cdev_device_del(&devnode->cdev, &devnode->dev); > } > > /* > * Initialise media for linux > */ > -static int __init media_devnode_init(void) > +static int __init media_devnode_module_init(void) > { > int ret; > > @@ -296,14 +299,14 @@ static int __init media_devnode_init(void) > return 0; > } > > -static void __exit media_devnode_exit(void) > +static void __exit media_devnode_module_exit(void) > { > bus_unregister(&media_bus_type); > unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES); > } > > -subsys_initcall(media_devnode_init); > -module_exit(media_devnode_exit) > +subsys_initcall(media_devnode_module_init); > +module_exit(media_devnode_module_exit) > > MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>"); > MODULE_DESCRIPTION("Device node registration for media drivers"); > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h > index 1117d1dfd6bf..6d46c658be21 100644 > --- a/include/media/media-devnode.h > +++ b/include/media/media-devnode.h > @@ -90,6 +90,17 @@ struct media_devnode { > /* dev to media_devnode */ > #define to_media_devnode(cd) container_of(cd, struct media_devnode, dev) > > +/** > + * media_devnode_init - initialise a media devnode > + * > + * @devnode: struct media_devnode we want to initialise > + * > + * Initialise a media devnode. Note that after initialising the media > + * devnode is refcounted. Releasing references to it may be done using > + * put_device(). > + */ > +void media_devnode_init(struct media_devnode *devnode); > + > /** > * media_devnode_register - register a media device node > * > @@ -100,11 +111,9 @@ struct media_devnode { > * with the kernel. An error is returned if no free minor number can be found, > * or if the registration of the device node fails. > * > - * Zero is returned on success. > - * > - * Note that if the media_devnode_register call fails, the release() callback of > - * the media_devnode structure is *not* called, so the caller is responsible for > - * freeing any data. > + * Zero is returned on success. Note that in case > + * media_devnode_register() fails, the caller is responsible for > + * releasing the reference to the device using put_device(). > */ > int __must_check media_devnode_register(struct media_devnode *devnode, > struct module *owner);
Hi Sakari, Thank you for the patch. On Wed, Dec 20, 2023 at 12:36:57PM +0200, Sakari Ailus wrote: > Call media_devnode_init() from media_device_init(). This has the effect of > creating a struct device for the media_devnode before it is registered, > making it possible to obtain a reference to it for e.g. video devices. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/mc/mc-device.c | 21 +++++++-------------- > 1 file changed, 7 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c > index 44685ab6a450..e6ac9b066524 100644 > --- a/drivers/media/mc/mc-device.c > +++ b/drivers/media/mc/mc-device.c > @@ -711,8 +711,8 @@ void media_device_init(struct media_device *mdev) > mutex_init(&mdev->req_queue_mutex); > mutex_init(&mdev->graph_mutex); > ida_init(&mdev->entity_internal_idx); > - > atomic_set(&mdev->request_id, 0); I would add a blank line here. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + media_devnode_init(&mdev->devnode); > > if (!*mdev->bus_info) > media_set_bus_info(mdev->bus_info, sizeof(mdev->bus_info), > @@ -729,6 +729,7 @@ void media_device_cleanup(struct media_device *mdev) > media_graph_walk_cleanup(&mdev->pm_count_walk); > mutex_destroy(&mdev->graph_mutex); > mutex_destroy(&mdev->req_queue_mutex); > + put_device(&mdev->devnode.dev); > } > EXPORT_SYMBOL_GPL(media_device_cleanup); > > @@ -744,26 +745,19 @@ int __must_check __media_device_register(struct media_device *mdev, > /* Set version 0 to indicate user-space that the graph is static */ > mdev->topology_version = 0; > > - media_devnode_init(&mdev->devnode); > - > ret = media_devnode_register(&mdev->devnode, owner); > if (ret < 0) > - goto out_put; > + return ret; > > ret = device_create_file(&mdev->devnode.dev, &dev_attr_model); > - if (ret < 0) > - goto out_unregister; > + if (ret < 0) { > + media_devnode_unregister(&mdev->devnode); > + return ret; > + } > > dev_dbg(mdev->dev, "Media device registered\n"); > > return 0; > - > -out_unregister: > - media_devnode_unregister(&mdev->devnode); > -out_put: > - put_device(&mdev->devnode.dev); > - > - return ret; > } > EXPORT_SYMBOL_GPL(__media_device_register); > > @@ -810,7 +804,6 @@ void media_device_unregister(struct media_device *mdev) > device_remove_file(&mdev->devnode.dev, &dev_attr_model); > dev_dbg(mdev->dev, "Media device unregistering\n"); > media_devnode_unregister(&mdev->devnode); > - put_device(&mdev->devnode.dev); > } > EXPORT_SYMBOL_GPL(media_device_unregister); >
Hi Sakari, I've just noticed that I send multiple Reviewed-by tags for this series using Laurent Pinchart <laurent.pinchart@ideasonboard.com> when I meant Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>. Could you replace the e-mail address when picking up the tags ?Sorry about the inconvenience. On Wed, Dec 20, 2023 at 12:36:44PM +0200, Sakari Ailus wrote: > Hi folks, > > This is a refresh of my 2016 RFC patchset to start addressing object > lifetime issues in Media controller. It further allows continuing work to > address lifetime management of media entities. > > The underlying problem is described in detail in v4 of the previous RFC: > <URL:https://lore.kernel.org/linux-media/20161108135438.GO3217@valkosipuli.retiisi.org.uk/>. > In brief, there is currently no connection between releasing media device > (and related) memory and IOCTL calls, meaning that there is a time window > during which released kernel memory can be accessed, and that access can be > triggered from the user space. The only reason why this is not a grave > security issue is that it is not triggerable by the user alone but requires > unbinding a device. That is still not an excuse for not fixing it. > > This set differs from the earlier RFC to address the issue in the > following respects: > > - Make changes for ipu3-cio2 driver, too. > > - Continue to provide best effort attempt to keep the window between device > removal and user space being able to access released memory as small as > possible. This means the problem won't become worse for drivers for which > Media device lifetime management has not been implemented. > > The latter is achieved by adding a new object, Media devnode compat > reference, which is allocated, refcounted and eventually released by the > Media controller framework itself, and where the information on registration > and open filehandles is maintained. This is only done if the driver does not > manage the lifetime of the media device itself, i.e. its release operation > is NULL. > > Due to this, Media device file handles will also be introduced by this > patchset. I thought the first user of this would be Media device events but > it seems we already need them here. > > Both ipu3-cio2 and omap3isp drivers are relieved of devm_request_irq() use, > as device_release() releases the resources before calling the driver's > remove function. While further work will be required also on these drivers > to safely stop he hardware at unbind time, I don't see a reason not to merge > these patches now. > > Some patches are temporarily reverted in order to make reworks easier, then > applied later on. > > I've tested this on ipu3-cio2 with and without the refcounting patch (media: > ipu3-cio2: Release the cio2 device context by media device callback), > including failures in a few parts of the driver initialisation process in > the MC framework. > > Questions and comments are welcome. > > since v1: > > - Align subject prefixes with current media tree practices. > > - Make release changes to the vimc driver (last patch of the set). This > was actually easy as vimc already centralised resource release to struct > v4l2_device, so it was just moved to the media device. > > - Move cdev field to struct media_devnode_compat_ref and add dev field to > the struct, these are needed during device release. This now includes > also the character device which is accessed by __fput(). I've now tested > ipu3-cio2 and vimc with KASAN. As a by-product the kref in struct > media_devnode_compat_ref becomes redundant and is removed. Both devices > are registered in case of best effort memory safety support and used for > refcounting. > > - Drop omap3isp driver patch moving away from devm_request_irq(). > > - Add a patch to warn of drivers not releasing media device safely (i.e. > relying on the best effort memory safety mechanism without refcounting). > > - Add a patch to document how the best effort memory release safety helper > works. > > - Add a note on releasing driver's context with the media device, not the > V4L2 device, in MC documentation. > > - Check media device is registered before accessing its fops in > media_read(), media_write(), media_ioctl and media_compat_ioctl(). > > - Document best effort media device lifetime management (new patch). > > - Use media_devnode_free_minor() in unallocating device node minor number > in media_devnode_register(). > > - Continue to rely on devm_register_irq() in ipu3-cio2 driver but register > the IRQ later on (compared to v1). > > - Drop the patch to move away from devm_request_irq() in omap3isp. > > - Fix putting references to media device and V4L2 device in > v4l2_device_release(). > > - Add missing media_device_get() (in v1) for M2M devices in > video_register_media_controller(). > > - Unconditionally set the media devnode release function in > media_device_init(). There's no harm doing so and the caller of > media_device_init() may set the ops after calling the function. > > Daniel Axtens (1): > media: uvcvideo: Refactor teardown of uvc on USB disconnect > > Laurent Pinchart (1): > media: mc: Add per-file-handle data support > > Logan Gunthorpe (1): > media: mc: utilize new cdev_device_add helper function > > Sakari Ailus (26): > Revert "[media] media: fix media devnode ioctl/syscall and unregister > race" > Revert "media: utilize new cdev_device_add helper function" > Revert "[media] media: fix use-after-free in cdev_put() when app exits > after driver unbind" > Revert "media: uvcvideo: Refactor teardown of uvc on USB disconnect" > Revert "[media] media-device: dynamically allocate struct > media_devnode" > media: mc: Drop nop release callback > media: mc: Do not call cdev_device_del() if cdev_device_add() fails > media: mc: Delete character device early > media: mc: Split initialising and adding media devnode > media: mc: Shuffle functions around > media: mc: Initialise media devnode in media_device_init() > media: mc: Refactor media devnode minor clearing > media: mc: Unassign minor only if it has been assigned > media: mc: Refcount the media device > media: v4l: Acquire a reference to the media device for every video > device > media: mc: Postpone graph object removal until free > media: omap3isp: Release the isp device struct by media device > callback > media: ipu3-cio2: Call v4l2_device_unregister() earlier > media: ipu3-cio2: Request IRQ earlier > media: ipu3-cio2: Release the cio2 device context by media device > callback > media: vimc: Release resources on media device release > media: Documentation: Document how Media device resources are released > media: mc: Maintain a list of open file handles in a media device > media: mc: Implement best effort media device removal safety sans > refcount > media: mc: Warn about drivers not releasing media device safely > media: Documentation: Document media device memory safety helper > > Documentation/driver-api/media/mc-core.rst | 18 +- > drivers/media/cec/core/cec-core.c | 2 +- > drivers/media/mc/mc-device.c | 260 ++++++++++++-------- > drivers/media/mc/mc-devnode.c | 230 +++++++++++------ > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 70 ++++-- > drivers/media/platform/ti/omap3isp/isp.c | 24 +- > drivers/media/test-drivers/vimc/vimc-core.c | 15 +- > drivers/media/usb/au0828/au0828-core.c | 4 +- > drivers/media/usb/uvc/uvc_driver.c | 2 +- > drivers/media/v4l2-core/v4l2-dev.c | 65 +++-- > drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +- > include/media/media-device.h | 46 +++- > include/media/media-devnode.h | 136 +++++++--- > include/media/media-fh.h | 32 +++ > 14 files changed, 632 insertions(+), 274 deletions(-) > create mode 100644 include/media/media-fh.h
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@linux.intel.com> > --- > 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.
On Mon, Feb 05, 2024 at 03:56:22PM +0100, Hans Verkuil wrote: > On 20/12/2023 11:37, Sakari Ailus wrote: > > The video device depends on the existence of its media device --- if there > > is one. Acquire a reference to it. > > > > Note that when the media device release callback is used, then the V4L2 > > device release callback is ignored and a warning is issued if both are > > set. Why is that ? The two are distinct objects, why can't they both have a release function ? > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-dev.c | 51 ++++++++++++++++++++---------- > > 1 file changed, 34 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > > index d13954bd31fd..c1e4995eaf5c 100644 > > --- a/drivers/media/v4l2-core/v4l2-dev.c > > +++ b/drivers/media/v4l2-core/v4l2-dev.c > > @@ -176,6 +176,11 @@ static void v4l2_device_release(struct device *cd) > > { > > struct video_device *vdev = to_video_device(cd); > > struct v4l2_device *v4l2_dev = vdev->v4l2_dev; > > + bool v4l2_dev_has_release = v4l2_dev->release; > > +#ifdef CONFIG_MEDIA_CONTROLLER > > + struct media_device *mdev = v4l2_dev->mdev; > > + bool mdev_has_release = mdev && mdev->ops && mdev->ops->release; > > +#endif > > > > mutex_lock(&videodev_lock); > > if (WARN_ON(video_devices[vdev->minor] != vdev)) { > > @@ -198,8 +203,8 @@ static void v4l2_device_release(struct device *cd) > > > > mutex_unlock(&videodev_lock); > > > > -#if defined(CONFIG_MEDIA_CONTROLLER) > > - if (v4l2_dev->mdev && vdev->vfl_dir != VFL_DIR_M2M) { > > +#ifdef CONFIG_MEDIA_CONTROLLER > > + if (mdev && vdev->vfl_dir != VFL_DIR_M2M) { > > /* Remove interfaces and interface links */ > > media_devnode_remove(vdev->intf_devnode); > > if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN) > > @@ -207,23 +212,31 @@ static void v4l2_device_release(struct device *cd) > > } > > #endif > > > > - /* Do not call v4l2_device_put if there is no release callback set. > > - * Drivers that have no v4l2_device release callback might free the > > - * v4l2_dev instance in the video_device release callback below, so we > > - * must perform this check here. > > - * > > - * TODO: In the long run all drivers that use v4l2_device should use the > > - * v4l2_device release callback. This check will then be unnecessary. > > - */ > > - if (v4l2_dev->release == NULL) > > - v4l2_dev = NULL; > > - > > /* Release video_device and perform other > > cleanups as needed. */ > > vdev->release(vdev); > > > > - /* Decrease v4l2_device refcount */ > > - if (v4l2_dev) > > +#ifdef CONFIG_MEDIA_CONTROLLER > > + if (mdev) > > + media_device_put(mdev); > > + > > + /* > > + * Generally both struct media_device and struct v4l2_device are > > + * embedded in the same driver's context struct so having a release > > + * callback in both is a bug. > > + */ > > + WARN_ON(v4l2_dev_has_release && mdev_has_release); > > How about: > > if (WARN_ON(v4l2_dev_has_release && mdev_has_release)) > v4l2_dev_has_release = false; > > > +#endif > > + > > + /* > > + * Decrease v4l2_device refcount, but only if the media device doesn't > > + * have a release callback. > > + */ > > + if (v4l2_dev_has_release > > +#ifdef CONFIG_MEDIA_CONTROLLER > > + && !mdev_has_release > > +#endif > > + ) > > Then this change is no longer needed. > > General question: do we have drivers today that set both release functions? > Because that would now cause a WARN in the kernel log with this patch. > > > v4l2_device_put(v4l2_dev); > > } > > > > @@ -792,11 +805,14 @@ static int video_register_media_controller(struct video_device *vdev) > > u32 intf_type; > > int ret; > > > > - /* Memory-to-memory devices are more complex and use > > + /* > > + * Memory-to-memory devices are more complex and use > > * their own function to register its mc entities. If you fix the comment style as a drive-by change, you could as well reflow it to 80 columns. > > */ > > - if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M) > > + if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M) { > > + media_device_get(vdev->v4l2_dev->mdev); > > return 0; > > + } > > > > vdev->entity.obj_type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE; > > vdev->entity.function = MEDIA_ENT_F_UNKNOWN; > > @@ -875,6 +891,7 @@ static int video_register_media_controller(struct video_device *vdev) > > > > /* FIXME: how to create the other interface links? */ > > > > + media_device_get(vdev->v4l2_dev->mdev); > > #endif > > return 0; > > }
Hi Sakari, Thank you for the patch. On Wed, Dec 20, 2023 at 12:37:03PM +0200, Sakari Ailus wrote: > Use the media device release callback to release the isp device's data > structure. This approach has the benefit of not releasing memory which may > still be accessed through open file handles whilst the isp driver is being > unbound. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/platform/ti/omap3isp/isp.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/platform/ti/omap3isp/isp.c b/drivers/media/platform/ti/omap3isp/isp.c > index 1cda23244c7b..ef6a781d6da1 100644 > --- a/drivers/media/platform/ti/omap3isp/isp.c > +++ b/drivers/media/platform/ti/omap3isp/isp.c > @@ -649,8 +649,11 @@ static irqreturn_t isp_isr(int irq, void *_isp) > return IRQ_HANDLED; > } > > +static void isp_release(struct media_device *mdev); > + Forward declarations are not nice :-( Any hope to reorder functions to fix this ? > static const struct media_device_ops isp_media_ops = { > .link_notify = v4l2_pipeline_link_notify, > + .release = isp_release, > }; > > /* ----------------------------------------------------------------------------- > @@ -1607,7 +1610,6 @@ static void isp_unregister_entities(struct isp_device *isp) > omap3isp_stat_unregister_entities(&isp->isp_hist); > > v4l2_device_unregister(&isp->v4l2_dev); > - media_device_cleanup(&isp->media_dev); > } > > static int isp_link_entity( > @@ -1955,6 +1957,19 @@ static void isp_detach_iommu(struct isp_device *isp) > #endif > } > > +static void isp_release(struct media_device *mdev) > +{ > + struct isp_device *isp = > + container_of(mdev, struct isp_device, media_dev); > + > + isp_cleanup_modules(isp); > + > + media_entity_enum_cleanup(&isp->crashed); > + v4l2_async_nf_cleanup(&isp->notifier); This duplicates the call in isp_remove(). > + > + kfree(isp); > +} > + > static int isp_attach_iommu(struct isp_device *isp) > { > #ifdef CONFIG_ARM_DMA_USE_IOMMU > @@ -2004,16 +2019,15 @@ static void isp_remove(struct platform_device *pdev) > v4l2_async_nf_unregister(&isp->notifier); > v4l2_async_nf_cleanup(&isp->notifier); > isp_unregister_entities(isp); > - isp_cleanup_modules(isp); > + > isp_xclk_cleanup(isp); > > __omap3isp_get(isp, false); > isp_detach_iommu(isp); > __omap3isp_put(isp, false); > > - media_entity_enum_cleanup(&isp->crashed); > - > - kfree(isp); > + /* May release isp immediately */ > + media_device_put(&isp->media_dev); > } > > enum isp_of_phy {
On Mon, Feb 05, 2024 at 03:58:45PM +0100, Hans Verkuil wrote: > On 20/12/2023 11:37, Sakari Ailus wrote: > > Call devm_request_irq() before registering the async notifier, as otherwise > > it would be possible to use the device before the interrupts could be > > deliveted to the driver. > > deliveted -> delivered > > Isn't this a regular fix? Ditto for the previous patch (20/29). > > I'd just queue this up in the next PR. Fixes: tags would then be nice. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > index da82d09b46ab..3222ec5b8345 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > @@ -1789,11 +1789,6 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > > > > v4l2_async_nf_init(&cio2->notifier, &cio2->v4l2_dev); > > > > - /* Register notifier for subdevices we care */ > > - r = cio2_parse_firmware(cio2); > > - if (r) > > - goto fail_clean_notifier; > > - > > r = devm_request_irq(dev, pci_dev->irq, cio2_irq, IRQF_SHARED, > > CIO2_NAME, cio2); > > if (r) { > > @@ -1801,6 +1796,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > > goto fail_clean_notifier; > > } > > > > + /* Register notifier for subdevices we care */ > > + r = cio2_parse_firmware(cio2); > > + if (r) > > + goto fail_clean_notifier; > > + > > pm_runtime_put_noidle(dev); > > pm_runtime_allow(dev); > >
On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: > On 20/12/2023 11:37, Sakari Ailus wrote: > > Release all the resources when the media device is related, moving away s/related/released/ > > form the struct v4l2_device used for that purpose. > > form -> from Please explain *why* in the commit message. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c > > index af127476e920..3e59f8c256c7 100644 > > --- a/drivers/media/test-drivers/vimc/vimc-core.c > > +++ b/drivers/media/test-drivers/vimc/vimc-core.c > > @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) > > return 0; > > } > > > > -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > > +static void vimc_mdev_release(struct media_device *mdev) > > { > > struct vimc_device *vimc = > > - container_of(v4l2_dev, struct vimc_device, v4l2_dev); > > + container_of_const(mdev, struct vimc_device, mdev); > > Why this change? > > > > > vimc_release_subdevs(vimc); > > - media_device_cleanup(&vimc->mdev); > > kfree(vimc->ent_devs); > > kfree(vimc); > > } > > @@ -336,6 +335,10 @@ static int vimc_register_devices(struct vimc_device *vimc) > > return ret; > > } > > > > +static const struct media_device_ops vimc_mdev_ops = { > > + .release = vimc_mdev_release, > > +}; > > + > > static int vimc_probe(struct platform_device *pdev) > > { > > const struct font_desc *font = find_font("VGA8x16"); > > @@ -369,12 +372,12 @@ static int vimc_probe(struct platform_device *pdev) > > snprintf(vimc->mdev.bus_info, sizeof(vimc->mdev.bus_info), > > "platform:%s", VIMC_PDEV_NAME); > > vimc->mdev.dev = &pdev->dev; > > + vimc->mdev.ops = &vimc_mdev_ops; > > media_device_init(&vimc->mdev); > > > > ret = vimc_register_devices(vimc); > > if (ret) { > > - media_device_cleanup(&vimc->mdev); > > - kfree(vimc); > > + media_device_put(&vimc->mdev); > > return ret; > > } > > /* > > @@ -382,7 +385,6 @@ static int vimc_probe(struct platform_device *pdev) > > * if the registration fails, we release directly from probe > > */ > > > > - vimc->v4l2_dev.release = vimc_v4l2_dev_release; > > platform_set_drvdata(pdev, vimc); > > return 0; > > } > > @@ -397,6 +399,7 @@ static void vimc_remove(struct platform_device *pdev) > > media_device_unregister(&vimc->mdev); > > v4l2_device_unregister(&vimc->v4l2_dev); > > v4l2_device_put(&vimc->v4l2_dev); > > + media_device_put(&vimc->mdev); > > } > > > > static void vimc_dev_release(struct device *dev)
Hi Laurent, Hans, On Wed, Feb 07, 2024 at 04:34:18PM +0200, Laurent Pinchart wrote: > On Mon, Feb 05, 2024 at 03:58:45PM +0100, Hans Verkuil wrote: > > On 20/12/2023 11:37, Sakari Ailus wrote: > > > Call devm_request_irq() before registering the async notifier, as otherwise > > > it would be possible to use the device before the interrupts could be > > > deliveted to the driver. > > > > deliveted -> delivered > > > > Isn't this a regular fix? Ditto for the previous patch (20/29). > > > > I'd just queue this up in the next PR. Yeah, I wrote it as part of the set but missed there are no further dependencies. I'll post a new version of this separately. > > Fixes: tags would then be nice. It was in the patch adding the driver. I'll add Fixes: line.
Hi Laurent, On Wed, Feb 07, 2024 at 04:38:05PM +0200, Laurent Pinchart wrote: > On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: > > On 20/12/2023 11:37, Sakari Ailus wrote: > > > Release all the resources when the media device is related, moving away > > s/related/released/ > > > > form the struct v4l2_device used for that purpose. > > > > form -> from > > Please explain *why* in the commit message. Just to show how the media device release callback is used. I'll add that to the commit message.
On Wed, Feb 21, 2024 at 10:53:43AM +0000, Sakari Ailus wrote: > Hi Hans, > > On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: > > On 20/12/2023 11:37, Sakari Ailus wrote: > > > Release all the resources when the media device is related, moving away > > s/related/released/ > > > > form the struct v4l2_device used for that purpose. > > > > form -> from > > Yes. > > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c > > > index af127476e920..3e59f8c256c7 100644 > > > --- a/drivers/media/test-drivers/vimc/vimc-core.c > > > +++ b/drivers/media/test-drivers/vimc/vimc-core.c > > > @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) > > > return 0; > > > } > > > > > > -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > > > +static void vimc_mdev_release(struct media_device *mdev) > > > { > > > struct vimc_device *vimc = > > > - container_of(v4l2_dev, struct vimc_device, v4l2_dev); > > > + container_of_const(mdev, struct vimc_device, mdev); > > > > Why this change? > > I changed the line already. There's no reason to continue using > container_of() instead of container_of_const() that takes const-ness into > account, too. It should then be at least mentioned in the commit message. Any plan to switch to container_of_const() globally in the subsystem ? > > > > > > vimc_release_subdevs(vimc); > > > - media_device_cleanup(&vimc->mdev); > > > kfree(vimc->ent_devs); > > > kfree(vimc); > > > } > > > @@ -336,6 +335,10 @@ static int vimc_register_devices(struct vimc_device *vimc) > > > return ret; > > > } > > > > > > +static const struct media_device_ops vimc_mdev_ops = { > > > + .release = vimc_mdev_release, > > > +}; > > > + > > > static int vimc_probe(struct platform_device *pdev) > > > { > > > const struct font_desc *font = find_font("VGA8x16"); > > > @@ -369,12 +372,12 @@ static int vimc_probe(struct platform_device *pdev) > > > snprintf(vimc->mdev.bus_info, sizeof(vimc->mdev.bus_info), > > > "platform:%s", VIMC_PDEV_NAME); > > > vimc->mdev.dev = &pdev->dev; > > > + vimc->mdev.ops = &vimc_mdev_ops; > > > media_device_init(&vimc->mdev); > > > > > > ret = vimc_register_devices(vimc); > > > if (ret) { > > > - media_device_cleanup(&vimc->mdev); > > > - kfree(vimc); > > > + media_device_put(&vimc->mdev); > > > return ret; > > > } > > > /* > > > @@ -382,7 +385,6 @@ static int vimc_probe(struct platform_device *pdev) > > > * if the registration fails, we release directly from probe > > > */ > > > > > > - vimc->v4l2_dev.release = vimc_v4l2_dev_release; > > > platform_set_drvdata(pdev, vimc); > > > return 0; > > > } > > > @@ -397,6 +399,7 @@ static void vimc_remove(struct platform_device *pdev) > > > media_device_unregister(&vimc->mdev); > > > v4l2_device_unregister(&vimc->v4l2_dev); > > > v4l2_device_put(&vimc->v4l2_dev); > > > + media_device_put(&vimc->mdev); > > > } > > > > > > static void vimc_dev_release(struct device *dev)
Hi Hans, On Wed, Feb 21, 2024 at 12:19:21PM +0100, Hans Verkuil wrote: > On 21/02/2024 11:53, Sakari Ailus wrote: > > Hi Hans, > > > > On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: > >> On 20/12/2023 11:37, Sakari Ailus wrote: > >>> Release all the resources when the media device is related, moving away > > > > s/related/released/ > > > >>> form the struct v4l2_device used for that purpose. > >> > >> form -> from > > > > Yes. > > > >> > >>> > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >>> --- > >>> drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ > >>> 1 file changed, 9 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c > >>> index af127476e920..3e59f8c256c7 100644 > >>> --- a/drivers/media/test-drivers/vimc/vimc-core.c > >>> +++ b/drivers/media/test-drivers/vimc/vimc-core.c > >>> @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) > >>> return 0; > >>> } > >>> > >>> -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > >>> +static void vimc_mdev_release(struct media_device *mdev) > >>> { > >>> struct vimc_device *vimc = > >>> - container_of(v4l2_dev, struct vimc_device, v4l2_dev); > >>> + container_of_const(mdev, struct vimc_device, mdev); > >> > >> Why this change? > > > > I changed the line already. There's no reason to continue using > > container_of() instead of container_of_const() that takes const-ness into > > account, too. > > But neither vimc nor mdev can be const anyway, that would make no sense > here. Neither is const, true. Yet container_of_const() is preferred over container_of(), due to the fact that it does take const-ness into account. container_of() should really be avoided. I'll add this to the commit message as Laurent suggested.
On 21/02/2024 12:40, Sakari Ailus wrote: > Hi Hans, > > On Wed, Feb 21, 2024 at 12:19:21PM +0100, Hans Verkuil wrote: >> On 21/02/2024 11:53, Sakari Ailus wrote: >>> Hi Hans, >>> >>> On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: >>>> On 20/12/2023 11:37, Sakari Ailus wrote: >>>>> Release all the resources when the media device is related, moving away >>> >>> s/related/released/ >>> >>>>> form the struct v4l2_device used for that purpose. >>>> >>>> form -> from >>> >>> Yes. >>> >>>> >>>>> >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>>> --- >>>>> drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ >>>>> 1 file changed, 9 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c >>>>> index af127476e920..3e59f8c256c7 100644 >>>>> --- a/drivers/media/test-drivers/vimc/vimc-core.c >>>>> +++ b/drivers/media/test-drivers/vimc/vimc-core.c >>>>> @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) >>>>> return 0; >>>>> } >>>>> >>>>> -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) >>>>> +static void vimc_mdev_release(struct media_device *mdev) >>>>> { >>>>> struct vimc_device *vimc = >>>>> - container_of(v4l2_dev, struct vimc_device, v4l2_dev); >>>>> + container_of_const(mdev, struct vimc_device, mdev); >>>> >>>> Why this change? >>> >>> I changed the line already. There's no reason to continue using >>> container_of() instead of container_of_const() that takes const-ness into >>> account, too. >> >> But neither vimc nor mdev can be const anyway, that would make no sense >> here. > > Neither is const, true. Yet container_of_const() is preferred over Says who? It makes sense in generic defines that use it, e.g.: drivers/base/firmware_loader/sysfs.h:#define to_fw_sysfs(__dev) container_of_const(__dev, struct fw_sysfs, dev) That way it can handle both const and non-const __dev pointers. In cases where this doesn't come into play I think there is no need to make code changes. Perhaps when writing new code it might make sense to use it, but changing it in existing code, esp. as part of a patch that deals with something else entirely, seems just unnecessary churn. I won't block this, but I recommend just dropping this change in this patch. Regards, Hans > container_of(), due to the fact that it does take const-ness into account. > container_of() should really be avoided. > > I'll add this to the commit message as Laurent suggested. >
On Wed, Feb 21, 2024 at 10:43:47AM +0000, Sakari Ailus wrote: > Hi Laurent, > > Thank you for reviewing the set! > > On Wed, Feb 07, 2024 at 01:13:44PM +0200, Laurent Pinchart wrote: > > On Mon, Feb 05, 2024 at 03:56:22PM +0100, Hans Verkuil wrote: > > > On 20/12/2023 11:37, Sakari Ailus wrote: > > > > The video device depends on the existence of its media device --- if there > > > > is one. Acquire a reference to it. > > > > > > > > Note that when the media device release callback is used, then the V4L2 > > > > device release callback is ignored and a warning is issued if both are > > > > set. > > > > Why is that ? The two are distinct objects, why can't they both have a > > release function ? > > You could, in principle, but in practice both of the structs are part of > the same driver's device context struct which is a single allocation. You > can only have a single release callback for it. If both release callbacks freed the same data structure, that would indeed be a problem. There could be other use cases though. For instance, in the uvcvideo driver, the top-level structure is reference-counted, and the release callbacks of the video devices decrement that reference count. I don't expect drivers to do something similar with media_device and v4l2_device, but I'm not sure if we should forbid it completely. If we do, I would then rather deprecate the release callback of v4l2_device completely. > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > --- > > > > drivers/media/v4l2-core/v4l2-dev.c | 51 ++++++++++++++++++++---------- > > > > 1 file changed, 34 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > > > > index d13954bd31fd..c1e4995eaf5c 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-dev.c > > > > +++ b/drivers/media/v4l2-core/v4l2-dev.c > > > > @@ -176,6 +176,11 @@ static void v4l2_device_release(struct device *cd) > > > > { > > > > struct video_device *vdev = to_video_device(cd); > > > > struct v4l2_device *v4l2_dev = vdev->v4l2_dev; > > > > + bool v4l2_dev_has_release = v4l2_dev->release; > > > > +#ifdef CONFIG_MEDIA_CONTROLLER > > > > + struct media_device *mdev = v4l2_dev->mdev; > > > > + bool mdev_has_release = mdev && mdev->ops && mdev->ops->release; > > > > +#endif > > > > > > > > mutex_lock(&videodev_lock); > > > > if (WARN_ON(video_devices[vdev->minor] != vdev)) { > > > > @@ -198,8 +203,8 @@ static void v4l2_device_release(struct device *cd) > > > > > > > > mutex_unlock(&videodev_lock); > > > > > > > > -#if defined(CONFIG_MEDIA_CONTROLLER) > > > > - if (v4l2_dev->mdev && vdev->vfl_dir != VFL_DIR_M2M) { > > > > +#ifdef CONFIG_MEDIA_CONTROLLER > > > > + if (mdev && vdev->vfl_dir != VFL_DIR_M2M) { > > > > /* Remove interfaces and interface links */ > > > > media_devnode_remove(vdev->intf_devnode); > > > > if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN) > > > > @@ -207,23 +212,31 @@ static void v4l2_device_release(struct device *cd) > > > > } > > > > #endif > > > > > > > > - /* Do not call v4l2_device_put if there is no release callback set. > > > > - * Drivers that have no v4l2_device release callback might free the > > > > - * v4l2_dev instance in the video_device release callback below, so we > > > > - * must perform this check here. > > > > - * > > > > - * TODO: In the long run all drivers that use v4l2_device should use the > > > > - * v4l2_device release callback. This check will then be unnecessary. > > > > - */ > > > > - if (v4l2_dev->release == NULL) > > > > - v4l2_dev = NULL; > > > > - > > > > /* Release video_device and perform other > > > > cleanups as needed. */ > > > > vdev->release(vdev); > > > > > > > > - /* Decrease v4l2_device refcount */ > > > > - if (v4l2_dev) > > > > +#ifdef CONFIG_MEDIA_CONTROLLER > > > > + if (mdev) > > > > + media_device_put(mdev); > > > > + > > > > + /* > > > > + * Generally both struct media_device and struct v4l2_device are > > > > + * embedded in the same driver's context struct so having a release > > > > + * callback in both is a bug. > > > > + */ > > > > + WARN_ON(v4l2_dev_has_release && mdev_has_release); > > > > > > How about: > > > > > > if (WARN_ON(v4l2_dev_has_release && mdev_has_release)) > > > v4l2_dev_has_release = false; > > > > > > > +#endif > > > > + > > > > + /* > > > > + * Decrease v4l2_device refcount, but only if the media device doesn't > > > > + * have a release callback. > > > > + */ > > > > + if (v4l2_dev_has_release > > > > +#ifdef CONFIG_MEDIA_CONTROLLER > > > > + && !mdev_has_release > > > > +#endif > > > > + ) > > > > > > Then this change is no longer needed. > > > > > > General question: do we have drivers today that set both release functions? > > > Because that would now cause a WARN in the kernel log with this patch. > > > > > > > v4l2_device_put(v4l2_dev); > > > > } > > > > > > > > @@ -792,11 +805,14 @@ static int video_register_media_controller(struct video_device *vdev) > > > > u32 intf_type; > > > > int ret; > > > > > > > > - /* Memory-to-memory devices are more complex and use > > > > + /* > > > > + * Memory-to-memory devices are more complex and use > > > > * their own function to register its mc entities. > > > > If you fix the comment style as a drive-by change, you could as well > > reflow it to 80 columns. > > I'll update this for the next version. > > > > > */ > > > > - if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M) > > > > + if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M) { > > > > + media_device_get(vdev->v4l2_dev->mdev); > > > > return 0; > > > > + } > > > > > > > > vdev->entity.obj_type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE; > > > > vdev->entity.function = MEDIA_ENT_F_UNKNOWN; > > > > @@ -875,6 +891,7 @@ static int video_register_media_controller(struct video_device *vdev) > > > > > > > > /* FIXME: how to create the other interface links? */ > > > > > > > > + media_device_get(vdev->v4l2_dev->mdev); > > > > #endif > > > > return 0; > > > > }
On Wed, Feb 07, 2024 at 12:55:00PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > I've just noticed that I send multiple Reviewed-by tags for this series > using Laurent Pinchart <laurent.pinchart@ideasonboard.com> when I meant > Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>. Could you > replace the e-mail address when picking up the tags ?Sorry about the > inconvenience. No problem. I'll do it for v3.
Hi Laurent, Thanks for the review. On Wed, Feb 07, 2024 at 04:23:11PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Wed, Dec 20, 2023 at 12:37:03PM +0200, Sakari Ailus wrote: > > Use the media device release callback to release the isp device's data > > structure. This approach has the benefit of not releasing memory which may > > still be accessed through open file handles whilst the isp driver is being > > unbound. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > --- > > drivers/media/platform/ti/omap3isp/isp.c | 24 +++++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/platform/ti/omap3isp/isp.c b/drivers/media/platform/ti/omap3isp/isp.c > > index 1cda23244c7b..ef6a781d6da1 100644 > > --- a/drivers/media/platform/ti/omap3isp/isp.c > > +++ b/drivers/media/platform/ti/omap3isp/isp.c > > @@ -649,8 +649,11 @@ static irqreturn_t isp_isr(int irq, void *_isp) > > return IRQ_HANDLED; > > } > > > > +static void isp_release(struct media_device *mdev); > > + > > Forward declarations are not nice :-( Any hope to reorder functions to > fix this ? This depends on moving isp_cleanup_modules up, too. If you think it's the lesser evil I'm fine with that. > > > static const struct media_device_ops isp_media_ops = { > > .link_notify = v4l2_pipeline_link_notify, > > + .release = isp_release, > > }; > > > > /* ----------------------------------------------------------------------------- > > @@ -1607,7 +1610,6 @@ static void isp_unregister_entities(struct isp_device *isp) > > omap3isp_stat_unregister_entities(&isp->isp_hist); > > > > v4l2_device_unregister(&isp->v4l2_dev); > > - media_device_cleanup(&isp->media_dev); > > } > > > > static int isp_link_entity( > > @@ -1955,6 +1957,19 @@ static void isp_detach_iommu(struct isp_device *isp) > > #endif > > } > > > > +static void isp_release(struct media_device *mdev) > > +{ > > + struct isp_device *isp = > > + container_of(mdev, struct isp_device, media_dev); > > + > > + isp_cleanup_modules(isp); > > + > > + media_entity_enum_cleanup(&isp->crashed); > > + v4l2_async_nf_cleanup(&isp->notifier); > > This duplicates the call in isp_remove(). I'll drop the one in isp_remove(). > > > + > > + kfree(isp); > > +} > > + > > static int isp_attach_iommu(struct isp_device *isp) > > { > > #ifdef CONFIG_ARM_DMA_USE_IOMMU > > @@ -2004,16 +2019,15 @@ static void isp_remove(struct platform_device *pdev) > > v4l2_async_nf_unregister(&isp->notifier); > > v4l2_async_nf_cleanup(&isp->notifier); > > isp_unregister_entities(isp); > > - isp_cleanup_modules(isp); > > + > > isp_xclk_cleanup(isp); > > > > __omap3isp_get(isp, false); > > isp_detach_iommu(isp); > > __omap3isp_put(isp, false); > > > > - media_entity_enum_cleanup(&isp->crashed); > > - > > - kfree(isp); > > + /* May release isp immediately */ > > + media_device_put(&isp->media_dev); > > } > > > > enum isp_of_phy { >
Hi folks, This is a refresh of my 2016 RFC patchset to start addressing object lifetime issues in Media controller. It further allows continuing work to address lifetime management of media entities. The underlying problem is described in detail in v4 of the previous RFC: <URL:https://lore.kernel.org/linux-media/20161108135438.GO3217@valkosipuli.retiisi.org.uk/>. In brief, there is currently no connection between releasing media device (and related) memory and IOCTL calls, meaning that there is a time window during which released kernel memory can be accessed, and that access can be triggered from the user space. The only reason why this is not a grave security issue is that it is not triggerable by the user alone but requires unbinding a device. That is still not an excuse for not fixing it. This set differs from the earlier RFC to address the issue in the following respects: - Make changes for ipu3-cio2 driver, too. - Continue to provide best effort attempt to keep the window between device removal and user space being able to access released memory as small as possible. This means the problem won't become worse for drivers for which Media device lifetime management has not been implemented. The latter is achieved by adding a new object, Media devnode compat reference, which is allocated, refcounted and eventually released by the Media controller framework itself, and where the information on registration and open filehandles is maintained. This is only done if the driver does not manage the lifetime of the media device itself, i.e. its release operation is NULL. Due to this, Media device file handles will also be introduced by this patchset. I thought the first user of this would be Media device events but it seems we already need them here. Both ipu3-cio2 and omap3isp drivers are relieved of devm_request_irq() use, as device_release() releases the resources before calling the driver's remove function. While further work will be required also on these drivers to safely stop he hardware at unbind time, I don't see a reason not to merge these patches now. Some patches are temporarily reverted in order to make reworks easier, then applied later on. I've tested this on ipu3-cio2 with and without the refcounting patch (media: ipu3-cio2: Release the cio2 device context by media device callback), including failures in a few parts of the driver initialisation process in the MC framework. Questions and comments are welcome. since v1: - Align subject prefixes with current media tree practices. - Make release changes to the vimc driver (last patch of the set). This was actually easy as vimc already centralised resource release to struct v4l2_device, so it was just moved to the media device. - Move cdev field to struct media_devnode_compat_ref and add dev field to the struct, these are needed during device release. This now includes also the character device which is accessed by __fput(). I've now tested ipu3-cio2 and vimc with KASAN. As a by-product the kref in struct media_devnode_compat_ref becomes redundant and is removed. Both devices are registered in case of best effort memory safety support and used for refcounting. - Drop omap3isp driver patch moving away from devm_request_irq(). - Add a patch to warn of drivers not releasing media device safely (i.e. relying on the best effort memory safety mechanism without refcounting). - Add a patch to document how the best effort memory release safety helper works. - Add a note on releasing driver's context with the media device, not the V4L2 device, in MC documentation. - Check media device is registered before accessing its fops in media_read(), media_write(), media_ioctl and media_compat_ioctl(). - Document best effort media device lifetime management (new patch). - Use media_devnode_free_minor() in unallocating device node minor number in media_devnode_register(). - Continue to rely on devm_register_irq() in ipu3-cio2 driver but register the IRQ later on (compared to v1). - Drop the patch to move away from devm_request_irq() in omap3isp. - Fix putting references to media device and V4L2 device in v4l2_device_release(). - Add missing media_device_get() (in v1) for M2M devices in video_register_media_controller(). - Unconditionally set the media devnode release function in media_device_init(). There's no harm doing so and the caller of media_device_init() may set the ops after calling the function. Daniel Axtens (1): media: uvcvideo: Refactor teardown of uvc on USB disconnect Laurent Pinchart (1): media: mc: Add per-file-handle data support Logan Gunthorpe (1): media: mc: utilize new cdev_device_add helper function Sakari Ailus (26): Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Revert "media: utilize new cdev_device_add helper function" Revert "[media] media: fix use-after-free in cdev_put() when app exits after driver unbind" Revert "media: uvcvideo: Refactor teardown of uvc on USB disconnect" Revert "[media] media-device: dynamically allocate struct media_devnode" media: mc: Drop nop release callback media: mc: Do not call cdev_device_del() if cdev_device_add() fails media: mc: Delete character device early media: mc: Split initialising and adding media devnode media: mc: Shuffle functions around media: mc: Initialise media devnode in media_device_init() media: mc: Refactor media devnode minor clearing media: mc: Unassign minor only if it has been assigned media: mc: Refcount the media device media: v4l: Acquire a reference to the media device for every video device media: mc: Postpone graph object removal until free media: omap3isp: Release the isp device struct by media device callback media: ipu3-cio2: Call v4l2_device_unregister() earlier media: ipu3-cio2: Request IRQ earlier media: ipu3-cio2: Release the cio2 device context by media device callback media: vimc: Release resources on media device release media: Documentation: Document how Media device resources are released media: mc: Maintain a list of open file handles in a media device media: mc: Implement best effort media device removal safety sans refcount media: mc: Warn about drivers not releasing media device safely media: Documentation: Document media device memory safety helper Documentation/driver-api/media/mc-core.rst | 18 +- drivers/media/cec/core/cec-core.c | 2 +- drivers/media/mc/mc-device.c | 260 ++++++++++++-------- drivers/media/mc/mc-devnode.c | 230 +++++++++++------ drivers/media/pci/intel/ipu3/ipu3-cio2.c | 70 ++++-- drivers/media/platform/ti/omap3isp/isp.c | 24 +- drivers/media/test-drivers/vimc/vimc-core.c | 15 +- drivers/media/usb/au0828/au0828-core.c | 4 +- drivers/media/usb/uvc/uvc_driver.c | 2 +- drivers/media/v4l2-core/v4l2-dev.c | 65 +++-- drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +- include/media/media-device.h | 46 +++- include/media/media-devnode.h | 136 +++++++--- include/media/media-fh.h | 32 +++ 14 files changed, 632 insertions(+), 274 deletions(-) create mode 100644 include/media/media-fh.h