Re: [PATCH v2 03/19] accel: ethosu: Disable clocks on PM setup failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver leaks hardware clocks on removal because it fails to synchronously suspend the device or flush pending autosuspends before PM runtime is disabled.
- [High] Destroying the DRM scheduler in `ethosu_remove()` while userspace holds open file descriptors leads to a Use-After-Free in `ethosu_postclose()`.
- [Medium] The return value of `ethosu_sram_init()` is ignored, which can cause the driver to silently mask memory allocation failures.
--

commit 527f62d6ec40e5aa68386e337081401539f1068b
Author: Rob Herring (Arm) <robh@kernel.org>

accel: ethosu: Disable clocks on PM setup failure

ethosu_init() enables the clocks directly to reset and query the NPU
before runtime PM takes over. If runtime-PM setup fails, the error path
returns while those clocks remain enabled. Disable the clocks before
returning the setup error to balance the initial direct resume.

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index c684e28ac7871..65b148e770e57 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -311,8 +311,10 @@ static int ethosu_init(struct ethosu_device *ethosudev)
>  	pm_runtime_set_autosuspend_delay(ethosudev->base.dev, 50);
>  	pm_runtime_use_autosuspend(ethosudev->base.dev);
>  	ret = devm_pm_runtime_set_active_enabled(ethosudev->base.dev);
> -	if (ret)
> +	if (ret) {
> +		ethosu_device_suspend(ethosudev->base.dev);
>  		return ret;
> +	}
>  	pm_runtime_get_noresume(ethosudev->base.dev);
>  
>  	ethosudev->npu_info.id = id = readl_relaxed(ethosudev->regs + NPU_REG_ID);

[Severity: Medium]
This is a pre-existing issue, but should the return value of
ethosu_sram_init() be checked in ethosu_init()?

drivers/accel/ethosu/ethosu_drv.c:ethosu_init() {
...
	ethosu_sram_init(ethosudev);

	if (!ethosu_is_u65(ethosudev))
		ethosudev->pmu_regs += 0x1000;
...
}

If gen_pool_dma_alloc() fails in ethosu_sram_init(), it returns -ENOMEM.
However, ethosu_init() ignores this return value and continues initialization
with a 0-sized SRAM pool.

If SRAM is configured in the device tree but allocation fails, the error is
suppressed.

[Severity: High]
This is a pre-existing issue, but does this driver leak hardware clocks on
removal?

drivers/accel/ethosu/ethosu_drv.c:ethosu_remove() {
	struct ethosu_device *ethosudev = dev_get_drvdata(&pdev->dev);

	drm_dev_unregister(&ethosudev->base);
	ethosu_job_fini(ethosudev);
	if (ethosudev->sram)
		gen_pool_free(ethosudev->srampool, (unsigned long)ethosudev->sram,
			      ethosudev->npu_info.sram_size);
}

It appears the remove function completes without synchronously suspending the
device or flushing pending autosuspends before PM runtime is disabled.

If a job completes and releases its PM reference via
pm_runtime_put_autosuspend() (delay 50ms), and the device is unbound via
ethosu_remove() before the autosuspend timer expires, the devm teardown
sequence will call pm_runtime_disable().

This cancels pending autosuspend timers without suspending the hardware,
leaving the clocks permanently enabled.

As stated in the subsystem guidelines, synchronous suspend
(pm_runtime_force_suspend() or pm_runtime_put_sync()) is required before
pm_runtime_disable().

[Severity: High]
This is a pre-existing issue, but could destroying the DRM scheduler in
ethosu_remove() while userspace holds open file descriptors lead to a
use-after-free?

If userspace has an open file descriptor for the device and a privileged user
unbinds the driver via sysfs, ethosu_remove() calls ethosu_job_fini(), which
executes drm_sched_fini(&dev->sched).

This frees the scheduler's runqueues (sched->sched_rq[i]).

Later, when userspace closes the file descriptor, the DRM core invokes:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_close() {
	struct drm_sched_entity *entity = &ethosu_priv->sched_entity;

	drm_sched_entity_destroy(entity);
}

drm_sched_entity_destroy() attempts to flush the entity by accessing the freed
scheduler runqueues. This accesses entity->rq->sched, where entity->rq is a
dangling pointer to the freed runqueue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-ethosu-fixes-v2-0-3767738756a4@kernel.org?part=3