Re: [PATCH v7 7/8] drm/panthor: Add suspend, resume and reset handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/panthor: perf: pending samples lost during slow reset on resume
- [Medium] drm/panthor: pending perf sample lost due to boot IRQ race during reset
- [Low] drm/panthor: perf: missing error check for perf_enable() in post_reset
--

commit d99b13df2062b5b287000f68e515acd129ee6cf2
Author: Lukas Zapolskas <lukas.zapolskas@arm.com>
Subject: drm/panthor: Add suspend, resume and reset handling

This commit introduces suspend, resume, and reset handling for the
performance counter subsystem in the Panthor driver. It ensures sampling
is disabled and re-enabled around suspends, and reprograms the firmware
interface after a reset to avoid losing pending sample data.

> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -539,8 +542,10 @@ int panthor_device_resume(struct device *dev)
>  			ret = panthor_device_resume_hw_components(ptdev);
>  		}
>  
> -		if (!ret)
> +		if (!ret) {
>  			panthor_sched_resume(ptdev);
> +			panthor_perf_resume(ptdev);
> +		}

[Severity: High]
Does calling panthor_perf_resume() unconditionally here cause pending
samples to be lost if a slow reset was performed during resume?

When the system resumes, panthor_device_resume() can perform a slow reset if
a fast reset fails or a reset was pending prior to suspend. A slow reset
zeroes out the firmware's shared memory, wiping out any pending requests.

Unlike panthor_perf_post_reset(), panthor_perf_resume() only calls
perf_enable() and omits the re-request logic. As a result,
fw_sample_requested remains permanently true in the driver while the
firmware is unaware of it.

This could cause all future performance sample requests for the session to be
silently dropped and permanently block session destruction.

> diff --git a/drivers/gpu/drm/panthor/panthor_perf.c b/drivers/gpu/drm/panthor/panthor_perf.c
> --- a/drivers/gpu/drm/panthor/panthor_perf.c
> +++ b/drivers/gpu/drm/panthor/panthor_perf.c
[ ... ]
> +void panthor_perf_post_reset(struct panthor_device *ptdev)
> +{
> +	bool rerequest = false;
> +
> +	if (!ptdev->perf)
> +		return;
> +
> +	guard(mutex)(&ptdev->perf->op_lock);
> +
> +	perf_enable(ptdev->perf);

[Severity: Low]
Is it intentional to discard the return value of perf_enable() here?

In panthor_perf_resume(), the return value of perf_enable() is checked and
logged upon failure via drm_warn(). 

If the firmware fails to acknowledge the sampling start request during a
reset recovery here, no warning is emitted and userspace will silently stop
receiving samples without any diagnostic trace in the kernel log.

> +
> +	scoped_guard(spinlock_irqsave, &ptdev->perf->pend_lock) {
> +		rerequest = ptdev->perf->fw_sample_requested;

[Severity: Medium]
Can this race with the boot interrupt handler and lose pending samples?

If a slow GPU reset occurs while a performance counter sample is pending,
the MCU is rebooted and fires a boot interrupt before
panthor_perf_post_reset() executes.

The interrupt handler panthor_job_irq_handler() processes this and calls
panthor_perf_report_irq(). Because the shared memory was zeroed by the
reset, req and ack will both be 0. This causes panthor_perf_report_irq() to
evaluate sample_acked as true and erroneously clear the host-side flag:

drivers/gpu/drm/panthor/panthor_perf.c:panthor_perf_report_irq() {
    ...
    const bool sample_acked = ((req & GLB_PERFCNT_SAMPLE) ==
                               (ack & GLB_PERFCNT_SAMPLE));
    ...
    if (sample_acked) {
        guard(spinlock_irqsave)(&perf->pend_lock);
        perf->fw_sample_requested = false;
    }
}

When the reset worker later reaches panthor_perf_post_reset(), it would see
rerequest as false and fail to re-request the sample, breaking the exact
feature introduced by this patch.

> +
> +		if (rerequest)
> +			ptdev->perf->fw_sample_requested = false;
> +	}
> +
> +	if (rerequest)
> +		perf_fw_request_sample(ptdev->perf);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011-panthor-performance-counters-v7-0-12c32495062f@arm.com?part=7