[PATCH v3 0/2] drm/imagination: FW trace bug fix and configuration update

[PATCH v3 0/2] drm/imagination: FW trace bug fix and configuration update

[Brajesh Gupta]

Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
Changes in v3:
- Fixed typo in v2 update of commit description.
- Link to v2: https://lore.kernel.org/r/20260424-ftrace_fix-v2-0-50146ff526d5@imgtec.com

Changes in v2:
- Fixed the name of module param in commit description for 2nd patch.
- Link to v1: https://lore.kernel.org/r/20260417-ftrace_fix-v1-0-9fefe194592a@imgtec.com

---
Brajesh Gupta (2):
      drm/imagination: Fix segfault when updating ftrace mask
      drm/imagination: Restrict init_fw_trace_mask module param to read only

 drivers/gpu/drm/imagination/pvr_fw_trace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
---
base-commit: 3bce3fdd1ff2ba242f76ab66659fff27207299f1
change-id: 20260417-ftrace_fix-5de47d2448c0

Best regards,
-- 
Brajesh Gupta <brajesh.gupta@imgtec.com>

[PATCH v3 1/2] drm/imagination: Fix segfault when updating ftrace mask

[Brajesh Gupta]

Fix invalid data access by passing right data for debugfs entry.

[  171.549793] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[  171.559248] Mem abort info:
[  171.562173]   ESR = 0x0000000096000044
[  171.566227]   EC = 0x25: DABT (current EL), IL = 32 bits
[  171.573108]   SET = 0, FnV = 0
[  171.576448]   EA = 0, S1PTW = 0
[  171.579745]   FSC = 0x04: level 0 translation fault
[  171.584760] Data abort info:
[  171.588012]   ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000
[  171.593734]   CM = 0, WnR = 1, TnD = 0, TagAccess = 0
[  171.598962]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[  171.604471] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000083837000
[  171.611358] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
[  171.618500] Internal error: Oops: 0000000096000044 [#1]  SMP
[  171.624222] Modules linked in: powervr drm_shmem_helper drm_gpuvm...
[  171.656580] CPU: 0 UID: 0 PID: 549 Comm: bash Not tainted 7.0.0-rc2-g730b257ba723-dirty #13 PREEMPT
[  171.665773] Hardware name: BeagleBoard.org BeaglePlay (DT)
[  171.671296] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  171.678306] pc : pvr_fw_trace_mask_set+0x78/0x154 [powervr]
[  171.683959] lr : pvr_fw_trace_mask_set+0x4c/0x154 [powervr]
[  171.689593] sp : ffff8000835ebb90
[  171.692929] x29: ffff8000835ebc00 x28: ffff000005c60f80 x27: 0000000000000000
[  171.700130] x26: 0000000000000000 x25: ffff00000504af28 x24: 0000000000000000
[  171.707324] x23: ffff00000504af50 x22: 0000000000000203 x21: 0000000000000000
[  171.714518] x20: ffff000005c44a80 x19: ffff000005c457b8 x18: 0000000000000000
[  171.721715] x17: 0000000000000000 x16: 0000000000000000 x15: 0000aaaae8887580
[  171.728908] x14: 0000000000000000 x13: 0000000000000000 x12: ffff8000835ebc30
[  171.736095] x11: ffff00000504af2a x10: ffff00008504af29 x9 : 0fffffffffffffff
[  171.743286] x8 : ffff8000835ebbf8 x7 : 0000000000000000 x6 : 000000000000002a
[  171.750479] x5 : ffff00000504af2e x4 : 0000000000000000 x3 : 0000000000000010
[  171.757674] x2 : 0000000000000203 x1 : 0000000000000000 x0 : ffff8000835ebba0
[  171.764871] Call trace:
[  171.767342]  pvr_fw_trace_mask_set+0x78/0x154 [powervr] (P)
[  171.772984]  simple_attr_write_xsigned.isra.0+0xe0/0x19c
[  171.778341]  simple_attr_write+0x18/0x24
[  171.782296]  debugfs_attr_write+0x50/0x98
[  171.786341]  full_proxy_write+0x6c/0xa8
[  171.790208]  vfs_write+0xd4/0x350
[  171.793561]  ksys_write+0x70/0x108
[  171.796995]  __arm64_sys_write+0x1c/0x28
[  171.800952]  invoke_syscall+0x48/0x10c
[  171.804740]  el0_svc_common.constprop.0+0x40/0xe0
[  171.809487]  do_el0_svc+0x1c/0x28
[  171.812834]  el0_svc+0x34/0x108
[  171.816013]  el0t_64_sync_handler+0xa0/0xe4
[  171.820237]  el0t_64_sync+0x198/0x19c
[  171.823939] Code: 32000262 b90ac293 1a931056 9134e293 (b9000036)
[  171.830073] ---[ end trace 0000000000000000 ]---

Fixes: a331631496a0 ("drm/imagination: Simplify module parameters")
Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_fw_trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imagination/pvr_fw_trace.c b/drivers/gpu/drm/imagination/pvr_fw_trace.c
index e154cb35f604..6193811ef7be 100644
--- a/drivers/gpu/drm/imagination/pvr_fw_trace.c
+++ b/drivers/gpu/drm/imagination/pvr_fw_trace.c
@@ -558,6 +558,6 @@ pvr_fw_trace_debugfs_init(struct pvr_device *pvr_dev, struct dentry *dir)
 				    &pvr_fw_trace_fops);
 	}
 
-	debugfs_create_file("trace_mask", 0600, dir, fw_trace,
+	debugfs_create_file("trace_mask", 0600, dir, pvr_dev,
 			    &pvr_fw_trace_mask_fops);
 }

-- 
2.43.0

[PATCH v3 2/2] drm/imagination: Restrict init_fw_trace_mask module param to read only

[Brajesh Gupta]

Param used for setting FW trace mask at module load time. Other debugfs
entry exist to allow update at run time.

Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_fw_trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imagination/pvr_fw_trace.c b/drivers/gpu/drm/imagination/pvr_fw_trace.c
index 6193811ef7be..6bb5baa6c41b 100644
--- a/drivers/gpu/drm/imagination/pvr_fw_trace.c
+++ b/drivers/gpu/drm/imagination/pvr_fw_trace.c
@@ -77,7 +77,7 @@ const struct kernel_param_ops pvr_fw_trace_init_mask_ops = {
 };
 
 param_check_hexint(init_fw_trace_mask, &pvr_fw_trace_init_mask);
-module_param_cb(init_fw_trace_mask, &pvr_fw_trace_init_mask_ops, &pvr_fw_trace_init_mask, 0600);
+module_param_cb(init_fw_trace_mask, &pvr_fw_trace_init_mask_ops, &pvr_fw_trace_init_mask, 0400);
 __MODULE_PARM_TYPE(init_fw_trace_mask, "hexint");
 MODULE_PARM_DESC(init_fw_trace_mask,
 		 "Enable FW trace for the specified groups at device init time");

-- 
2.43.0

Re: [PATCH v3 0/2] drm/imagination: FW trace bug fix and configuration update

[Alessio Belle]

On Mon, 2026-04-27 at 11:01 +0530, Brajesh Gupta wrote:
> Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
> ---
> Changes in v3:
> - Fixed typo in v2 update of commit description.
> - Link to v2: https://lore.kernel.org/r/20260424-ftrace_fix-v2-0-50146ff526d5@imgtec.com
> 
> Changes in v2:
> - Fixed the name of module param in commit description for 2nd patch.
> - Link to v1: https://lore.kernel.org/r/20260417-ftrace_fix-v1-0-9fefe194592a@imgtec.com
> 
> ---
> Brajesh Gupta (2):
>       drm/imagination: Fix segfault when updating ftrace mask
>       drm/imagination: Restrict init_fw_trace_mask module param to read only
> 
>  drivers/gpu/drm/imagination/pvr_fw_trace.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> ---
> base-commit: 3bce3fdd1ff2ba242f76ab66659fff27207299f1
> change-id: 20260417-ftrace_fix-5de47d2448c0
> 
> Best regards,

For the whole serie,

Reviewed-by: Alessio Belle <alessio.belle@imgtec.com>

Thanks,
Alessio

Re: (subset) [PATCH v3 0/2] drm/imagination: FW trace bug fix and configuration update

[Matt Coster]

On Mon, 27 Apr 2026 11:01:36 +0530, Brajesh Gupta wrote:
> 


Applied, thanks!

[1/2] drm/imagination: Fix segfault when updating ftrace mask
      commit: 5dfd429591f8d7185bf63a08b5c30863fb605611

Best regards,
-- 
Matt Coster <matt.coster@imgtec.com>

Re: (subset) [PATCH v3 0/2] drm/imagination: FW trace bug fix and configuration update

[Matt Coster]

On Mon, 27 Apr 2026 11:01:36 +0530, Brajesh Gupta wrote:
> 


Applied, thanks!

[2/2] drm/imagination: Restrict init_fw_trace_mask module param to read only
      commit: 96f865ed44b0d4f7ecadee3855861ef38f25a3cf

Best regards,
-- 
Matt Coster <matt.coster@imgtec.com>

Claude review: drm/imagination: FW trace bug fix and configuration update

[Claude Code Review Bot]

Overall Series Review

Subject: drm/imagination: FW trace bug fix and configuration update
Author: Brajesh Gupta <brajesh.gupta@imgtec.com>
Patches: 6
Reviewed: 2026-04-28T15:07:03.099203

---

This is a clean, well-motivated 2-patch series fixing a real NULL pointer dereference in the PowerVR imagination driver's firmware trace debugfs interface, and a follow-up to restrict the module parameter to read-only.

**Patch 1** fixes a genuine crash: the `debugfs_create_file("trace_mask", ...)` call passed `fw_trace` (a `struct pvr_fw_trace *`) as private data, but the debugfs get/set callbacks (`pvr_fw_trace_mask_get`/`pvr_fw_trace_mask_set`) cast the data pointer to `struct pvr_device *`. This type mismatch causes a NULL pointer dereference when writing to the debugfs file, exactly as shown in the crash trace.

**Patch 2** is a sensible hardening: since `init_fw_trace_mask` is only consumed at device init time and the debugfs `trace_mask` file (now fixed) is the proper runtime interface, making the sysfs parameter read-only prevents misleading writes that would have no effect on the running firmware.

Both patches are correct and ready to merge. No issues found.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imagination: Fix segfault when updating ftrace mask

[Claude Code Review Bot]

Patch Review

**Verdict: Good to merge.**

The fix is correct and minimal. Looking at the current source (`pvr_fw_trace.c`), the debugfs get/set functions both expect `struct pvr_device *`:

```c
static int pvr_fw_trace_mask_get(void *data, u64 *value)
{
	struct pvr_device *pvr_dev = data;
	*value = pvr_dev->fw_dev.fw_trace.group_mask;
	return 0;
}

static int pvr_fw_trace_mask_set(void *data, u64 value)
{
	struct pvr_device *pvr_dev = data;
	...
}
```

But the buggy code passed `fw_trace` (a `struct pvr_fw_trace *`):

```c
-	debugfs_create_file("trace_mask", 0600, dir, fw_trace,
+	debugfs_create_file("trace_mask", 0600, dir, pvr_dev,
 			    &pvr_fw_trace_mask_fops);
```

When `pvr_fw_trace_mask_set` then dereferences `pvr_dev->fw_dev.fw_trace.group_mask` using the mistyped pointer, it reads/writes garbage memory, causing the NULL pointer dereference shown in the crash log. The fix correctly passes `pvr_dev` which is already available as the function parameter.

The `Fixes:` tag (`a331631496a0 "drm/imagination: Simplify module parameters"`) is appropriate — that commit presumably changed the private data from `pvr_dev` to `fw_trace` as part of a refactor, introducing the regression.

The crash trace in the commit message is helpful for backporting and triage. No concerns.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imagination: Restrict init_fw_trace_mask module param to read only

[Claude Code Review Bot]

Patch Review

**Verdict: Good to merge.**

```c
-module_param_cb(init_fw_trace_mask, &pvr_fw_trace_init_mask_ops, &pvr_fw_trace_init_mask, 0600);
+module_param_cb(init_fw_trace_mask, &pvr_fw_trace_init_mask_ops, &pvr_fw_trace_init_mask, 0400);
```

This changes the sysfs permissions from `0600` (root read/write) to `0400` (root read-only). The rationale is sound: `pvr_fw_trace_init_mask` is only consumed once, during `pvr_fw_trace_init()`:

```c
fw_trace->group_mask = pvr_fw_trace_init_mask;
```

Writing to the sysfs parameter at runtime would update the variable but would **not** send a KCCB command to the firmware, so the change would be silently ignored. The debugfs `trace_mask` file (fixed in patch 1) calls `update_logtype()` which properly notifies the firmware. Making the parameter read-only prevents user confusion.

One minor note: the `pvr_fw_trace_init_mask_set` callback in `pvr_fw_trace_init_mask_ops` becomes dead code with `0400` permissions (the `.set` handler will never be called via sysfs). It's still used if someone passes the parameter on the module load command line, so it's not truly unused — but if the authors wanted to be thorough, they could set `.set = NULL` in the ops struct since `module_param_cb` with read-only permissions won't invoke it. This is a very minor observation, not a blocker.

---
Generated by Claude Code Patch Reviewer