Re: [PATCH v2 4/5] accel/ethosu: propagate SRAM initialization errors

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unsafe hardware access in shared IRQ handler without Runtime PM check.
- [High] Missing synchronize_irq() in runtime suspend callback.
- [High] SRAM is allocated but left uninitialized during probe, leading to potential use of uninitialized memory.
--

--- Patch [4]: [PATCH v2 4/5] accel/ethosu: propagate SRAM initialization errors ---
commit 4382c986c5c63a85ef8db5d2db6b39f2605d7154
Author: GuoHan Zhao <zhaoguohan@kylinos.cn>

accel/ethosu: propagate SRAM initialization errors

ethosu_sram_init() returns -ENOMEM if allocation from a declared SRAM
pool fails. ethosu_init() ignores the error after recording the pool size.
Probe then registers a device with a nonzero SRAM size and a zero sramphys
address, allowing jobs to program address zero for SRAM.

Propagate the error and synchronously suspend the device before the probe
path releases the scheduler.

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 6b97d22444a7c..c825b67e9cba0 100644
> --- a/drivers/accel/ethosu/ethosu_drv.c
> +++ b/drivers/accel/ethosu/ethosu_drv.c
> @@ -331,7 +331,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;

[Severity: High]
This is a pre-existing issue, but is the SRAM left uninitialized during the
first active window after probe?

Earlier in ethosu_init(), we call ethosu_device_resume(), which attempts to
zero the SRAM via ethosu_reset(). However, since ethosu_sram_init() hasn't
run yet, SRAM isn't allocated and zeroing is skipped.

If a job is submitted within the 50ms autosuspend delay window after probe
finishes, pm_runtime_resume_and_get() will increment the usage counter but
skip ethosu_device_resume() since the device is already active.

Does this mean the first job could run with uninitialized garbage data in
the SRAM, leading to unpredictable hardware behavior?

>  
>  	dev_info(ethosudev->base.dev,
>  		 "Ethos-U NPU, arch v%ld.%ld.%ld, rev r%ldp%ld, cmd stream ver%ld, %d MACs, %dKB SRAM\n",
[ ... ]
> @@ -346,6 +348,9 @@ static int ethosu_init(struct ethosu_device *ethosudev)
>  
>  	return 0;
>  
> +err_runtime_suspend:
> +	ethosu_runtime_suspend(ethosudev);
> +	return ret;
>  err_suspend:
>  	pm_runtime_dont_use_autosuspend(ethosudev->base.dev);
>  	ethosu_device_suspend(ethosudev->base.dev);

[Severity: High]
This is a pre-existing issue, but does ethosu_device_suspend() need to call
synchronize_irq() before disabling the clocks?

Looking at ethosu_device_suspend(), it calls clk_bulk_disable_unprepare()
directly without synchronizing with the interrupt handler:

drivers/accel/ethosu/ethosu_drv.c:ethosu_device_suspend() {
	struct ethosu_device *ethosudev = dev_get_drvdata(dev);

	clk_bulk_disable_unprepare(ethosudev->num_clks, ethosudev->clks);
	return 0;
}

If the autosuspend timer expires and ethosu_device_suspend() executes while
an interrupt is mid-flight on another CPU, could the clocks be disabled
while the IRQ handler is actively accessing hardware registers, resulting in
a bus fault?

[Severity: High]
This is a pre-existing issue, but is it safe for ethosu_job_irq_handler()
to access hardware registers without checking if the device is active?

When the device enters runtime suspend and its clocks are disabled, a
different device sharing the same IRQ line could trigger an interrupt.

The ethosu_job_irq_handler() unconditionally reads NPU_REG_STATUS:

drivers/accel/ethosu/ethosu_job.c:ethosu_job_irq_handler() {
	...
	u32 status = readl_relaxed(dev->regs + NPU_REG_STATUS);
	...
}

Without calling pm_runtime_get_if_active() to check if the device is active
first, could this lead to a bus fault and kernel panic if the device is
suspended and unclocked?

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