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: link connectors to backlight devices
Date: Thu, 04 Jun 2026 14:54:30 +1000
Message-ID: <review-patch3-20260531114908.1693426-4-superm1@kernel.org>
In-Reply-To: <20260531114908.1693426-4-superm1@kernel.org>
References: <20260531114908.1693426-1-superm1@kernel.org> <20260531114908.1693426-4-superm1@kernel.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

This is the core patch and has the most issues.

**Bug: Spinlock used in sleeping context**

`drm_backlight_lock` is defined as:
```c
+static DEFINE_SPINLOCK(drm_backlight_lock);
```

But `__drm_backlight_link` is called under this spinlock, and it calls:
- `backlight_device_unref(b->link)` =E2=86=92 `put_device()` which can trig=
ger device cleanup (sleeping)
- `backlight_device_ref(b->link)` =E2=86=92 `get_device()` (this is fine)

And in the notifier callback `drm_backlight_notify`, the spinlock is held w=
hile iterating and calling `__drm_backlight_link(b, NULL)`, which again cal=
ls `put_device`.

This should be a mutex, not a spinlock, or the ref/unref should be deferred=
 outside the lock.

**Bug: Global property range shared across connectors**

```c
+static void __drm_backlight_update_prop_range(struct drm_backlight *b)
+{
+	struct drm_device *dev =3D b->connector->dev;
+	struct drm_property *prop =3D dev->mode_config.luminance_property;
+	...
+	if (prop->values[1] !=3D max) {
+		prop->values[0] =3D max ? 1 : 0;
+		prop->values[1] =3D max;
+	}
```

The `luminance_property` is a single property object shared by all connecto=
rs on the device. Modifying `prop->values[0]` and `prop->values[1]` affects=
 the range for ALL connectors, not just the one being linked. If two eDP pa=
nels have different `max_brightness`, the last one linked wins. This is a d=
esign problem =E2=80=94 each connector should have its own property instanc=
e, or a per-connector range mechanism is needed.

**Bug: drm_backlight_get_device returns pointer without refcount**

```c
+struct backlight_device *drm_backlight_get_device(struct drm_backlight *b)
+{
+	if (!b)
+		return NULL;
+
+	guard(spinlock)(&drm_backlight_lock);
+	return b->link;
+}
```

The pointer is returned after the spinlock is released. The caller in `drm_=
sysfs_connector_add_late` uses it to create a sysfs link:
```c
+		struct backlight_device *bd =3D drm_backlight_get_device(connector->back=
light);
+		if (bd) {
+			int ret =3D sysfs_create_link(...)
```

Between `drm_backlight_get_device` returning and `sysfs_create_link`, the l=
ink could be changed and `bd` could be freed. The function should take a re=
ference, or the caller should hold a lock.

**Issue: `__drm_backlight_real_changed` is a no-op in this patch**

```c
+static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t=
 v)
+{
+	unsigned int max, set;
+	...
+	set =3D v;
+	if (set >=3D max)
+		set =3D max;
+}
```

`set` is computed but never used. The function does nothing except check bo=
unds. It's only completed in patch 4 where `connector->state->luminance =3D=
 set` is added. This makes the intermediate state confusing and means patch=
 3 alone doesn't actually do anything useful with brightness values.

**Issue: select BACKLIGHT_CLASS_DEVICE unconditionally**

```c
+	select BACKLIGHT_CLASS_DEVICE
```

This forces all DRM builds to pull in backlight support. This may be undesi=
rable for embedded/server builds that never use backlight. Consider making =
it conditional or using `depends on`.

**Issue: WARN_ON outside spinlock in drm_backlight_unregister**

```c
+void drm_backlight_unregister(struct drm_backlight *b)
+{
+	...
+	WARN_ON(!__drm_backlight_is_registered(b));
+
+	scoped_guard(spinlock, &drm_backlight_lock) {
```

`__drm_backlight_is_registered` asserts `lockdep_assert_held(&drm_backlight=
_lock)`, but it's called outside the spinlock guard. This will trigger a lo=
ckdep warning. Similarly in `drm_backlight_register`:

```c
+	WARN_ON(__drm_backlight_is_registered(b));
+	guard(spinlock)(&drm_backlight_lock);
```

---
Generated by Claude Code Patch Reviewer