Re: [PATCH v2] accel/ivpu: Defer recovery until probe completes

[Karol Wachowski <karol.wachowski@linux.intel.com>]

On 10-Nov-26 10:13, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Medium] accel/ivpu: race in ivpu_pm_enable() queues recovery_work
>    twice
> --
> 
> commit 53feec71cd6cb0035da92ea965a242f974fa21d7
> Author: Karol Wachowski <karol.wachowski@linux.intel.com>
> 
> accel/ivpu: Defer recovery until probe completes
> 
> This commit disables recovery_work in ivpu_pm_init() and re-enables and
> flushes it in ivpu_pm_enable() to prevent recovery from running during
> probe before device initialization finishes.
> 
>> diff --git a/drivers/accel/ivpu/ivpu_pm.c b/drivers/accel/ivpu/ivpu_pm.c
>> index aa813262253e4..7aee728b19ef1 100644
>> --- a/drivers/accel/ivpu/ivpu_pm.c
>> +++ b/drivers/accel/ivpu/ivpu_pm.c
>> @@ -415,6 +415,7 @@ void ivpu_pm_init(struct ivpu_device *vdev)
>>   	atomic_set(&pm->engine_reset_counter, 0);
>>   
>>   	INIT_WORK(&pm->recovery_work, ivpu_pm_recovery_work);
>> +	disable_work(&pm->recovery_work);
>>   	INIT_DELAYED_WORK(&pm->job_timeout_work, ivpu_job_timeout_work);
>>   
>>   	if (ivpu_disable_recovery)
>> @@ -439,6 +440,12 @@ void ivpu_pm_enable(struct ivpu_device *vdev)
>>   {
>>   	struct device *dev = vdev->drm.dev;
>>   
>> +	enable_work(&vdev->pm->recovery_work);
>> +	if (atomic_read(&vdev->pm->reset_pending)) {
>> +		queue_work(system_dfl_wq, &vdev->pm->recovery_work);
>> +		flush_work(&vdev->pm->recovery_work);
>> +	}
> 
> [Severity: Medium]
> Can ivpu_pm_enable() race with ivpu_pm_trigger_recovery() and queue
> recovery_work twice for a single fault?
> 
> In ivpu_pm_trigger_recovery(), reset_pending is set to 1 before
> ivpu_hw_diagnose_failure(), ivpu_hw_irq_disable(), and queue_work():
> 
> drivers/accel/ivpu/ivpu_pm.c:ivpu_pm_trigger_recovery() {
> 	...
> 	if (atomic_cmpxchg(&vdev->pm->reset_pending, 0, 1) == 0) {
> 		ivpu_hw_diagnose_failure(vdev);
> 		ivpu_hw_irq_disable(vdev); /* Disable IRQ early to protect from IRQ storm */
> 		queue_work(system_dfl_wq, &vdev->pm->recovery_work);
> 	}
> }
> 
> And reset_pending stays 1 until ivpu_pm_reset_complete() finishes, while
> the workqueue clears WORK_STRUCT_PENDING_BIT as soon as a worker dequeues
> recovery_work:
> 
> CPU0                              CPU1                        kworker
> ivpu_pm_trigger_recovery()
>    atomic_cmpxchg(reset_pending, 0, 1)
>    ivpu_hw_diagnose_failure()
>                                    ivpu_pm_enable()
>                                      enable_work(&recovery_work)
>                                      if (atomic_read(reset_pending))
>                                        queue_work(&recovery_work)
>                                        flush_work(&recovery_work)
>                                                                ivpu_pm_recovery_work()
>    ivpu_hw_irq_disable()
>    queue_work(&recovery_work)  <-- queued a second time
> 
> The same double queueing happens if ivpu_pm_trigger_recovery() runs right
> after enable_work() and a worker starts ivpu_pm_recovery_work() before
> ivpu_pm_enable() checks reset_pending.
> 
> In the first interleaving, ivpu_pm_recovery_work() runs concurrently with
> ivpu_hw_diagnose_failure() and ivpu_hw_irq_disable(), and flush_work()
> only waits for the first execution.  The second queued recovery_work then
> runs after flush_work() returns, performing a second full NPU reset
> concurrently with pm_runtime_allow() and pm_runtime_put_autosuspend() and
> reintroducing the race where recovery disables runtime PM during the
> initial autosuspend request.
> 

I'll send V3 that addresses recovery that leaves the device in D0.

Karol