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/drm_ras: Add clear-error-counter netlink command to drm_ras Date: Thu, 12 Mar 2026 07:06:12 +1000 Message-ID: In-Reply-To: <20260311102913.3387468-7-riana.tauro@intel.com> References: <20260311102913.3387468-6-riana.tauro@intel.com> <20260311102913.3387468-7-riana.tauro@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review Overall a clean patch. The YAML spec, documentation, and uapi additions look correct. **Concurrency concern in `drm_ras_nl_clear_error_counter_doit`:** ```c node = xa_load(&drm_ras_xa, node_id); if (!node || !node->clear_error_counter) return -ENOENT; ``` The same concern likely exists in the pre-existing `get_error_counter` paths, but there is no locking or RCU protection around the `xa_load` + subsequent use of `node`. If a node is unregistered concurrently, this could use-after-free. This is a pre-existing design issue, but adding a new operation makes it worth mentioning. Consider using `xa_lock`/`xa_unlock` or RCU to protect node lifetime. **Return value for unsupported clear operation:** ```c if (!node || !node->clear_error_counter) return -ENOENT; ``` Returning `-ENOENT` when `clear_error_counter` is NULL (i.e., the node exists but doesn't support clearing) is slightly misleading. `-EOPNOTSUPP` would be more appropriate when the node exists but the operation is not implemented. **YAML anchor reuse is a nice touch:** ```yaml attributes: &id-attrs - node-id - error-id ``` Using `&id-attrs` / `*id-attrs` to avoid duplicating the attribute list is good. --- Generated by Claude Code Patch Reviewer