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/drm_ras: Add per node cleanup action Date: Sat, 16 May 2026 10:24:56 +1000 Message-ID: In-Reply-To: <20260514202839.1888688-4-raag.jadav@intel.com> References: <20260514202839.1888688-1-raag.jadav@intel.com> <20260514202839.1888688-4-raag.jadav@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 **Verdict: Has a bug =E2=80=94 double cleanup on `drmm_add_action_or_reset`= failure.** The overall refactoring from a single bulk cleanup action (`xe_drm_ras_unre= gister_nodes`) to per-node cleanup actions is a sound improvement. It guara= ntees that each successfully registered node is individually cleaned up dur= ing unwind, which solves the problem described. However, this code has a bug: ```c ret =3D drmm_add_action_or_reset(&xe->drm, cleanup_node, node); if (ret) { cleanup_node(&xe->drm, node); return ret; } ``` The `_or_reset` suffix in `drmm_add_action_or_reset` means the action is **= already called on failure** (see `include/drm/drm_managed.h` line 38-39: *"= upon failure @action is directly called for any cleanup work"*). Explicitly= calling `cleanup_node()` again results in: 1. Double call to `drm_ras_node_unregister(node)` =E2=80=94 the second call= operates on an already-unregistered node 2. Double `kfree(node->device_name)` =E2=80=94 harmless because the first c= all NULLs the pointer, but still wrong The fix is simple =E2=80=94 just `return ret`: ```c ret =3D drmm_add_action_or_reset(&xe->drm, cleanup_node, node); if (ret) return ret; ``` **Commit message nit**: The commit message repeats the same description as = patch 2 verbatim ("cleanup_node_param() is not registered in case of counte= r allocation failure, which results in stale memory of previous node..."). = Since patch 2 already fixed the counter allocation issue, patch 3's descrip= tion should focus on what it actually does: moving to per-node drmm cleanup= actions for robust unwind and simplification. --- Generated by Claude Code Patch Reviewer