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

[PATCH v4 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 v4:
- Fixed errors and issues uncovered by Sashiko in the previous revision.
- Changed the way RPM checks whether the device is initialised when resuming.
- Simplified perfcnt treatment of potential resets.
- Link to v3: https://patch.msgid.link/20260724-claude-fixes-v3-0-542af61abd0f@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       | 400 +++++++++++++++--------
 drivers/gpu/drm/panfrost/panfrost_device.h       |  18 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c          |  31 +-
 drivers/gpu/drm/panfrost/panfrost_gem.c          |  19 +-
 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          |  19 +-
 drivers/gpu/drm/panfrost/panfrost_mmu.c          |  25 +-
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c      | 236 ++++++++-----
 drivers/gpu/drm/panfrost/panfrost_perfcnt.h      |   2 +
 include/uapi/drm/panfrost_drm.h                  |   3 +-
 13 files changed, 498 insertions(+), 278 deletions(-)
---
base-commit: 671b7825dbfe9ea6e3ad3001003aeee0df48d1b5
change-id: 20260523-claude-fixes-82b03a134a8c

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

[PATCH v4 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 v4 02/13] drm/panfrost: Prevent division by 0

[Adrián Larumbe]

When updating and debug-printing devfreq stats, in the very unlikely
off-chance that total device time is less than 100 ns, division by 0
would happen. Fix the divisor when relevant and rearrange operands.

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 v4 02/13] drm/panfrost: Prevent division by 0

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Integer overflow in devfreq load calculation on 32-bit architectures
--

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

drm/panfrost: Prevent division by 0

This commit modifies the devfreq status calculation to prevent a division
by zero when the total device time is less than 100 ns. It updates the
debug logging divisor and rearranges the operands.

Closes: https://gitlab.freedesktop.org/panfrost/linux/-/work_items/88

> 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 cause an integer overflow on 32-bit architectures?

In panfrost_devfreq_get_dev_status(), status->busy_time is an unsigned
long, which is 32 bits on platforms like ARM32.

Since the Panfrost devfreq polling interval is 50ms, a fully busy GPU will
have a status->busy_time around 50,000,000 ns. Multiplying 50,000,000 by
100 yields 5,000,000,000, which overflows the 32-bit ULONG_MAX
(4,294,967,295) and wraps around to 705,032,704.

When this is divided by 50,000,000, it results in 14% instead of the
expected 100%.

Could this be calculated using a 64-bit type or a div-related macro to
prevent the debug log from reporting incorrect load percentages on heavily
loaded 32-bit systems?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=2

[PATCH v4 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 v4 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          | 19 ++++++++++---------
 drivers/gpu/drm/panfrost/panfrost_gem.h          |  6 +++---
 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c |  8 ++------
 5 files changed, 22 insertions(+), 25 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..aa3a604b3439 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -15,20 +15,21 @@
 #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;
+	    panfrost_transparent_hugepage) {
+		err = drm_gem_huge_mnt_create(&pfdev->base, "within_size");
+		if (drm_gem_get_huge_mnt(&pfdev->base))
+			drm_info(&pfdev->base, "Using Transparent Hugepage\n");
+		else if (err)
+			drm_warn(&pfdev->base,
+				 "Can't use Transparent Hugepage (%d)\n", err);
+	}
 
-	err = drm_gem_huge_mnt_create(&pfdev->base, "within_size");
-	if (drm_gem_get_huge_mnt(&pfdev->base))
-		drm_info(&pfdev->base, "Using Transparent Hugepage\n");
-	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

[PATCH v4 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 | 375 +++++++++++++++++------------
 drivers/gpu/drm/panfrost/panfrost_device.h |   9 +
 drivers/gpu/drm/panfrost/panfrost_drv.c    |  10 +-
 drivers/gpu/drm/panfrost/panfrost_gpu.c    |   7 +-
 drivers/gpu/drm/panfrost/panfrost_job.c    |   7 +-
 5 files changed, 237 insertions(+), 171 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 03905495aee2..a9d29d9ee22b 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -34,6 +34,40 @@ 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 void panfrost_clk_disable(struct panfrost_device *pfdev)
+{
+	clk_disable(pfdev->bus_ace_clock);
+	clk_disable(pfdev->bus_clock);
+	clk_disable(pfdev->clock);
+	reset_control_assert(pfdev->rstc);
+}
+
 static int panfrost_clk_init(struct panfrost_device *pfdev)
 {
 	int err;
@@ -48,7 +82,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,44 +91,55 @@ 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_ace_clock);
+unprepare_bus_clock:
+	clk_unprepare(pfdev->bus_clock);
+unprepare_clock:
+	clk_unprepare(pfdev->clock);
 
 	return err;
 }
 
 static void panfrost_clk_fini(struct panfrost_device *pfdev)
 {
-	clk_disable_unprepare(pfdev->bus_ace_clock);
-	clk_disable_unprepare(pfdev->bus_clock);
-	clk_disable_unprepare(pfdev->clock);
+	if (!(pfdev->comp->pm_features & BIT(GPU_PM_RT)))
+		panfrost_clk_disable(pfdev);
+
+	clk_unprepare(pfdev->bus_ace_clock);
+	clk_unprepare(pfdev->bus_clock);
+	clk_unprepare(pfdev->clock);
 }
 
 static int panfrost_regulator_init(struct panfrost_device *pfdev)
@@ -212,6 +257,133 @@ 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 (pfdev->comp->pm_features & BIT(GPU_PM_RT))
+		panfrost_clk_disable(pfdev);
+
+	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 +437,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 +482,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 +502,10 @@ 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_try_suspend_device(pfdev);
 	panfrost_regulator_fini(pfdev);
+	panfrost_devfreq_fini(pfdev);
 	panfrost_clk_fini(pfdev);
-	panfrost_reset_fini(pfdev);
 	panfrost_pm_domain_fini(pfdev);
 }
 
@@ -424,148 +611,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;
-
-	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
-		if (pfdev->bus_clock)
-			clk_disable(pfdev->bus_clock);
+	/* Do the actual device reset */
+	ret = panfrost_gpu_soft_reset(pfdev);
+	panfrost_gpu_power_on(pfdev);
 
-		clk_disable(pfdev->clock);
+	/* Post-reset */
+	if (panfrost_device_started(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;
+	if (!panfrost_device_started(pfdev) && !ret)
+		pfdev->gpu_started = true;
 }
-
-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..eeca57efcbec 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];
+	bool gpu_started;
 	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 {
@@ -342,4 +345,10 @@ panfrost_device_schedule_reset(struct panfrost_device *pfdev)
 	queue_work(pfdev->reset.wq, &pfdev->reset.work);
 }
 
+static inline bool
+panfrost_device_started(struct panfrost_device *pfdev)
+{
+	return pfdev->gpu_started;
+}
+
 #endif
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 331a3bd5b98c..27f4c4b217f5 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,12 @@ 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);
 err_out0:
 	return err;
 }
@@ -1019,10 +1016,9 @@ 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);
 }
 
 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..416964282138 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 (!panfrost_device_started(pfdev))
+		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;
 }
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 2d12b83e900a..35ff5b0f0013 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -446,7 +446,9 @@ void panfrost_jm_suspend_irq(struct panfrost_device *pfdev)
 	set_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
 
 	job_write(pfdev, JOB_INT_MASK, 0);
-	synchronize_irq(pfdev->js->irq);
+
+	if (pfdev->js)
+		synchronize_irq(pfdev->js->irq);
 }
 
 static void panfrost_job_handle_err(struct panfrost_device *pfdev,
@@ -971,6 +973,9 @@ int panfrost_jm_is_idle(struct panfrost_device *pfdev)
 	struct panfrost_job_slot *js = pfdev->js;
 	int i;
 
+	if (!js)
+		return true;
+
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
 		/* If there are any jobs in the HW queue, we're not idle */
 		if (atomic_read(&js->queue[i].sched.credit_count))

-- 
2.55.0

Re: [PATCH v4 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] GPU cores remain unpowered after probe due to uninitialized feature masks during the initial RPM resume.
- [High] Driver unbind leaves hardware active and violates PM guidelines, risking Use-After-Free and clock API violations.
- [High] Error paths in probe trigger RPM suspend callbacks that call `synchronize_irq()` on uninitialized or negative IRQ numbers.
- [Low] Duplicate hardware teardown in `panfrost_device_init` error path.
- [Low] Unbalanced reset control assertion in probe error path.
--

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

This patch moves all GPU enabling and disabling into runtime PM resume and
suspend callbacks. It aims to fix issues where the device remains powered on
forever due to usage count mismatches during probe failures or module unloads.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 03905495aee2d..a9d29d9ee22bb 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
[ ... ]
> @@ -48,7 +82,7 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
[ ... ]
>  static void panfrost_clk_fini(struct panfrost_device *pfdev)
>  {
> -	clk_disable_unprepare(pfdev->bus_ace_clock);
> -	clk_disable_unprepare(pfdev->bus_clock);
> -	clk_disable_unprepare(pfdev->clock);
> +	if (!(pfdev->comp->pm_features & BIT(GPU_PM_RT)))
> +		panfrost_clk_disable(pfdev);

[Severity: Low]
Can this lead to an unbalanced reset control assertion in the probe error path?

When panfrost_clk_fini() is called during the out_clk error path in
panfrost_device_init(), it calls panfrost_clk_disable(), which asserts
pfdev->rstc. 

The error path then falls through to out_reset, which calls
panfrost_reset_fini(). This asserts the exact same reset control a second
time, which can trigger a kernel warning from the reset controller subsystem
due to unbalanced assertion counts.

[ ... ]
> @@ -265,10 +437,25 @@ int panfrost_device_init(struct panfrost_device *pfdev)
[ ... ]
> +	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);
> +	err = panfrost_gpu_init(pfdev);

[Severity: Critical]
Does this sequence leave the GPU cores unpowered?

When pm_runtime_resume_and_get() triggers the runtime resume callback
(panfrost_device_runtime_resume()), it calls panfrost_gpu_power_on(). However,
this happens before panfrost_gpu_init() has initialized pfdev->features.

Because pfdev->features is uninitialized at this point, it is entirely zero.

Consequently, panfrost_gpu_power_on() will write 0 to the power-on registers
(L2_PWRON_LO, etc.), which fails to power on the cores.

[ ... ]
> @@ -295,6 +482,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);

[Severity: Low]
Does this error path duplicate hardware teardown?

If initialization fails after MMU init, the code jumps to out_mmu, manually
calling panfrost_mmu_fini() and panfrost_gpu_fini(). It then falls through
to out_rpm_put and calls panfrost_try_suspend_device().

This triggers the RPM suspend callback, which unconditionally calls
panfrost_mmu_suspend_irq(), panfrost_gpu_suspend_irq(), and
panfrost_gpu_power_off() a second time.

[Severity: High]
Can this error path trigger an out-of-bounds access in the IRQ descriptor tree?

If panfrost_device_init() fails early (e.g., inside panfrost_gpu_init()),
the error path jumps to out_rpm_put and forces a runtime suspend. This
unconditionally executes panfrost_mmu_suspend_irq() and
panfrost_gpu_suspend_irq().

Because initialization failed early, pfdev->mmu_irq is 0, and if
platform_get_irq_byname() failed, pfdev->gpu_irq contains a negative error
code.

Passing these uninitialized or negative values to synchronize_irq()
can cause undefined behavior or synchronize unrelated system interrupts.

>  out_regulator:
>  	panfrost_regulator_fini(pfdev);
[ ... ]
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 331a3bd5b98cb..27f4c4b217f5b 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
[ ... ]
> @@ -1019,10 +1016,9 @@ 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);
>  }

[Severity: High]
Does this driver unbind sequence risk leaving the hardware active?

By removing pm_runtime_disable(), the teardown relies on
panfrost_try_suspend_device() (called inside panfrost_device_fini) to suspend
the device.

However, if panfrost_jm_is_idle() returns false, or if a userspace process
holds the PM usage count > 0 via sysfs power/control, the RPM suspend callback
is never executed.

In this scenario, panfrost_device_fini() proceeds to unconditionally free
resources and unprepare clocks while they are still enabled and the GPU is
still active.

This risks clock API violations and unhandled interrupt storms since the
hardware was never powered off or masked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=5

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

[Philipp Zabel]

On Mi, 2026-07-29 at 03:54 +0100, Adrián Larumbe wrote:
> 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 | 375 +++++++++++++++++------------
>  drivers/gpu/drm/panfrost/panfrost_device.h |   9 +
>  drivers/gpu/drm/panfrost/panfrost_drv.c    |  10 +-
>  drivers/gpu/drm/panfrost/panfrost_gpu.c    |   7 +-
>  drivers/gpu/drm/panfrost/panfrost_job.c    |   7 +-
>  5 files changed, 237 insertions(+), 171 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 03905495aee2..a9d29d9ee22b 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -34,6 +34,40 @@ 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 void panfrost_clk_disable(struct panfrost_device *pfdev)
> +{
> +	clk_disable(pfdev->bus_ace_clock);
> +	clk_disable(pfdev->bus_clock);
> +	clk_disable(pfdev->clock);
> +	reset_control_assert(pfdev->rstc);
> +}

This asymmetry is weird. Why assert the reset control in
panfrost_clock_disable() but not deassert it in panfrost_clk_enable()?
It's also unexpected to have a function called _clk_disable do
something else.

> +
>  static int panfrost_clk_init(struct panfrost_device *pfdev)
>  {
>  	int err;
> @@ -48,7 +82,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,44 +91,55 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
[...]
>  static void panfrost_clk_fini(struct panfrost_device *pfdev)
>  {
> -	clk_disable_unprepare(pfdev->bus_ace_clock);
> -	clk_disable_unprepare(pfdev->bus_clock);
> -	clk_disable_unprepare(pfdev->clock);
> +	if (!(pfdev->comp->pm_features & BIT(GPU_PM_RT)))
> +		panfrost_clk_disable(pfdev);

So here you are effectively adding a reset_control_assert() that wasn't
there before. That seems to be intentional because you are removing the
call to panfrost_reset_fini() in panfrost_device_fini().

It's confusing though, panfrost_clk_fini() now cleans up things that
panfrost_clk_init() didn't set up.

> +
> +	clk_unprepare(pfdev->bus_ace_clock);
> +	clk_unprepare(pfdev->bus_clock);
> +	clk_unprepare(pfdev->clock);
>  }
>  
>  static int panfrost_regulator_init(struct panfrost_device *pfdev)
> @@ -212,6 +257,133 @@ 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;

Here we clearly deassert the reset and enable the clocks ...

> +	}
> +
> +	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 (pfdev->comp->pm_features & BIT(GPU_PM_RT))
> +		panfrost_clk_disable(pfdev);

... and here we don't seem to assert the reset, but secretly do.

> +
> +	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) {

Unnecessary check, clk_enable() handles NULL pointers.

> +			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)

Unnecessary check.

> +		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)

Unnecessary check.

> +			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 +437,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 +482,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:

Directly below this:

          panfrost_devfreq_fini(pfdev);                                                                                                       
  out_clk:                                                                                                                                    
          panfrost_clk_fini(pfdev);                                                                                                           
  out_reset:                                                                                                                                  
          panfrost_reset_fini(pfdev);                                                                                                         

_reset_fini is still called right after _clk_fini asserted the reset
control.

> @@ -313,12 +502,10 @@ 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_try_suspend_device(pfdev);
>  	panfrost_regulator_fini(pfdev);
> +	panfrost_devfreq_fini(pfdev);
>  	panfrost_clk_fini(pfdev);
> -	panfrost_reset_fini(pfdev);

Here _reset_fini is dropped because _clk_fini  already asserted the
reset.

regards
Philipp

[PATCH v4 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_job.c |  3 ++-
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 14 ++++++++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 35ff5b0f0013..bda1494eb430 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -871,7 +871,6 @@ int panfrost_jm_init(struct panfrost_device *pfdev)
 	js = devm_kzalloc(pfdev->base.dev, sizeof(*js), GFP_KERNEL);
 	if (!js)
 		return -ENOMEM;
-	pfdev->js = js;
 
 	INIT_WORK(&pfdev->reset.work, panfrost_reset_work);
 	spin_lock_init(&js->job_lock);
@@ -906,6 +905,8 @@ int panfrost_jm_init(struct panfrost_device *pfdev)
 		}
 	}
 
+	pfdev->js = js;
+
 	panfrost_jm_reset_interrupts(pfdev);
 	panfrost_jm_enable_interrupts(pfdev);
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 4a3162c3b659..aad0cd31516d 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)
@@ -880,6 +885,9 @@ static irqreturn_t panfrost_mmu_irq_handler_thread(int irq, void *data)
 	u32 status = mmu_read(pfdev, MMU_INT_RAWSTAT);
 	int ret;
 
+	if (!pfdev->js)
+		return IRQ_NONE;
+
 	while (status) {
 		u32 as = ffs(status | (status >> 16)) - 1;
 		u32 mask = BIT(as) | BIT(as + 16);
@@ -970,6 +978,8 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
 		return err;
 	}
 
+	panfrost_mmu_enable_interrupts(pfdev);
+
 	return 0;
 }
 

-- 
2.55.0

[PATCH v4 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 aad0cd31516d..2d0ef6575f06 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);
 
@@ -983,11 +984,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);
@@ -995,3 +991,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 v4 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:

New issues:
- [High] Deadlock between DRM scheduler fence signalling and the MMU threaded IRQ handler during device reset.

Pre-existing issues:
- [High] Incomplete IRQ synchronization during initialization and probe failure leads to use-after-free and NULL pointer dereferences on `pfdev->reset.wq`.
- [High] TOCTOU race in MMU and JM threaded IRQ handlers can lead to IRQ storms after device suspend.
--

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

The patch ensures that the MMU threaded IRQ handler is synchronized before
disabling the MMU during a reset or device teardown. It adds an IRQ
synchronization call during device reset and uses the suspend IRQ routine
in the MMU teardown sequence.

> --- 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);

[Severity: High]
Does this code introduce a deadlock between DRM scheduler fence signalling and
the MMU threaded IRQ handler during device reset?

During a job timeout recovery, panfrost_reset() establishes a DMA fence
signalling critical section via dma_fence_begin_signalling() before calling
panfrost_device_reset().

By adding synchronize_irq(pfdev->mmu_irq) here, the fence signalling thread
waits for the MMU threaded IRQ handler to complete.

However, the page fault handler panfrost_mmu_irq_handler_thread() allocates
memory with GFP_KERNEL:

    bo->sgts = kvmalloc_objs(struct sg_table,
                             bo->base.base.size / SZ_2M,
                             GFP_KERNEL | __GFP_ZERO);

If low memory conditions trigger direct reclaim, and direct reclaim waits for
GPU fences to signal, will this create a circular lock dependency since the
fence signalling thread is blocked on synchronize_irq()?

>  
>  	spin_lock(&pfdev->as_lock);
>  

[ ... ]

> @@ -983,11 +984,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);
> @@ -995,3 +991,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 isn't a bug introduced by this patch, but does panfrost_mmu_suspend_irq()
race with panfrost_mmu_irq_handler_thread() and cause an IRQ storm?

In the MMU threaded IRQ handler, the suspended state is checked before
unmasking:

    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);
        ...
    }

If the threaded handler is preempted immediately after passing the test_bit()
check, panfrost_mmu_suspend_irq() can execute, set the suspended bit, mask
the interrupts, and block on synchronize_irq().

When the threaded handler resumes, does it blindly write the unmask value back
to MMU_INT_MASK?

Since the hardirq handler panfrost_mmu_irq_handler() returns IRQ_NONE when
the suspended bit is set, will any subsequent unmasked interrupt cause an
endless IRQ loop?

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

[Severity: High]
This is a pre-existing issue, but can incomplete IRQ synchronization during
initialization or probe failure lead to use-after-free or NULL pointer
dereferences on pfdev->reset.wq?

During initialization, panfrost_device_init() calls panfrost_mmu_init() which
enables MMU interrupts, and then it calls panfrost_jm_init() which allocates
pfdev->reset.wq.

If an MMU interrupt triggers between these two calls, can the handler call
panfrost_device_schedule_reset() and invoke queue_work() with a NULL
pfdev->reset.wq?

During probe failure unwinding, panfrost_device_init() handles errors by
executing panfrost_jm_fini() (destroying the workqueue) before calling
panfrost_mmu_fini().

If an MMU interrupt fires in this teardown window, will it execute
queue_work() on the already freed pfdev->reset.wq?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=7

[PATCH v4 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    | 7 ++++---
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index a9d29d9ee22b..a51f1ef8563c 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 eeca57efcbec..f0acfc634cd4 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);
 }
 
 static inline bool
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index bda1494eb430..ed8a1fabd47b 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -499,10 +499,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,
@@ -757,6 +755,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
@@ -893,6 +893,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 v4 08/13] drm/panfrost: Rewire reset sequence to avoid concurrent attempts

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Removing the synchronous `atomic_set(&pfdev->reset.pending, 1)` from `panfrost_job_handle_err` opens a vulnerable window where new jobs are erroneously submitted to faulted hardware.
- [Medium] The introduced `pfdev->reset.wait` wait queue is never waited on, rendering it dead code and breaking the intended synchronization.
--

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

drm/panfrost: Rewire reset sequence to avoid concurrent attempts

This rewires the reset sequence to use a single reset thread and a wait queue
for other threads wanting to trigger a reset.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index eeca57efcbec0..f0acfc634cd47 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
[ ... ]
> @@ -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);
>  }

[Severity: Medium]
The commit message states that other threads will wait on the ongoing reset
to finish, and a wait queue is added and woken up in panfrost_reset().
However, there do not appear to be any wait_event() calls added for
pfdev->reset.wait in the codebase.

When the atomic_cmpxchg() fails here, the caller returns immediately instead
of blocking on the ongoing reset. Are we missing the wait logic for
concurrent reset requests?

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index bda1494eb430c..ed8a1fabd47b4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
[ ... ]
> @@ -499,10 +499,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);
> -	}
>  }

[Severity: High]
Does removing the synchronous atomic_set() here create a vulnerability window?

By delegating this to the async timeout worker via drm_sched_fault(),
reset.pending remains 0 when panfrost_job_handle_err() returns.

If reset.pending is still 0, will panfrost_jm_handle_irq() incorrectly
requeue jobs onto the faulted hardware?

panfrost_jm_handle_irq() {
    ...
    } else if (!atomic_read(&pfdev->reset.pending)) {
        /* Requeue the job we removed if no reset is pending */
        job_write(pfdev, JS_COMMAND_NEXT(j), JS_COMMAND_START);
    }
}

Similarly, concurrent job submissions from the scheduler in
panfrost_job_hw_submit() might also incorrectly proceed because they check
if (!atomic_read(&pfdev->reset.pending)).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=8

[PATCH v4 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 | 41 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_device.h |  3 +++
 drivers/gpu/drm/panfrost/panfrost_drv.c    |  1 +
 3 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index a51f1ef8563c..c0334b72c70b 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>
@@ -629,3 +630,43 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 	if (!panfrost_device_started(pfdev) && !ret)
 		pfdev->gpu_started = true;
 }
+
+#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);
+	int ret;
+
+	ret = pm_runtime_get_if_in_use(pfdev->base.dev);
+
+	if (ret > 0) {
+		panfrost_device_schedule_reset(pfdev);
+		ret = wait_event_interruptible_timeout(pfdev->reset.wait,
+						       !atomic_read(&pfdev->reset.pending),
+						       msecs_to_jiffies(60));
+		pm_runtime_put(pfdev->base.dev);
+	}
+
+	return (ret > 0) ? 0 : -ETIMEDOUT;
+}
+
+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 f0acfc634cd4..f96ba50dfa91 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -352,4 +352,7 @@ panfrost_device_started(struct panfrost_device *pfdev)
 	return pfdev->gpu_started;
 }
 
+#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 27f4c4b217f5..fac5be20ad60 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 v4 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:

New issues:
- [High] Debugfs `reset_set` drops the PM runtime reference on wait timeout or signal before the reset work completes, leading to hardware access while the device is suspended.
- [Medium] Incorrect return value logic in `reset_set` masks signal interruptions and suspended PM states by unconditionally returning `-ETIMEDOUT`.

Pre-existing issues:
- [High] The GPU reset sequence drops PM runtime references from stuck jobs before accessing hardware registers, risking a crash if the device suspends concurrently.
--

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

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

This commit adds a debugfs file to manually trigger a GPU reset. This is
intended to help test potential race conditions between the GPU reset
sequence and other parts of the code accessing hardware registers.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index a51f1ef8563ce..c0334b72c70b3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c

[ ... ]

> @@ -629,3 +630,43 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>  	if (!panfrost_device_started(pfdev) && !ret)
>  		pfdev->gpu_started = true;
>  }
> +
> +#ifdef CONFIG_DEBUG_FS
> +static int reset_get(void *data, u64 *val)
> +{

[ ... ]

> +static int reset_set(void *data, u64 val)
> +{
> +	struct panfrost_device *pfdev =
> +		container_of(data, struct panfrost_device, base);
> +	int ret;
> +
> +	ret = pm_runtime_get_if_in_use(pfdev->base.dev);
> +
> +	if (ret > 0) {
> +		panfrost_device_schedule_reset(pfdev);
> +		ret = wait_event_interruptible_timeout(pfdev->reset.wait,
> +						       !atomic_read(&pfdev->reset.pending),
> +						       msecs_to_jiffies(60));
> +		pm_runtime_put(pfdev->base.dev);

[Severity: High]
If the wait is interrupted by a signal (like Ctrl-C) or times out, does
this immediately drop the PM usage count to 0 and queue a suspend request?

Since the scheduled reset work continues executing asynchronously without a
PM reference, could it access the GPU registers via panfrost_device_reset()
while the device is already powered down, leading to a system crash?

> +	}
> +
> +	return (ret > 0) ? 0 : -ETIMEDOUT;

[Severity: Medium]
Does this ternary return statement mask signal interruptions and PM runtime
errors?

If wait_event_interruptible_timeout() is interrupted, it returns -ERESTARTSYS.
Converting this negative error code to -ETIMEDOUT prevents the kernel from
correctly restarting the syscall or returning -EINTR to user space. It also
masks legitimate errors if pm_runtime_get_if_in_use() returned 0 initially.

> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(panfrost_reset_debugfs_fops,
> +			 reset_get, reset_set,
> +			 "0x%08llx\n");

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the reset
sequences, I noticed a potential race condition in the existing
panfrost_reset() handler in drivers/gpu/drm/panfrost/panfrost_job.c:

panfrost_job.c:panfrost_reset() {
    ...
    for (i = 0; i < NUM_JOB_SLOTS; i++) {
        if (pfdev->jobs[i][0])
            pm_runtime_put_noidle(pfdev->base.dev);
        ...
    }
    memset(pfdev->jobs, 0, sizeof(pfdev->jobs));
    ...
    panfrost_device_reset(pfdev, true);
}

Does dropping the PM runtime references for all jobs before executing the
hardware reset risk a crash?

By dropping the usage count and clearing the jobs array, panfrost_jm_is_idle()
will return true. If a concurrent thread had triggered a PM autosuspend timer,
could the device be suspended before panfrost_device_reset() accesses the GPU
registers? Should panfrost_reset() acquire its own PM reference before
dropping the job references?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=9

[PATCH v4 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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Inversion of register write order when disabling performance counters can cause hardware hangs or unintended asynchronous dumps.
--

--- Patch [10]: [PATCH v4 10/13] drm/panfrost: Move perfcnt GPU disable sequence into a helper ---
commit 2406c3dc1e67f4022955a0b8844241dd50763d30
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Move perfcnt GPU disable sequence into a helper

This patch introduces a helper function to consolidate the sequence 
that disables GPU performance counters. It replaces the open-coded 
register writes across the panfrost driver to reduce repetition.

> --- 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);
> +}

[Severity: High]
Does this helper invert the intended register write order?

Looking at the original implementation being replaced below, the _EN registers
were cleared before writing GPU_PERFCNT_CFG_MODE_OFF to GPU_PERFCNT_CFG. By
turning off the CFG mode first, we might violate hardware quirks like
PRLAM-8186 which require TILER_EN to be disabled before writing to CFG.

Could writing CFG while the counter enable registers are still active trigger
an unintended asynchronous dump? Additionally, if the block clock-gates after
CFG is turned off, could the subsequent writes to clear the _EN registers be
ignored?

[ ... ]

> @@ -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);

[Severity: High]
If an asynchronous dump is triggered by the inverted register order above,
could it hit unmapped memory here?

Since panfrost_perfcnt_disable_locked() immediately proceeds to unmap the BO
and tear down the MMU address space, any unexpected dump in progress would
cause an IOMMU fault and potentially a GPU reset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=10

[PATCH v4 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 c0334b72c70b..926bdf21389f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -615,6 +615,10 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 {
 	int ret;
 
+	/* Pre-reset */
+	if (panfrost_device_started(pfdev))
+		down_write(&pfdev->reset.lock);
+
 	/* Do the actual device reset */
 	ret = panfrost_gpu_soft_reset(pfdev);
 	panfrost_gpu_power_on(pfdev);
@@ -625,6 +629,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);
 	}
 
 	if (!panfrost_device_started(pfdev) && !ret)
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index f96ba50dfa91..81f2290d3e86 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 fac5be20ad60..3212dbadcdb8 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 ed8a1fabd47b..f40563682860 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -893,7 +893,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 v4 11/13] drm/panfrost: Introduce a reset lock

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing `reset.lock` protection for other hardware-accessing entry points, risking concurrent register access during a GPU reset.
--

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

drm/panfrost: Introduce a reset lock

This commit introduces a read/write semaphore to prevent driver entry points
from accessing hardware registers while a reset is ongoing. It wraps the
timestamp query ioctl with this lock to avoid races during GPU soft resets.

> drm/panfrost: Introduce a reset lock
>
> 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 isn't a bug introduced by this patch, but are there other entry points
that also race with GPU resets by writing to hardware registers?

For instance, panfrost_ioctl_perfcnt_enable() calls
panfrost_perfcnt_enable_locked() which writes directly to GPU_CMD:

drivers/gpu/drm/panfrost/panfrost_perfcnt.c:panfrost_perfcnt_enable_locked() {
    ...
    gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR);
    gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_INV_CACHES);
    ...
}

panfrost_ioctl_perfcnt_dump() and mmu_hw_do_operation() also write to
hardware registers (GPU_CMD and AS_COMMAND respectively) without acquiring
reset.lock.

Could these be invoked concurrently with a job timeout reset? If so,
should they also acquire pfdev->reset.lock to prevent hardware state
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=11

[PATCH v4 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 | 207 ++++++++++++++++++++--------
 drivers/gpu/drm/panfrost/panfrost_perfcnt.h |   2 +
 include/uapi/drm/panfrost_drm.h             |   3 +-
 4 files changed, 154 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 926bdf21389f..ae0f10ba7455 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -616,8 +616,10 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 	int ret;
 
 	/* Pre-reset */
-	if (panfrost_device_started(pfdev))
+	if (panfrost_device_started(pfdev)) {
 		down_write(&pfdev->reset.lock);
+		panfrost_perfcnt_reset(pfdev);
+	}
 
 	/* Do the actual device reset */
 	ret = panfrost_gpu_soft_reset(pfdev);
@@ -629,6 +631,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..b92ad9061893 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;
+	bool reset_happened;
 };
 
 static void panfrost_perfcnt_gpu_disable(struct panfrost_device *pfdev)
@@ -55,38 +59,97 @@ 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)
 {
-	u64 gpuva;
+	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
+	u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT;
+	u64 retries = PERFCNT_DUMP_MAX_RETRIES;
 	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);
+dump_retry:
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		*reset_happened = perfcnt->reset_happened;
+		perfcnt->reset_happened = false;
+		reinit_completion(&pfdev->perfcnt->dump_comp);
+		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;
+
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		if (ret > 0) {
+			if (perfcnt->reset_happened) {
+				if (--retries >= 0)
+					goto dump_retry;
+				else
+					ret = -EBUSY;
+			} else {
+				ret = 0;
+			}
+		} else if (!ret) {
+			ret = -ETIMEDOUT;
+		}
+	}
 
 	return ret;
 }
 
+static int panfrost_perfcnt_hw_enable(struct panfrost_device *pfdev)
+{
+	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
+	u32 cfg, as;
+	int ret;
+
+	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)
@@ -119,7 +182,9 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 	ret = drm_gem_vmap(&bo->base, &map);
 	if (ret)
 		goto err_put_mapping;
+
 	perfcnt->buf = map.vaddr;
+	perfcnt->counterset = counterset;
 
 	panfrost_gem_internal_set_label(&bo->base, "Perfcnt sample buffer");
 
@@ -127,60 +192,46 @@ 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);
+		perfcnt->reset_happened = false;
+		perfcnt->user = user;
+	}
+
+	/*
+	 * 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 it takes long enough for the completion to time out.
+	 */
 	ret = wait_for_completion_timeout(&pfdev->perfcnt->dump_comp,
 					  msecs_to_jiffies(1000));
-	if (!ret) {
+	if (!ret && !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) {
+		if (!perfcnt->reset_happened) {
+			ret = panfrost_perfcnt_hw_enable(pfdev);
+			if (ret)
+				goto err_vunmap;
+		}
+		perfcnt->reset_happened = false;
+	}
 
 	/* The BO ref is retained by the mapping. */
 	drm_gem_object_put(&bo->base);
 
-	perfcnt->user = user;
-
 	return 0;
 
 err_vunmap:
+	scoped_guard(rwsem_read, &pfdev->reset.lock)
+		perfcnt->user = user;
 	drm_gem_vunmap(&bo->base, &map);
 err_put_mapping:
 	panfrost_gem_mapping_put(perfcnt->mapping);
@@ -203,13 +254,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 +316,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 +399,35 @@ 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;
+
+	if (drm_WARN_ON(&pfdev->base, !perfcnt))
+		return;
+
+	lockdep_assert_held(&pfdev->reset.lock);
+
+	if (!perfcnt->user)
+		return;
+
+	perfcnt->reset_happened = true;
+	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 v4 12/13] drm/panfrost: Fix races between perfcnt and reset sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] The error path in `panfrost_perfcnt_enable_locked` incorrectly assigns `perfcnt->user = user` instead of clearing it, leading to a dangling session state and severe Use-After-Free vulnerabilities.
- [High] In `panfrost_perfcnt_disable_locked`, the MMU Address Space (AS) is released before the GPU performance counters are disabled.
- [High] The loop counter `retries` in `panfrost_perfcnt_dump_locked` is typed as an unsigned 64-bit integer, making the loop condition `--retries >= 0` always true.
- [High] `panfrost_perfcnt_postreset()` ignores the return value of `panfrost_perfcnt_hw_enable()`, leading to Address Space (AS) reference count underflows and unconfigured hardware.
- [Medium] Clearing `perfcnt->reset_happened` before an interruptible wait in `panfrost_perfcnt_dump_locked` causes reset notifications to be lost if the wait is interrupted.
--

--- Patch [12]: [PATCH v4 12/13] drm/panfrost: Fix races between perfcnt and reset sequence ---
commit 33e476c0f2af99299bbeb687048c33539616ce05
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Fix races between perfcnt and reset sequence

This commit uses the reset r/w semaphore to govern hardware access during 
resets. It expands the DRM uAPI for the perfcnt DUMP operation to notify
userspace if a reset occurred, and reestablishes perfcnt to its original
configuration post-reset.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index ad1156678e91c..b92ad90618931 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)
>  {
> -	u64 gpuva;
> +	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> +	u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT;
> +	u64 retries = PERFCNT_DUMP_MAX_RETRIES;
>  	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);
> +dump_retry:
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		*reset_happened = perfcnt->reset_happened;
> +		perfcnt->reset_happened = false;

[Severity: Medium]
Could clearing perfcnt->reset_happened before the wait cause lost
notifications?

If wait_for_completion_interruptible_timeout() is interrupted by a signal, it
returns a negative error code (like -ERESTARTSYS). The ioctl will fail and
skip copying the hw_reset state to userspace.

When the kernel transparently restarts the syscall, the flag has already been
cleared here. Does this result in userspace successfully getting a dump but
missing the reset notification, leading to invalid performance counter deltas?

> +		reinit_completion(&pfdev->perfcnt->dump_comp);
> +		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;
> +
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		if (ret > 0) {
> +			if (perfcnt->reset_happened) {
> +				if (--retries >= 0)

[Severity: High]
Can this loop condition ever be false?

Since retries is declared as an unsigned u64 at the top of the function,
decrementing it past zero wraps around to ULLONG_MAX, which is always >= 0.

If a hardware reset is continually detected (for example, a workload
repeatedly hanging the GPU), does this trap the ioctl in an infinite loop
instead of gracefully returning -EBUSY?

> +					goto dump_retry;
> +				else
> +					ret = -EBUSY;
> +			} else {
> +				ret = 0;
> +			}
> +		} else if (!ret) {
> +			ret = -ETIMEDOUT;
> +		}
> +	}
>  
>  	return ret;
>  }
[ ... ]
>  static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  					  struct drm_file *file_priv,
>  					  unsigned int counterset)
>  {
[ ... ]
>  	/* The BO ref is retained by the mapping. */
>  	drm_gem_object_put(&bo->base);
>  
> -	perfcnt->user = user;
> -
>  	return 0;
>  
>  err_vunmap:
> +	scoped_guard(rwsem_read, &pfdev->reset.lock)
> +		perfcnt->user = user;

[Severity: Critical]
Is this intended to assign 'user', or should it clear the user pointer?

If a failure occurs (such as in panfrost_perfcnt_hw_enable()), the error
path jumps to err_vunmap where it unmaps the buffers and frees the mappings. 

By assigning perfcnt->user = user here instead of NULL, the user pointer
remains valid while the underlying resources are freed. Can a subsequent call
to disable or dump the counters by the same user bypass the active session
check and trigger a use-after-free on the freed memory?

>  	drm_gem_vunmap(&bo->base, &map);
>  err_put_mapping:
>  	panfrost_gem_mapping_put(perfcnt->mapping);
> @@ -203,13 +254,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);

[Severity: High]
Is it safe to release the Address Space (AS) before disabling the hardware
that uses it?

By calling panfrost_mmu_as_put() first, the AS reference count drops and it
becomes immediately available for reallocation to another concurrent context.

However, the performance counters are still actively configured to write to
this AS index until panfrost_perfcnt_gpu_disable() executes. Does this create
a race window where performance counters emit data into a repurposed address
space, potentially causing memory corruption or page faults?

> +		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);
[ ... ]
> +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);

[Severity: High]
Should we check the return value of panfrost_perfcnt_hw_enable() here?

During a GPU reset, all Address Spaces are reset and their reference counts
zeroed. If panfrost_perfcnt_hw_enable() fails (for example, if no AS is
available and panfrost_mmu_as_get returns -EBUSY), the hardware remains
unconfigured and the AS reference count is not incremented.

Since this failure is silently discarded, when the session is eventually
disabled, panfrost_perfcnt_disable_locked() will unconditionally call
panfrost_mmu_as_put(). Will this decrement the AS count below zero,
triggering a warning and corrupting the MMU AS tracking logic?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=12

[PATCH v4 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 3212dbadcdb8..4384f8f744e5 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 v4 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 2 potential issue(s) to consider:
- [Critical] The error path in `panfrost_perfcnt_enable_locked` incorrectly sets `perfcnt->user = user` instead of `NULL`, leading to a corrupt state and Use-After-Free.
- [Medium] Hardware reset notification is permanently lost if the ioctl wait is interrupted by a signal.
--

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

drm/panfrost: Fix races between perfcnt and reset sequence

This commit fixes races between the reset sequence and perfcnt session
teardown. It uses a read/write semaphore to serialize access to the
hardware during resets and updates the DRM uAPI to notify userspace of
resets during a perfcnt DUMP operation.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index ad1156678e91c..b92ad90618931 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
[ ... ]
> @@ -55,38 +59,97 @@ 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)
>  {
> -	u64 gpuva;
> +	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> +	u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT;
> +	u64 retries = PERFCNT_DUMP_MAX_RETRIES;
>  	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);
> +dump_retry:
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		*reset_happened = perfcnt->reset_happened;
> +		perfcnt->reset_happened = false;

[Severity: Medium]
Does this code permanently lose the hardware reset notification if the ioctl
wait is interrupted by a signal?

Since the internal state is cleared prior to the interruptible wait and not
restored on the -ERESTARTSYS error path, could a restarted syscall overwrite
req->hw_reset with the cleared value, causing userspace to accumulate
counters that were actually reset?

> +		reinit_completion(&pfdev->perfcnt->dump_comp);
> +		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));
[ ... ]
> @@ -127,60 +192,46 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
[ ... ]
>  	/* The BO ref is retained by the mapping. */
>  	drm_gem_object_put(&bo->base);
>  
> -	perfcnt->user = user;
> -
>  	return 0;
>  
>  err_vunmap:
> +	scoped_guard(rwsem_read, &pfdev->reset.lock)
> +		perfcnt->user = user;

[Severity: Critical]
Is it possible this error path incorrectly reassigns perfcnt->user to user
instead of setting it to NULL?

If panfrost_perfcnt_hw_enable() fails (e.g. when panfrost_mmu_as_get fails
because address spaces are exhausted), could this leave perfcnt->user set
while the backing resources are freed, possibly leading to a use-after-free
on subsequent ioctls?

>  	drm_gem_vunmap(&bo->base, &map);
>  err_put_mapping:
>  	panfrost_gem_mapping_put(perfcnt->mapping);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=13