Claude review: Re: [PATCH] drm/amdgpu: Fix resource leaks

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

Patch Review

**The problem being fixed is real**: after `ioremap()` at line 4537, multiple 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 = NULL` breaks hot-unplug guard logic**

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

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

This guard prevents `amdgpu_device_fini_hw()` from running on an already-torn-down device during hot-unplug. With the patch applied, `adev->rmmio` is never set to NULL, so this guard never triggers, potentially causing use-after-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 immediately unmap MMIO when the PCI device disappears, so accesses to the now-gone hardware stop immediately. With `devm_ioremap()`, the unmap is deferred to device release time, meaning `adev->rmmio` remains a valid pointer to an ioremap'd region for a physically absent device — the register space behind it is gone. While `no_hw_access` flag guards most register accesses, 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 = NULL;
-		drm_dev_exit(idx);
-	}
```

The `drm_dev_enter()` guard here is deliberate — it ensures the iounmap only happens when the DRM device is still accessible (i.e., NOT hot-unplugged). This two-site cleanup pattern (`amdgpu_device_unmap_mmio` for hot-unplug, `amdgpu_device_fini_sw` for normal shutdown) is intentional and correct. Removing it collapses two distinct lifecycle paths into "let devm handle it," which is the wrong abstraction for a driver with explicit hot-unplug 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_discovery_fini`, `amdgpu_pmu_fini`) may still need the MMIO mapping to be present (or at least deterministically know whether it is).

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

---
Generated by Claude Code Patch Reviewer