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/xe/uapi: Define drm_xe_vm_get_property Date: Wed, 11 Mar 2026 13:12:59 +1000 Message-ID: In-Reply-To: <20260310144914.7525-8-jonathan.cavitt@intel.com> References: <20260310144914.7525-6-jonathan.cavitt@intel.com> <20260310144914.7525-8-jonathan.cavitt@intel.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 **uAPI naming concern: `struct xe_vm_fault` lacks `drm_xe_` prefix.** All o= ther xe uAPI structs use `drm_xe_` prefix (e.g., `drm_xe_vm_bind`, `drm_xe_= vm_get_property`). The `struct xe_vm_fault` should be `struct drm_xe_vm_fau= lt` for consistency. Same for the `FAULT_*` defines =E2=80=94 they are very= generic namespace polluters and should have a `DRM_XE_` or similar prefix = (e.g., `DRM_XE_FAULT_ACCESS_TYPE_READ`). **Struct padding/alignment concern.** In `struct xe_vm_fault`: ```c __u64 address; __u32 address_precision; __u8 access_type; __u8 fault_type; __u8 fault_level; __u8 pad; __u64 reserved[4]; ``` The layout is fine from a padding perspective (4+1+1+1+1 =3D 8, aligns to t= he next `__u64`). Total size is 48 bytes, which is reasonable. **The `data`/`value` union semantics are unusual.** The documentation says = if `size` is 0, the driver fills in the required size. But there's also a `= value` field for "scalar queries" =E2=80=94 yet no scalar property is defin= ed. This adds complexity for a hypothetical future use case. Not a blocker,= but worth questioning whether the union is needed in v1 of this uAPI. **Missing `pad` validation.** The ioctl handler in patch 4 checks `reserved= [0..2]` but does not check `args->pad`. For uAPI, MBZ fields should be vali= dated to ensure forward compatibility. --- Generated by Claude Code Patch Reviewer