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. [ ... ] [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/[email protected]?part=1
