Re: [PATCH v4 04/10] media: microchip-isc: disable histogram and flush AWB work on teardown

On 8/3/26 13:20, Balakrishnan Sambath wrote:
> isc_stop_streaming() and the isc_start_streaming() error path dropped the
> runtime PM reference with the histogram still enabled. A HISDONE firing
> just before the stop, or a failed isc_update_profile() on the start path,
> can queue isc_awb_work(), which reads the histogram registers before
> taking its own PM reference and faults on the unclocked device.
> 
> Disable the histogram, synchronize the IRQ and flush the work before
> dropping the PM reference on both paths. synchronize_irq() must come
> before cancel_work_sync(), so an in-flight handler cannot re-queue
> awb_work after it is cancelled.
> 
> Fixes: 93d4a26c3dab ("[media] atmel-isc: add the isc pipeline function")
> Cc: [email protected]
> Signed-off-by: Balakrishnan Sambath <[email protected]>
> ---
>  drivers/media/platform/microchip/microchip-isc-base.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index debbc38717de..54f3093e14fc 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -382,6 +382,13 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	return 0;
>  
>  err_configure:
> +	isc_set_histogram(isc, false);

I find it odd that an error path would clean up something that the
enable path did not do. So if the histogram is enabled by calling a
different function (isc_configure()) , then if isc_configure() failed,
isc_configure() should cleanup after itself, and you should not do the
cleanup here.

> +
> +	/* let a running IRQ handler finish before the clock is disabled */
> +	synchronize_irq(isc->irq);
> +
> +	cancel_work_sync(&isc->awb_work);
Same here, if something failed, there should not be any pending
workqueue left, it should be cleaned by the failing enabler.

Am I missing something ?

> +
>  	pm_runtime_put_sync(isc->dev);
>  err_pm_get:
>  	v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);
> @@ -425,9 +432,13 @@ static void isc_stop_streaming(struct vb2_queue *vq)
>  	/* Disable DMA interrupt */
>  	regmap_write(isc->regmap, ISC_INTDIS, ISC_INT_DDONE);
>  
> +	isc_set_histogram(isc, false);
> +
>  	/* let a running IRQ handler finish before the clock is disabled */
>  	synchronize_irq(isc->irq);
>  
> +	cancel_work_sync(&isc->awb_work);
> +
>  	pm_runtime_put_sync(isc->dev);
>  
>  	/* Disable stream on the sub device */
>