[PATCH 0/3] spi: fsl-qspi: Fix double cleanup in probe error path

[PATCH 0/3] spi: fsl-qspi: Fix double cleanup in probe error path

[Kevin Hao]

This patch series fixes double cleanup issues in the fsl-qspi probe
error path and also simplifies the probe error handling using managed APIs.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
Kevin Hao (3):
      spi: fsl-qspi: Fix double cleanup in probe error path
      spi: fsl-spi: Remove redundant probe error message
      spi: fsl-qspi: Simplify probe error handling using managed API

 drivers/spi/spi-fsl-qspi.c | 77 +++++++++++++++++++---------------------------
 1 file changed, 31 insertions(+), 46 deletions(-)
---
base-commit: 29e7bf01ed8033c9a14ed0dc990dfe2736dbcd18
change-id: 20250410-spi-10adc098d566

Best regards,
-- 
Kevin Hao <haokexin@gmail.com>

[PATCH 1/3] spi: fsl-qspi: Fix double cleanup in probe error path

[Kevin Hao]

Commit 40369bfe717e ("spi: fsl-qspi: use devm function instead of driver
remove") introduced managed cleanup via fsl_qspi_cleanup(), but
incorrectly retain manual cleanup in two scenarios:

- On devm_add_action_or_reset() failure, the function automatically call
  fsl_qspi_cleanup(). However, the current code still jumps to
  err_destroy_mutex, repeating cleanup.

- After the fsl_qspi_cleanup() action is added successfully, there is no
  need to manually perform the cleanup in the subsequent error path.
  However, the current code still jumps to err_destroy_mutex on spi
  controller failure, repeating cleanup.

Skip redundant manual cleanup calls to fix these issues.

Cc: stable@vger.kernel.org
Fixes: 40369bfe717e ("spi: fsl-qspi: use devm function instead of driver remove")
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/spi/spi-fsl-qspi.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
index 5c59fddb32c1b9cc030e7abb49484662ec7b382c..2f54dc09d11b1c56cfe57ceec8452fbb29322d3f 100644
--- a/drivers/spi/spi-fsl-qspi.c
+++ b/drivers/spi/spi-fsl-qspi.c
@@ -949,17 +949,14 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 
 	ret = devm_add_action_or_reset(dev, fsl_qspi_cleanup, q);
 	if (ret)
-		goto err_destroy_mutex;
+		goto err_put_ctrl;
 
 	ret = devm_spi_register_controller(dev, ctlr);
 	if (ret)
-		goto err_destroy_mutex;
+		goto err_put_ctrl;
 
 	return 0;
 
-err_destroy_mutex:
-	mutex_destroy(&q->lock);
-
 err_disable_clk:
 	fsl_qspi_clk_disable_unprep(q);
 

-- 
2.49.0

Re: [PATCH 1/3] spi: fsl-qspi: Fix double cleanup in probe error path

[Frank Li]

On Thu, Apr 10, 2025 at 02:56:09PM +0800, Kevin Hao wrote:
> Commit 40369bfe717e ("spi: fsl-qspi: use devm function instead of driver
> remove") introduced managed cleanup via fsl_qspi_cleanup(), but
> incorrectly retain manual cleanup in two scenarios:
>
> - On devm_add_action_or_reset() failure, the function automatically call
>   fsl_qspi_cleanup(). However, the current code still jumps to
>   err_destroy_mutex, repeating cleanup.
>
> - After the fsl_qspi_cleanup() action is added successfully, there is no
>   need to manually perform the cleanup in the subsequent error path.
>   However, the current code still jumps to err_destroy_mutex on spi
>   controller failure, repeating cleanup.
>
> Skip redundant manual cleanup calls to fix these issues.
>
> Cc: stable@vger.kernel.org
> Fixes: 40369bfe717e ("spi: fsl-qspi: use devm function instead of driver remove")
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
>  drivers/spi/spi-fsl-qspi.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
> index 5c59fddb32c1b9cc030e7abb49484662ec7b382c..2f54dc09d11b1c56cfe57ceec8452fbb29322d3f 100644
> --- a/drivers/spi/spi-fsl-qspi.c
> +++ b/drivers/spi/spi-fsl-qspi.c
> @@ -949,17 +949,14 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>
>  	ret = devm_add_action_or_reset(dev, fsl_qspi_cleanup, q);
>  	if (ret)
> -		goto err_destroy_mutex;
> +		goto err_put_ctrl;

fsl_qspi_cleanup() already included mutex_destroy() and
fsl_qspi_clk_disable_unprep()

simple
		return ret;
>
>  	ret = devm_spi_register_controller(dev, ctlr);
>  	if (ret)
> -		goto err_destroy_mutex;
> +		goto err_put_ctrl;

return ret;
>
>  	return 0;
>
> -err_destroy_mutex:
> -	mutex_destroy(&q->lock);
> -
>  err_disable_clk:
>  	fsl_qspi_clk_disable_unprep(q);

remove these two labels

Frank
>
>
> --
> 2.49.0
>

Re: [PATCH 1/3] spi: fsl-qspi: Fix double cleanup in probe error path

[Frank Li]

On Thu, Apr 10, 2025 at 11:45:13AM -0400, Frank Li wrote:
> On Thu, Apr 10, 2025 at 02:56:09PM +0800, Kevin Hao wrote:
> > Commit 40369bfe717e ("spi: fsl-qspi: use devm function instead of driver
> > remove") introduced managed cleanup via fsl_qspi_cleanup(), but
> > incorrectly retain manual cleanup in two scenarios:
> >
> > - On devm_add_action_or_reset() failure, the function automatically call
> >   fsl_qspi_cleanup(). However, the current code still jumps to
> >   err_destroy_mutex, repeating cleanup.
> >
> > - After the fsl_qspi_cleanup() action is added successfully, there is no
> >   need to manually perform the cleanup in the subsequent error path.
> >   However, the current code still jumps to err_destroy_mutex on spi
> >   controller failure, repeating cleanup.
> >
> > Skip redundant manual cleanup calls to fix these issues.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 40369bfe717e ("spi: fsl-qspi: use devm function instead of driver remove")
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> > ---
> >  drivers/spi/spi-fsl-qspi.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
> > index 5c59fddb32c1b9cc030e7abb49484662ec7b382c..2f54dc09d11b1c56cfe57ceec8452fbb29322d3f 100644
> > --- a/drivers/spi/spi-fsl-qspi.c
> > +++ b/drivers/spi/spi-fsl-qspi.c
> > @@ -949,17 +949,14 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> >
> >  	ret = devm_add_action_or_reset(dev, fsl_qspi_cleanup, q);
> >  	if (ret)
> > -		goto err_destroy_mutex;
> > +		goto err_put_ctrl;
>
> fsl_qspi_cleanup() already included mutex_destroy() and
> fsl_qspi_clk_disable_unprep()
>
> simple
> 		return ret;
> >
> >  	ret = devm_spi_register_controller(dev, ctlr);
> >  	if (ret)
> > -		goto err_destroy_mutex;
> > +		goto err_put_ctrl;
>
> return ret;
> >
> >  	return 0;
> >
> > -err_destroy_mutex:
> > -	mutex_destroy(&q->lock);
> > -
> >  err_disable_clk:
> >  	fsl_qspi_clk_disable_unprep(q);
>
> remove these two labels

Sorry, I missed your patch3 and Mark already applied. Please discard
my comment. it should be fine.

Frank
>
> Frank
> >
> >
> > --
> > 2.49.0
> >

[PATCH 2/3] spi: fsl-spi: Remove redundant probe error message

[Kevin Hao]

An error message is already emitted by the driver core function
call_driver_probe() when the driver probe fails. Therefore, this
redundant probe error message is removed.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/spi/spi-fsl-qspi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
index 2f54dc09d11b1c56cfe57ceec8452fbb29322d3f..b5ecffcaf7955e2ec9bb3e2857f8bc14d73a2f90 100644
--- a/drivers/spi/spi-fsl-qspi.c
+++ b/drivers/spi/spi-fsl-qspi.c
@@ -963,7 +963,6 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 err_put_ctrl:
 	spi_controller_put(ctlr);
 
-	dev_err(dev, "Freescale QuadSPI probe failed\n");
 	return ret;
 }
 

-- 
2.49.0

[PATCH 3/3] spi: fsl-qspi: Simplify probe error handling using managed API

[Kevin Hao]

- Switch to devm_spi_alloc_host() to avoid manual spi_controller_put()
  calls in error paths.

- Factor out the hardware disable logic into a dedicated
  fsl_qspi_disable() helper and register it as a managed cleanup action,
  removing the need to explicitly disable hardware after
  fsl_qspi_default_setup() failures.

- Move fsl_qspi_cleanup() earlier in the probe sequence to eliminate
  the need for manual cleanup in the irq failure path.

With these changes we can completely eliminate the messy goto labels
in probe function.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/spi/spi-fsl-qspi.c | 73 ++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 42 deletions(-)

diff --git a/drivers/spi/spi-fsl-qspi.c b/drivers/spi/spi-fsl-qspi.c
index b5ecffcaf7955e2ec9bb3e2857f8bc14d73a2f90..f879533535e853dac6bff22a3a5e87431a44a1d2 100644
--- a/drivers/spi/spi-fsl-qspi.c
+++ b/drivers/spi/spi-fsl-qspi.c
@@ -844,13 +844,18 @@ static const struct spi_controller_mem_caps fsl_qspi_mem_caps = {
 	.per_op_freq = true,
 };
 
-static void fsl_qspi_cleanup(void *data)
+static void fsl_qspi_disable(void *data)
 {
 	struct fsl_qspi *q = data;
 
 	/* disable the hardware */
 	qspi_writel(q, QUADSPI_MCR_MDIS_MASK, q->iobase + QUADSPI_MCR);
 	qspi_writel(q, 0x0, q->iobase + QUADSPI_RSER);
+}
+
+static void fsl_qspi_cleanup(void *data)
+{
+	struct fsl_qspi *q = data;
 
 	fsl_qspi_clk_disable_unprep(q);
 
@@ -866,7 +871,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 	struct fsl_qspi *q;
 	int ret;
 
-	ctlr = spi_alloc_host(&pdev->dev, sizeof(*q));
+	ctlr = devm_spi_alloc_host(&pdev->dev, sizeof(*q));
 	if (!ctlr)
 		return -ENOMEM;
 
@@ -876,68 +881,60 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 	q = spi_controller_get_devdata(ctlr);
 	q->dev = dev;
 	q->devtype_data = of_device_get_match_data(dev);
-	if (!q->devtype_data) {
-		ret = -ENODEV;
-		goto err_put_ctrl;
-	}
+	if (!q->devtype_data)
+		return -ENODEV;
 
 	platform_set_drvdata(pdev, q);
 
 	/* find the resources */
 	q->iobase = devm_platform_ioremap_resource_byname(pdev, "QuadSPI");
-	if (IS_ERR(q->iobase)) {
-		ret = PTR_ERR(q->iobase);
-		goto err_put_ctrl;
-	}
+	if (IS_ERR(q->iobase))
+		return PTR_ERR(q->iobase);
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 					"QuadSPI-memory");
-	if (!res) {
-		ret = -EINVAL;
-		goto err_put_ctrl;
-	}
+	if (!res)
+		return -EINVAL;
 	q->memmap_phy = res->start;
 	/* Since there are 4 cs, map size required is 4 times ahb_buf_size */
 	q->ahb_addr = devm_ioremap(dev, q->memmap_phy,
 				   (q->devtype_data->ahb_buf_size * 4));
-	if (!q->ahb_addr) {
-		ret = -ENOMEM;
-		goto err_put_ctrl;
-	}
+	if (!q->ahb_addr)
+		return -ENOMEM;
 
 	/* find the clocks */
 	q->clk_en = devm_clk_get(dev, "qspi_en");
-	if (IS_ERR(q->clk_en)) {
-		ret = PTR_ERR(q->clk_en);
-		goto err_put_ctrl;
-	}
+	if (IS_ERR(q->clk_en))
+		return PTR_ERR(q->clk_en);
 
 	q->clk = devm_clk_get(dev, "qspi");
-	if (IS_ERR(q->clk)) {
-		ret = PTR_ERR(q->clk);
-		goto err_put_ctrl;
-	}
+	if (IS_ERR(q->clk))
+		return PTR_ERR(q->clk);
+
+	mutex_init(&q->lock);
 
 	ret = fsl_qspi_clk_prep_enable(q);
 	if (ret) {
 		dev_err(dev, "can not enable the clock\n");
-		goto err_put_ctrl;
+		return ret;
 	}
 
+	ret = devm_add_action_or_reset(dev, fsl_qspi_cleanup, q);
+	if (ret)
+		return ret;
+
 	/* find the irq */
 	ret = platform_get_irq(pdev, 0);
 	if (ret < 0)
-		goto err_disable_clk;
+		return ret;
 
 	ret = devm_request_irq(dev, ret,
 			fsl_qspi_irq_handler, 0, pdev->name, q);
 	if (ret) {
 		dev_err(dev, "failed to request irq: %d\n", ret);
-		goto err_disable_clk;
+		return ret;
 	}
 
-	mutex_init(&q->lock);
-
 	ctlr->bus_num = -1;
 	ctlr->num_chipselect = 4;
 	ctlr->mem_ops = &fsl_qspi_mem_ops;
@@ -947,23 +944,15 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 
 	ctlr->dev.of_node = np;
 
-	ret = devm_add_action_or_reset(dev, fsl_qspi_cleanup, q);
+	ret = devm_add_action_or_reset(dev, fsl_qspi_disable, q);
 	if (ret)
-		goto err_put_ctrl;
+		return ret;
 
 	ret = devm_spi_register_controller(dev, ctlr);
 	if (ret)
-		goto err_put_ctrl;
+		return ret;
 
 	return 0;
-
-err_disable_clk:
-	fsl_qspi_clk_disable_unprep(q);
-
-err_put_ctrl:
-	spi_controller_put(ctlr);
-
-	return ret;
 }
 
 static int fsl_qspi_suspend(struct device *dev)

-- 
2.49.0

Re: [PATCH 0/3] spi: fsl-qspi: Fix double cleanup in probe error path

[Mark Brown]

On Thu, 10 Apr 2025 14:56:08 +0800, Kevin Hao wrote:
> This patch series fixes double cleanup issues in the fsl-qspi probe
> error path and also simplifies the probe error handling using managed APIs.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/3] spi: fsl-qspi: Fix double cleanup in probe error path
      commit: ed4db69169121ffd9d5a4bcf4d7acd5856cb20cd
[2/3] spi: fsl-spi: Remove redundant probe error message
      commit: 82bedbfedd2fc7cd1287732879e515ceb94f8963
[3/3] spi: fsl-qspi: Simplify probe error handling using managed API
      commit: 3f7b48efb79d91883d98dd7e33dc2a0abfa9f923

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark