Claude review: accel/amdxdna: Expose per-client BO memory usage via fdinfo

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Positive aspects:**
- Follows the same pattern as panthor's `panthor_show_internal_memory_stats` (even uses the same `char *drv_name = file->minor->dev->driver->name` idiom).
- Correctly locks `mm_lock` while reading the usage counters and unlocks before printing.
- Documentation addition references the standard DRM client usage stats spec.

**Issues:**

1. **Potential unsigned underflow in external_usage calculation (medium severity):**

```c
external_usage = client->total_bo_usage - internal_usage;
```

Both `total_bo_usage` and `internal_usage` are `size_t` (unsigned). If there's ever a bug or race where `total_int_bo_usage > total_bo_usage`, this silently wraps to a huge value and prints nonsense in fdinfo. While the lock should prevent races, a defensive check or at minimum a `WARN_ON` would be appropriate:

```c
if (WARN_ON(client->total_bo_usage < client->total_int_bo_usage))
    external_usage = 0;
else
    external_usage = client->total_bo_usage - internal_usage;
```

2. **Type width mismatch: `size_t` passed where `u64` expected (minor):**

`drm_fdinfo_print_size()` takes `u64 sz` as its last parameter, but `heap_usage`, `internal_usage`, and `external_usage` are all `size_t`. On 32-bit architectures `size_t` is 32-bit while `u64` is 64-bit. This is not incorrect (implicit promotion), but it does mean the values are silently truncated to 32 bits on 32-bit builds. This likely doesn't matter for this driver (NPU hardware probably doesn't target 32-bit), but explicitly casting to `u64` or declaring the local variables as `u64` would be cleaner. Note that panthor avoids this by passing through `struct drm_memory_stats` which uses `u64` fields.

3. **Blank line removal in `drm_driver` struct (nit):**

```c
 	.num_ioctls = ARRAY_SIZE(amdxdna_drm_ioctls),
-
+	.show_fdinfo = amdxdna_show_fdinfo,
 	.gem_create_object = amdxdna_gem_create_shmem_object_cb,
```

The original code had a blank line separating the ioctl-related fields from the gem-related fields. The patch removes that visual grouping to insert `.show_fdinfo`. It would be better to keep the blank line and place `.show_fdinfo` either before the blank line (with the other callbacks like `.open`/`.postclose`) or after a new blank line:

```c
	.num_ioctls = ARRAY_SIZE(amdxdna_drm_ioctls),
	.show_fdinfo = amdxdna_show_fdinfo,

	.gem_create_object = amdxdna_gem_create_shmem_object_cb,
```

4. **Documentation: driver name prefix is rather long and verbose (observation):**

The fdinfo keys use the full driver name as prefix:
```
amdxdna_accel_driver-heap-alloc:    60 KiB
amdxdna_accel_driver-internal-alloc:    67588 KiB
amdxdna_accel_driver-external-alloc:    0
```

This is correct per the `drm_fdinfo_print_size` convention (it uses `drm_driver.name` as the prefix), but the driver name `amdxdna_accel_driver` is unusually long compared to other drivers. This is arguably an existing issue with the driver name choice rather than a problem with this patch, but it's worth noting for readability of fdinfo output. Tools like `gputop` may need to handle this.

5. **Documentation example shows `0` without units for zero external alloc (observation):**

```
amdxdna_accel_driver-external-alloc:	0
```

This is actually correct behavior — `drm_fdinfo_print_size` prints `0` without units when the value is zero (the loop breaks at `u=0` when `sz == 0`). Just noting this is intentional and consistent with the implementation.

**Overall: The patch is reasonable and well-structured. The underflow concern (issue 1) is the most important to address; the rest are minor.**

---
Generated by Claude Code Patch Reviewer