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@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 ?
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)
>
--
Regards,
Sakari Ailus
