Re: [PATCH v11 02/14] accel/rocket: take the completion register writes under job_lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Reference count leak of the IOMMU group in rocket_job_handle_irq().
- [High] The hardirq handler accesses hardware registers without ensuring the device is active, causing invalid register reads or bus faults when a shared IRQ fires while the device is runtime-suspended.
- [High] Threaded IRQ handler accesses hardware registers without a PM reference if there is no in-flight job, causing a bus fault.
- [High] Missing `synchronize_irq()` in runtime suspend callback for a shared IRQ, risking mid-flight handler execution.
- [High] Calling drm_sched_fini() on a failed scheduler initialization causes a null pointer dereference.
--

commit 873cb013be0f4784d0270a98eec1dd55e8a02c68
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: take the completion register writes under job_lock

This commit moves the OPERATION_ENABLE and INTERRUPT_CLEAR register writes
inside the job_lock scoped_guard block in rocket_job_handle_irq(). It fixes
a race condition where a completion being handled could overwrite a newly
submitted task's enable bit.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 3141f210fcd1b..5f0f9682e57ca 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -345,10 +345,15 @@ static void rocket_job_handle_irq(struct rocket_core *core)
>  {
>  	pm_runtime_mark_last_busy(core->dev);
>  
> -	rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
> -	rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
> +	scoped_guard(mutex, &core->job_lock) {
> +		/*
> +		 * Stopping the block belongs under the lock. hw_submit() writes
> +		 * OPERATION_ENABLE too, and outside the lock this zero can land
> +		 * after that one and stop a task that has only just started.
> +		 */
> +		rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
> +		rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);

[Severity: High]
This is a pre-existing issue, but do these unconditional hardware writes risk
causing a bus fault?

If a shared interrupt fires while the NPU is runtime-suspended, the threaded
handler will execute with core->in_flight_job == NULL. Because no PM reference
is held in that case, the hardware is powered down, and writing to these
registers could cause an async SError.

>  
> -	scoped_guard(mutex, &core->job_lock)
>  		if (core->in_flight_job) {
>  			if (core->in_flight_job->next_task_idx < core->in_flight_job->task_count) {
>  				rocket_job_hw_submit(core, core->in_flight_job);
> @@ -360,6 +365,7 @@ static void rocket_job_handle_irq(struct rocket_core *core)
>  			pm_runtime_put_autosuspend(core->dev);
>  			core->in_flight_job = NULL;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does this code leak a reference
to the IOMMU group?

drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() {
    ...
    if (core->in_flight_job) {
        ...
        iommu_detach_group(NULL, iommu_group_get(core->dev));
        dma_fence_signal(core->in_flight_job->done_fence);
        ...
}

The call to iommu_group_get() increments the kobject reference count for the
group, but its result is leaked without a corresponding put. Over time on the
hot success path, could this overflow the refcount and prevent the IOMMU group
from ever being freed?

> +	}
>  }

[ ... ]

[Severity: High]
This is a pre-existing issue, but can the hardirq handler access hardware
registers while the device is runtime-suspended?

drivers/accel/rocket/rocket_job.c:rocket_job_irq_handler() {
    struct rocket_core *core = data;
    u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);
    ...
}

Since the driver registers a shared IRQ (IRQF_SHARED), another device on the
same IRQ line can trigger an interrupt. The handler will unconditionally read
INTERRUPT_RAW_STATUS without calling pm_runtime_get_if_active(). Could this
cause invalid register reads or a bus fault if the NPU's power domain is down?

[ ... ]

[Severity: High]
This is a pre-existing issue, but is it safe to disable the device clocks here
without synchronizing the shared IRQ line?

drivers/accel/rocket/rocket_drv.c:rocket_device_runtime_suspend() {
    ...
    if (!rocket_job_is_idle(&rdev->cores[core]))
        return -EBUSY;

    clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks), rdev->cores[core].clks);
    return 0;
}

Because the interrupt line is shared, an IRQ handler could be executing on
another CPU mid-flight. Does this need a synchronize_irq() before powering
down hardware to prevent the handler from accessing hardware whose clocks have
just been disabled?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but will drm_sched_fini() cause a
null pointer dereference if scheduler initialization fails?

drivers/accel/rocket/rocket_job.c:rocket_job_init() {
    ...
    ret = drm_sched_init(&core->sched, &args);
    if (ret) {
        dev_err(core->dev, "Failed to create scheduler: %d.", ret);
        goto err_sched;
    }
    return 0;

err_sched:
    drm_sched_fini(&core->sched);
    ...
}

If drm_sched_init() fails (e.g. due to memory allocation failure), it frees
its internal structures and leaves sched->sched_rq as NULL but leaves
sched->num_rqs > 0. Would calling drm_sched_fini() on the error path attempt
to dereference the NULL sched->sched_rq array?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831081956.84871-1-gahing@gahingwoo.com?part=2