From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/imagination: Restrict init_fw_trace_mask module param to read only Date: Tue, 28 Apr 2026 15:07:03 +1000 Message-ID: In-Reply-To: <20260427-ftrace_fix-v3-2-e081530759a8@imgtec.com> References: <20260427-ftrace_fix-v3-0-e081530759a8@imgtec.com> <20260427-ftrace_fix-v3-2-e081530759a8@imgtec.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Verdict: Good to merge.** ```c -module_param_cb(init_fw_trace_mask, &pvr_fw_trace_init_mask_ops, &pvr_fw_t= race_init_mask, 0600); +module_param_cb(init_fw_trace_mask, &pvr_fw_trace_init_mask_ops, &pvr_fw_t= race_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 =3D pvr_fw_trace_init_mask; ``` Writing to the sysfs parameter at runtime would update the variable but wou= ld **not** send a KCCB command to the firmware, so the change would be sile= ntly ignored. The debugfs `trace_mask` file (fixed in patch 1) calls `updat= e_logtype()` which properly notifies the firmware. Making the parameter rea= d-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` handle= r will never be called via sysfs). It's still used if someone passes the pa= rameter on the module load command line, so it's not truly unused =E2=80=94= but if the authors wanted to be thorough, they could set `.set =3D NULL` i= n the ops struct since `module_param_cb` with read-only permissions won't i= nvoke it. This is a very minor observation, not a blocker. --- Generated by Claude Code Patch Reviewer