From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/amdgpu: amdgpu{_reset}.h: fix all kernel-doc warnings
Date: Tue, 28 Apr 2026 14:07:10 +1000
Message-ID: <review-patch1-20260427183854.672967-1-rdunlap@infradead.org>
In-Reply-To: <20260427183854.672967-1-rdunlap@infradead.org>
References: <20260427183854.672967-1-rdunlap@infradead.org> <20260427183854.672967-1-rdunlap@infradead.org>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Kernel-doc fixes (correct):**

The `struct` keyword fix is straightforward and correct:
```c
- * amdgpu_wb - This struct is used for small GPU memory allocation.
+ * struct amdgpu_wb - This struct is used for small GPU memory allocation.
```

The enum doc tag renames are correct =E2=80=94 the old doc used short names=
 like `@AMD_RESET_LEGACY` while the actual enum values are `AMD_RESET_METHO=
D_LEGACY`. The patch also adds the previously undocumented `@AMD_RESET_METH=
OD_ON_INIT` entry:
```c
+ * @AMD_RESET_METHOD_ON_INIT: Does a device reset during the driver init
+ *                   sequence.
```

**Potential build breakage with `#include` placement:**

In the drm-next tree, `struct amdgpu_asic_funcs` at `amdgpu.h:610` uses the=
 enum:
```c
enum amd_reset_method (*reset_method)(struct amdgpu_device *adev);
```

The patch removes the enum from its original position (around line 567) and=
 adds `#include "amdgpu_reset.h"` much later in the file, after the RBIOS m=
acros (~line 1375):
```c
 #define RBIOS32(i) ((RBIOS16(i)) | (RBIOS16((i)+2) << 16))

+#include "amdgpu_reset.h"
```

Since C does not support forward declarations of enums, `struct amdgpu_asic=
_funcs` at line 610 would reference an undefined type =E2=80=94 the enum de=
finition (now in `amdgpu_reset.h`) isn't available until ~line 1375. This s=
hould be a compile error. The patch might build against the author's linux-=
next tree if `struct amdgpu_asic_funcs` was modified there, but it will not=
 work against drm-next as-is.

If the `#include` must stay in `amdgpu.h`, it should be placed **before** `=
struct amdgpu_asic_funcs`, ideally near the top of the file with the other =
local includes (lines 59=E2=80=93101). The v2 changelog says "The simpler m=
ethod causes build errors" =E2=80=94 likely because `amdgpu_reset.h` alread=
y `#include`s `amdgpu.h` (line 27), creating a circular dependency. This is=
 the root problem: fixing it properly would require breaking the circular i=
nclude between `amdgpu.h` and `amdgpu_reset.h`, not burying the include dee=
p in the file to sidestep ordering.

**Circular include:**

Adding `#include "amdgpu_reset.h"` to `amdgpu.h` while `amdgpu_reset.h` alr=
eady has `#include "amdgpu.h"` creates a circular dependency. The include g=
uards prevent infinite recursion, but the approach is fragile =E2=80=94 cor=
rectness depends on which file is included first and exactly where in the f=
ile each cross-include appears. This should at minimum be acknowledged in t=
he commit message.

**Unexplained struct reordering:**

The patch moves `struct amdgpu_reset_control` ahead of `struct amdgpu_reset=
_handler` in `amdgpu_reset.h`. This change is not mentioned in the commit m=
essage and is not related to fixing kernel-doc warnings. Both structs refer=
ence each other only via pointers, so the reorder is safe in practice, but =
unexplained changes should not be mixed in with doc fixes. If there is a re=
ason (e.g., putting it closer to the enum it uses), it should be stated.

**Minor: preserved typo in documentation:**

The moved doc block preserves a minor grammatical error from the original:
```c
+ * @AMD_RESET_METHOD_MODE0: Reset the entire ASIC. Not currently available=
 for
+ *                          the any device.
```
Should be "for any device" (drop "the"). Worth fixing while touching this b=
lock.

**Summary:** The kernel-doc tag fixes are correct and welcome, but the incl=
ude placement likely breaks the build against drm-next, and the circular in=
clude / struct reordering need to be addressed or at least justified. I'd s=
uggest either moving the `#include` to before the first use of the enum (an=
d dealing with the resulting circular include issues properly) or using a f=
orward declaration approach with a separate header for just the enum.

---
Generated by Claude Code Patch Reviewer