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

[Balakrishnan.S]

Hi Eugen,

Thanks for the feedback on this.

On 21/06/26 11:53 am, Eugen Hristev wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 6/16/26 14:50, Balakrishnan Sambath wrote:
>> isc_stop_streaming() masked the DMA done interrupt but left the
>> histogram enabled, so a HISDONE that fired just before the stop could
>> still queue isc_awb_work() after pm_runtime_put_sync() gated the clocks.
>> isc_awb_work() reads the histogram registers in isc_hist_count() before
>> taking its own PM reference, so the access faults on the suspended
>> device.
>>
>> Disable the histogram and flush the work before dropping the PM
>> reference, mirroring the start_streaming error path.
>>
> 
> In here you reference some error path that you mirror, but you add it in
> the next commit in which you say you mirror this exact commit :/
> Circular mirroring of paths which initially did not exist.
> I would squash this commit with the next one and show the facts as they
> are : stop histogram and work queue in different stop/error scenarios,
> no more mirroring.

Agreed, indeed its circular.
Will squash both commits and reword so each path stands on its own.

> 
> Eugen
> 
>> Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@xxxxxxxxxxxxx>
>> ---
>>   drivers/media/platform/microchip/microchip-isc-base.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
>> index 3245dd7cb980..8f255a4c4e7a 100644
>> --- a/drivers/media/platform/microchip/microchip-isc-base.c
>> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
>> @@ -425,9 +425,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 */
>>
>