From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH] drm/panthor: extend timestamp query with flags Date: Sun, 22 Mar 2026 04:32:33 +1000 Message-ID: In-Reply-To: References: <20260318112952.645160-1-marcin.slusarz@arm.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 This is the current version. Review follows: **1. Typo in UAPI header** ```c /** @cpu_timestamp_nsec: Nanseconds part of CPU timestamp. */ ``` Should be "Nanoseconds". **2. `duration_nsec` is `__u32` but assigned from `u64` subtraction** ```c if (flags & DRM_PANTHOR_TIMESTAMP_DURATION) arg->duration_nsec =3D local_clock() - query_start_time; ``` `local_clock()` returns `u64`, so the subtraction result is `u64` and gets = silently truncated to `__u32`. While a u32 in nanoseconds (~4.2 seconds) is= more than enough for a few MMIO reads, the truncation should be documented= in the UAPI header comment for `duration_nsec`, or alternatively consider = making it `__u64` for future-proofing since there's space (the struct has a= `__u64 cpu_timestamp_nsec` right after, so alignment works either way). **3. `gpu_read64_counter` in an IRQ-disabled, preempt-disabled section cont= ains a retry loop** ```c static inline u64 gpu_read64_counter(struct panthor_device *ptdev, u32 reg) { u32 lo, hi1, hi2; do { hi1 =3D gpu_read(ptdev, reg + 4); lo =3D gpu_read(ptdev, reg); hi2 =3D gpu_read(ptdev, reg + 4); } while (hi1 !=3D hi2); return lo | ((u64)hi2 << 32); } ``` The patch calls this up to twice (once for `GPU_TIMESTAMP`, once for `GPU_C= YCLE_COUNT`) inside `local_irq_save`/`preempt_disable`. The retry loop is t= heoretically unbounded. In practice the high word changes extremely rarely,= so this is very unlikely to be an issue, but worth noting. **4. Ordering: `gpu_read64` for `GPU_TIMESTAMP_OFFSET` is done outside the = critical section** ```c if (flags & DRM_PANTHOR_TIMESTAMP_GPU_OFFSET) arg->timestamp_offset =3D gpu_read64(ptdev, GPU_TIMESTAMP_OFFSET); if (minimize_interruption) { preempt_disable(); local_irq_save(irq_flags); } ``` The `GPU_TIMESTAMP_OFFSET` read happens before disabling preemption/IRQs. I= f userspace requests both `GPU_OFFSET` and `GPU` timestamp in the same quer= y, the offset and the timestamp might not be perfectly correlated. This is = probably fine since the offset is presumably static or slow-changing, but t= he design rationale should be documented. **5. `timens_add_monotonic` called for `MONOTONIC_RAW` =E2=80=94 correct bu= t potentially surprising** ```c if (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) { timens_add_monotonic(&cpu_ts); ... } ``` This applies the time namespace monotonic offset to both `CLOCK_MONOTONIC` = and `CLOCK_MONOTONIC_RAW` timestamps. This matches the kernel's own `posix_= get_monotonic_raw()` behavior in `kernel/time/posix-timers.c`, so it's corr= ect. However, a brief comment here would help readers understand this is in= tentional and follows kernel convention. **6. Missing `PANTHOR_UOBJ_MIN_SIZE` update consideration** The `PANTHOR_UOBJ_MIN_SIZE` for `drm_panthor_timestamp_info` remains anchor= ed at `current_timestamp` (line 175 of the existing code). Since the struct= is extended with new fields after `timestamp_offset`, old userspace that p= asses `size =3D 24` (the old struct size) will: - `copy_struct_from_user`: copy 24 bytes, zero-fill the rest =E2=86=92 `fla= gs =3D=3D 0` =E2=86=92 legacy path. Correct. - `PANTHOR_UOBJ_SET`: copy back `min(24, new_size) =3D 24` bytes. Correct. This works, but the `PANTHOR_UOBJ_MIN_SIZE` min-size is currently set to en= d at `current_timestamp` (offset 16), not `timestamp_offset` (offset 24). T= his means userspace could theoretically pass a 16-byte struct and get no `t= imestamp_offset` back. This was already the case before this patch, so not = a regression =E2=80=94 just noting the pre-existing behavior. **7. Multi-line comment style** ```c /* If user asked to obtain timestamps from more than one source, * then it very likely means they want them to be as close as possible. ``` Kernel comment style for multi-line comments prefers: ```c /* * If user asked to obtain timestamps from more than one source, * then it very likely means they want them to be as close as possible. ``` **8. `ktime_get_ts64` / `ktime_get_raw_ts64` called with IRQs disabled** These functions are safe to call with IRQs disabled (they use seqcount-base= d lockless reads), so this is fine. **9. No `default` case in the CPU type switch inside the critical section** ```c switch (flags & DRM_PANTHOR_TIMESTAMP_CPU_TYPE_MASK) { case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC: ktime_get_ts64(&cpu_ts); break; case DRM_PANTHOR_TIMESTAMP_CPU_MONOTONIC_RAW: ktime_get_raw_ts64(&cpu_ts); break; default: break; } ``` v3 correctly added `default: break;` here (v2 was missing it). Good. **10. No validation that args->size is large enough for copy_struct_from_us= er before powering up GPU** In the current flow, `copy_struct_from_user` succeeds even for very small `= args->size` (zero-fills the rest), then `panthor_query_timestamp_info` powe= rs up the GPU and does MMIO reads, and only then `PANTHOR_UOBJ_SET` would r= eject the small `usr_size`. This matches the pre-existing behavior (the old= code also powered up the GPU before the size check in PANTHOR_UOBJ_SET), s= o it's not a regression. Minor optimization opportunity for future work. **Summary of actionable items:** 1. Fix "Nanseconds" =E2=86=92 "Nanoseconds" typo 2. Consider making `duration_nsec` `__u64` or document truncation behavior 3. Fix multi-line comment style to match kernel convention 4. Add a brief comment explaining `timens_add_monotonic` applies to both cl= ock types --- Generated by Claude Code Patch Reviewer