[PATCH v3 00/13] RPM, perfcnt and other minor fixes for Panfrost

[PATCH v3 00/13] RPM, perfcnt and other minor fixes for Panfrost

[Adrián Larumbe]

This patch series comes with some minor fixes for Panfrost. Some of the
issues addressed were uncovered by LLM tools, while the one about RPM was
traditionally debugged.

The only proper bug fixes are the ones for perfcnt and RPM, and the others
are more about ensuring robustness. perfcnt bug was never observed in the
open, because no one had faced a HW reset when a perfcnt session was
ongoing. RPM fix addresses a well-known issue I had been observing for
quite some time, but that I had completely misunderstood.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
Changes in v3:
- Applied some minor suggestions for the first couple of patches in the series.
- Moved shrinker initialisation into gem initialisation.
- Fixed RPM bugs by moving all clock and power initialisation into RPM resume.
- Added patch for reset sequence fixes and also a userspace knob to trigger it.
- Reworked perfcnt fix by having the reset sequence restore its initial state.
- Link to v2: https://patch.msgid.link/20260604-claude-fixes-v2-0-57c6bd4c1655@collabora.com

Changes in v2:
- Fixed race conditions introduced by the previous revision, in the RPM and
HWPerf commits specifically. These can be consulted at [1]
- Don't attempt to suspend the device unconditionally at the end of
probe(), and do it through autosuspend instead.
- Broke the RPM patch into one that fixes PM refcnt proper and another one
for MMU enablement at device init time.
- Moved perfcnt GPU disable helper into its own commmit. Same for
transplating shrinker initialisation and unplug.
- Added 'Fixes' and 'Reported-by' tags to all the relevant commits
- [1] https://sashiko.dev/#/patchset/20260526-claude-fixes-v1-0-16e92eaa4949%40collabora.com
- Link to v1: https://patch.msgid.link/20260526-claude-fixes-v1-0-16e92eaa4949@collabora.com

To: Boris Brezillon <boris.brezillon@collabora.com>
To: Rob Herring <robh@kernel.org>
To: Steven Price <steven.price@arm.com>
To: Adrián Larumbe <adrian.larumbe@collabora.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Faith Ekstrand <faith.ekstrand@collabora.com>
To: "Marty E. Plummer" <hanetzer@startmail.com>
To: Tomeu Vizoso <tomeu@tomeuvizoso.net>
To: Eric Anholt <eric@anholt.net>
To: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
To: Robin Murphy <robin.murphy@arm.com>
To: Philipp Zabel <p.zabel@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: Neil Armstrong <neil.armstrong@linaro.org>

---
Adrián Larumbe (13):
      drm/panfrost: Check another bo field for cache option query
      drm/panfrost: Prevent division by 0
      drm/panfrost: Remove unnecessary header file include
      drm/panfrost: Move shrinker initialization and unplug one level down
      drm/panfrost: Move all device power up and down into RPM callbacks
      drm/panfrost: Explicitly enable MMU interrupts at device init
      drm/panfrost: Sync with IRQ before MMU disable and reset
      drm/panfrost: Rewire reset sequence to avoid concurrent attempts
      drm/panfrost: Add debugfs knob for manually triggering a GPU reset
      drm/panfrost: Move perfcnt GPU disable sequence into a helper
      drm/panfrost: Introduce a reset lock
      drm/panfrost: Fix races between perfcnt and reset sequence
      drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field

 drivers/gpu/drm/panfrost/panfrost_devfreq.c      |   2 +-
 drivers/gpu/drm/panfrost/panfrost_device.c       | 401 +++++++++++++++--------
 drivers/gpu/drm/panfrost/panfrost_device.h       |  12 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c          |  34 +-
 drivers/gpu/drm/panfrost/panfrost_gem.c          |   6 +-
 drivers/gpu/drm/panfrost/panfrost_gem.h          |   6 +-
 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c |   8 +-
 drivers/gpu/drm/panfrost/panfrost_gpu.c          |   7 +-
 drivers/gpu/drm/panfrost/panfrost_job.c          |  18 +-
 drivers/gpu/drm/panfrost/panfrost_mmu.c          |  22 +-
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c      | 225 ++++++++-----
 drivers/gpu/drm/panfrost/panfrost_perfcnt.h      |   2 +
 include/uapi/drm/panfrost_drm.h                  |   3 +-
 13 files changed, 476 insertions(+), 270 deletions(-)
---
base-commit: 671b7825dbfe9ea6e3ad3001003aeee0df48d1b5
change-id: 20260523-claude-fixes-82b03a134a8c

Best regards,
--  
Adrián Larumbe <adrian.larumbe@collabora.com>

[PATCH v3 01/13] drm/panfrost: Check another bo field for cache option query

[Adrián Larumbe]

When querying the properties of a BO, whether it was created in
Writeback caching mode for userspace mappings, is recorded in a Panfrost
BO's wb_mmap field.

Reported-by: Claude <noreply@anthropic.com>
Closes: https://gitlab.freedesktop.org/panfrost/linux/-/work_items/88
Fixes: 62eedf1ccba5 ("drm/panfrost: Add flag to map GEM object Write-Back Cacheable")
Reviewed-by: Steven Price <steven.price@arm.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 784e36d72c2b..2e931fa90da0 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -661,7 +661,7 @@ static int panfrost_ioctl_query_bo_info(struct drm_device *dev, void *data,
 		if (bo->is_heap)
 			args->create_flags |= PANFROST_BO_HEAP;
 
-		if (!bo->base.map_wc)
+		if (bo->wb_mmap)
 			args->create_flags |= PANFROST_BO_WB_MMAP;
 	}
 

-- 
2.55.0

[PATCH v3 02/13] drm/panfrost: Prevent division by 0

[Adrián Larumbe]

When updating and debug-printing devfreq stats, in the very unlikely
offchance that total device time is less than 100 ns, clamp it to 0 to
avoid division by 0.

Reported-by: Claude <noreply@anthropic.com>
Closes: https://gitlab.freedesktop.org/panfrost/linux/-/work_items/88
Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index b51c30778811..0fe81d259274 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -78,7 +78,7 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
 
 	dev_dbg(pfdev->base.dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
 		status->busy_time, status->total_time,
-		status->busy_time / (status->total_time / 100),
+		status->busy_time * 100 / MAX(status->total_time, 1),
 		status->current_frequency / 1000 / 1000);
 
 	return 0;

-- 
2.55.0

Re: [PATCH v3 02/13] drm/panfrost: Prevent division by 0

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Multiplication of `status->busy_time` by 100 overflows the 32-bit `unsigned long` type on 32-bit architectures, leading to completely incorrect utilization percentages in the debug output.
- [Medium] Meaningful code and commit message mismatch regarding the clamping logic.
--

--- Patch [2]: [PATCH v3 02/13] drm/panfrost: Prevent division by 0 ---
commit 5fb0fa4b6ede9db49d32560487844d56d5235d47
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Prevent division by 0
    
When updating and debug-printing devfreq stats, in the very unlikely
offchance that total device time is less than 100 ns, clamp it to 0 to
avoid division by 0.

[Severity: Medium]
The commit message says "clamp it to 0 to avoid division by 0". Is this
description accurate? Clamping a divisor to 0 would guarantee a division
by zero, and the actual code clamps the minimum value to 1 instead of 0.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index b51c307788110..0fe81d259274e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -78,7 +78,7 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
>  
>  	dev_dbg(pfdev->base.dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
>  		status->busy_time, status->total_time,
> -		status->busy_time / (status->total_time / 100),
> +		status->busy_time * 100 / MAX(status->total_time, 1),

[Severity: Medium]
Will this multiplication overflow on 32-bit architectures?

Because `status->busy_time` is in nanoseconds, a fully busy GPU over a typical
50ms devfreq polling interval results in 50,000,000 ns. Multiplying this by 100
yields 5,000,000,000, which exceeds ULONG_MAX (4,294,967,295) on 32-bit systems
and would wrap around, reporting a roughly 14% utilization instead of 100%.

>  		status->current_frequency / 1000 / 1000);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=2

[PATCH v3 03/13] drm/panfrost: Remove unnecessary header file include

[Adrián Larumbe]

None of panfrost_features.h definitions are used in panfrost_device.c

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 7fed22d555a5..d8421fd6a662 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -11,7 +11,6 @@
 
 #include "panfrost_device.h"
 #include "panfrost_devfreq.h"
-#include "panfrost_features.h"
 #include "panfrost_gem.h"
 #include "panfrost_issues.h"
 #include "panfrost_gpu.h"

-- 
2.55.0

[PATCH v3 04/13] drm/panfrost: Move shrinker initialization and unplug one level down

[Adrián Larumbe]

Since the moment we call drm_dev_register() the device should be in a
position to accept jobs, so it's best if the shrinker is already
initialized by then.

On top of that, make shrinker functions take an panfrost_device pointer
like other functions in the same sequence and rename them accordingly.

Essentially mimic the init/fini behaviour in Panthor.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c       | 8 +++++++-
 drivers/gpu/drm/panfrost/panfrost_drv.c          | 6 ------
 drivers/gpu/drm/panfrost/panfrost_gem.c          | 6 ++++--
 drivers/gpu/drm/panfrost/panfrost_gem.h          | 6 +++---
 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c | 8 ++------
 5 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index d8421fd6a662..03905495aee2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -281,9 +281,14 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 	if (err)
 		goto out_job;
 
-	panfrost_gem_init(pfdev);
+	err = panfrost_gem_init(pfdev);
+	if (err)
+		goto out_perfcnt;
 
 	return 0;
+
+out_perfcnt:
+	panfrost_perfcnt_fini(pfdev);
 out_job:
 	panfrost_jm_fini(pfdev);
 out_mmu:
@@ -305,6 +310,7 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 
 void panfrost_device_fini(struct panfrost_device *pfdev)
 {
+	panfrost_gem_shrinker_fini(pfdev);
 	panfrost_perfcnt_fini(pfdev);
 	panfrost_jm_fini(pfdev);
 	panfrost_mmu_fini(pfdev);
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 2e931fa90da0..331a3bd5b98c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -1002,14 +1002,9 @@ static int panfrost_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto err_out1;
 
-	err = panfrost_gem_shrinker_init(&pfdev->base);
-	if (err)
-		goto err_out2;
 
 	return 0;
 
-err_out2:
-	drm_dev_unregister(&pfdev->base);
 err_out1:
 	pm_runtime_disable(pfdev->base.dev);
 	panfrost_device_fini(pfdev);
@@ -1023,7 +1018,6 @@ static void panfrost_remove(struct platform_device *pdev)
 	struct panfrost_device *pfdev = platform_get_drvdata(pdev);
 
 	drm_dev_unregister(&pfdev->base);
-	panfrost_gem_shrinker_cleanup(&pfdev->base);
 
 	pm_runtime_get_sync(pfdev->base.dev);
 	pm_runtime_disable(pfdev->base.dev);
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 3a7fce428898..d2cd357a2ae3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -15,13 +15,13 @@
 #include "panfrost_gem.h"
 #include "panfrost_mmu.h"
 
-void panfrost_gem_init(struct panfrost_device *pfdev)
+int panfrost_gem_init(struct panfrost_device *pfdev)
 {
 	int err;
 
 	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
 	    !panfrost_transparent_hugepage)
-		return;
+		return 0;
 
 	err = drm_gem_huge_mnt_create(&pfdev->base, "within_size");
 	if (drm_gem_get_huge_mnt(&pfdev->base))
@@ -29,6 +29,8 @@ void panfrost_gem_init(struct panfrost_device *pfdev)
 	else if (err)
 		drm_warn(&pfdev->base, "Can't use Transparent Hugepage (%d)\n",
 			 err);
+
+	return panfrost_gem_shrinker_init(pfdev);
 }
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
index 79d4377019e9..d6e55c2e4d86 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -129,7 +129,7 @@ drm_mm_node_to_panfrost_mapping(struct drm_mm_node *node)
 	return container_of(node, struct panfrost_gem_mapping, mmnode);
 }
 
-void panfrost_gem_init(struct panfrost_device *pfdev);
+int panfrost_gem_init(struct panfrost_device *pfdev);
 
 struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size);
 
@@ -154,8 +154,8 @@ panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
 void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping);
 void panfrost_gem_teardown_mappings_locked(struct panfrost_gem_object *bo);
 
-int panfrost_gem_shrinker_init(struct drm_device *dev);
-void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
+int panfrost_gem_shrinker_init(struct panfrost_device *pfdev);
+void panfrost_gem_shrinker_fini(struct panfrost_device *pfdev);
 
 void panfrost_gem_set_label(struct drm_gem_object *obj, const char *label);
 int panfrost_gem_sync(struct drm_gem_object *obj, u32 type,
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
index 2fe967a90bcb..fefae87535d6 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
@@ -95,10 +95,8 @@ panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
  *
  * This function registers and sets up the panfrost shrinker.
  */
-int panfrost_gem_shrinker_init(struct drm_device *dev)
+int panfrost_gem_shrinker_init(struct panfrost_device *pfdev)
 {
-	struct panfrost_device *pfdev = to_panfrost_device(dev);
-
 	pfdev->shrinker = shrinker_alloc(0, "drm-panfrost");
 	if (!pfdev->shrinker)
 		return -ENOMEM;
@@ -118,10 +116,8 @@ int panfrost_gem_shrinker_init(struct drm_device *dev)
  *
  * This function unregisters the panfrost shrinker.
  */
-void panfrost_gem_shrinker_cleanup(struct drm_device *dev)
+void panfrost_gem_shrinker_fini(struct panfrost_device *pfdev)
 {
-	struct panfrost_device *pfdev = to_panfrost_device(dev);
-
 	if (pfdev->shrinker)
 		shrinker_free(pfdev->shrinker);
 }

-- 
2.55.0

Re: [PATCH v3 04/13] drm/panfrost: Move shrinker initialization and unplug one level down

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The Panfrost GEM shrinker is silently skipped and left uninitialized if transparent hugepages (THP) are disabled.
--

--- Patch [4]: [PATCH v3 04/13] drm/panfrost: Move shrinker initialization and unplug one level down ---
commit ba73347cedd0ca5a2e99301107bfd0b9268d44b4
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Move shrinker initialization and unplug one level down

This commit moves the panfrost shrinker initialization into
panfrost_gem_init() so it is ready before drm_dev_register() completes.
It also updates the shrinker functions to take a panfrost_device pointer.

[ ... ]

> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 3a7fce4288987..d2cd357a2ae33 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -15,13 +15,13 @@
>  #include "panfrost_gem.h"
>  #include "panfrost_mmu.h"
>  
> -void panfrost_gem_init(struct panfrost_device *pfdev)
> +int panfrost_gem_init(struct panfrost_device *pfdev)
>  {
>  	int err;
>  
>  	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
>  	    !panfrost_transparent_hugepage)
> -		return;
> +		return 0;

[Severity: High]
Will this early return bypass the newly added shrinker initialization at the
end of the function?

If the kernel is compiled with CONFIG_TRANSPARENT_HUGEPAGE and the module
parameter panfrost.transparent_hugepage is set to false, panfrost_gem_init() 
returns early here. This leaves the GEM shrinker completely uninitialized, 
which could lead to out-of-memory conditions under memory pressure because
memory reclaim capabilities for GEM objects are lost.

>  
>  	err = drm_gem_huge_mnt_create(&pfdev->base, "within_size");
>  	if (drm_gem_get_huge_mnt(&pfdev->base))
> @@ -29,6 +29,8 @@ void panfrost_gem_init(struct panfrost_device *pfdev)
>  	else if (err)
>  		drm_warn(&pfdev->base, "Can't use Transparent Hugepage (%d)\n",
>  			 err);
> +
> +	return panfrost_gem_shrinker_init(pfdev);
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=4

[PATCH v3 05/13] drm/panfrost: Move all device power up and down into RPM callbacks

[Adrián Larumbe]

During device probe(), failure to do a PM get() will leave the usage_count
set to 0, which is the value assigned at device creation time. That means
when the autosuspend delay expires, runtime suspend callback won't be
invoked, so the device will remain powered on forever.

On top of that, failure to call PM put() during device unplug means
Panfrost device's PM usage_count increases monotonically for every new
module reload.

The combined outcome of both of the above was that devfreq OPP transition
notifications would be printed all the time, even when no jobs are being
submitted. This quickly fills the kernel ring buffer with junk.

Even direr than that was the fact MMU interrupts are only enabled when
the device is reset, so after device probe() the very first job targeting
the tiler heap BO would always time out, because the driver's PM runtime
resume callback would not be invoked.

Fix all that by moving all GPU enabling and disabling into RPM resume and
suspend callbacks, and making sure we resume the device right before
touching any HW registers. This is done in imitation of the Panthor model.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Fixes: 635430797d3f ("drm/panfrost: Rework runtime PM initialization")
Fixes: 876b15d2c88d ("drm/panfrost: Fix module unload")
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 373 +++++++++++++++++------------
 drivers/gpu/drm/panfrost/panfrost_device.h |   3 +
 drivers/gpu/drm/panfrost/panfrost_drv.c    |  13 +-
 drivers/gpu/drm/panfrost/panfrost_gpu.c    |   7 +-
 4 files changed, 228 insertions(+), 168 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 03905495aee2..d68f1ec82895 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -18,6 +18,11 @@
 #include "panfrost_mmu.h"
 #include "panfrost_perfcnt.h"
 
+static bool panfrost_device_is_initialized(struct panfrost_device *pfdev)
+{
+	return !!pfdev->js;
+}
+
 static int panfrost_reset_init(struct panfrost_device *pfdev)
 {
 	pfdev->rstc = devm_reset_control_array_get_optional_exclusive(pfdev->base.dev);
@@ -34,6 +39,32 @@ static void panfrost_reset_fini(struct panfrost_device *pfdev)
 	reset_control_assert(pfdev->rstc);
 }
 
+static int panfrost_clk_enable(struct panfrost_device *pfdev)
+{
+	int err;
+
+	err = clk_enable(pfdev->clock);
+	if (err)
+		return err;
+
+	err = clk_enable(pfdev->bus_clock);
+	if (err)
+		goto disable_clock;
+
+	err = clk_enable(pfdev->bus_ace_clock);
+	if (err)
+		goto disable_bus_clock;
+
+	return 0;
+
+disable_bus_clock:
+	clk_disable(pfdev->bus_clock);
+disable_clock:
+	clk_disable(pfdev->clock);
+
+	return err;
+}
+
 static int panfrost_clk_init(struct panfrost_device *pfdev)
 {
 	int err;
@@ -48,7 +79,7 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
 	rate = clk_get_rate(pfdev->clock);
 	dev_info(pfdev->base.dev, "clock rate = %lu\n", rate);
 
-	err = clk_prepare_enable(pfdev->clock);
+	err = clk_prepare(pfdev->clock);
 	if (err)
 		return err;
 
@@ -57,35 +88,43 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
 		dev_err(pfdev->base.dev, "get bus_clock failed %ld\n",
 			PTR_ERR(pfdev->bus_clock));
 		err = PTR_ERR(pfdev->bus_clock);
-		goto disable_clock;
+		goto unprepare_clock;
 	}
 
 	if (pfdev->bus_clock) {
 		rate = clk_get_rate(pfdev->bus_clock);
 		dev_info(pfdev->base.dev, "bus_clock rate = %lu\n", rate);
 
-		err = clk_prepare_enable(pfdev->bus_clock);
+		err = clk_prepare(pfdev->bus_clock);
 		if (err)
-			goto disable_clock;
+			goto unprepare_clock;
 	}
 
 	pfdev->bus_ace_clock = devm_clk_get_optional(pfdev->base.dev, "bus_ace");
 	if (IS_ERR(pfdev->bus_ace_clock)) {
 		err = PTR_ERR(pfdev->bus_ace_clock);
 		dev_err(pfdev->base.dev, "get bus_ace_clock failed %d\n", err);
-		goto disable_bus_clock;
+		goto unprepare_bus_clock;
 	}
 
-	err = clk_prepare_enable(pfdev->bus_ace_clock);
+	err = clk_prepare(pfdev->bus_ace_clock);
 	if (err)
-		goto disable_bus_clock;
+		goto unprepare_bus_clock;
+
+	if (!(pfdev->comp->pm_features & BIT(GPU_PM_RT))) {
+		err = panfrost_clk_enable(pfdev);
+		if (err)
+			goto unprepare_bus_ace_clock;
+	}
 
 	return 0;
 
-disable_bus_clock:
-	clk_disable_unprepare(pfdev->bus_clock);
-disable_clock:
-	clk_disable_unprepare(pfdev->clock);
+unprepare_bus_ace_clock:
+	clk_unprepare(pfdev->bus_clock);
+unprepare_bus_clock:
+	clk_unprepare(pfdev->bus_clock);
+unprepare_clock:
+	clk_unprepare(pfdev->clock);
 
 	return err;
 }
@@ -212,6 +251,143 @@ static int panfrost_pm_domain_init(struct panfrost_device *pfdev)
 	return err;
 }
 
+static int panfrost_device_runtime_resume(struct device *dev)
+{
+	struct panfrost_device *pfdev = dev_get_drvdata(dev);
+	int ret;
+
+	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
+		ret = reset_control_deassert(pfdev->rstc);
+		if (ret)
+			return ret;
+
+		ret = panfrost_clk_enable(pfdev);
+		if (ret)
+			goto err_clk;
+	}
+
+	panfrost_device_reset(pfdev, true);
+	panfrost_devfreq_resume(pfdev);
+
+	return 0;
+
+err_clk:
+	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
+		reset_control_assert(pfdev->rstc);
+	return ret;
+}
+
+static int panfrost_device_runtime_suspend(struct device *dev)
+{
+	struct panfrost_device *pfdev = dev_get_drvdata(dev);
+
+	if (!panfrost_jm_is_idle(pfdev))
+		return -EBUSY;
+
+	panfrost_devfreq_suspend(pfdev);
+	panfrost_jm_suspend_irq(pfdev);
+	panfrost_mmu_suspend_irq(pfdev);
+	panfrost_gpu_suspend_irq(pfdev);
+	panfrost_gpu_power_off(pfdev);
+
+	if (panfrost_device_is_initialized(pfdev) &&
+	    pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
+		clk_disable(pfdev->bus_ace_clock);
+		clk_disable(pfdev->bus_clock);
+		clk_disable(pfdev->clock);
+		reset_control_assert(pfdev->rstc);
+	}
+
+	if (!panfrost_device_is_initialized(pfdev)) {
+		panfrost_clk_fini(pfdev);
+		reset_control_assert(pfdev->rstc);
+	}
+
+	return 0;
+}
+
+static int panfrost_device_resume(struct device *dev)
+{
+	struct panfrost_device *pfdev = dev_get_drvdata(dev);
+	int ret;
+
+	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF)) {
+		unsigned long freq = pfdev->pfdevfreq.fast_rate;
+		struct dev_pm_opp *opp;
+
+		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+		if (IS_ERR(opp))
+			return PTR_ERR(opp);
+		dev_pm_opp_set_opp(dev, opp);
+		dev_pm_opp_put(opp);
+	}
+
+	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
+		ret = clk_enable(pfdev->clock);
+		if (ret)
+			goto err_clk;
+
+		if (pfdev->bus_clock) {
+			ret = clk_enable(pfdev->bus_clock);
+			if (ret)
+				goto err_bus_clk;
+		}
+	}
+
+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		goto err_resume;
+
+	return 0;
+
+err_resume:
+	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS) && pfdev->bus_clock)
+		clk_disable(pfdev->bus_clock);
+err_bus_clk:
+	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS))
+		clk_disable(pfdev->clock);
+err_clk:
+	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
+		dev_pm_opp_set_opp(dev, NULL);
+	return ret;
+}
+
+static int panfrost_device_suspend(struct device *dev)
+{
+	struct panfrost_device *pfdev = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		return ret;
+
+	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
+		if (pfdev->bus_clock)
+			clk_disable(pfdev->bus_clock);
+
+		clk_disable(pfdev->clock);
+	}
+
+	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
+		dev_pm_opp_set_opp(dev, NULL);
+
+	return 0;
+}
+
+EXPORT_GPL_DEV_PM_OPS(panfrost_pm_ops) = {
+	RUNTIME_PM_OPS(panfrost_device_runtime_suspend, panfrost_device_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(panfrost_device_suspend, panfrost_device_resume)
+};
+
+void panfrost_try_suspend_device(struct panfrost_device *pfdev)
+{
+	pm_runtime_put_sync_suspend(pfdev->base.dev);
+
+	/* If PM is disabled, we need to call the suspend handler manually. */
+	if (!IS_ENABLED(CONFIG_PM))
+		panfrost_device_runtime_suspend(pfdev->base.dev);
+}
+
 int panfrost_device_init(struct panfrost_device *pfdev)
 {
 	int err;
@@ -265,10 +441,25 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 		goto out_regulator;
 	}
 
-	err = panfrost_gpu_init(pfdev);
+	err = devm_pm_runtime_enable(pfdev->base.dev);
+	if (err)
+		goto out_regulator;
+
+	err = pm_runtime_resume_and_get(pfdev->base.dev);
 	if (err)
 		goto out_regulator;
 
+	/* If PM is disabled, we need to call panfrost_device_runtime_resume() manually. */
+	if (!IS_ENABLED(CONFIG_PM)) {
+		err = panfrost_device_runtime_resume(pfdev->base.dev);
+		if (err)
+			goto out_regulator;
+	}
+
+	err = panfrost_gpu_init(pfdev);
+	if (err)
+		goto out_rpm_put;
+
 	err = panfrost_mmu_init(pfdev);
 	if (err)
 		goto out_gpu;
@@ -295,6 +486,8 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 	panfrost_mmu_fini(pfdev);
 out_gpu:
 	panfrost_gpu_fini(pfdev);
+out_rpm_put:
+	panfrost_try_suspend_device(pfdev);
 out_regulator:
 	panfrost_regulator_fini(pfdev);
 out_devfreq:
@@ -313,12 +506,8 @@ void panfrost_device_fini(struct panfrost_device *pfdev)
 	panfrost_gem_shrinker_fini(pfdev);
 	panfrost_perfcnt_fini(pfdev);
 	panfrost_jm_fini(pfdev);
-	panfrost_mmu_fini(pfdev);
-	panfrost_gpu_fini(pfdev);
-	panfrost_devfreq_fini(pfdev);
 	panfrost_regulator_fini(pfdev);
-	panfrost_clk_fini(pfdev);
-	panfrost_reset_fini(pfdev);
+	panfrost_devfreq_fini(pfdev);
 	panfrost_pm_domain_fini(pfdev);
 }
 
@@ -424,148 +613,20 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
 
 void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 {
-	panfrost_gpu_soft_reset(pfdev);
-
-	panfrost_gpu_power_on(pfdev);
-	panfrost_mmu_reset(pfdev);
-
-	panfrost_jm_reset_interrupts(pfdev);
-	if (enable_job_int)
-		panfrost_jm_enable_interrupts(pfdev);
-}
-
-static int panfrost_device_runtime_resume(struct device *dev)
-{
-	struct panfrost_device *pfdev = dev_get_drvdata(dev);
 	int ret;
 
-	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
-		ret = reset_control_deassert(pfdev->rstc);
-		if (ret)
-			return ret;
-
-		ret = clk_enable(pfdev->clock);
-		if (ret)
-			goto err_clk;
-
-		ret = clk_enable(pfdev->bus_clock);
-		if (ret)
-			goto err_bus_clk;
-
-		ret = clk_enable(pfdev->bus_ace_clock);
-		if (ret)
-			goto err_bus_ace_clk;
-	}
-
-	panfrost_device_reset(pfdev, true);
-	panfrost_devfreq_resume(pfdev);
-
-	return 0;
-
-err_bus_ace_clk:
-	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
-		clk_disable(pfdev->bus_clock);
-err_bus_clk:
-	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
-		clk_disable(pfdev->clock);
-err_clk:
-	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
-		reset_control_assert(pfdev->rstc);
-	return ret;
-}
-
-static int panfrost_device_runtime_suspend(struct device *dev)
-{
-	struct panfrost_device *pfdev = dev_get_drvdata(dev);
-
-	if (!panfrost_jm_is_idle(pfdev))
-		return -EBUSY;
-
-	panfrost_devfreq_suspend(pfdev);
-	panfrost_jm_suspend_irq(pfdev);
-	panfrost_mmu_suspend_irq(pfdev);
-	panfrost_gpu_suspend_irq(pfdev);
-	panfrost_gpu_power_off(pfdev);
-
-	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
-		clk_disable(pfdev->bus_ace_clock);
-		clk_disable(pfdev->bus_clock);
-		clk_disable(pfdev->clock);
-		reset_control_assert(pfdev->rstc);
-	}
-
-	return 0;
-}
-
-static int panfrost_device_resume(struct device *dev)
-{
-	struct panfrost_device *pfdev = dev_get_drvdata(dev);
-	int ret;
-
-	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF)) {
-		unsigned long freq = pfdev->pfdevfreq.fast_rate;
-		struct dev_pm_opp *opp;
-
-		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
-		if (IS_ERR(opp))
-			return PTR_ERR(opp);
-		dev_pm_opp_set_opp(dev, opp);
-		dev_pm_opp_put(opp);
-	}
-
-	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
-		ret = clk_enable(pfdev->clock);
-		if (ret)
-			goto err_clk;
-
-		if (pfdev->bus_clock) {
-			ret = clk_enable(pfdev->bus_clock);
-			if (ret)
-				goto err_bus_clk;
-		}
-	}
-
-	ret = pm_runtime_force_resume(dev);
-	if (ret)
-		goto err_resume;
-
-	return 0;
-
-err_resume:
-	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS) && pfdev->bus_clock)
-		clk_disable(pfdev->bus_clock);
-err_bus_clk:
-	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS))
-		clk_disable(pfdev->clock);
-err_clk:
-	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
-		dev_pm_opp_set_opp(dev, NULL);
-	return ret;
-}
-
-static int panfrost_device_suspend(struct device *dev)
-{
-	struct panfrost_device *pfdev = dev_get_drvdata(dev);
-	int ret;
-
-	ret = pm_runtime_force_suspend(dev);
-	if (ret)
-		return ret;
+	/* Do the actual device reset */
+	ret = panfrost_gpu_soft_reset(pfdev);
+	panfrost_gpu_power_on(pfdev);
 
-	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
-		if (pfdev->bus_clock)
-			clk_disable(pfdev->bus_clock);
+	if (!panfrost_device_is_initialized(pfdev) && !ret)
+		atomic_set(&pfdev->gpu_powered, 1);
 
-		clk_disable(pfdev->clock);
+	/* Post-reset */
+	if (panfrost_device_is_initialized(pfdev)) {
+		panfrost_mmu_reset(pfdev);
+		panfrost_jm_reset_interrupts(pfdev);
+		if (enable_job_int)
+			panfrost_jm_enable_interrupts(pfdev);
 	}
-
-	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
-		dev_pm_opp_set_opp(dev, NULL);
-
-	return 0;
 }
-
-EXPORT_GPL_DEV_PM_OPS(panfrost_pm_ops) = {
-	RUNTIME_PM_OPS(panfrost_device_runtime_suspend, panfrost_device_runtime_resume, NULL)
-	SYSTEM_SLEEP_PM_OPS(panfrost_device_suspend, panfrost_device_resume)
-};
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index ec55c136b1b6..6bec8fcb701e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -142,6 +142,7 @@ struct panfrost_device {
 	/* pm_domains for devices with more than one. */
 	struct device *pm_domain_devs[MAX_PM_DOMAINS];
 	struct device_link *pm_domain_links[MAX_PM_DOMAINS];
+	atomic_t gpu_powered;
 	bool coherent;
 
 	struct panfrost_features features;
@@ -253,6 +254,8 @@ int panfrost_device_init(struct panfrost_device *pfdev);
 void panfrost_device_fini(struct panfrost_device *pfdev);
 void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int);
 
+void panfrost_try_suspend_device(struct panfrost_device *pfdev);
+
 extern const struct dev_pm_ops panfrost_pm_ops;
 
 enum drm_panfrost_exception_type {
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 331a3bd5b98c..5eb4b095b1f8 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -988,9 +988,7 @@ static int panfrost_probe(struct platform_device *pdev)
 		goto err_out0;
 	}
 
-	pm_runtime_set_active(pfdev->base.dev);
 	pm_runtime_mark_last_busy(pfdev->base.dev);
-	pm_runtime_enable(pfdev->base.dev);
 	pm_runtime_set_autosuspend_delay(pfdev->base.dev, 50); /* ~3 frames */
 	pm_runtime_use_autosuspend(pfdev->base.dev);
 
@@ -1002,13 +1000,13 @@ static int panfrost_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto err_out1;
 
+	pm_runtime_put_autosuspend(pfdev->base.dev);
 
 	return 0;
 
 err_out1:
-	pm_runtime_disable(pfdev->base.dev);
 	panfrost_device_fini(pfdev);
-	pm_runtime_set_suspended(pfdev->base.dev);
+	panfrost_try_suspend_device(pfdev);
 err_out0:
 	return err;
 }
@@ -1019,10 +1017,11 @@ static void panfrost_remove(struct platform_device *pdev)
 
 	drm_dev_unregister(&pfdev->base);
 
-	pm_runtime_get_sync(pfdev->base.dev);
-	pm_runtime_disable(pfdev->base.dev);
+	drm_WARN_ON(&pfdev->base, pm_runtime_get_sync(pfdev->base.dev) < 0);
+
 	panfrost_device_fini(pfdev);
-	pm_runtime_set_suspended(pfdev->base.dev);
+
+	panfrost_try_suspend_device(pfdev);
 }
 
 static ssize_t profiling_show(struct device *dev,
diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index 7d555e63e21a..1b22578f71bc 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -516,9 +516,8 @@ int panfrost_gpu_init(struct panfrost_device *pfdev)
 {
 	int err;
 
-	err = panfrost_gpu_soft_reset(pfdev);
-	if (err)
-		return err;
+	if (!atomic_read(&pfdev->gpu_powered))
+		return -ETIMEDOUT;
 
 	err = panfrost_gpu_init_features(pfdev);
 	if (err)
@@ -543,8 +542,6 @@ int panfrost_gpu_init(struct panfrost_device *pfdev)
 		return err;
 	}
 
-	panfrost_gpu_power_on(pfdev);
-
 	return 0;
 }
 

-- 
2.55.0

Re: [PATCH v3 05/13] drm/panfrost: Move all device power up and down into RPM callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Unconditional access to uninitialized subsystem states causes a NULL pointer dereference on early init failures.
- [Critical] Hardware MMIO registers are accessed after PM domains and regulators are detached, causing a system fault.
- [High] Double free/cleanup underflow of clocks and resets on early initialization failure.
- [High] Clock prepare counts are leaked and never cleaned up during module unload.
- [Medium] Typo in panfrost_clk_init error path causes double unprepare of bus_clock and leaks bus_ace_clock.
--

--- Patch [5]: [PATCH v3 05/13] drm/panfrost: Move all device power up and down into RPM callbacks ---
commit 79a6eca5bba776aeb8e6557364e0049b9f9eebb5
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Move all device power up and down into RPM callbacks

This commit moves the GPU enablement and disablement into runtime PM
resume and suspend callbacks. It is designed to prevent the device from
remaining powered on forever if probe fails and to prevent devfreq OPP
transition notifications from filling the kernel ring buffer.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -88,14 +109,21 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)

[ ... ]

> -	err = clk_prepare_enable(pfdev->bus_ace_clock);
> +	err = clk_prepare(pfdev->bus_ace_clock);
>  	if (err)
> -		goto disable_bus_clock;
> +		goto unprepare_bus_clock;
> +
> +	if (!(pfdev->comp->pm_features & BIT(GPU_PM_RT))) {
> +		err = panfrost_clk_enable(pfdev);
> +		if (err)
> +			goto unprepare_bus_ace_clock;
> +	}
>  
>  	return 0;
>  
> -disable_bus_clock:
> -	clk_disable_unprepare(pfdev->bus_clock);
> -disable_clock:
> -	clk_disable_unprepare(pfdev->clock);
> +unprepare_bus_ace_clock:
> +	clk_unprepare(pfdev->bus_clock);
> +unprepare_bus_clock:
> +	clk_unprepare(pfdev->bus_clock);

[Severity: Medium]
Is there a typo in this error path? 

If clock enablement fails, the code jumps here, but it looks like
clk_unprepare(pfdev->bus_clock) is called twice under both labels, and
bus_ace_clock is left prepared.

[ ... ]

> @@ -212,6 +251,143 @@ static int panfrost_pm_domain_init(struct panfrost_device *pfdev)
>  	return err;
>  }
>  
> +static int panfrost_device_runtime_resume(struct device *dev)

[ ... ]

> +static int panfrost_device_runtime_suspend(struct device *dev)
> +{
> +	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> +
> +	if (!panfrost_jm_is_idle(pfdev))
> +		return -EBUSY;

[Severity: Critical]
Does this cause a NULL pointer dereference on early initialization failures?

If panfrost_gpu_init() or panfrost_mmu_init() fails during probe, the
error unwinding path calls panfrost_try_suspend_device(), which
synchronously invokes this callback.

At this point, panfrost_jm_init() hasn't run yet, so pfdev->js is NULL.
The unconditional call to panfrost_jm_is_idle(pfdev) dereferences
pfdev->js without checking for NULL.

> +
> +	panfrost_devfreq_suspend(pfdev);
> +	panfrost_jm_suspend_irq(pfdev);
> +	panfrost_mmu_suspend_irq(pfdev);
> +	panfrost_gpu_suspend_irq(pfdev);
> +	panfrost_gpu_power_off(pfdev);
> +
> +	if (panfrost_device_is_initialized(pfdev) &&
> +	    pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
> +		clk_disable(pfdev->bus_ace_clock);
> +		clk_disable(pfdev->bus_clock);
> +		clk_disable(pfdev->clock);
> +		reset_control_assert(pfdev->rstc);
> +	}
> +
> +	if (!panfrost_device_is_initialized(pfdev)) {
> +		panfrost_clk_fini(pfdev);
> +		reset_control_assert(pfdev->rstc);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Can this lead to a clock prepare count leak on module unload?

During a clean module unload, panfrost_device_is_initialized() is true
since pfdev->js is still allocated. This means panfrost_clk_fini() will
be skipped here. 

Since panfrost_clk_fini() was removed from panfrost_device_fini() in this
patch, it looks like the clocks are never disabled or unprepared during
driver removal.

[ ... ]

> @@ -295,6 +486,8 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  	panfrost_mmu_fini(pfdev);
>  out_gpu:
>  	panfrost_gpu_fini(pfdev);
> +out_rpm_put:
> +	panfrost_try_suspend_device(pfdev);
>  out_regulator:
>  	panfrost_regulator_fini(pfdev);

[Severity: High]
Does this cause a double free and clock prepare underflow?

If panfrost_device_init() fails early, the out_rpm_put label calls
panfrost_try_suspend_device(pfdev). The suspend callback explicitly calls
panfrost_clk_fini(pfdev) and reset_control_assert(pfdev->rstc).

Execution then falls through the error ladder to out_clk and out_reset,
calling panfrost_clk_fini(pfdev) and panfrost_reset_fini(pfdev) again.

[ ... ]

> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -1002,13 +1000,13 @@ static int panfrost_probe(struct platform_device *pdev)
>  	if (err < 0)
>  		goto err_out1;
>  
> +	pm_runtime_put_autosuspend(pfdev->base.dev);
>  
>  	return 0;
>  
>  err_out1:
> -	pm_runtime_disable(pfdev->base.dev);
>  	panfrost_device_fini(pfdev);
> -	pm_runtime_set_suspended(pfdev->base.dev);
> +	panfrost_try_suspend_device(pfdev);
>  err_out0:
>  	return err;
>  }

[Severity: Critical]
Does this write to hardware MMIO registers after the device is powered off?

Here, panfrost_device_fini(pfdev) runs first and detaches PM domains and
regulators, physically powering off the GPU.

Then panfrost_try_suspend_device(pfdev) runs. It invokes
panfrost_device_runtime_suspend(), which calls panfrost_gpu_power_off()
and writes to MMIO registers like SHADER_PWROFF_LO.

Writing to unpowered hardware registers can trigger a synchronous external
abort or system lockup.

> @@ -1019,10 +1017,11 @@ static void panfrost_remove(struct platform_device *pdev)
>  
>  	drm_dev_unregister(&pfdev->base);
>  
> -	pm_runtime_get_sync(pfdev->base.dev);
> -	pm_runtime_disable(pfdev->base.dev);
> +	drm_WARN_ON(&pfdev->base, pm_runtime_get_sync(pfdev->base.dev) < 0);
> +
>  	panfrost_device_fini(pfdev);
> -	pm_runtime_set_suspended(pfdev->base.dev);
> +
> +	panfrost_try_suspend_device(pfdev);
>  }

[Severity: Critical]
This appears to have the same teardown ordering issue mentioned above.

panfrost_device_fini() powers down the PM domains before
panfrost_try_suspend_device() writes to the unpowered MMIO registers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=5

[PATCH v3 06/13] drm/panfrost: Explicitly enable MMU interrupts at device init

[Adrián Larumbe]

Because the device must be in a position to accept jobs between the time
drm_dev_register() is called and autosuspend first kicks in, there's a very
narrow window inbetween during which jobs targeting the tiler buffer
object would time out, since the device's PM status is 'Active', but no MMU
interrupts were enabled at device initialisation time.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Fixes: 73e467f60acd ("drm/panfrost: Consolidate reset handling")
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 4a3162c3b659..bb6c1614964b 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -336,6 +336,12 @@ void panfrost_mmu_as_put(struct panfrost_device *pfdev, struct panfrost_mmu *mmu
 	WARN_ON(atomic_read(&mmu->as_count) < 0);
 }
 
+static void panfrost_mmu_enable_interrupts(struct panfrost_device *pfdev)
+{
+	mmu_write(pfdev, MMU_INT_CLEAR, ~0);
+	mmu_write(pfdev, MMU_INT_MASK, ~0);
+}
+
 void panfrost_mmu_reset(struct panfrost_device *pfdev)
 {
 	struct panfrost_mmu *mmu, *mmu_tmp;
@@ -355,8 +361,7 @@ void panfrost_mmu_reset(struct panfrost_device *pfdev)
 
 	spin_unlock(&pfdev->as_lock);
 
-	mmu_write(pfdev, MMU_INT_CLEAR, ~0);
-	mmu_write(pfdev, MMU_INT_MASK, ~0);
+	panfrost_mmu_enable_interrupts(pfdev);
 }
 
 static size_t get_pgsize(u64 addr, size_t size, size_t *count)
@@ -970,6 +975,8 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
 		return err;
 	}
 
+	panfrost_mmu_enable_interrupts(pfdev);
+
 	return 0;
 }
 

-- 
2.55.0

Re: [PATCH v3 06/13] drm/panfrost: Explicitly enable MMU interrupts at device init

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Premature unmasking of MMU interrupts before reset workqueue initialization leads to a NULL pointer dereference.
--

--- Patch [6]: [PATCH v3 06/13] drm/panfrost: Explicitly enable MMU interrupts at device init ---
commit 28382212258245805fe4be63994981403a3d9c1c
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Explicitly enable MMU interrupts at device init

Because the device must be in a position to accept jobs between the time
drm_dev_register() is called and autosuspend first kicks in, there's a very
narrow window inbetween during which jobs targeting the tiler buffer
object would time out, since the device's PM status is 'Active', but no MMU
interrupts were enabled at device initialisation time.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 4a3162c3b659f..bb6c1614964b1 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c

[ ... ]

> @@ -970,6 +975,8 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>  		return err;
>  	}
>  
> +	panfrost_mmu_enable_interrupts(pfdev);
> +

[Severity: High]
Could this premature unmasking of MMU interrupts lead to a NULL pointer
dereference during driver initialization?

If the GPU asserts an MMU interrupt (e.g., due to leftover state from the
bootloader or a hardware glitch) right after interrupts are enabled here, the
IRQ handler thread might encounter a fault and eventually attempt a reset.

However, the reset workqueue hasn't been allocated yet, because
panfrost_mmu_init() is called before panfrost_jm_init():

drivers/gpu/drm/panfrost/panfrost_device.c:panfrost_device_init() {
    ...
    err = panfrost_mmu_init(pfdev);
    if (err)
        goto out_gpu;

    err = panfrost_jm_init(pfdev);
    ...
}

If panfrost_mmu_irq_handler_thread() triggers a timeout in wait_ready(), it
will call panfrost_device_schedule_reset(), which assumes the reset
workqueue is already initialized:

drivers/gpu/drm/panfrost/panfrost_device.h:panfrost_device_schedule_reset() {
    ...
    queue_work(pfdev->reset.wq, &pfdev->reset.work);
}

Will this dereference the NULL pfdev->reset.wq pointer if the interrupt
fires before panfrost_jm_init() runs?

>  	return 0;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=6

[PATCH v3 07/13] drm/panfrost: Sync with IRQ before MMU disable and reset

[Adrián Larumbe]

There was a pitfall of the MMU threaded IRQ handler accessing HW
registers after a spurious interrupt at the same time the device is
being premature taking down because of a failed probe sequence.

Let's make sure MMU IRQ's are marked suspended and synchronised with
before unwinding further.

Also, during a device reset, make sure we also sync with the MMU IRQ
handler so that AS aren't clobbered while a map operation is ongoing.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index bb6c1614964b..f067bcd360a8 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -347,6 +347,7 @@ void panfrost_mmu_reset(struct panfrost_device *pfdev)
 	struct panfrost_mmu *mmu, *mmu_tmp;
 
 	clear_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended);
+	synchronize_irq(pfdev->mmu_irq);
 
 	spin_lock(&pfdev->as_lock);
 
@@ -980,11 +981,6 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
 	return 0;
 }
 
-void panfrost_mmu_fini(struct panfrost_device *pfdev)
-{
-	mmu_write(pfdev, MMU_INT_MASK, 0);
-}
-
 void panfrost_mmu_suspend_irq(struct panfrost_device *pfdev)
 {
 	set_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended);
@@ -992,3 +988,8 @@ void panfrost_mmu_suspend_irq(struct panfrost_device *pfdev)
 	mmu_write(pfdev, MMU_INT_MASK, 0);
 	synchronize_irq(pfdev->mmu_irq);
 }
+
+void panfrost_mmu_fini(struct panfrost_device *pfdev)
+{
+	panfrost_mmu_suspend_irq(pfdev);
+}

-- 
2.55.0

Re: [PATCH v3 07/13] drm/panfrost: Sync with IRQ before MMU disable and reset

[sashiko-bot]

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

Pre-existing issues:
- [High] panfrost_device_runtime_suspend unconditionally dereferences pfdev->js, causing a kernel panic on early probe failure.
- [High] Missing IRQ synchronization in panfrost_jm_fini and panfrost_gpu_fini allows concurrent IRQ handlers to access freed memory or disabled hardware.
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition between the threaded IRQ handler and the IRQ suspend function can leave MMU hardware interrupts enabled, leading to an unhandled interrupt storm.
--

--- Patch [7]: [PATCH v3 07/13] drm/panfrost: Sync with IRQ before MMU disable and reset ---
commit 50e811614859715a2c6f2e0b562e19db3f2c571c
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Sync with IRQ before MMU disable and reset

This commit synchronizes the MMU IRQ handler before disabling and resetting
the MMU, preventing HW register accesses from spurious interrupts during
device teardown or reset. It introduces panfrost_mmu_suspend_irq() to
handle the IRQ suspension sequence.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index bb6c1614964b1..f067bcd360a89 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -347,6 +347,7 @@ void panfrost_mmu_reset(struct panfrost_device *pfdev)
>  	struct panfrost_mmu *mmu, *mmu_tmp;
>  
>  	clear_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended);
> +	synchronize_irq(pfdev->mmu_irq);
>  
>  	spin_lock(&pfdev->as_lock);

[ ... ]

> @@ -980,11 +981,6 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>  	return 0;
>  }
>  
> -void panfrost_mmu_fini(struct panfrost_device *pfdev)
> -{
> -	mmu_write(pfdev, MMU_INT_MASK, 0);
> -}
> -
>  void panfrost_mmu_suspend_irq(struct panfrost_device *pfdev)
>  {
>  	set_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended);
> @@ -992,3 +988,8 @@ void panfrost_mmu_suspend_irq(struct panfrost_device *pfdev)
>  	mmu_write(pfdev, MMU_INT_MASK, 0);
>  	synchronize_irq(pfdev->mmu_irq);
>  }

[Severity: High]
This is a pre-existing issue, but is there a Time-of-Check to Time-of-Use
(TOCTOU) race condition between this suspend function and the threaded IRQ
handler?

If the threaded handler panfrost_mmu_irq_handler_thread() checks
PANFROST_COMP_BIT_MMU without holding locks:

	if (!test_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended)) {
		spin_lock(&pfdev->as_lock);
		mmu_write(pfdev, MMU_INT_MASK, ~pfdev->as_faulty_mask);
		spin_unlock(&pfdev->as_lock);
	}

Concurrently, panfrost_mmu_suspend_irq() sets the bit, writes 0 to
MMU_INT_MASK, and calls synchronize_irq().

If the threaded handler performs the test_bit() check just before the bit
is set, it evaluates to true. Then suspend_irq() masks interrupts and
waits, and finally the threaded handler overwrites the 0 with
~pfdev->as_faulty_mask, re-enabling interrupts.

Could this lead to an unhandled interrupt storm if the device asserts the
interrupt and panfrost_mmu_irq_handler() returns IRQ_NONE?

> +
> +void panfrost_mmu_fini(struct panfrost_device *pfdev)
> +{
> +	panfrost_mmu_suspend_irq(pfdev);
> +}


[Severity: High]
This isn't a bug introduced by this patch, but does early probe failure
cause a kernel panic due to a NULL pointer dereference?

If the device probe fails early (e.g., in panfrost_gpu_init() or
panfrost_mmu_init()), panfrost_jm_init() is never reached, leaving
pfdev->js as NULL. The error unwinding path calls
panfrost_try_suspend_device(), which synchronously triggers
panfrost_device_runtime_suspend().

This suspend handler unconditionally calls panfrost_jm_is_idle(pfdev):

drivers/gpu/drm/panfrost/panfrost_device.c:panfrost_device_runtime_suspend() {
    ...
    if (!panfrost_jm_is_idle(pfdev))
        return -EBUSY;
    ...
}

Which then dereferences pfdev->js->queue:

drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_jm_is_idle() {
    struct panfrost_job_slot *js = pfdev->js;
    ...
    if (atomic_read(&js->queue[i].sched.credit_count))
    ...
}

Can we safely call panfrost_jm_is_idle() before verifying if the job
manager is initialized?


[Severity: High]
This is a pre-existing issue, but are there missing IRQ synchronizations in
panfrost_jm_fini() and panfrost_gpu_fini() that could allow concurrent IRQ
handlers to access freed memory or disabled hardware?

In panfrost_jm_fini(), the driver masks job interrupts but fails to set the
suspend bit (PANFROST_COMP_BIT_JOB) or wait for executing threaded handlers:

drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_jm_fini() {
    ...
    job_write(pfdev, JOB_INT_MASK, 0);

    for (j = 0; j < NUM_JOB_SLOTS; j++) {
        drm_sched_fini(&js->queue[j].sched);
    }
    ...
}

Could a concurrently running IRQ thread access the destroyed scheduler
queues or re-enable the interrupts?

Similarly, in panfrost_gpu_fini(), the GPU is powered off without
synchronizing or suspending the shared GPU IRQ:

drivers/gpu/drm/panfrost/panfrost_gpu.c:panfrost_gpu_fini() {
    panfrost_gpu_power_off(pfdev);
}

If a shared interrupt fires, panfrost_gpu_irq_handler() will read
GPU_FAULT_STATUS from powered-off hardware. Could this cause a bus fault?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=7

[PATCH v3 08/13] drm/panfrost: Rewire reset sequence to avoid concurrent attempts

[Adrián Larumbe]

This way, and in imitation of Panthor, only a single reset thread can
progress at a time, and other threads wanting to trigger it just wait
on the ongoing one to finish and wake them up.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c |  1 +
 drivers/gpu/drm/panfrost/panfrost_device.h |  5 +++--
 drivers/gpu/drm/panfrost/panfrost_job.c    | 16 ++++++++++------
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index d68f1ec82895..cf115db9cebf 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -8,6 +8,7 @@
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
+#include <linux/wait.h>
 
 #include "panfrost_device.h"
 #include "panfrost_devfreq.h"
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 6bec8fcb701e..23dea923bd04 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -167,6 +167,7 @@ struct panfrost_device {
 	struct {
 		struct workqueue_struct *wq;
 		struct work_struct work;
+		wait_queue_head_t wait;
 		atomic_t pending;
 	} reset;
 
@@ -341,8 +342,8 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
 static inline void
 panfrost_device_schedule_reset(struct panfrost_device *pfdev)
 {
-	atomic_set(&pfdev->reset.pending, 1);
-	queue_work(pfdev->reset.wq, &pfdev->reset.work);
+	if (!atomic_cmpxchg(&pfdev->reset.pending, 0, 1))
+		queue_work(pfdev->reset.wq, &pfdev->reset.work);
 }
 
 #endif
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 2d12b83e900a..c2a1670a74e8 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -497,10 +497,8 @@ static void panfrost_job_handle_err(struct panfrost_device *pfdev,
 
 	pm_runtime_put_autosuspend(pfdev->base.dev);
 
-	if (panfrost_exception_needs_reset(pfdev, js_status)) {
-		atomic_set(&pfdev->reset.pending, 1);
+	if (panfrost_exception_needs_reset(pfdev, js_status))
 		drm_sched_fault(&pfdev->js->queue[js].sched);
-	}
 }
 
 static void panfrost_jm_handle_done(struct panfrost_device *pfdev,
@@ -755,6 +753,8 @@ panfrost_reset(struct panfrost_device *pfdev,
 	panfrost_jm_enable_interrupts(pfdev);
 
 	dma_fence_end_signalling(cookie);
+
+	wake_up_all(&pfdev->reset.wait);
 }
 
 static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
@@ -763,6 +763,7 @@ static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
 	struct panfrost_job *job = to_panfrost_job(sched_job);
 	struct panfrost_device *pfdev = job->pfdev;
 	int js = panfrost_job_get_slot(job);
+	int ret;
 
 	/*
 	 * If the GPU managed to complete this jobs fence, the timeout has
@@ -797,10 +798,12 @@ static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
 
 	panfrost_core_dump(job);
 
-	atomic_set(&pfdev->reset.pending, 1);
-	panfrost_reset(pfdev, sched_job);
+	panfrost_device_schedule_reset(pfdev);
+	ret = wait_event_timeout(pfdev->reset.wait,
+				 !atomic_read(&pfdev->reset.pending),
+				 msecs_to_jiffies(60));
 
-	return DRM_GPU_SCHED_STAT_RESET;
+	return (ret) ? DRM_GPU_SCHED_STAT_RESET : DRM_GPU_SCHED_STAT_ENODEV;
 }
 
 static void panfrost_reset_work(struct work_struct *work)
@@ -892,6 +895,7 @@ int panfrost_jm_init(struct panfrost_device *pfdev)
 	if (!pfdev->reset.wq)
 		return -ENOMEM;
 	args.timeout_wq = pfdev->reset.wq;
+	init_waitqueue_head(&pfdev->reset.wait);
 
 	for (j = 0; j < NUM_JOB_SLOTS; j++) {
 		js->queue[j].fence_context = dma_fence_context_alloc(1);

-- 
2.55.0

Re: [PATCH v3 08/13] drm/panfrost: Rewire reset sequence to avoid concurrent attempts

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Workqueue deadlock in `panfrost_job_timedout()` due to waiting on an ordered workqueue from a task executing on the same workqueue.
- [High] The timed-out job is permanently leaked and bypasses karma accounting because the asynchronous reset work executes with a NULL 'bad' job reference.
- [Medium] Unrealistically short wait timeout (60 milliseconds) for the GPU reset sequence.
- [Medium] Premature clearing of the `reset.pending` flag creates a data race with the TDR worker.
--

--- Patch [8]: [PATCH v3 08/13] drm/panfrost: Rewire reset sequence to avoid concurrent attempts ---
commit 2c4929474201c3a58918917376876f5a96009f31
Author: Adrián Larumbe <adrian.larumbe@collabora.com>
Subject: drm/panfrost: Rewire reset sequence to avoid concurrent attempts

This commit introduces a dedicated reset worker to serialize GPU resets.
It uses an ordered workqueue and wait_event_timeout to block concurrent
reset attempts while the ongoing reset thread completes.

Link: None provided

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -797,10 +798,12 @@ static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
>  
>  	panfrost_core_dump(job);
>  
> -	atomic_set(&pfdev->reset.pending, 1);
> -	panfrost_reset(pfdev, sched_job);
> +	panfrost_device_schedule_reset(pfdev);

[Severity: High]
By removing the synchronous panfrost_reset() call and relying on
panfrost_reset_work(), we seem to lose the reference to the timed-out job.

Looking at the new worker function:

static void panfrost_reset_work(struct work_struct *work)
{
	struct panfrost_device *pfdev;

	pfdev = container_of(work, struct panfrost_device, reset.work);
	panfrost_reset(pfdev, NULL);
}

Since the guilty job is passed as NULL, it bypasses karma accounting and
drm_sched_stop() will fail to reinsert it into the pending list. Will this
cause the bad job to permanently leak and hang userspace?

> +	ret = wait_event_timeout(pfdev->reset.wait,
> +				 !atomic_read(&pfdev->reset.pending),
> +				 msecs_to_jiffies(60));

[Severity: High]
Could this wait trigger a workqueue deadlock?

In panfrost_jm_init(), args.timeout_wq is assigned to pfdev->reset.wq.
This means the timeout handler runs on the ordered reset workqueue.

By queuing pfdev->reset.work onto this exact same workqueue and synchronously
waiting for it to complete here, it seems the reset work can never execute
because the single-threaded queue is blocked waiting for it.

[Severity: Medium]
Is this timeout duration sufficient?

A 60 millisecond wait seems extremely short for a full GPU reset, which
involves soft-stopping jobs and polling the hardware. If it expires
prematurely, it will return ENODEV and abandon recovery. Could this have
been intended to be 60 seconds (60000 msecs)?

[Severity: Medium]
Is it safe to rely on the pending flag to synchronize scheduler restart?

In panfrost_reset(), atomic_set(&pfdev->reset.pending, 0) is called
immediately after the hardware reset, but before drm_sched_resubmit_jobs()
and drm_sched_start() have finished.

If this wait_event_timeout() wakes up early or spuriously, it will unblock
the DRM scheduler while the reset worker is still actively modifying
scheduler lists. Should the flag be cleared only after the full reset
sequence completes?

> -	return DRM_GPU_SCHED_STAT_RESET;
> +	return (ret) ? DRM_GPU_SCHED_STAT_RESET : DRM_GPU_SCHED_STAT_ENODEV;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=8

[PATCH v3 09/13] drm/panfrost: Add debugfs knob for manually triggering a GPU reset

[Adrián Larumbe]

This will be of great help when testing potential races between the GPU
reset sequence and other parts of the code accessing HW registers.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 38 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_device.h |  3 +++
 drivers/gpu/drm/panfrost/panfrost_drv.c    |  1 +
 3 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index cf115db9cebf..80746e375410 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -2,6 +2,7 @@
 /* Copyright 2018 Marty E. Plummer <hanetzer@startmail.com> */
 /* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
 
+#include <linux/debugfs.h>
 #include <linux/clk.h>
 #include <linux/reset.h>
 #include <linux/platform_device.h>
@@ -631,3 +632,40 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 			panfrost_jm_enable_interrupts(pfdev);
 	}
 }
+
+#ifdef CONFIG_DEBUG_FS
+static int reset_get(void *data, u64 *val)
+{
+	struct panfrost_device *pfdev =
+		container_of(data, struct panfrost_device, base);
+
+	*val = atomic_read(&pfdev->reset.pending);
+	return 0;
+}
+
+static int reset_set(void *data, u64 val)
+{
+	struct panfrost_device *pfdev =
+		container_of(data, struct panfrost_device, base);
+
+	if (pm_runtime_get_if_in_use(pfdev->base.dev)) {
+		panfrost_device_schedule_reset(pfdev);
+		wait_event_interruptible_timeout(pfdev->reset.wait,
+						 !atomic_read(&pfdev->reset.pending),
+						 msecs_to_jiffies(60));
+		pm_runtime_put(pfdev->base.dev);
+	}
+
+	return 0;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(panfrost_reset_debugfs_fops,
+			 reset_get, reset_set,
+			 "0x%08llx\n");
+
+void panfrost_reset_debugfs_init(struct drm_minor *minor)
+{
+	debugfs_create_file("reset", 0600, minor->debugfs_root,
+			    minor->dev, &panfrost_reset_debugfs_fops);
+}
+#endif // CONFIG_DEBUG_FS
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 23dea923bd04..855ee4d0fb40 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -346,4 +346,7 @@ panfrost_device_schedule_reset(struct panfrost_device *pfdev)
 		queue_work(pfdev->reset.wq, &pfdev->reset.work);
 }
 
+#ifdef CONFIG_DEBUG_FS
+void panfrost_reset_debugfs_init(struct drm_minor *minor);
+#endif // CONFIG_DEBUG_FS
 #endif
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 5eb4b095b1f8..2490e9785057 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -916,6 +916,7 @@ static void panfrost_debugfs_init(struct drm_minor *minor)
 {
 	panthor_gems_debugfs_init(minor);
 	panfrost_sched_debugfs_init(minor);
+	panfrost_reset_debugfs_init(minor);
 }
 #endif
 

-- 
2.55.0

Re: [PATCH v3 09/13] drm/panfrost: Add debugfs knob for manually triggering a GPU reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The return value of `pm_runtime_get_if_in_use()` is evaluated as a boolean, which incorrectly treats negative error codes (e.g., `-EINVAL`) as success, leading to a PM usage count underflow and unsafe hardware access.
- [High] Dropping the runtime PM reference prematurely when `wait_event_interruptible_timeout()` times out or is interrupted allows the device to suspend while the asynchronous hardware reset is still executing.
- [Medium] The return value of `wait_event_interruptible_timeout()` is ignored, masking failures from userspace.
--

--- Patch [9]: [PATCH v3 09/13] drm/panfrost: Add debugfs knob for manually triggering a GPU reset ---
commit 30e9d18cee2bcd687cf4536c70ee21485ca931d0
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Add debugfs knob for manually triggering a GPU reset

This commit introduces a new debugfs file "reset" to the panfrost driver.
It allows userspace to manually trigger a GPU reset by writing to it, and
to read whether a reset is currently pending.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index cf115db9cebfd..80746e3754100 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
[ ... ]
> @@ -631,3 +632,40 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
[ ... ]
> +static int reset_set(void *data, u64 val)
> +{
> +	struct panfrost_device *pfdev =
> +		container_of(data, struct panfrost_device, base);
> +
> +	if (pm_runtime_get_if_in_use(pfdev->base.dev)) {

[Severity: High]
Does this incorrectly treat negative error codes as success?

If runtime PM is disabled (e.g., CONFIG_PM is disabled or during unbind),
pm_runtime_get_if_in_use() returns -EINVAL. This condition will evaluate to
true, causing the code to schedule a reset and call pm_runtime_put() without
an actual reference being taken. 

This can lead to a PM usage counter underflow and potential hardware access
on an unpowered device. Should this explicitly check for a return value > 0?

> +		panfrost_device_schedule_reset(pfdev);
> +		wait_event_interruptible_timeout(pfdev->reset.wait,
> +						 !atomic_read(&pfdev->reset.pending),
> +						 msecs_to_jiffies(60));

[Severity: Medium]
Is it intentional to ignore the return value of the wait?

If the wait is interrupted by a signal (-ERESTARTSYS) or times out (0), the
function unconditionally returns 0, falsely indicating success to userspace.

[Severity: High]
Can dropping the PM reference on timeout or signal interruption cause the
device to suspend while the reset is still executing?

If the wait is interrupted or times out, the code proceeds to pm_runtime_put()
even though the reset work might still be pending or running. 

Because panfrost_reset() drops PM references for active jobs before the
hardware reset, dropping this reference can drop the usage counter to zero.
If the device suspends while the async reset work is concurrently accessing
MMIO registers, could this trigger a synchronous external abort?

> +		pm_runtime_put(pfdev->base.dev);
> +	}
> +
> +	return 0;
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=9

[PATCH v3 10/13] drm/panfrost: Move perfcnt GPU disable sequence into a helper

[Adrián Larumbe]

Just for the sake of avoiding repetition.

Reviewed-by: Steven Price <steven.price@arm.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 31 ++++++++++++-----------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
index 7020c0192e18..ad1156678e91 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
@@ -35,6 +35,16 @@ struct panfrost_perfcnt {
 	struct completion dump_comp;
 };
 
+static void panfrost_perfcnt_gpu_disable(struct panfrost_device *pfdev)
+{
+	gpu_write(pfdev, GPU_PERFCNT_CFG,
+		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
+	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0x0);
+	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0x0);
+	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0x0);
+	gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
+}
+
 void panfrost_perfcnt_clean_cache_done(struct panfrost_device *pfdev)
 {
 	complete(&pfdev->perfcnt->dump_comp);
@@ -193,12 +203,7 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
 	if (user != perfcnt->user)
 		return -EINVAL;
 
-	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0x0);
-	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0x0);
-	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0x0);
-	gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
-	gpu_write(pfdev, GPU_PERFCNT_CFG,
-		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
+	panfrost_perfcnt_gpu_disable(pfdev);
 
 	perfcnt->user = NULL;
 	drm_gem_vunmap(&perfcnt->mapping->obj->base.base, &map);
@@ -327,12 +332,7 @@ int panfrost_perfcnt_init(struct panfrost_device *pfdev)
 	perfcnt->bosize = size;
 
 	/* Start with everything disabled. */
-	gpu_write(pfdev, GPU_PERFCNT_CFG,
-		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
-	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0);
-	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0);
-	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0);
-	gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
+	panfrost_perfcnt_gpu_disable(pfdev);
 
 	init_completion(&perfcnt->dump_comp);
 	mutex_init(&perfcnt->lock);
@@ -344,10 +344,5 @@ int panfrost_perfcnt_init(struct panfrost_device *pfdev)
 void panfrost_perfcnt_fini(struct panfrost_device *pfdev)
 {
 	/* Disable everything before leaving. */
-	gpu_write(pfdev, GPU_PERFCNT_CFG,
-		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
-	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0);
-	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0);
-	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0);
-	gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
+	panfrost_perfcnt_gpu_disable(pfdev);
 }

-- 
2.55.0

[PATCH v3 11/13] drm/panfrost: Introduce a reset lock

[Adrián Larumbe]

So as to avoid accessing HW register while a reset is ongoing, a read/write
semaphore that envelopes the reset sequence will help driver entry points
avoid racing with it. For now, the only such racy entry point is the ioctl
that returns the current GPU timestmap.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 5 +++++
 drivers/gpu/drm/panfrost/panfrost_device.h | 1 +
 drivers/gpu/drm/panfrost/panfrost_drv.c    | 9 ++++++---
 drivers/gpu/drm/panfrost/panfrost_job.c    | 2 ++
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 80746e375410..46c6e911e562 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -617,6 +617,10 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 {
 	int ret;
 
+	/* Pre-reset */
+	if (panfrost_device_is_initialized(pfdev))
+		down_write(&pfdev->reset.lock);
+
 	/* Do the actual device reset */
 	ret = panfrost_gpu_soft_reset(pfdev);
 	panfrost_gpu_power_on(pfdev);
@@ -630,6 +634,7 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 		panfrost_jm_reset_interrupts(pfdev);
 		if (enable_job_int)
 			panfrost_jm_enable_interrupts(pfdev);
+		up_write(&pfdev->reset.lock);
 	}
 }
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 855ee4d0fb40..7f5202ff6469 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -167,6 +167,7 @@ struct panfrost_device {
 	struct {
 		struct workqueue_struct *wq;
 		struct work_struct work;
+		struct rw_semaphore lock;
 		wait_queue_head_t wait;
 		atomic_t pending;
 	} reset;
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 2490e9785057..e04eb416ea0f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -41,9 +41,12 @@ static int panfrost_ioctl_query_timestamp(struct panfrost_device *pfdev,
 	if (ret)
 		return ret;
 
-	panfrost_cycle_counter_get(pfdev);
-	*arg = panfrost_timestamp_read(pfdev);
-	panfrost_cycle_counter_put(pfdev);
+	/* We should not read timestamp register while the GPU  is being reset */
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		panfrost_cycle_counter_get(pfdev);
+		*arg = panfrost_timestamp_read(pfdev);
+		panfrost_cycle_counter_put(pfdev);
+	}
 
 	pm_runtime_put(pfdev->base.dev);
 	return 0;
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index c2a1670a74e8..c1d0b111dde1 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -895,7 +895,9 @@ int panfrost_jm_init(struct panfrost_device *pfdev)
 	if (!pfdev->reset.wq)
 		return -ENOMEM;
 	args.timeout_wq = pfdev->reset.wq;
+
 	init_waitqueue_head(&pfdev->reset.wait);
+	init_rwsem(&pfdev->reset.lock);
 
 	for (j = 0; j < NUM_JOB_SLOTS; j++) {
 		js->queue[j].fence_context = dma_fence_context_alloc(1);

-- 
2.55.0

Re: [PATCH v3 11/13] drm/panfrost: Introduce a reset lock

[sashiko-bot]

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

New issues:
- [High] Clocks and reset controllers are leaked on driver unbind because `pfdev->js` is never set to NULL during teardown.

Pre-existing issues:
- [High] The perfcnt ioctls (`panfrost_ioctl_perfcnt_enable` and `panfrost_ioctl_perfcnt_dump`) access hardware registers directly without holding the newly introduced `pfdev->reset.lock`, which can race with a concurrent GPU reset.
--

--- Patch [11]: [PATCH v3 11/13] drm/panfrost: Introduce a reset lock ---
commit 17d0aa882609f1a77cf16c55ac27f808cf38cb0a
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Introduce a reset lock

This commit introduces a read/write semaphore to envelop the GPU reset
sequence. This lock is used to prevent driver entry points from
accessing hardware registers while a reset is ongoing.

>     So as to avoid accessing HW register while a reset is ongoing, a read/write
>     semaphore that envelopes the reset sequence will help driver entry points
>     avoid racing with it. For now, the only such racy entry point is the ioctl
>     that returns the current GPU timestmap.

[Severity: High]
This is a pre-existing issue, but are the perfcnt ioctls
(panfrost_ioctl_perfcnt_enable and panfrost_ioctl_perfcnt_dump in
drivers/gpu/drm/panfrost/panfrost_perfcnt.c) also racy?

Both ioctls call functions that write directly to GPU_CMD and other HW
registers. If a userspace thread calls DRM_IOCTL_PANFROST_PERFCNT_DUMP
while the GPU is undergoing a soft reset (e.g., triggered by a job
timeout handled asynchronously by panfrost_reset_work), could this
corrupt the reset state machine and lead to undefined hardware behavior?

Should these ioctls also be wrapped with pfdev->reset.lock?

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index c2a1670a74e86..c1d0b111dde1a 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -895,7 +895,9 @@ int panfrost_jm_init(struct panfrost_device *pfdev)
>  	if (!pfdev->reset.wq)
>  		return -ENOMEM;
>  	args.timeout_wq = pfdev->reset.wq;
> +
>  	init_waitqueue_head(&pfdev->reset.wait);
> +	init_rwsem(&pfdev->reset.lock);
>  
>  	for (j = 0; j < NUM_JOB_SLOTS; j++) {
>  		js->queue[j].fence_context = dma_fence_context_alloc(1);

[Severity: High]
Does the job manager teardown leak clocks and reset controllers?

This regression was introduced earlier in this patch series by commit
"drm/panfrost: Move all device power up and down into RPM callbacks".

When unbinding the device, panfrost_remove() calls panfrost_device_fini().
panfrost_jm_fini() in drivers/gpu/drm/panfrost/panfrost_job.c destroys
the job manager but does not set pfdev->js to NULL.

Later, panfrost_try_suspend_device() invokes panfrost_device_runtime_suspend()
in drivers/gpu/drm/panfrost/panfrost_device.c. Because pfdev->js is still
non-NULL, panfrost_device_is_initialized() incorrectly evaluates to true.
This causes the cleanup block in panfrost_device_runtime_suspend() to be
skipped, and panfrost_clk_fini() and reset_control_assert() are never executed.

Should panfrost_jm_fini() set pfdev->js to NULL after cleaning up the
job slots?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=11

[PATCH v3 12/13] drm/panfrost: Fix races between perfcnt and reset sequence

[Adrián Larumbe]

Formerly, the reset sequence would race with panfrost_mmu_as_put()
when tearing down a perfcnt session. On top of that, poking GPU
registers to program a perfcnt session or obtaining a dump might lead to
undefined behaviour when done at the same time a reset was ongoing.

Use the reset r/w semaphore to govern access to the hardware at reset
time. On top of that, expand the DRM uAPI for the perfcnt DUMP operation
so that userspace can be made aware of a reset having happened, because
that means counters will go back to 0 and can no longer be accumulated
to values previously kept in user space.

The new perfcnt-aware reset sequence also takes care to reestablish
perfcnt to its original configuration if there was an enabled session.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c  |   5 +-
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 196 ++++++++++++++++++++--------
 drivers/gpu/drm/panfrost/panfrost_perfcnt.h |   2 +
 include/uapi/drm/panfrost_drm.h             |   3 +-
 4 files changed, 146 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 46c6e911e562..2ee8d9584c36 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -618,8 +618,10 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 	int ret;
 
 	/* Pre-reset */
-	if (panfrost_device_is_initialized(pfdev))
+	if (panfrost_device_is_initialized(pfdev)) {
 		down_write(&pfdev->reset.lock);
+		panfrost_perfcnt_reset(pfdev);
+	}
 
 	/* Do the actual device reset */
 	ret = panfrost_gpu_soft_reset(pfdev);
@@ -634,6 +636,7 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 		panfrost_jm_reset_interrupts(pfdev);
 		if (enable_job_int)
 			panfrost_jm_enable_interrupts(pfdev);
+		panfrost_perfcnt_postreset(pfdev);
 		up_write(&pfdev->reset.lock);
 	}
 }
diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
index ad1156678e91..a6f1b0b961fc 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
@@ -11,6 +11,7 @@
 #include <drm/drm_file.h>
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/panfrost_drm.h>
+#include <drm/drm_print.h>
 
 #include "panfrost_device.h"
 #include "panfrost_features.h"
@@ -25,14 +26,17 @@
 #define BYTES_PER_COUNTER		4
 #define BLOCKS_PER_COREGROUP		8
 #define V4_SHADERS_PER_COREGROUP	4
+#define PERFCNT_DUMP_MAX_RETRIES	5
 
 struct panfrost_perfcnt {
 	struct panfrost_gem_mapping *mapping;
+	unsigned int counterset;
 	size_t bosize;
 	void *buf;
 	struct panfrost_file_priv *user;
 	struct mutex lock;
 	struct completion dump_comp;
+	atomic_t reset_happened;
 };
 
 static void panfrost_perfcnt_gpu_disable(struct panfrost_device *pfdev)
@@ -55,38 +59,98 @@ void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev)
 	gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_CACHES);
 }
 
-static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
+static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev,
+					u64 *reset_happened)
 {
+	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
+	u64 retries = PERFCNT_DUMP_MAX_RETRIES;
+	bool previous_reset;
 	u64 gpuva;
 	int ret;
 
 	reinit_completion(&pfdev->perfcnt->dump_comp);
 	gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT;
-	gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva));
-	gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva));
-	gpu_write(pfdev, GPU_INT_CLEAR,
-		  GPU_IRQ_CLEAN_CACHES_COMPLETED |
-		  GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
-	gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE);
+	previous_reset = atomic_read(&perfcnt->reset_happened);
+
+dump_retry:
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva));
+		gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva));
+		gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_CLEAN_CACHES_COMPLETED |
+						GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
+		gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE);
+	}
+
 	ret = wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp,
 							msecs_to_jiffies(1000));
-	if (!ret)
-		ret = -ETIMEDOUT;
-	else if (ret > 0)
-		ret = 0;
+	if (retries == PERFCNT_DUMP_MAX_RETRIES)
+		*reset_happened = previous_reset || atomic_read(&perfcnt->reset_happened);
+
+	if (atomic_cmpxchg(&perfcnt->reset_happened, 1, 0)) {
+		if (--retries >= 0)
+			goto dump_retry;
+		ret = -EBUSY;
+	} else {
+		if (!ret)
+			ret = -ETIMEDOUT;
+		else if (ret > 0)
+			ret = 0;
+	}
 
 	return ret;
 }
 
+static int panfrost_perfcnt_hw_enable(struct panfrost_device *pfdev)
+{
+	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
+	u32 cfg, as;
+	int ret;
+
+	/* TODO: could keeping the as somehow race with perfcnt_disable? */
+	ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
+	if (ret < 0)
+		return ret;
+
+	as = ret;
+	cfg = GPU_PERFCNT_CFG_AS(as) |
+	      GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_MANUAL);
+
+	/*
+	 * Bifrost GPUs have 2 set of counters, but we're only interested by
+	 * the first one for now.
+	 */
+	if (panfrost_model_is_bifrost(pfdev))
+		cfg |= GPU_PERFCNT_CFG_SETSEL(perfcnt->counterset);
+
+	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0xffffffff);
+	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0xffffffff);
+	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0xffffffff);
+
+	/*
+	 * Due to PRLAM-8186 we need to disable the Tiler before we enable HW
+	 * counters.
+	 */
+	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
+		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
+	else
+		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
+
+	gpu_write(pfdev, GPU_PERFCNT_CFG, cfg);
+
+	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
+		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
+
+	return 0;
+}
+
 static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 					  struct drm_file *file_priv,
 					  unsigned int counterset)
 {
 	struct panfrost_file_priv *user = file_priv->driver_priv;
 	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
-	struct iosys_map map;
 	struct drm_gem_shmem_object *bo;
-	u32 cfg, as;
+	struct iosys_map map;
 	int ret;
 
 	if (user == perfcnt->user)
@@ -127,57 +191,37 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 	 * Invalidate the cache and clear the counters to start from a fresh
 	 * state.
 	 */
-	reinit_completion(&pfdev->perfcnt->dump_comp);
-	gpu_write(pfdev, GPU_INT_CLEAR,
-		  GPU_IRQ_CLEAN_CACHES_COMPLETED |
-		  GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
-	gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR);
-	gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_INV_CACHES);
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		reinit_completion(&pfdev->perfcnt->dump_comp);
+		gpu_write(pfdev, GPU_INT_CLEAR,
+			  GPU_IRQ_CLEAN_CACHES_COMPLETED |
+			  GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
+		gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR);
+		gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_INV_CACHES);
+	}
+
+	/*
+	 * If a reset happens during the wait for the IRQ notification that caches
+	 * are clean and invalidated, then we know the reset sequence did the job
+	 * for us, even if resetting IRQs means the completion might time out.
+	 */
 	ret = wait_for_completion_timeout(&pfdev->perfcnt->dump_comp,
 					  msecs_to_jiffies(1000));
-	if (!ret) {
+	if (!ret && !atomic_read(&perfcnt->reset_happened)) {
 		ret = -ETIMEDOUT;
 		goto err_vunmap;
 	}
 
-	ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
-	if (ret < 0)
-		goto err_vunmap;
-
-	as = ret;
-	cfg = GPU_PERFCNT_CFG_AS(as) |
-	      GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_MANUAL);
-
-	/*
-	 * Bifrost GPUs have 2 set of counters, but we're only interested by
-	 * the first one for now.
-	 */
-	if (panfrost_model_is_bifrost(pfdev))
-		cfg |= GPU_PERFCNT_CFG_SETSEL(counterset);
-
-	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0xffffffff);
-	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0xffffffff);
-	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0xffffffff);
-
-	/*
-	 * Due to PRLAM-8186 we need to disable the Tiler before we enable HW
-	 * counters.
-	 */
-	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
-		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
-	else
-		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
-
-	gpu_write(pfdev, GPU_PERFCNT_CFG, cfg);
-
-	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
-		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		perfcnt->counterset = counterset;
+		perfcnt->user = user;
+		atomic_set(&perfcnt->reset_happened, 0);
+		panfrost_perfcnt_hw_enable(pfdev);
+	}
 
 	/* The BO ref is retained by the mapping. */
 	drm_gem_object_put(&bo->base);
 
-	perfcnt->user = user;
-
 	return 0;
 
 err_vunmap:
@@ -203,13 +247,15 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
 	if (user != perfcnt->user)
 		return -EINVAL;
 
-	panfrost_perfcnt_gpu_disable(pfdev);
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
+		panfrost_perfcnt_gpu_disable(pfdev);
+		perfcnt->user = NULL;
+	}
 
-	perfcnt->user = NULL;
 	drm_gem_vunmap(&perfcnt->mapping->obj->base.base, &map);
 	perfcnt->buf = NULL;
 	panfrost_gem_close(&perfcnt->mapping->obj->base.base, file_priv);
-	panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
 	panfrost_gem_mapping_put(perfcnt->mapping);
 	perfcnt->mapping = NULL;
 	pm_runtime_put_autosuspend(pfdev->base.dev);
@@ -263,7 +309,7 @@ int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	ret = panfrost_perfcnt_dump_locked(pfdev);
+	ret = panfrost_perfcnt_dump_locked(pfdev, &req->hw_reset);
 	if (ret)
 		goto out;
 
@@ -346,3 +392,37 @@ void panfrost_perfcnt_fini(struct panfrost_device *pfdev)
 	/* Disable everything before leaving. */
 	panfrost_perfcnt_gpu_disable(pfdev);
 }
+
+void panfrost_perfcnt_reset(struct panfrost_device *pfdev)
+{
+	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
+
+	/* Could a reset somehow race with perfcnt_fini? */
+	if (drm_WARN_ON(&pfdev->base, !perfcnt))
+		return;
+
+	lockdep_assert_held(&pfdev->reset.lock);
+
+	if (!perfcnt->user)
+		return;
+
+	atomic_set(&perfcnt->reset_happened, 1);
+	synchronize_irq(pfdev->gpu_irq);
+	complete(&perfcnt->dump_comp);
+	panfrost_perfcnt_gpu_disable(pfdev);
+}
+
+void panfrost_perfcnt_postreset(struct panfrost_device *pfdev)
+{
+	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
+
+	if (drm_WARN_ON(&pfdev->base, !perfcnt))
+		return;
+
+	lockdep_assert_held(&pfdev->reset.lock);
+
+	if (!perfcnt->user)
+		return;
+
+	panfrost_perfcnt_hw_enable(pfdev);
+}
diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
index 8bbcf5f5fb33..e14e760641fd 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
@@ -14,5 +14,7 @@ int panfrost_ioctl_perfcnt_enable(struct drm_device *dev, void *data,
 				  struct drm_file *file_priv);
 int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
 				struct drm_file *file_priv);
+void panfrost_perfcnt_reset(struct panfrost_device *pfdev);
+void panfrost_perfcnt_postreset(struct panfrost_device *pfdev);
 
 #endif
diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
index 50d5337f35ef..3bbf9220103d 100644
--- a/include/uapi/drm/panfrost_drm.h
+++ b/include/uapi/drm/panfrost_drm.h
@@ -47,7 +47,7 @@ extern "C" {
  * them for anything but debugging purpose.
  */
 #define DRM_IOCTL_PANFROST_PERFCNT_ENABLE	DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_ENABLE, struct drm_panfrost_perfcnt_enable)
-#define DRM_IOCTL_PANFROST_PERFCNT_DUMP		DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct drm_panfrost_perfcnt_dump)
+#define DRM_IOCTL_PANFROST_PERFCNT_DUMP		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct drm_panfrost_perfcnt_dump)
 
 #define PANFROST_JD_REQ_FS (1 << 0)
 #define PANFROST_JD_REQ_CYCLE_COUNT (1 << 1)
@@ -272,6 +272,7 @@ struct drm_panfrost_perfcnt_enable {
 
 struct drm_panfrost_perfcnt_dump {
 	__u64 buf_ptr;
+	__u64 hw_reset;
 };
 
 /* madvise provides a way to tell the kernel in case a buffers contents

-- 
2.55.0

Re: [PATCH v3 12/13] drm/panfrost: Fix races between perfcnt and reset sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Adding a field to `struct drm_panfrost_perfcnt_dump` breaks UAPI ABI compatibility.
- [High] Ignored return value of `panfrost_perfcnt_hw_enable()` causes MMU Address Space refcount imbalance and hardware enablement failure.
- [High] Missing `reinit_completion()` inside the retry loop causes premature success returns with incomplete data.
- [High] The dump retry logic uses an unsigned integer for a `>= 0` check, creating an infinite loop.
- [Medium] Early return in `panfrost_perfcnt_reset()` prevents `panfrost_perfcnt_enable_locked()` from surviving a concurrent reset.
- [Medium] A TOCTOU race in `panfrost_perfcnt_dump_locked()` can conceal hardware reset events from userspace.
- [Medium] `synchronize_irq()` is called before the GPU interrupt source is masked.
--

--- Patch [12]: [PATCH v3 12/13] drm/panfrost: Fix races between perfcnt and reset sequence ---
Note: The format of this report has been altered due to recitation
restrictions. Instead of providing inline diff quotes, this report provides a
free-form summary of the findings.

commit 636e7ce6a7687c01b696bf2f1a79d4f8987d7340
Author: Adrián Larumbe <adrian.larumbe@collabora.com>
Subject: drm/panfrost: Fix races between perfcnt and reset sequence

This commit introduces a reset lock to govern hardware access during
performance counter operations. It also expands the DRM uAPI for the 
perfcnt DUMP operation to notify userspace of hardware resets, and ensures
performance counter configuration is restored after a reset completes.

[Severity: Critical]
Adding the hw_reset field to struct drm_panfrost_perfcnt_dump changes its
size, and the ioctl macro is updated from DRM_IOW to DRM_IOWR. Does this
alter the generated ioctl number for DRM_IOCTL_PANFROST_PERFCNT_DUMP and
break backwards compatibility for existing userspace binaries?

[Severity: High]
In panfrost_perfcnt_enable_locked(), the return value of
panfrost_perfcnt_hw_enable() is ignored. Since panfrost_perfcnt_hw_enable()
can return an error if panfrost_mmu_as_get() fails to find a free Address
Space slot, will ignoring this error cause the subsequent teardown in
panfrost_perfcnt_disable_locked() to unconditionally call
panfrost_mmu_as_put() and trigger a refcount underflow?

[Severity: High]
In panfrost_perfcnt_dump_locked(), reinit_completion() is called outside the
dump_retry loop. If a hardware reset occurs just after the wait times out,
the reset handler calls complete(). Won't the next wait attempt inside the
retry loop see a positive done count and return immediately without actually
waiting for the new sample?

[Severity: High]
Also in panfrost_perfcnt_dump_locked(), the loop counter retries is declared
as a u64. Since it is an unsigned integer, won't the retry check
(--retries >= 0) wrap around to U64_MAX and always evaluate to true,
creating an infinite loop if the GPU undergoes continuous resets?

[Severity: Medium]
In panfrost_perfcnt_reset(), there is an early return if !perfcnt->user.
However, panfrost_perfcnt_enable_locked() expects reset_happened to be
updated to handle timeouts gracefully. Since perfcnt->user is not assigned
until after the wait completes in the enable function, won't a concurrent
reset hit this early return and fail to update reset_happened, causing an
unintended -ETIMEDOUT error?

[Severity: Medium]
In panfrost_perfcnt_dump_locked(), the *reset_happened flag exported to
userspace is only updated on the very first iteration of the retry loop. If a
reset is detected by the atomic_cmpxchg check and the loop retries, will
userspace remain unaware of the reset if the second dump attempt succeeds?

[Severity: Medium]
In panfrost_device_reset(), panfrost_perfcnt_reset() is called, which then
calls synchronize_irq(pfdev->gpu_irq). However, the GPU interrupts are not
masked until panfrost_gpu_soft_reset() is executed on the following line.
Can the GPU raise a new interrupt immediately after synchronize_irq() returns
and before the mask is applied?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=12

[PATCH v3 13/13] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field

[Adrián Larumbe]

Expose expanded PERFCNT_DUMP IOCTL interface to user mode.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index e04eb416ea0f..d9d41236cc71 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -936,6 +936,7 @@ static void panfrost_debugfs_init(struct drm_minor *minor)
  * - 1.6 - adds PANFROST_BO_MAP_WB, PANFROST_IOCTL_SYNC_BO,
  *	   PANFROST_IOCTL_QUERY_BO_INFO and
  *	   DRM_PANFROST_PARAM_SELECTED_COHERENCY
+ * - 1.7 - adds PERFCNT_DUMP req hw_reset field
  */
 static const struct drm_driver panfrost_drm_driver = {
 	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,
@@ -948,7 +949,7 @@ static const struct drm_driver panfrost_drm_driver = {
 	.name			= "panfrost",
 	.desc			= "panfrost DRM",
 	.major			= 1,
-	.minor			= 6,
+	.minor			= 7,
 
 	.gem_create_object	= panfrost_gem_create_object,
 	.gem_prime_import	= panfrost_gem_prime_import,

-- 
2.55.0

Re: [PATCH v3 13/13] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Infinite loop in `panfrost_perfcnt_dump_locked` due to unsigned integer underflow.
- [High] Race condition bypassing wait for completion in `panfrost_perfcnt_dump_locked`.
- [High] Dead code in reset recovery during performance counter enablement leads to spurious timeouts.
- [High] Ignored return value of `panfrost_perfcnt_hw_enable()` leads to address space reference count underflow.
- [Medium] Stale `reset_happened` flag masks timeout errors in subsequent performance counter sessions.
- [Medium] Race condition in `panfrost_perfcnt_dump_locked()` fails to notify userspace of hardware resets.
--

--- Patch [13]: [PATCH v3 13/13] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field ---
commit a7210878c461c0a2b1d591ac0e3e2de27fb4740f
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field
    
Expose expanded PERFCNT_DUMP IOCTL interface to user mode.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index 7020c0192e189..a6f1b0b961fcd 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
[ ... ]
> -static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
> +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev,
> +					u64 *reset_happened)
>  {
> +	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> +	u64 retries = PERFCNT_DUMP_MAX_RETRIES;
> +	bool previous_reset;
>  	u64 gpuva;
>  	int ret;
>  
>  	reinit_completion(&pfdev->perfcnt->dump_comp);
>  	gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT;
> +	previous_reset = atomic_read(&perfcnt->reset_happened);
> +
> +dump_retry:

[Severity: High]
Does this sequence have a race condition if a GPU reset occurs during the dump?

The reinit_completion() is called outside the dump_retry loop.

If a hardware sample completion IRQ fires (waking the wait and consuming the
completion so done=0), and a GPU reset occurs immediately after but before
atomic_cmpxchg(), the reset handler calls complete(&dump_comp) (done=1).

The code then jumps to dump_retry because of the reset.

Will the second wait_for_completion immediately consume the pending completion
(done=1) and return without actually waiting for the new sample commands to
finish?

> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva));
[ ... ]
> +	}
> +
>  	ret = wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp,
>  							msecs_to_jiffies(1000));
> +	if (retries == PERFCNT_DUMP_MAX_RETRIES)
> +		*reset_happened = previous_reset || atomic_read(&perfcnt->reset_happened);
> +
> +	if (atomic_cmpxchg(&perfcnt->reset_happened, 1, 0)) {

[Severity: Medium]
Can this miss notifying userspace of a hardware reset?

If a GPU reset occurs precisely between the atomic_read() and
atomic_cmpxchg(&perfcnt->reset_happened, 1, 0), *reset_happened is assigned 0.

The atomic_cmpxchg() then succeeds, decrements retries, and loops to
dump_retry. On the subsequent successful iteration, the condition
(retries == PERFCNT_DUMP_MAX_RETRIES) is false, so *reset_happened is never
updated to true.

Will this break userspace counter accumulation logic by causing it to aggregate
post-reset small counter values as if they were continuous?

> +		if (--retries >= 0)
> +			goto dump_retry;

[Severity: High]
Is it possible for this to enter an infinite loop?

The retries variable is declared as a u64.

When it reaches 0, --retries will underflow to UINT64_MAX, making the condition
>= 0 always evaluate to true.

If resets continuously occur, could this bypass the PERFCNT_DUMP_MAX_RETRIES
limit and cause a soft lockup?

> +		ret = -EBUSY;
> +	} else {
[ ... ]
> @@ -117,57 +191,37 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
[ ... ]
> +	ret = wait_for_completion_timeout(&pfdev->perfcnt->dump_comp,
>  					  msecs_to_jiffies(1000));
> -	if (!ret) {
> +	if (!ret && !atomic_read(&perfcnt->reset_happened)) {
>  		ret = -ETIMEDOUT;
>  		goto err_vunmap;
>  	}

[Severity: High]
Is this reset recovery path dead code?

In panfrost_perfcnt_enable_locked(), the code waits for cache cleaning and
expects perfcnt->reset_happened to be set if a reset occurs during the wait.
However, panfrost_perfcnt_reset() checks if (!perfcnt->user) and returns early.

Because perfcnt->user is only assigned after the wait completes, any reset
during the wait causes the reset handler to return without setting
reset_happened.

Does this mean the wait will just time out and return -ETIMEDOUT instead of
recovering?

[ ... ]
> -	gpu_write(pfdev, GPU_PERFCNT_CFG, cfg);
> -
> -	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
> -		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		perfcnt->counterset = counterset;
> +		perfcnt->user = user;
> +		atomic_set(&perfcnt->reset_happened, 0);
> +		panfrost_perfcnt_hw_enable(pfdev);
> +	}

[Severity: High]
Is it safe to ignore the return value of panfrost_perfcnt_hw_enable()?

If panfrost_mmu_as_get() fails inside the helper, the hardware is not enabled
and the AS refcount is not incremented. However, perfcnt->user is still set
and the ioctl returns success.

Upon closing the session, panfrost_perfcnt_disable_locked() unconditionally
calls panfrost_mmu_as_put().

Will this cause an MMU address space reference count underflow?

[ ... ]
> @@ -193,18 +247,15 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
>  	if (user != perfcnt->user)
>  		return -EINVAL;
>  
> -	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0x0);
[ ... ]
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
> +		panfrost_perfcnt_gpu_disable(pfdev);
> +		perfcnt->user = NULL;
> +	}

[Severity: Medium]
Does tearing down the session here without clearing the reset_happened atomic
flag cause issues for future sessions?

If a GPU reset occurs during a performance counter session, the flag is set
to 1.

If the session is then closed without calling PERFCNT_DUMP (which normally
clears it), the flag remains 1.

When a new session is started via panfrost_perfcnt_enable_locked(), will the
stale flag cause the wait timeout condition !ret &&
!atomic_read(&perfcnt->reset_happened) to evaluate to false, masking any
legitimate timeouts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=13