[v2,19/29] media: omap3isp: Release the isp device struct by media device callback

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(-)
  

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 {
  

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 {
>