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/loongson: clean up KMS polling on probe failure
Date: Sat, 16 May 2026 14:50:22 +1000
Message-ID: <review-patch1-20260511170152.16957-1-mhun512@gmail.com>
In-Reply-To: <20260511170152.16957-1-mhun512@gmail.com>
References: <20260511170152.16957-1-mhun512@gmail.com> <20260511170152.16957-1-mhun512@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

**The bug is real, the fix is suboptimal.**

The commit message correctly identifies that `drm_kms_helper_poll_init()` a=
t line 296 of `lsdc_drv.c` starts a polling timer that is never cleaned up =
on failure or removal. However:

1. **Prefer `drmm_kms_helper_poll_init()` instead.** The driver already use=
s managed DRM resources (`devm_drm_dev_alloc`, `drmm_mode_config_init`, `dr=
mm_add_action_or_reset` for TTM/PLL/I2C). The idiomatic fix is a one-liner =
=E2=80=94 change:

```c
drm_kms_helper_poll_init(ddev);
```
to:
```c
drmm_kms_helper_poll_init(ddev);
```

This automatically handles cleanup on both error paths and device removal, =
with no `goto` labels or manual `poll_fini` calls needed. The managed varia=
nt exists specifically for this purpose (see `drm_probe_helper.c:963`).

2. **Remove-path ordering is wrong.** The patch adds:
```c
+	drm_kms_helper_poll_fini(ddev);
 	drm_dev_unregister(ddev);
```
Calling `poll_fini` before `drm_dev_unregister` means the polling work is c=
ancelled while the device is still registered with userspace. The conventio=
nal order (used by verisilicon and others) is `drm_dev_unregister` first, t=
hen `drm_kms_helper_poll_fini`. With the `drmm_` approach this ordering que=
stion goes away entirely.

3. **The `err_poll_fini` label itself is fine** in terms of correctness for=
 the error paths =E2=80=94 all three failure points (vblank init, IRQ reque=
st, dev register) happen after `poll_init`, so jumping to the cleanup label=
 is correct. But again, this whole error-path dance is unnecessary with the=
 managed API.

**Recommendation:** Request a v2 that simply switches from `drm_kms_helper_=
poll_init()` to `drmm_kms_helper_poll_init()` and drops the `err_poll_fini`=
 label and the explicit `poll_fini` in remove. This is a smaller, cleaner, =
and more correct fix.

---
Generated by Claude Code Patch Reviewer