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: gpu: host1x: Fix device reference leak in device_add() error path
Date: Mon, 13 Apr 2026 19:10:40 +1000
Message-ID: <review-patch1-20260412121836.2461556-1-lgs201920130244@gmail.com>
In-Reply-To: <20260412121836.2461556-1-lgs201920130244@gmail.com>
References: <20260412121836.2461556-1-lgs201920130244@gmail.com> <20260412121836.2461556-1-lgs201920130244@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

**Bug identification is correct.** In the original code at `bus.c:511-520` =
(pre-patch), when `device_add()` fails:
```c
if (list_empty(&device->subdevs)) {
    err =3D device_add(&device->dev);
    if (err < 0)
        dev_err(&device->dev, "failed to add device: %d\n", err);
    else
        device->registered =3D true;
}

return 0;
```
The function always returns 0, the device stays on `host1x->devices`, and t=
he reference from `device_initialize()` is never dropped. This is indeed a =
leak.

**Critical bug in the fix =E2=80=94 double list removal:**

The patch adds:
```c
if (err < 0) {
    dev_err(&device->dev, "failed to add device: %d\n", err);
    list_del(&device->list);       /* poisons prev/next */
    put_device(&device->dev);      /* triggers release callback */
    return err;
}
```

`put_device()` drops the last reference, triggering `host1x_device_release(=
)` (set at `bus.c:479`):
```c
static void host1x_device_release(struct device *dev)
{
    struct host1x_device *device =3D to_host1x_device(dev);
    __host1x_device_del(device);
    kfree(device);
}
```

And `__host1x_device_del()` at line 443 does:
```c
list_del_init(&device->list);
```

Since `list_del()` already poisoned the pointers with `LIST_POISON1`/`LIST_=
POISON2`, this second removal dereferences those sentinel addresses. Withou=
t `CONFIG_DEBUG_LIST`, `__list_del_entry_valid()` returns `true` unconditio=
nally and the kernel will oops trying to write through poisoned pointers.

**Fix:** Remove the `list_del(&device->list)` line entirely. The `put_devic=
e()` =E2=86=92 `host1x_device_release` =E2=86=92 `__host1x_device_del` path=
 already handles list removal at line 443. So the error handling should be =
simply:
```c
if (err < 0) {
    dev_err(&device->dev, "failed to add device: %d\n", err);
    put_device(&device->dev);
    return err;
}
```

Alternatively, if the author wants to keep the explicit `list_del`, use `li=
st_del_init()` instead so the entry points to itself and the subsequent `li=
st_del_init()` in `__host1x_device_del` is a harmless no-op. But this is le=
ss clean since it duplicates logic already in the release path.

**Minor style issue:** The patch produces:
```c
    }
    else
        device->registered =3D true;
```
Per kernel coding style (Chapter 3), when the `if` branch uses braces, the =
`else` should be on the same line as the closing brace and should also use =
braces:
```c
    } else {
        device->registered =3D true;
    }
```
The original code had no braces on either branch so it was consistent; now =
it's mixed.

**Pre-existing issue (not this patch's fault):** The `host1x_device_parse_d=
t()` error path at line 487-489 uses `kfree(device)` directly after `device=
_initialize()`, which is technically incorrect (should use `put_device()`).=
 However, it happens to be safe because the device isn't on any list at tha=
t point, and the release function's `list_del_init` on a self-pointing entr=
y is benign. Mentioning this only for completeness =E2=80=94 it shouldn't b=
e addressed in this patch.

---
Generated by Claude Code Patch Reviewer