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

[sashiko-bot@kernel.org]

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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110085003.3527014-1-karol.wachowski@linux.intel.com?part=1