Re: [PATCH v12 05/14] accel/rocket: factor the completion tail out of the IRQ handler

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Leaking IOMMU group reference on job completion in rocket_job_next_locked().
- [High] Unconditional hardware access in shared IRQ handler while device may be suspended.
- [High] Missing synchronize_irq() in runtime suspend callback allows shared IRQ to race with clock disable.
--

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

accel/rocket: factor the completion tail out of the IRQ handler

rocket_job_handle_irq() stops the block and then either starts the job's
next task or retires the job. The second half is a step of its own and
reads better with a name, now that taking the register writes under
job_lock has moved it a level deeper inside the scoped guard.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index b588049aaa6ce..8cffe93f6c5ab 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -341,6 +341,25 @@ static struct dma_fence *rocket_job_run(struct drm_sched_job *sched_job)
>  	return ERR_PTR(ret);
>  }
>  
> +/* Start the job's next task, or retire it. Caller holds job_lock. */
> +static void rocket_job_next_locked(struct rocket_core *core)
> +{
> +	lockdep_assert_held(&core->job_lock);
> +
> +	if (!core->in_flight_job)
> +		return;
> +
> +	if (core->in_flight_job->next_task_idx < core->in_flight_job->task_count) {
> +		rocket_job_hw_submit(core, core->in_flight_job);
> +		return;
> +	}
> +
> +	iommu_detach_group(NULL, iommu_group_get(core->dev));

[Severity: High]
This is a pre-existing issue, but does this code leak the iommu_group
reference?

iommu_group_get() explicitly increments the reference count of the IOMMU group,
but iommu_detach_group() only uses the pointer and doesn't take ownership.

Since iommu_group_put() is never called here, the reference leaks on every
job completion. This can eventually saturate the refcount and prevent release
during device teardown.

Would it be better to use the already-cached core->iommu_group directly here?

> +	dma_fence_signal(core->in_flight_job->done_fence);
> +	pm_runtime_put_autosuspend(core->dev);
> +	core->in_flight_job = NULL;
> +}
> +
>  static void rocket_job_handle_irq(struct rocket_core *core)
>  {

[ ... ]

[Severity: High]
This is another pre-existing issue, but can this unconditionally access
hardware registers while the device is suspended?

The IRQ is requested with IRQF_SHARED:

rocket_job_init() {
    ...
    ret = devm_request_threaded_irq(core->dev, core->irq,
                                    rocket_job_irq_handler,
                                    ...
                                    IRQF_SHARED, dev_name(core->dev),
                                    core);
    ...
}

However, the handler immediately reads hardware registers without checking if
the device is active:

rocket_job_irq_handler() {
    ...
    u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);
    ...
}

If another device on the same shared line raises an interrupt while the Rocket
device is suspended, reading the suspended device's registers can cause a bus
fault or return 0xffffffff.

If it returns 0xffffffff, bitwise checks will falsely pass, leading to
invalid writes to INTERRUPT_MASK and spuriously waking the threaded handler.

Should this use pm_runtime_get_if_active() before accessing the registers?


[Severity: High]
This is a pre-existing issue, but can the shared IRQ handler race with
clock disable during runtime suspend?

The runtime suspend callback disables clocks without first synchronizing the
IRQ:

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

If another device on the shared IRQ line triggers an interrupt concurrently,
rocket_job_irq_handler() could execute on another CPU while the hardware clocks
are being disabled, potentially leading to a bus fault or system crash.

Does this need a call to synchronize_irq() before powering down the hardware?

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