[PATCH AUTOSEL 6.19] spi: cadence-qspi: Fix probe error path and remove

[Sasha Levin <sashal@kernel.org>]

From: "Miquel Raynal (Schneider Electric)" <miquel.raynal@bootlin.com>

[ Upstream commit f18c8cfa4f1af2cf7d68d86989a7d6109acfa1bb ]

The probe has been modified by many different users, it is hard to track
history, but for sure its current state is partially broken. One easy
rule to follow is to drop/free/release the resources in the opposite
order they have been queried.

Fix the labels, the order for freeing the resources, and add the
missing DMA channel step. Replicate these changes in the remove path as
well.

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
Tested-by: Santhosh Kumar K <s-k6@ti.com>
Link: https://patch.msgid.link/20260122-schneider-6-19-rc1-qspi-v4-8-f9c21419a3e6@bootlin.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Analysis of spi: cadence-qspi: Fix probe error path and remove

### 1. COMMIT MESSAGE ANALYSIS

The commit message explicitly says "Fix probe error path and remove" —
this is a bug fix. The author clearly states:
- "its current state is partially broken"
- Resources were not being freed in the correct order
- DMA channel release was missing from error paths
- The remove path also had ordering issues

The commit has strong trust indicators:
- **Two "Tested-by:" tags** (Wolfram Sang and Santhosh Kumar K) — both
  are well-known kernel contributors
- Accepted by Mark Brown (SPI subsystem maintainer)
- Part of a versioned series (v4), indicating careful review iterations

### 2. CODE CHANGE ANALYSIS

Let me trace through the specific fixes:

#### A. Probe Error Path Fixes

**Missing DMA channel release:** The old code had `probe_setup_failed`
label that jumped directly to disabling the controller and runtime PM,
but **never released `cqspi->rx_chan`**. The new code adds a dedicated
`release_dma_chan` label:
```c
release_dma_chan:
    if (cqspi->rx_chan)
        dma_release_channel(cqspi->rx_chan);
```
This fixes a **resource leak** — if `spi_register_controller()` fails
after DMA channels are requested, they would never be freed.

**Incorrect error label targets:** Several error paths were jumping to
`probe_reset_failed` which would attempt to disable clocks/reset that
hadn't been enabled yet, or skip cleanup that was needed. The new labels
(`disable_rpm`, `disable_clk`, `disable_clks`, `disable_controller`,
`release_dma_chan`) provide proper granularity matching the actual
resources acquired at each point.

**Reordering of runtime PM disable:** The old code disabled runtime PM
in `probe_setup_failed` (early in cleanup), but the new code moves
`pm_runtime_disable()` to `disable_rpm` (the last cleanup step before
return). This is correct because PM was enabled relatively late in the
probe sequence and should be disabled early in the reverse-order
cleanup.

#### B. Remove Path Fixes

**DMA channel release added before controller disable:** The old remove
path had:
1. Disable controller
2. Release DMA channel

The new order:
1. Release DMA channel
2. Disable controller

This is the correct reverse order — DMA was requested after controller
init, so it should be released before controller disable.

**Clock disable ordering fixed:** The old code disabled clocks before
JH7110-specific clock cleanup. The new code reverses this to match probe
order.

### 3. BUG CLASSIFICATION

This fixes multiple real bugs:
1. **Resource leak**: Missing `dma_release_channel()` in probe error
   path — DMA channels leaked on probe failure
2. **Incorrect cleanup ordering**: Resources freed in wrong order could
   cause use-after-free or access to already-disabled hardware
3. **Wrong goto targets**: Error paths jumping to labels that free
   resources not yet acquired (wasteful but potentially harmful) or that
   skip freeing resources that were acquired (leak)

### 4. SCOPE AND RISK ASSESSMENT

- **Files changed**: 1 file (`drivers/spi/spi-cadence-quadspi.c`)
- **Nature of changes**: Primarily label renaming and reordering of
  cleanup steps — no logic changes to the normal (non-error) path
- **Risk**: LOW. The normal successful probe/remove paths are minimally
  affected. The changes only affect error handling and teardown
  ordering.
- **Subsystem**: SPI driver (Cadence QSPI) — widely used on ARM/embedded
  platforms (TI, Intel, StarFive, Renesas, etc.)

### 5. USER IMPACT

The Cadence QSPI controller is used on many SoCs. Users affected
include:
- Anyone whose probe fails for any reason (missing firmware, DT
  misconfiguration, resource conflicts) — they'd leak DMA channels
- Anyone unloading/reloading the module — remove path had ordering bugs
- Embedded systems that are sensitive to resource leaks

### 6. STABLE SUITABILITY

- **Obviously correct**: Yes — reverse-order cleanup is a well-
  established pattern, and the missing DMA release is clearly a bug
- **Fixes a real bug**: Yes — resource leaks and incorrect cleanup
  ordering
- **Small and contained**: Yes — single file, ~80 lines changed, mostly
  label renaming
- **No new features**: Correct — purely error handling fixes
- **Tested**: Yes — two Tested-by tags from reputable contributors
- **Clean application**: The patch modifies only one file with
  straightforward changes. May need minor adjustment depending on the
  stable version but should be relatively clean.

### 7. DEPENDENCY CHECK

This appears to be self-contained. The cleanup labels and DMA channel
handling are local to this file. The `cqspi_jh7110_disable_clk()`
function and `cqspi_request_mmap_dma()` need to exist in the stable
tree, which they should for any kernel version that has this driver with
JH7110 support and DMA support.

### Conclusion

This commit fixes genuine resource leaks (DMA channels) and incorrect
cleanup ordering in both the probe error path and remove function of a
widely-used SPI controller driver. It's small, well-tested, obviously
correct (follows reverse-order cleanup pattern), and has zero risk to
the normal operation path. It meets all stable kernel criteria.

**YES**

 drivers/spi/spi-cadence-quadspi.c | 44 ++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 597a04dbbe2e7..b1c95b1c343fc 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1891,7 +1891,7 @@ static int cqspi_probe(struct platform_device *pdev)
 	ret = clk_prepare_enable(cqspi->clk);
 	if (ret) {
 		dev_err(dev, "Cannot enable QSPI clock.\n");
-		goto probe_clk_failed;
+		goto disable_rpm;
 	}
 
 	/* Obtain QSPI reset control */
@@ -1899,14 +1899,14 @@ static int cqspi_probe(struct platform_device *pdev)
 	if (IS_ERR(rstc)) {
 		ret = PTR_ERR(rstc);
 		dev_err(dev, "Cannot get QSPI reset.\n");
-		goto probe_reset_failed;
+		goto disable_clk;
 	}
 
 	rstc_ocp = devm_reset_control_get_optional_exclusive(dev, "qspi-ocp");
 	if (IS_ERR(rstc_ocp)) {
 		ret = PTR_ERR(rstc_ocp);
 		dev_err(dev, "Cannot get QSPI OCP reset.\n");
-		goto probe_reset_failed;
+		goto disable_clk;
 	}
 
 	if (of_device_is_compatible(pdev->dev.of_node, "starfive,jh7110-qspi")) {
@@ -1914,7 +1914,7 @@ static int cqspi_probe(struct platform_device *pdev)
 		if (IS_ERR(rstc_ref)) {
 			ret = PTR_ERR(rstc_ref);
 			dev_err(dev, "Cannot get QSPI REF reset.\n");
-			goto probe_reset_failed;
+			goto disable_clk;
 		}
 		reset_control_assert(rstc_ref);
 		reset_control_deassert(rstc_ref);
@@ -1956,7 +1956,7 @@ static int cqspi_probe(struct platform_device *pdev)
 		if (ddata->jh7110_clk_init) {
 			ret = cqspi_jh7110_clk_init(pdev, cqspi);
 			if (ret)
-				goto probe_reset_failed;
+				goto disable_clk;
 		}
 		if (ddata->quirks & CQSPI_DISABLE_STIG_MODE)
 			cqspi->disable_stig_mode = true;
@@ -1964,7 +1964,7 @@ static int cqspi_probe(struct platform_device *pdev)
 		if (ddata->quirks & CQSPI_DMA_SET_MASK) {
 			ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
 			if (ret)
-				goto probe_reset_failed;
+				goto disable_clks;
 		}
 	}
 
@@ -1975,7 +1975,7 @@ static int cqspi_probe(struct platform_device *pdev)
 			       pdev->name, cqspi);
 	if (ret) {
 		dev_err(dev, "Cannot request IRQ.\n");
-		goto probe_reset_failed;
+		goto disable_clks;
 	}
 
 	cqspi_wait_idle(cqspi);
@@ -2002,14 +2002,14 @@ static int cqspi_probe(struct platform_device *pdev)
 		ret = cqspi_request_mmap_dma(cqspi);
 		if (ret == -EPROBE_DEFER) {
 			dev_err_probe(&pdev->dev, ret, "Failed to request mmap DMA\n");
-			goto probe_setup_failed;
+			goto disable_controller;
 		}
 	}
 
 	ret = spi_register_controller(host);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register SPI ctlr %d\n", ret);
-		goto probe_setup_failed;
+		goto release_dma_chan;
 	}
 
 	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) {
@@ -2018,17 +2018,22 @@ static int cqspi_probe(struct platform_device *pdev)
 	}
 
 	return 0;
-probe_setup_failed:
-	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM)))
-		pm_runtime_disable(dev);
+
+release_dma_chan:
+	if (cqspi->rx_chan)
+		dma_release_channel(cqspi->rx_chan);
+disable_controller:
 	cqspi_controller_enable(cqspi, 0);
-probe_reset_failed:
+disable_clks:
 	if (cqspi->is_jh7110)
 		cqspi_jh7110_disable_clk(pdev, cqspi);
-
+disable_clk:
 	if (pm_runtime_get_sync(&pdev->dev) >= 0)
 		clk_disable_unprepare(cqspi->clk);
-probe_clk_failed:
+disable_rpm:
+	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM)))
+		pm_runtime_disable(dev);
+
 	return ret;
 }
 
@@ -2046,18 +2051,19 @@ static void cqspi_remove(struct platform_device *pdev)
 		cqspi_wait_idle(cqspi);
 
 	spi_unregister_controller(cqspi->host);
-	cqspi_controller_enable(cqspi, 0);
 
 	if (cqspi->rx_chan)
 		dma_release_channel(cqspi->rx_chan);
 
-	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM)))
-		if (pm_runtime_get_sync(&pdev->dev) >= 0)
-			clk_disable(cqspi->clk);
+	cqspi_controller_enable(cqspi, 0);
 
 	if (cqspi->is_jh7110)
 		cqspi_jh7110_disable_clk(pdev, cqspi);
 
+	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM)))
+		if (pm_runtime_get_sync(&pdev->dev) >= 0)
+			clk_disable(cqspi->clk);
+
 	if (!(ddata && (ddata->quirks & CQSPI_DISABLE_RUNTIME_PM))) {
 		pm_runtime_put_sync(&pdev->dev);
 		pm_runtime_disable(&pdev->dev);
-- 
2.51.0