Re: [PATCH v3 01/22] accel: ethosu: Suspend after initialization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Hardware Access in IRQ Handler
- [High] TOCTOU race on `dev->in_flight_job` between the IRQ handler and the timeout worker causes job tracking corruption
- [Medium] Missing pm_runtime_mark_last_busy() defeats autosuspend functionality
--

commit 70e7c7d4b9e3f59284be594e6021ace41a2927aa
Author: Rob Herring (Arm) <robh@kernel.org>

accel: ethosu: Suspend after initialization

The initial runtime-PM reference is held only while initializing the
NPU. Release it synchronously from ethosu_init() after the final
hardware access, before registering the DRM device.

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index 8108622de258f..df76253d01a64 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -338,6 +338,8 @@ static int ethosu_init(struct ethosu_device *ethosudev)
>  		 ethosudev->npu_info.sram_size / 1024,
>  		 ethosudev->npu_info.pmu_counters);
>  
> +	pm_runtime_put_sync_suspend(ethosudev->base.dev);
> +
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does explicitly disabling the
NPU clocks here expose a risk of crashing in the IRQ handler?

If a spurious interrupt occurs while the device is suspended, the IRQ handler
will unconditionally read from the unclocked NPU registers:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_irq_handler() {
	struct ethosu_device *dev = data;
	u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS);

	if (!(status & STATUS_IRQ_RAISED))
		return IRQ_NONE;
	...
}

Subsystem guidelines mandate that IRQ handlers use pm_runtime_get_if_active()
to verify the device is powered on before accessing any registers. Accessing
memory-mapped registers of an unclocked device typically triggers a
synchronous bus fault or asynchronous external abort, resulting in a system
crash.

[ ... ]

[Severity: High]
This is a pre-existing issue, but can the timeout worker and IRQ handler race
and corrupt the job tracking?

ethosu_job_handle_irq() reads dev->in_flight_job without locking:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_handle_irq() {
	...
	job = READ_ONCE(dev->in_flight_job);
	if (job) {
		WRITE_ONCE(dev->in_flight_job, NULL);
		dma_fence_signal(job->done_fence);
	}
}

Concurrently, ethosu_job_timedout() can run in a workqueue, stop the scheduler,
and modify the state without locking:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_timedout() {
	...
	drm_sched_stop(&dev->sched, bad);

	WRITE_ONCE(dev->in_flight_job, NULL);

	/* Proceed with reset now. */
	pm_runtime_force_suspend(dev->base.dev);
	pm_runtime_force_resume(dev->base.dev);

	/* Restart the scheduler */
	drm_sched_start(&dev->sched, 0);
	...
}

If the scheduler immediately pushes a new job, updating dev->in_flight_job,
could the preempted IRQ thread resume and blindly execute
WRITE_ONCE(dev->in_flight_job, NULL), overwriting the new job's tracking?

When the new job completes, the IRQ handler would read NULL and fail to signal
its fence, causing a perfectly valid job to falsely time out and trigger an
unnecessary hardware reset loop.

[Severity: Medium]
This is a pre-existing issue, but does this autosuspend implementation actually
delay powering down the NPU?

In ethosu_job_cleanup(), the driver drops the runtime PM reference:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_cleanup() {
	struct ethosu_job *job = container_of(ref, struct ethosu_job,
						refcount);

	pm_runtime_put_autosuspend(job->dev->base.dev);
	...
}

Since pm_runtime_mark_last_busy() is never called, the autosuspend timer
evaluates from the beginning of the active PM transition rather than its
completion.

Will the old last_busy timestamp cause the autosuspend delay to evaluate as
already expired, defeating the autosuspend mechanism and leading to excessive
hardware power cycling?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-ethosu-fixes-v3-0-490fe215286f@kernel.org?part=1