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

[<Balakrishnan.S@microchip.com>]

Hi Eugen,

On 06/08/26 11:56 am, Eugen Hristev wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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: stable@vger.kernel.org
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
>> ---
>>   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 ?

Nope, you're right. Histogram is enabled in isc_configure(), so it 
should clean up there on the isc_update_profile() failure, not in the 
caller. Will move it in v5 and drop from err_configure.

Thanks for the catch.

> 
>> +
>>        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 */
>>
>