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 <[email protected]> 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/[email protected]?part=7
