ethosu_perfmon_start() stores the perfmon in perfmon_state.active without taking a reference, and nothing stops the counters when the job that started them completes: the perfmon stays active until a later job runs with a different perfmon or none, GET_VALUES or CLEAR_GLOBAL stops it, or it is destroyed. Meanwhile it is only kept alive by the references held by the file's perfmon xarray, the global perfmon and the jobs using it. If userspace destroys a perfmon while a job using it is still queued, ethosu_perfmon_delete() can only stop it if it happens to be active at that point, and the queued job makes it active when it runs. When the last such job is released, the perfmon is freed, but perfmon_state.active still points to it. The next job's ethosu_switch_perfmon() then passes it to ethosu_perfmon_stop_locked(), which reads ncounters and updates values[] of the freed perfmon. Make perfmon_state.active hold its own reference: take it in ethosu_perfmon_start() and drop it in ethosu_perfmon_stop_locked() when the perfmon stops being active. As a perfmon can now be kept alive by perfmon_state.active alone, also drop that reference when the DRM device is released. The counters are still left running after a job completes, as before. The problem was found through AI-assisted source code review. The fix is compile-tested only and there is no reproducer, as I don't have Ethos-U hardware. Fixes: 17cdb54644e7 ("accel: ethosu: Add performance counter support") Cc: stable@vger.kernel.org Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Jiwoo Kim --- Notes: This keeps the counters running after the job that started them completes, as the driver does today, and only fixes the lifetime of the active perfmon. v3d instead stops the job's perfmon when the job completes; I can do that instead if you prefer, but it would also stop the idle and cycle counters at that point. It applies to drm-misc-fixes, and on top of v4 of "accel: ethosu: Another batch of fixes". drivers/accel/ethosu/ethosu_drv.c | 20 ++++++++++++++++++++ drivers/accel/ethosu/ethosu_perfmon.c | 15 +++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c index 4a24bbd2bb..b6778196c9 100644 --- a/drivers/accel/ethosu/ethosu_drv.c +++ b/drivers/accel/ethosu/ethosu_drv.c @@ -341,6 +341,21 @@ static int ethosu_init(struct ethosu_device *ethosudev) return 0; } +static void ethosu_put_active_perfmon(struct drm_device *ddev, void *res) +{ + struct ethosu_device *ethosudev = to_ethosu_device(ddev); + struct ethosu_perfmon *perfmon = ethosudev->perfmon_state.active; + + /* + * A perfmon left active holds a reference, see ethosu_perfmon_start(). + * This runs at the final drm_dev_put(), possibly long after unbind, + * when no job or file can use the perfmon any more: just drop the + * reference, without touching the hardware. + */ + ethosudev->perfmon_state.active = NULL; + ethosu_perfmon_put(perfmon); +} + static int ethosu_probe(struct platform_device *pdev) { int ret; @@ -367,6 +382,11 @@ static int ethosu_probe(struct platform_device *pdev) if (ret) return ret; + ret = drmm_add_action_or_reset(ðosudev->base, + ethosu_put_active_perfmon, NULL); + if (ret) + return ret; + ret = ethosu_job_init(ethosudev); if (ret) return ret; diff --git a/drivers/accel/ethosu/ethosu_perfmon.c b/drivers/accel/ethosu/ethosu_perfmon.c index 26f625374f..69c0967f87 100644 --- a/drivers/accel/ethosu/ethosu_perfmon.c +++ b/drivers/accel/ethosu/ethosu_perfmon.c @@ -54,9 +54,20 @@ void ethosu_perfmon_start(struct ethosu_device *ethosu, struct ethosu_perfmon *p writel_relaxed(mask, ethosu->pmu_regs + NPU_REG_PMCNTENSET); writel_relaxed(PMCR_CNT_EN | PMCR_EVENT_CNT_RST | PMCR_CYCLE_CNT_RST, ethosu->pmu_regs + NPU_REG_PMCR); + + /* + * The counters keep running after the job that started them + * completes, so the perfmon can stay active after that job and its + * handle are gone. Hold a reference for as long as it is active. + */ + ethosu_perfmon_get(perfmon); ethosu->perfmon_state.active = perfmon; } +/* + * Stops @perfmon if it is the active perfmon. This drops the reference + * held by perfmon_state.active, which can free @perfmon. + */ void ethosu_perfmon_stop_locked(struct ethosu_device *ethosu, struct ethosu_perfmon *perfmon, bool capture) { @@ -73,6 +84,7 @@ void ethosu_perfmon_stop_locked(struct ethosu_device *ethosu, struct ethosu_perf if (!pm_runtime_get_if_active(ethosu->base.dev)) { ethosu->perfmon_state.active = NULL; + ethosu_perfmon_put(perfmon); return; } @@ -94,6 +106,9 @@ void ethosu_perfmon_stop_locked(struct ethosu_device *ethosu, struct ethosu_perf ethosu->perfmon_state.active = NULL; pm_runtime_put(ethosu->base.dev); + + /* Drop the reference taken by ethosu_perfmon_start(). */ + ethosu_perfmon_put(perfmon); } void ethosu_perfmon_stop(struct ethosu_device *ethosu, struct ethosu_perfmon *perfmon, base-commit: bafda823590ddf7031ecb765d673572fae7ab44a -- 2.43.0