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@xxxxxxxxxxxxxxx>
> > Acked-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> > ---
> > 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 {
>
--
Kind regards,
Sakari Ailus
