Re: [PATCH v17 03/12] accel/rocket: wait for a running IRQ handler before resetting a core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] accel/rocket: raw IRQ status can re-latch before reset in
  rocket_reset()
- [High] accel/rocket: race with pm_runtime_force_suspend() in
  rocket_reset()
- [Low] accel/rocket: missing Fixes trailer for IRQ race fix in
  rocket_reset()
--

commit 9e5305abf9afea9e1a362f158ce4e7b07f213cce
Author: Jiaxing Hu <gahing@gahingwoo.com>

> accel/rocket: wait for a running IRQ handler before resetting a core
>
> drm_sched_stop() does not wait for a threaded IRQ handler that is already
> running, so call synchronize_irq() after it to make sure the handler has
> finished before the reset.
>
> Before synchronize_irq(), mask the interrupt and clear the raw status, so
> that the NPU core cannot report a completion afterwards.
>
> Do this under job_lock, because the submit path sets the interrupt mask
> under the same lock.
>
> Only touch these registers when the device is powered: check with
> pm_runtime_get_if_active(), so that a register access with the power
> domain off cannot raise an SError.
>
> Suggested-by: Igor Paunovic <royalnet026@gmail.com>
> Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
> Tested-by: Igor Paunovic <royalnet026@gmail.com> # RK3588, three cores, induced reset, JOB_TIMEOUT_MS=2

[Severity: Low]
This fixes the IRQ race in rocket_reset() introduced with job submission.
Should this include:

  Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index b61bb4d242ce3..8b4ad028bc9b2 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -402,6 +402,16 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
>  
>  	drm_sched_stop(&core->sched, bad);
>  
> +	scoped_guard(mutex, &core->job_lock) {
> +		if (pm_runtime_get_if_active(core->dev) > 0) {
> +			rocket_pc_writel(core, INTERRUPT_MASK, 0x0);
> +			rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
> +			pm_runtime_put_autosuspend(core->dev);
> +		}
> +	}

[Severity: High]
Can pm_runtime_force_suspend() disable the core clocks while rocket_reset()
writes INTERRUPT_MASK and INTERRUPT_CLEAR?

core->reset.wq is not WQ_FREEZABLE, and drm_sched_stop() drops
sched.credit_count to 0, making rocket_job_is_idle() return true while
rocket_reset() is still running.  Because pm_runtime_force_suspend() ignores
dev->power.usage_count and rocket_device_runtime_suspend() does not take
core->job_lock, system suspend can disable the clocks concurrently:

kworker (core->reset.wq)          system suspend
rocket_reset()
  drm_sched_stop()
    credit_count -> 0
  mutex_lock(&core->job_lock)
  pm_runtime_get_if_active() > 0
                                  pm_runtime_force_suspend()
                                    rocket_device_runtime_suspend()
                                      rocket_job_is_idle() -> true
                                      clk_bulk_disable_unprepare()
  rocket_pc_writel(...)
    <-- unclocked MMIO access (SError / bus hang)

In addition, in rocket_core_fini(), an in-flight job holds a runtime PM
reference that keeps the core active across pm_runtime_disable() before
rocket_job_fini() cancels the reset worker.  pm_runtime_get_if_active() then
returns -EINVAL and skips masking and clearing interrupts on the active core
before synchronize_irq().

> +
> +	synchronize_irq(core->irq);

[Severity: High]
Can an active DPU task complete after INTERRUPT_CLEAR is written and re-latch
INTERRUPT_RAW_STATUS before rocket_core_reset() stops the hardware?

Unlike rocket_job_handle_irq(), which writes 0x0 to OPERATION_ENABLE before
writing INTERRUPT_CLEAR, rocket_reset() leaves the core running across
synchronize_irq() and iommu_detach_group() until rocket_core_reset() asserts
the reset lines.  And because rocket_job_irq_handler() checks the unmasked
INTERRUPT_RAW_STATUS register on a shared IRQ line (shared with the IOMMU):

drivers/accel/rocket/rocket_job.c:rocket_job_irq_handler() {
	u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);
...
	if (!(raw_status & PC_INTERRUPT_RAW_STATUS_DPU_0 ||
	      raw_status & PC_INTERRUPT_RAW_STATUS_DPU_1))
		return IRQ_NONE;

	rocket_pc_writel(core, INTERRUPT_MASK, 0x0);

	return IRQ_WAKE_THREAD;
}

a task finishing in that window sets PC_INTERRUPT_RAW_STATUS_DPU_0/1 again,
and a shared IOMMU interrupt on core->irq can wake
rocket_job_irq_handler_thread() after synchronize_irq() has already returned:

kworker (core->reset.wq)          <hardirq> / irq thread
rocket_reset()
  rocket_pc_writel(INTERRUPT_MASK, 0x0)
  rocket_pc_writel(INTERRUPT_CLEAR, 0x1ffff)
  [DPU finishes, re-latches INTERRUPT_RAW_STATUS]
  synchronize_irq(core->irq)
                                  [shared IOMMU IRQ fires]
                                  rocket_job_irq_handler()
                                    reads INTERRUPT_RAW_STATUS
                                    returns IRQ_WAKE_THREAD
  iommu_detach_group()
  rocket_core_reset()
                                  rocket_job_irq_handler_thread()
                                    rocket_job_handle_irq()  <-- races with reset

This also detaches the IOMMU group in iommu_detach_group() while
OPERATION_ENABLE is still set and the NPU has not yet been reset.  Clearing
OPERATION_ENABLE before INTERRUPT_CLEAR (or reading the masked
INTERRUPT_STATUS register in rocket_job_irq_handler()) would prevent the raw
status from re-triggering the IRQ thread.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794615324.git.gahing@gahingwoo.com?part=3