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

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

[Daksh Sahni]

If the NPU raises an error interrupt while ivpu_dev_init() boots the
firmware, the Buttress, MMU or WDT IRQ handler calls
ivpu_pm_trigger_recovery(), which queues recovery_work. When ivpu_boot()
then fails, the err_ipc_fini path tears the device down without
disabling recovery_work and probe returns an error. devres unmaps the
register BARs while the work is still queued or running, and the work
faults on the first register access:

  BUG: unable to handle page fault for address: ffffcfc6680200f4
  Workqueue: events_unbound ivpu_pm_recovery_work [intel_vpu]
  RIP: 0010:ivpu_hw_ip_ipc_rx_addr_get+0xc/0xb0 [intel_vpu]
  Call Trace:
   ivpu_wait_for_ready+0x155/0x220 [intel_vpu]
   ivpu_boot+0x9e/0x270 [intel_vpu]
   ivpu_resume+0xa4/0xd0 [intel_vpu]
   ivpu_pm_reset_complete+0x2c/0x90 [intel_vpu]
   ivpu_pm_recovery_work+0xf6/0x140 [intel_vpu]
   process_one_work+0x1da/0x3c0
   worker_thread+0x1af/0x340
   kthread+0xfa/0x130
   ret_from_fork+0x2b9/0x380
   ret_from_fork_asm+0x1a/0x30

The work dies between pm_runtime_disable() in ivpu_pm_reset_begin() and
pm_runtime_enable() in ivpu_pm_reset_complete(), so the PCI device is
also left with runtime PM disabled and never autosuspends after a later
rebind.

Commit 5eaa49741119 ("accel/ivpu: Prevent recovery invocation during
probe and resume") stopped IPC failures during probe from triggering
recovery, but the IRQ handlers still can.

Call ivpu_pm_disable_recovery() at the start of the err_ipc_fini path,
as ivpu_dev_fini() already does, so the work is flushed and cannot be
queued again before the device is torn down.

Reproduced on Meteor Lake (Core Ultra 5 125H, vpu_37xx firmware
20260219) by kexec'ing with the NPU runtime-suspended: probe in the new
kernel gets an ATS_ERR Buttress IRQ during firmware boot. With this
change probe still fails with -ECANCELED, there is no oops, and an FLR
followed by a rebind brings the NPU back.

Fixes: 852be13f3bd3 ("accel/ivpu: Add PM support")
Cc: stable@vger.kernel.org
Signed-off-by: Daksh Sahni <swizzguy76@gmail.com>
---

Notes:
    Tested on v7.4-rc1 (d48723711bae).
    
    Two things this patch does not address:
    
    The trigger is kexec with the NPU runtime-suspended in the old kernel.
    The driver has no .shutdown hook, so the new kernel's probe sees stale
    NPU state and gets ATS_ERR. A cold boot doesn't hit it. Whether that
    wants a shutdown hook or a reset at probe seems like your call.
    
    In one of four runs that hit ATS_ERR, probe succeeded while
    recovery_work was running concurrently. Probe returned 0, but the
    autosuspend request from ivpu_pm_enable() landed while recovery had
    runtime PM disabled, so the NPU stayed active until first use. Not
    allowing IRQ-triggered recovery until ivpu_pm_enable() would cover both
    cases; I can send a v2 that does that if you prefer.

 drivers/accel/ivpu/ivpu_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
index 247ed6b2ff62..4fcccf78ec5d 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);
-- 
2.53.0

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

[Karol Wachowski]

On 07-Nov-26 18:42, Daksh Sahni wrote:
> If the NPU raises an error interrupt while ivpu_dev_init() boots the
> firmware, the Buttress, MMU or WDT IRQ handler calls
> ivpu_pm_trigger_recovery(), which queues recovery_work. When ivpu_boot()
> then fails, the err_ipc_fini path tears the device down without
> disabling recovery_work and probe returns an error. devres unmaps the
> register BARs while the work is still queued or running, and the work
> faults on the first register access:
> 
>    BUG: unable to handle page fault for address: ffffcfc6680200f4
>    Workqueue: events_unbound ivpu_pm_recovery_work [intel_vpu]
>    RIP: 0010:ivpu_hw_ip_ipc_rx_addr_get+0xc/0xb0 [intel_vpu]
>    Call Trace:
>     ivpu_wait_for_ready+0x155/0x220 [intel_vpu]
>     ivpu_boot+0x9e/0x270 [intel_vpu]
>     ivpu_resume+0xa4/0xd0 [intel_vpu]
>     ivpu_pm_reset_complete+0x2c/0x90 [intel_vpu]
>     ivpu_pm_recovery_work+0xf6/0x140 [intel_vpu]
>     process_one_work+0x1da/0x3c0
>     worker_thread+0x1af/0x340
>     kthread+0xfa/0x130
>     ret_from_fork+0x2b9/0x380
>     ret_from_fork_asm+0x1a/0x30
> 
> The work dies between pm_runtime_disable() in ivpu_pm_reset_begin() and
> pm_runtime_enable() in ivpu_pm_reset_complete(), so the PCI device is
> also left with runtime PM disabled and never autosuspends after a later
> rebind.
> 
> Commit 5eaa49741119 ("accel/ivpu: Prevent recovery invocation during
> probe and resume") stopped IPC failures during probe from triggering
> recovery, but the IRQ handlers still can.
> 
> Call ivpu_pm_disable_recovery() at the start of the err_ipc_fini path,
> as ivpu_dev_fini() already does, so the work is flushed and cannot be
> queued again before the device is torn down.
> 
> Reproduced on Meteor Lake (Core Ultra 5 125H, vpu_37xx firmware
> 20260219) by kexec'ing with the NPU runtime-suspended: probe in the new
> kernel gets an ATS_ERR Buttress IRQ during firmware boot. With this
> change probe still fails with -ECANCELED, there is no oops, and an FLR
> followed by a rebind brings the NPU back.
> 
> Fixes: 852be13f3bd3 ("accel/ivpu: Add PM support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Daksh Sahni <swizzguy76@gmail.com>

  Hi Daksh,

  Thanks for the report and the thorough analysis, it's correct. An error
  IRQ that fires once ivpu_boot() has enabled interrupts can queue
  recovery_work during probe, and nothing on the probe error path stops
  it.

  As Sashiko pointed out, disabling the work on err_ipc_fini comes too
  late. By then recovery_work may already have run concurrently with
  ivpu_boot(). It can also race with a probe that succeeds, which you saw
  yourself in one of your runs.

  I'll send a v2 that addresses this along the lines you offered and
  Sashiko suggested. recovery_work stays disabled for the whole of probe
  and is enabled in ivpu_pm_enable(). A recovery requested during probe
  is not lost. If ivpu_boot() fails, probe unwinds with nothing pending.

  If  probe succeeds, ivpu_pm_enable() runs the deferred recovery and
  waits for it before issuing the initial autosuspend request. The NPU is
  then reset before first use, and the autosuspend no longer lands while
  runtime PM is disabled, which covers your second note too. The patch
  will keep your Reported-by.

  I'm running it through our internal CI first and will post it once that
  passes. When it's out, could you test it on your Meteor Lake kexec
  setup and reply with your Tested-by? Once I have it, I'll go ahead
  and merge.

  Thanks,
  Karol
> ---
> 
> Notes:
>      Tested on v7.4-rc1 (d48723711bae).
>      
>      Two things this patch does not address:
>      
>      The trigger is kexec with the NPU runtime-suspended in the old kernel.
>      The driver has no .shutdown hook, so the new kernel's probe sees stale
>      NPU state and gets ATS_ERR. A cold boot doesn't hit it. Whether that
>      wants a shutdown hook or a reset at probe seems like your call.
>      
>      In one of four runs that hit ATS_ERR, probe succeeded while
>      recovery_work was running concurrently. Probe returned 0, but the
>      autosuspend request from ivpu_pm_enable() landed while recovery had
>      runtime PM disabled, so the NPU stayed active until first use. Not
>      allowing IRQ-triggered recovery until ivpu_pm_enable() would cover both
>      cases; I can send a v2 that does that if you prefer.
> 
>   drivers/accel/ivpu/ivpu_drv.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
> index 247ed6b2ff62..4fcccf78ec5d 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);