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: Re: [PATCH] drm/amdgpu: Fix resource leaks
Date: Fri, 27 Feb 2026 14:17:10 +1000
Message-ID: <review-patch1-202602251459.1jr0PuUf-lkp@intel.com>
In-Reply-To: <202602251459.1jr0PuUf-lkp@intel.com>
References: <202602251459.1jr0PuUf-lkp@intel.com> <202602251459.1jr0PuUf-lkp@intel.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 problem being fixed is real**: after `ioremap()` at line 4537, multip=
le error paths in `amdgpu_device_init()` return without calling `iounmap()`=
, and none of them go through `amdgpu_device_fini_sw()`. The Smatch warning=
 is legitimate.

However, using `devm_ioremap()` is the wrong fix here for several reasons:

**1. Removal of `adev->rmmio =3D NULL` breaks hot-unplug guard logic**

In `amdgpu_device_unmap_mmio()`, the patch removes:
```c
-	iounmap(adev->rmmio);
-	adev->rmmio =3D NULL;
```

The `adev->rmmio =3D NULL` assignment is critical because it's checked in `=
amdgpu_driver_unload_kms()` (`amdgpu_kms.c:91`):
```c
if (adev->rmmio =3D=3D NULL)
    return;
```

This guard prevents `amdgpu_device_fini_hw()` from running on an already-to=
rn-down device during hot-unplug. With the patch applied, `adev->rmmio` is =
never set to NULL, so this guard never triggers, potentially causing use-af=
ter-free or double-teardown issues during PCI hot-unplug.

**2. Lifetime mismatch with hot-unplug path**

`amdgpu_device_unmap_mmio()` is called from `amdgpu_device_fini_hw()` when =
`pci_dev_is_disconnected()` is true (line 4937-4938). This is designed to i=
mmediately unmap MMIO when the PCI device disappears, so accesses to the no=
w-gone hardware stop immediately. With `devm_ioremap()`, the unmap is defer=
red to device release time, meaning `adev->rmmio` remains a valid pointer t=
o an ioremap'd region for a physically absent device =E2=80=94 the register=
 space behind it is gone. While `no_hw_access` flag guards most register ac=
cesses, the explicit unmap provided a second safety net.

**3. Removal of `drm_dev_enter()` guarded cleanup in `amdgpu_device_fini_sw=
()` is wrong**

The patch removes:
```c
-	if (drm_dev_enter(adev_to_drm(adev), &idx)) {
-
-		iounmap(adev->rmmio);
-		adev->rmmio =3D NULL;
-		drm_dev_exit(idx);
-	}
```

The `drm_dev_enter()` guard here is deliberate =E2=80=94 it ensures the iou=
nmap only happens when the DRM device is still accessible (i.e., NOT hot-un=
plugged). This two-site cleanup pattern (`amdgpu_device_unmap_mmio` for hot=
-unplug, `amdgpu_device_fini_sw` for normal shutdown) is intentional and co=
rrect. Removing it collapses two distinct lifecycle paths into "let devm ha=
ndle it," which is the wrong abstraction for a driver with explicit hot-unp=
lug support.

**4. devm cleanup ordering may conflict with other teardown**

`devm_ioremap()` resources are freed when the `struct device` is released. =
With `adev` allocated via `devm_drm_dev_alloc()`, the ordering between DRM =
device release and PCI device release can be subtle. Code in `amdgpu_device=
_fini_sw()` that runs *after* the removed iounmap block (e.g., `amdgpu_disc=
overy_fini`, `amdgpu_pmu_fini`) may still need the MMIO mapping to be prese=
nt (or at least deterministically know whether it is).

**Suggested alternative fix**: Instead of converting to `devm_ioremap()`, f=
ix the actual error paths in `amdgpu_device_init()` by adding `iounmap(adev=
->rmmio)` before the failing returns after the ioremap, or consolidating th=
e error cleanup through a goto label that performs the unmap. This is the m=
ore targeted and lower-risk fix for the Smatch warning.

---
Generated by Claude Code Patch Reviewer