On Tue, Sep 08, 2026 at 10:18:06PM +0000, [email protected] wrote:
> 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) <[email protected]>
>
> 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.
Not an issue as explained before.
>
> [ ... ]
>
> [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.
This may be an issue, but isn't related at all to this patch. Will
investigate more.
>
> [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?
No, according to the documentation:
Documentation/power/runtime_pm.rst: `int pm_runtime_put_autosuspend(struct
device *dev);`
Documentation/power/runtime_pm.rst- - set the power.last_busy field to the
current time and decrement the
Documentation/power/runtime_pm.rst- device's usage counter; if the result
is 0 then run
Documentation/power/runtime_pm.rst- pm_request_autosuspend(dev) and return
its result
pm_runtime_put_autosuspend() will update last_busy, so no issue here.
Rob
