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/accel/amdxdna: add debugfs support Date: Mon, 25 May 2026 22:09:05 +1000 Message-ID: In-Reply-To: <20260519203236.252068-1-shofiqtest@gmail.com> References: <20260519203236.252068-1-shofiqtest@gmail.com> <20260519203236.252068-1-shofiqtest@gmail.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 **1. Superseded by existing implementation (fatal)** The current tree already has `drivers/accel/amdxdna/amdxdna_debugfs.c` with= a working debugfs implementation using `debugfs_create_file()`. The patch = attempts to create this file from scratch, which would discard the existing= carveout debugfs support. The existing code also already calls `amdxdna_de= bugfs_init()` from `amdxdna_pci_drv.c` and includes the header. **2. Wrong field access =E2=80=94 `xdna->dev_info->vbnv` does not exist (co= mpilation error)** ```c + seq_printf(s, "vbnv: %s\n", xdna->dev_info->vbnv); ``` `struct amdxdna_dev_info` has `default_vbnv` (at `amdxdna_pci_drv.h:96`), n= ot `vbnv`. The resolved vbnv string is stored directly on the device struct= at `xdna->vbnv` (`amdxdna_pci_drv.h:128`). This would fail to compile. The= existing `amdxdna_sysfs.c` shows the correct access pattern: ```c // amdxdna_sysfs.c:23 return sprintf(buf, "%s\n", xdna->vbnv); ``` **3. Unconditional Makefile addition breaks `!CONFIG_DEBUG_FS` builds** ```diff + amdxdna_debugfs.o \ ``` The patch adds `amdxdna_debugfs.o` unconditionally to `amdxdna-y`. The curr= ent tree correctly uses: ```makefile amdxdna-$(CONFIG_DEBUG_FS) +=3D amdxdna_debugfs.o ``` While the header provides a `static inline` stub when `CONFIG_DEBUG_FS` is = disabled, the `.c` file includes `` and calls `drm_debug= fs_add_files()`, which won't be available in `!CONFIG_DEBUG_FS` builds. The= Makefile should use the conditional form. **4. Uses `drm_debugfs_add_files()` / `drm_debugfs_info` instead of the dri= ver's existing pattern** ```c +static const struct drm_debugfs_info amdxdna_debugfs_list[] =3D { + { "fw_version", fw_version_show, 0 }, + { "device_info", device_info_show, 0 }, + { "clients", clients_show, 0 }, +}; + +void amdxdna_debugfs_init(struct amdxdna_dev *xdna) +{ + drm_debugfs_add_files(&xdna->ddev, amdxdna_debugfs_list, + ARRAY_SIZE(amdxdna_debugfs_list)); +} ``` The existing code uses `debugfs_create_file()` with custom `file_operations= ` (via the `AMDXDNA_DBGFS_FOPS` macro), passing `xdna` as `inode->i_private= `. The DRM helpers approach passes `entry->dev` through `seq_to_xdna()`. If= new entries were to be added, they should follow the existing driver patte= rn for consistency. The DRM helper approach is cleaner for simple read-only= entries, but mixing two different debugfs paradigms in one driver would be= confusing. **5. Copyright year is stale** ```c +/* + * Copyright (C) 2024, Advanced Micro Devices, Inc. + */ ``` The existing file in the tree uses `Copyright (C) 2026`. New code should us= e the current year. **6. Missing NULL check on `xdna->vbnv` (minor, even if field were correcte= d)** The existing `amdxdna_sysfs.c:20` checks `if (!xdna->vbnv)` before printing= . The debugfs `device_info_show` doesn't, which could print `(null)` if cal= led before firmware initialization populates the vbnv string. **Recommendation:** If the intent is to add fw_version, device_info, and cl= ients debugfs entries, this should be done as additions to the existing `am= dxdna_debugfs.c`, following the established `AMDXDNA_DBGFS_FOPS` / `debugfs= _create_file()` pattern, and with the correct field accesses. The patch in = its current form cannot be applied. --- Generated by Claude Code Patch Reviewer