Re: [PATCH] accel/ivpu: Disable recovery work on probe failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] accel/ivpu: ivpu_pm_recovery_work() races with ivpu_boot()
  during probe
--

commit 9885035bb27e3920959278569c3258e9ccfc3b7e
Author: Daksh Sahni <swizzguy76@gmail.com>

accel/ivpu: Disable recovery work on probe failure

This commit calls ivpu_pm_disable_recovery() on the err_ipc_fini error
path in ivpu_dev_init() to flush and disable recovery_work before device
teardown.

> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
> index 247ed6b2ff62a..4fcccf78ec5db 100644
> --- a/drivers/accel/ivpu/ivpu_drv.c
> +++ b/drivers/accel/ivpu/ivpu_drv.c
> @@ -852,6 +852,7 @@ static int ivpu_dev_init(struct ivpu_device *vdev)
>  	return 0;
>  
>  err_ipc_fini:
> +	ivpu_pm_disable_recovery(vdev);
>  	ivpu_ipc_fini(vdev);
>  err_fw_fini:
>  	ivpu_fw_fini(vdev);

[Severity: Medium]
Can ivpu_pm_recovery_work() still race with ivpu_boot() before
ivpu_dev_init() reaches err_ipc_fini?

ivpu_pm_init() leaves recovery_work enabled before ivpu_boot() enables
IRQs.  When an error IRQ queues recovery_work during ivpu_boot(), both the
probe thread and the worker tear down the device concurrently:

CPU0 (probe)                      CPU1 (kworker)
ivpu_dev_init()
  ivpu_boot()
    enable_irq(vdev->irq)
    <error IRQ>
      ivpu_pm_trigger_recovery()
        queue_work(recovery_work)
                                  ivpu_pm_recovery_work()
                                    ivpu_suspend()
                                      ivpu_prepare_for_reset()
                                        disable_irq(vdev->irq)
                                        ivpu_ipc_disable()
    err_disable_ipc:
      disable_irq(vdev->irq)
      ivpu_mmu_evtq_dump()
                                      ivpu_shutdown()
                                    ivpu_pm_reset_complete()
                                      ivpu_resume()
                                        ivpu_boot()
                                          enable_irq(vdev->irq)
                                          ...
                                        ivpu_hw_power_down()
  err_ipc_fini:
    ivpu_pm_disable_recovery()
    ...
    ivpu_shutdown()  <-- already in PCI_D3hot

Because both ivpu_prepare_for_reset() and err_disable_ipc call
disable_irq(vdev->irq), the single enable_irq() in ivpu_resume() leaves
vdev->irq disabled, causing the recovery boot to time out and power down
the device into PCI_D3hot.  Once disable_work_sync() in
ivpu_pm_disable_recovery() returns, err_shutdown calls ivpu_shutdown() a
second time while already in PCI_D3hot, accessing MMIO registers and
overwriting the saved PCI state.

Conversely, if ivpu_resume() in recovery_work succeeds, the NPU is left
powered on in PCI_D0 with IRQs, IPC, and MMU active while err_ipc_fini
frees the IPC, firmware, and MMU buffers without calling
ivpu_prepare_for_reset() first; and if ivpu_boot() in ivpu_dev_init()
finishes before recovery_work calls ivpu_ipc_disable(), probe returns 0
while recovery_work runs with runtime PM disabled, dropping the
autosuspend request from ivpu_pm_enable().

Initializing recovery_work in the disabled state in ivpu_pm_init() and
enabling it in ivpu_pm_enable() would prevent IRQ handlers from queuing
recovery until probe succeeds.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107174215.15818-1-swizzguy76@gmail.com?part=1