| Message ID | 20231220103713.113386-23-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Media device lifetime management | expand |
Hi Sakari, Thank you for the patch. On Wed, Dec 20, 2023 at 12:37:06PM +0200, Sakari Ailus wrote: > Use the media device release callback to release the cio2 device's data > structure. This approach has the benefit of not releasing memory which may > still be accessed through open file handles whilst the ipu3-cio2 driver is > being unbound. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 58 ++++++++++++++++-------- > 1 file changed, 40 insertions(+), 18 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index 3222ec5b8345..bff66e6d3b1e 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -238,9 +238,15 @@ static int cio2_fbpt_init(struct cio2_device *cio2, struct cio2_queue *q) > return 0; > } > > -static void cio2_fbpt_exit(struct cio2_queue *q, struct device *dev) > +static int cio2_fbpt_exit(struct cio2_queue *q, struct device *dev) > { > + if (!q->fbpt) > + return -ENOENT; > + > dma_free_coherent(dev, CIO2_FBPT_SIZE, q->fbpt, q->fbpt_bus_addr); > + q->fbpt = NULL; > + > + return 0; > } > > /**************** CSI2 hardware setup ****************/ > @@ -1643,13 +1649,13 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q) > > static void cio2_queue_exit(struct cio2_device *cio2, struct cio2_queue *q) > { > - vb2_video_unregister_device(&q->vdev); > media_entity_cleanup(&q->vdev.entity); > v4l2_device_unregister_subdev(&q->subdev); Is the release callback the right time for this ? > media_entity_cleanup(&q->subdev.entity); > - cio2_fbpt_exit(q, &cio2->pci_dev->dev); > - mutex_destroy(&q->subdev_lock); > - mutex_destroy(&q->lock); > + if (!cio2_fbpt_exit(q, &cio2->pci_dev->dev)) { This doesn't look very nice, but I suppose there are many other things to clean up in this driver, so I'll close my eyes. > + mutex_destroy(&q->subdev_lock); > + mutex_destroy(&q->lock); > + } > } > > static int cio2_queues_init(struct cio2_device *cio2) > @@ -1695,6 +1701,23 @@ static int cio2_check_fwnode_graph(struct fwnode_handle *fwnode) > return cio2_check_fwnode_graph(fwnode->secondary); > } > > +static void cio2_media_release(struct media_device *mdev) > +{ > + struct cio2_device *cio2 = > + container_of(mdev, struct cio2_device, media_dev); > + > + v4l2_async_nf_cleanup(&cio2->notifier); > + cio2_queues_exit(cio2); > + cio2_fbpt_exit_dummy(cio2); > + mutex_destroy(&cio2->lock); > + > + kfree(cio2); > +} > + > +static const struct media_device_ops cio2_mdev_ops = { > + .release = cio2_media_release, > +}; > + > /**************** PCI interface ****************/ > > static int cio2_pci_probe(struct pci_dev *pci_dev, > @@ -1722,7 +1745,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > return r; > } > > - cio2 = devm_kzalloc(dev, sizeof(*cio2), GFP_KERNEL); > + cio2 = kzalloc(sizeof(*cio2), GFP_KERNEL); > if (!cio2) > return -ENOMEM; > cio2->pci_dev = pci_dev; > @@ -1767,6 +1790,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > mutex_init(&cio2->lock); > > cio2->media_dev.dev = dev; > + cio2->media_dev.ops = &cio2_mdev_ops; > strscpy(cio2->media_dev.model, CIO2_DEVICE_NAME, > sizeof(cio2->media_dev.model)); > cio2->media_dev.hw_revision = 0; > @@ -1774,7 +1798,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > media_device_init(&cio2->media_dev); > r = media_device_register(&cio2->media_dev); > if (r < 0) > - goto fail_mutex_destroy; > + goto fail_media_device_put; > > cio2->v4l2_dev.mdev = &cio2->media_dev; > r = v4l2_device_register(dev, &cio2->v4l2_dev); > @@ -1808,35 +1832,33 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > > fail_clean_notifier: > v4l2_async_nf_unregister(&cio2->notifier); > - v4l2_async_nf_cleanup(&cio2->notifier); > - cio2_queues_exit(cio2); > + > fail_v4l2_device_unregister: > v4l2_device_unregister(&cio2->v4l2_dev); > + > fail_media_device_unregister: > media_device_unregister(&cio2->media_dev); > - media_device_cleanup(&cio2->media_dev); > -fail_mutex_destroy: > - mutex_destroy(&cio2->lock); > - cio2_fbpt_exit_dummy(cio2); > > +fail_media_device_put: > + media_device_put(&cio2->media_dev); > return r; > } > > static void cio2_pci_remove(struct pci_dev *pci_dev) > { > struct cio2_device *cio2 = pci_get_drvdata(pci_dev); > + unsigned int i; > > media_device_unregister(&cio2->media_dev); > + for (i = 0; i < CIO2_QUEUES; i++) > + vb2_video_unregister_device(&cio2->queue[i].vdev); > v4l2_device_unregister(&cio2->v4l2_dev); > v4l2_async_nf_unregister(&cio2->notifier); > - v4l2_async_nf_cleanup(&cio2->notifier); > - cio2_queues_exit(cio2); > - cio2_fbpt_exit_dummy(cio2); > - media_device_cleanup(&cio2->media_dev); > - mutex_destroy(&cio2->lock); > > pm_runtime_forbid(&pci_dev->dev); > pm_runtime_get_noresume(&pci_dev->dev); > + > + media_device_put(&cio2->media_dev); > } > > static int __maybe_unused cio2_runtime_suspend(struct device *dev)
Hi Laurent, On Wed, Feb 07, 2024 at 04:33:50PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Wed, Dec 20, 2023 at 12:37:06PM +0200, Sakari Ailus wrote: > > Use the media device release callback to release the cio2 device's data > > structure. This approach has the benefit of not releasing memory which may > > still be accessed through open file handles whilst the ipu3-cio2 driver is > > being unbound. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > --- > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 58 ++++++++++++++++-------- > > 1 file changed, 40 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > index 3222ec5b8345..bff66e6d3b1e 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > @@ -238,9 +238,15 @@ static int cio2_fbpt_init(struct cio2_device *cio2, struct cio2_queue *q) > > return 0; > > } > > > > -static void cio2_fbpt_exit(struct cio2_queue *q, struct device *dev) > > +static int cio2_fbpt_exit(struct cio2_queue *q, struct device *dev) > > { > > + if (!q->fbpt) > > + return -ENOENT; > > + > > dma_free_coherent(dev, CIO2_FBPT_SIZE, q->fbpt, q->fbpt_bus_addr); > > + q->fbpt = NULL; > > + > > + return 0; > > } > > > > /**************** CSI2 hardware setup ****************/ > > @@ -1643,13 +1649,13 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q) > > > > static void cio2_queue_exit(struct cio2_device *cio2, struct cio2_queue *q) > > { > > - vb2_video_unregister_device(&q->vdev); > > media_entity_cleanup(&q->vdev.entity); > > v4l2_device_unregister_subdev(&q->subdev); > > Is the release callback the right time for this ? Neither driver remove or media device release is entirely correct as we don't have refcounting for these yet. A better place would still be in the remove callback. I'll move it there. > > > media_entity_cleanup(&q->subdev.entity); > > - cio2_fbpt_exit(q, &cio2->pci_dev->dev); > > - mutex_destroy(&q->subdev_lock); > > - mutex_destroy(&q->lock); > > + if (!cio2_fbpt_exit(q, &cio2->pci_dev->dev)) { > > This doesn't look very nice, but I suppose there are many other things > to clean up in this driver, so I'll close my eyes. I'd say ipu3-cio2 is one of the better CSI-2 receiver drivers. This is related to error handing: you can't call mutex_destroy() on a mutex that's been already destroyed. cio2_queue_exit() is called when the media device is released but we don't know here whether mutexes have been initialised yet. I guess that's something that could be changed but it would create more lines of code elsewhere. > > > + mutex_destroy(&q->subdev_lock); > > + mutex_destroy(&q->lock); > > + } > > } > > > > static int cio2_queues_init(struct cio2_device *cio2) > > @@ -1695,6 +1701,23 @@ static int cio2_check_fwnode_graph(struct fwnode_handle *fwnode) > > return cio2_check_fwnode_graph(fwnode->secondary); > > } > > > > +static void cio2_media_release(struct media_device *mdev) > > +{ > > + struct cio2_device *cio2 = > > + container_of(mdev, struct cio2_device, media_dev); > > + > > + v4l2_async_nf_cleanup(&cio2->notifier); > > + cio2_queues_exit(cio2); > > + cio2_fbpt_exit_dummy(cio2); > > + mutex_destroy(&cio2->lock); > > + > > + kfree(cio2); > > +} > > + > > +static const struct media_device_ops cio2_mdev_ops = { > > + .release = cio2_media_release, > > +}; > > + > > /**************** PCI interface ****************/ > > > > static int cio2_pci_probe(struct pci_dev *pci_dev, > > @@ -1722,7 +1745,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > > return r; > > } > > > > - cio2 = devm_kzalloc(dev, sizeof(*cio2), GFP_KERNEL); > > + cio2 = kzalloc(sizeof(*cio2), GFP_KERNEL); > > if (!cio2) > > return -ENOMEM; > > cio2->pci_dev = pci_dev; > > @@ -1767,6 +1790,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > > mutex_init(&cio2->lock); > > > > cio2->media_dev.dev = dev; > > + cio2->media_dev.ops = &cio2_mdev_ops; > > strscpy(cio2->media_dev.model, CIO2_DEVICE_NAME, > > sizeof(cio2->media_dev.model)); > > cio2->media_dev.hw_revision = 0; > > @@ -1774,7 +1798,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > > media_device_init(&cio2->media_dev); > > r = media_device_register(&cio2->media_dev); > > if (r < 0) > > - goto fail_mutex_destroy; > > + goto fail_media_device_put; > > > > cio2->v4l2_dev.mdev = &cio2->media_dev; > > r = v4l2_device_register(dev, &cio2->v4l2_dev); > > @@ -1808,35 +1832,33 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > > > > fail_clean_notifier: > > v4l2_async_nf_unregister(&cio2->notifier); > > - v4l2_async_nf_cleanup(&cio2->notifier); > > - cio2_queues_exit(cio2); > > + > > fail_v4l2_device_unregister: > > v4l2_device_unregister(&cio2->v4l2_dev); > > + > > fail_media_device_unregister: > > media_device_unregister(&cio2->media_dev); > > - media_device_cleanup(&cio2->media_dev); > > -fail_mutex_destroy: > > - mutex_destroy(&cio2->lock); > > - cio2_fbpt_exit_dummy(cio2); > > > > +fail_media_device_put: > > + media_device_put(&cio2->media_dev); > > return r; > > } > > > > static void cio2_pci_remove(struct pci_dev *pci_dev) > > { > > struct cio2_device *cio2 = pci_get_drvdata(pci_dev); > > + unsigned int i; > > > > media_device_unregister(&cio2->media_dev); > > + for (i = 0; i < CIO2_QUEUES; i++) > > + vb2_video_unregister_device(&cio2->queue[i].vdev); > > v4l2_device_unregister(&cio2->v4l2_dev); > > v4l2_async_nf_unregister(&cio2->notifier); > > - v4l2_async_nf_cleanup(&cio2->notifier); > > - cio2_queues_exit(cio2); > > - cio2_fbpt_exit_dummy(cio2); > > - media_device_cleanup(&cio2->media_dev); > > - mutex_destroy(&cio2->lock); > > > > pm_runtime_forbid(&pci_dev->dev); > > pm_runtime_get_noresume(&pci_dev->dev); > > + > > + media_device_put(&cio2->media_dev); > > } > > > > static int __maybe_unused cio2_runtime_suspend(struct device *dev) >
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index 3222ec5b8345..bff66e6d3b1e 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -238,9 +238,15 @@ static int cio2_fbpt_init(struct cio2_device *cio2, struct cio2_queue *q) return 0; } -static void cio2_fbpt_exit(struct cio2_queue *q, struct device *dev) +static int cio2_fbpt_exit(struct cio2_queue *q, struct device *dev) { + if (!q->fbpt) + return -ENOENT; + dma_free_coherent(dev, CIO2_FBPT_SIZE, q->fbpt, q->fbpt_bus_addr); + q->fbpt = NULL; + + return 0; } /**************** CSI2 hardware setup ****************/ @@ -1643,13 +1649,13 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q) static void cio2_queue_exit(struct cio2_device *cio2, struct cio2_queue *q) { - vb2_video_unregister_device(&q->vdev); media_entity_cleanup(&q->vdev.entity); v4l2_device_unregister_subdev(&q->subdev); media_entity_cleanup(&q->subdev.entity); - cio2_fbpt_exit(q, &cio2->pci_dev->dev); - mutex_destroy(&q->subdev_lock); - mutex_destroy(&q->lock); + if (!cio2_fbpt_exit(q, &cio2->pci_dev->dev)) { + mutex_destroy(&q->subdev_lock); + mutex_destroy(&q->lock); + } } static int cio2_queues_init(struct cio2_device *cio2) @@ -1695,6 +1701,23 @@ static int cio2_check_fwnode_graph(struct fwnode_handle *fwnode) return cio2_check_fwnode_graph(fwnode->secondary); } +static void cio2_media_release(struct media_device *mdev) +{ + struct cio2_device *cio2 = + container_of(mdev, struct cio2_device, media_dev); + + v4l2_async_nf_cleanup(&cio2->notifier); + cio2_queues_exit(cio2); + cio2_fbpt_exit_dummy(cio2); + mutex_destroy(&cio2->lock); + + kfree(cio2); +} + +static const struct media_device_ops cio2_mdev_ops = { + .release = cio2_media_release, +}; + /**************** PCI interface ****************/ static int cio2_pci_probe(struct pci_dev *pci_dev, @@ -1722,7 +1745,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, return r; } - cio2 = devm_kzalloc(dev, sizeof(*cio2), GFP_KERNEL); + cio2 = kzalloc(sizeof(*cio2), GFP_KERNEL); if (!cio2) return -ENOMEM; cio2->pci_dev = pci_dev; @@ -1767,6 +1790,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, mutex_init(&cio2->lock); cio2->media_dev.dev = dev; + cio2->media_dev.ops = &cio2_mdev_ops; strscpy(cio2->media_dev.model, CIO2_DEVICE_NAME, sizeof(cio2->media_dev.model)); cio2->media_dev.hw_revision = 0; @@ -1774,7 +1798,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, media_device_init(&cio2->media_dev); r = media_device_register(&cio2->media_dev); if (r < 0) - goto fail_mutex_destroy; + goto fail_media_device_put; cio2->v4l2_dev.mdev = &cio2->media_dev; r = v4l2_device_register(dev, &cio2->v4l2_dev); @@ -1808,35 +1832,33 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, fail_clean_notifier: v4l2_async_nf_unregister(&cio2->notifier); - v4l2_async_nf_cleanup(&cio2->notifier); - cio2_queues_exit(cio2); + fail_v4l2_device_unregister: v4l2_device_unregister(&cio2->v4l2_dev); + fail_media_device_unregister: media_device_unregister(&cio2->media_dev); - media_device_cleanup(&cio2->media_dev); -fail_mutex_destroy: - mutex_destroy(&cio2->lock); - cio2_fbpt_exit_dummy(cio2); +fail_media_device_put: + media_device_put(&cio2->media_dev); return r; } static void cio2_pci_remove(struct pci_dev *pci_dev) { struct cio2_device *cio2 = pci_get_drvdata(pci_dev); + unsigned int i; media_device_unregister(&cio2->media_dev); + for (i = 0; i < CIO2_QUEUES; i++) + vb2_video_unregister_device(&cio2->queue[i].vdev); v4l2_device_unregister(&cio2->v4l2_dev); v4l2_async_nf_unregister(&cio2->notifier); - v4l2_async_nf_cleanup(&cio2->notifier); - cio2_queues_exit(cio2); - cio2_fbpt_exit_dummy(cio2); - media_device_cleanup(&cio2->media_dev); - mutex_destroy(&cio2->lock); pm_runtime_forbid(&pci_dev->dev); pm_runtime_get_noresume(&pci_dev->dev); + + media_device_put(&cio2->media_dev); } static int __maybe_unused cio2_runtime_suspend(struct device *dev)