Claude review: drm: link connectors to backlight devices

[Claude Code Review Bot <claude-review@example.com>]

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)` → `put_device()` which can trigger device cleanup (sleeping)
- `backlight_device_ref(b->link)` → `get_device()` (this is fine)

And in the notifier callback `drm_backlight_notify`, the spinlock is held while iterating and calling `__drm_backlight_link(b, NULL)`, which again calls `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 = b->connector->dev;
+	struct drm_property *prop = dev->mode_config.luminance_property;
+	...
+	if (prop->values[1] != max) {
+		prop->values[0] = max ? 1 : 0;
+		prop->values[1] = max;
+	}
```

The `luminance_property` is a single property object shared by all connectors 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 panels have different `max_brightness`, the last one linked wins. This is a design problem — each connector should have its own property instance, 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 = drm_backlight_get_device(connector->backlight);
+		if (bd) {
+			int ret = sysfs_create_link(...)
```

Between `drm_backlight_get_device` returning and `sysfs_create_link`, the link could be changed and `bd` could be freed. The function should take a reference, 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 = v;
+	if (set >= max)
+		set = max;
+}
```

`set` is computed but never used. The function does nothing except check bounds. It's only completed in patch 4 where `connector->state->luminance = 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 undesirable 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 lockdep 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