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@xxxxxxxxxxxxxxx>
> Acked-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> ---
> 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)
--
Regards,
Laurent Pinchart
