Re: [PATCH 2/3] accel/ethosu: propagate SRAM initialization errors

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing hardware stop and wait for active jobs in the device removal path leads to DMA Use-After-Free.
- [High] The driver registers a shared interrupt handler but fails to check the device PM status before accessing hardware, and its suspend callback lacks IRQ synchronization.
- [High] Prematurely calling `drm_sched_fini()` during device removal leads to a Use-After-Free when userspace closes open DRM file entities.
- [High] Missing `drm_dev_enter()` and `drm_dev_exit()` protection in `ethosu_ioctl_submit` allows a TOCTOU race with device removal.
- [High] Calling `drm_sched_fini()` on `drm_sched_init()` failure causes a crash on uninitialized work items.
- [Medium] The device is not synchronously suspended during driver unbind/removal, leaving its hardware clocks permanently enabled.
--

--- Patch [2]: [PATCH 2/3] accel/ethosu: propagate SRAM initialization errors ---
commit d63b5342071ce711d48106e8b0fded82dbcf48e8
Author: GuoHan Zhao <zhaoguohan@kylinos.cn>

accel/ethosu: propagate SRAM initialization errors

This patch fixes a bug where ethosu_sram_init() errors were ignored in
ethosu_init(), leading to jobs programming physical address zero for SRAM. It
adds proper error propagation and synchronous suspension in the probe path.

Link: https://sashiko.dev/#/patchset/20260716065219.931088-1-zhaoguohan@kylinos.cn?part=1

> diff --git a/drivers/accel/ethosu/ethosu_drv.c b/drivers/accel/ethosu/ethosu_drv.c
> index d1153b15ca3ee..479156f79bc18 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -320,7 +320,9 @@ static int ethosu_init(struct ethosu_device *ethosudev)
>  	ethosudev->npu_info.id = id = readl_relaxed(ethosudev->regs + NPU_REG_ID);
>  	ethosudev->npu_info.config = config = readl_relaxed(ethosudev->regs + NPU_REG_CONFIG);
>  
> -	ethosu_sram_init(ethosudev);
> +	ret = ethosu_sram_init(ethosudev);
> +	if (ret)
> +		goto err_runtime_suspend;
>  
>  	dev_info(ethosudev->base.dev,

[ ... ]

> @@ -389,7 +391,7 @@ static void ethosu_remove(struct platform_device *pdev)
>  {
>  	struct ethosu_device *ethosudev = dev_get_drvdata(&pdev->dev);
>  
>  	drm_dev_unregister(&ethosudev->base);
>  	ethosu_job_fini(ethosudev);

[Severity: Medium]
This is a pre-existing issue, but does this removal path lack a synchronous
suspend of the device?

Since devres cleanup will eventually call pm_runtime_disable() which clears
the state but leaves hardware powered on if it was active, will failing to
call pm_runtime_put_sync_suspend() here lead to the clocks and power domains
leaking and remaining active indefinitely?

[Severity: High]
This isn't a bug introduced by this patch, but doesn't ethosu_job_fini()
destroy the DRM scheduler via drm_sched_fini() prematurely?

If userspace still holds a DRM file descriptor open after the device is
unbound, closing the file descriptor will call ethosu_job_close() which
invokes drm_sched_entity_destroy() on the already finalized scheduler.

Can this cause a use-after-free accessing destroyed DRM scheduler structures?

>  	ethosu_sram_fini(ethosudev);
>  }

[Severity: Critical]
This is a pre-existing issue, but is the SRAM freed while the hardware might
still be active?

If ethosu_remove() is executed while the NPU is actively executing a job,
ethosu_sram_fini() will immediately free the SRAM pool via gen_pool_free().

Since there is no halt of the hardware or wait for the in-flight job to
finish, could the hardware continue performing DMA read/writes to the freed
SRAM pool, leading to a DMA use-after-free?

> diff --git a/drivers/accel/ethosu/ethosu_job.c b/drivers/accel/ethosu/ethosu_job.c
> --- a/drivers/accel/ethosu/ethosu_job.c
> +++ b/drivers/accel/ethosu/ethosu_job.c
> @@ -230,8 +230,8 @@ static irqreturn_t ethosu_job_irq_handler(int irq, void *data)
>  {
>  	struct ethosu_device *dev = data;
>  	u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS);

[Severity: High]
This is a pre-existing issue, but this interrupt is requested with IRQF_SHARED
in ethosu_job_init(). If the device enters autosuspend and turns off its
clocks, could this unconditionally read NPU_REG_STATUS and cause a synchronous
external abort or spurious interrupt logic?

Should ethosu_job_irq_handler() use pm_runtime_get_if_active() before checking
the hardware registers?

Additionally, shouldn't ethosu_device_suspend() call synchronize_irq() to
ensure no handler is mid-flight before disabling clocks?

[ ... ]

> @@ -337,9 +337,7 @@ int ethosu_job_init(struct ethosu_device *edev)
>  	ret = drm_sched_init(&edev->sched, &args);
>  	if (ret) {
>  		dev_err(dev, "Failed to create scheduler: %d\n", ret);
>  		goto err_sched;
>  	}
>  
>  	return 0;
>  
>  err_sched:
>  	drm_sched_fini(&edev->sched);

[Severity: High]
This isn't a bug introduced by this patch, but should drm_sched_fini() be
called if drm_sched_init() fails?

If drm_sched_init() cleans up its internal resources upon returning an error,
would unconditionally calling drm_sched_fini() here attempt to destroy
workqueues and free resources that were never successfully initialized,
potentially leading to a panic?

[ ... ]

> @@ -485,9 +483,8 @@ int ethosu_ioctl_submit(struct drm_device *dev, void *data, struct drm_file *file
>  {
>  	struct drm_ethosu_submit *args = data;
>  	int ret = 0;

[Severity: High]
This is a pre-existing issue, but doesn't this ioctl lack drm_dev_enter() and
drm_dev_exit() unplug protection?

If userspace aggressively calls the DRM_ETHOSU_SUBMIT ioctl while the device
is being unbound, can it race with ethosu_remove()?

Without drm_dev_enter(), this could push a job onto the scheduler via
drm_sched_entity_push_job() while the removal thread is simultaneously
tearing it down with drm_sched_fini(), causing a kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716082530.1023538-1-zhaoguohan@kylinos.cn?part=2