* [PATCH 1/4] spi: atcspi200: return error from failed controller suspend
[not found] <20260620083931.1120616-1-1298662399@qq.com>
@ 2026-06-20 8:39 ` Jiawen Liu
2026-06-20 8:57 ` sashiko-bot
2026-06-20 8:39 ` [PATCH 2/4] spi: bcmbca-hsspi: " Jiawen Liu
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Jiawen Liu @ 2026-06-20 8:39 UTC (permalink / raw)
To: broonie
Cc: cl634, william.zhang, kursad.oney, jonas.gorski,
bcm-kernel-feedback-list, anand.gore, f.fainelli, rafal, olteanv,
han.xu, haibo.chen, yogeshgaur.83, linux-spi, linux-kernel,
linux-arm-kernel, imx, Jiawen Liu
spi_controller_suspend() can fail when the SPI core cannot stop the
controller. atcspi_suspend() ignored that error and disabled the
controller clock anyway.
Return the error before disabling the clock.
Signed-off-by: Jiawen Liu <1298662399@qq.com>
---
drivers/spi/spi-atcspi200.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-atcspi200.c b/drivers/spi/spi-atcspi200.c
index 6d4b6aeb3f5b..e0fd101a62bc 100644
--- a/drivers/spi/spi-atcspi200.c
+++ b/drivers/spi/spi-atcspi200.c
@@ -599,8 +599,11 @@ static int atcspi_suspend(struct device *dev)
{
struct spi_controller *host = dev_get_drvdata(dev);
struct atcspi_dev *spi = spi_controller_get_devdata(host);
+ int ret;
- spi_controller_suspend(host);
+ ret = spi_controller_suspend(host);
+ if (ret)
+ return ret;
clk_disable_unprepare(spi->clk);
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/4] spi: bcmbca-hsspi: return error from failed controller suspend
[not found] <20260620083931.1120616-1-1298662399@qq.com>
2026-06-20 8:39 ` [PATCH 1/4] spi: atcspi200: return error from failed controller suspend Jiawen Liu
@ 2026-06-20 8:39 ` Jiawen Liu
2026-06-20 8:54 ` sashiko-bot
2026-06-20 8:39 ` [PATCH 3/4] spi: fsl-dspi: clean up after failed suspend and resume Jiawen Liu
2026-06-20 8:39 ` [PATCH 4/4] spi: nxp-fspi: disable runtime PM on probe failures Jiawen Liu
3 siblings, 1 reply; 9+ messages in thread
From: Jiawen Liu @ 2026-06-20 8:39 UTC (permalink / raw)
To: broonie
Cc: cl634, william.zhang, kursad.oney, jonas.gorski,
bcm-kernel-feedback-list, anand.gore, f.fainelli, rafal, olteanv,
han.xu, haibo.chen, yogeshgaur.83, linux-spi, linux-kernel,
linux-arm-kernel, imx, Jiawen Liu
spi_controller_suspend() can fail if pending transfers cannot stop.
bcmbca_hsspi_suspend() ignored the error and still disabled the
PLL and core clocks.
Return the error before disabling the clocks.
Signed-off-by: Jiawen Liu <1298662399@qq.com>
---
drivers/spi/spi-bcmbca-hsspi.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-bcmbca-hsspi.c b/drivers/spi/spi-bcmbca-hsspi.c
index 09c1472ae4fa..af88ce04948b 100644
--- a/drivers/spi/spi-bcmbca-hsspi.c
+++ b/drivers/spi/spi-bcmbca-hsspi.c
@@ -568,8 +568,12 @@ static int bcmbca_hsspi_suspend(struct device *dev)
{
struct spi_controller *host = dev_get_drvdata(dev);
struct bcmbca_hsspi *bs = spi_controller_get_devdata(host);
+ int ret;
+
+ ret = spi_controller_suspend(host);
+ if (ret)
+ return ret;
- spi_controller_suspend(host);
clk_disable_unprepare(bs->pll_clk);
clk_disable_unprepare(bs->clk);
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/4] spi: fsl-dspi: clean up after failed suspend and resume
[not found] <20260620083931.1120616-1-1298662399@qq.com>
2026-06-20 8:39 ` [PATCH 1/4] spi: atcspi200: return error from failed controller suspend Jiawen Liu
2026-06-20 8:39 ` [PATCH 2/4] spi: bcmbca-hsspi: " Jiawen Liu
@ 2026-06-20 8:39 ` Jiawen Liu
2026-06-20 8:53 ` sashiko-bot
2026-06-20 8:39 ` [PATCH 4/4] spi: nxp-fspi: disable runtime PM on probe failures Jiawen Liu
3 siblings, 1 reply; 9+ messages in thread
From: Jiawen Liu @ 2026-06-20 8:39 UTC (permalink / raw)
To: broonie
Cc: cl634, william.zhang, kursad.oney, jonas.gorski,
bcm-kernel-feedback-list, anand.gore, f.fainelli, rafal, olteanv,
han.xu, haibo.chen, yogeshgaur.83, linux-spi, linux-kernel,
linux-arm-kernel, imx, Jiawen Liu
dspi_suspend() disabled the IRQ before spi_controller_suspend(),
but ignored a suspend failure and kept tearing the device down.
Restore the IRQ and return the error if suspend fails.
dspi_resume() also left the clock prepared if controller resume or
hardware init failed. Route those failures through clock cleanup.
Signed-off-by: Jiawen Liu <1298662399@qq.com>
---
drivers/spi/spi-fsl-dspi.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 019d05cdefe6..c2d283876ef8 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -1464,10 +1464,18 @@ static int dspi_init(struct fsl_dspi *dspi)
static int dspi_suspend(struct device *dev)
{
struct fsl_dspi *dspi = dev_get_drvdata(dev);
+ int ret;
if (dspi->irq)
disable_irq(dspi->irq);
- spi_controller_suspend(dspi->ctlr);
+
+ ret = spi_controller_suspend(dspi->ctlr);
+ if (ret) {
+ if (dspi->irq)
+ enable_irq(dspi->irq);
+ return ret;
+ }
+
clk_disable_unprepare(dspi->clk);
pinctrl_pm_select_sleep_state(dev);
@@ -1485,12 +1493,15 @@ static int dspi_resume(struct device *dev)
ret = clk_prepare_enable(dspi->clk);
if (ret)
return ret;
- spi_controller_resume(dspi->ctlr);
+
+ ret = spi_controller_resume(dspi->ctlr);
+ if (ret)
+ goto disable_clk;
ret = dspi_init(dspi);
if (ret) {
dev_err(dev, "failed to initialize dspi during resume\n");
- return ret;
+ goto disable_clk;
}
dspi_set_mtf(dspi);
@@ -1499,6 +1510,10 @@ static int dspi_resume(struct device *dev)
enable_irq(dspi->irq);
return 0;
+
+disable_clk:
+ clk_disable_unprepare(dspi->clk);
+ return ret;
}
#endif /* CONFIG_PM_SLEEP */
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/4] spi: nxp-fspi: disable runtime PM on probe failures
[not found] <20260620083931.1120616-1-1298662399@qq.com>
` (2 preceding siblings ...)
2026-06-20 8:39 ` [PATCH 3/4] spi: fsl-dspi: clean up after failed suspend and resume Jiawen Liu
@ 2026-06-20 8:39 ` Jiawen Liu
2026-06-20 8:57 ` sashiko-bot
2026-06-20 14:09 ` Frank Li
3 siblings, 2 replies; 9+ messages in thread
From: Jiawen Liu @ 2026-06-20 8:39 UTC (permalink / raw)
To: broonie
Cc: cl634, william.zhang, kursad.oney, jonas.gorski,
bcm-kernel-feedback-list, anand.gore, f.fainelli, rafal, olteanv,
han.xu, haibo.chen, yogeshgaur.83, linux-spi, linux-kernel,
linux-arm-kernel, imx, Jiawen Liu
nxp_fspi_probe() enables runtime PM and autosuspend before
several operations that can fail.
Some failure paths returned directly before the devm cleanup
action was installed, leaving runtime PM enabled.
Route those failures through a common runtime PM cleanup path.
Use pm_runtime_resume_and_get() for the initial clock enable.
Signed-off-by: Jiawen Liu <1298662399@qq.com>
---
drivers/spi/spi-nxp-fspi.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index 1e36ae084dd8..d94a2a7b98d4 100644
--- a/drivers/spi/spi-nxp-fspi.c
+++ b/drivers/spi/spi-nxp-fspi.c
@@ -1350,9 +1350,11 @@ static int nxp_fspi_probe(struct platform_device *pdev)
pm_runtime_use_autosuspend(dev);
/* enable clock */
- ret = pm_runtime_get_sync(f->dev);
- if (ret < 0)
- return dev_err_probe(dev, ret, "Failed to enable clock");
+ ret = pm_runtime_resume_and_get(f->dev);
+ if (ret < 0) {
+ ret = dev_err_probe(dev, ret, "Failed to enable clock");
+ goto err_disable_pm;
+ }
/* Clear potential interrupts */
reg = fspi_readl(f, f->iobase + FSPI_INTR);
@@ -1362,18 +1364,24 @@ static int nxp_fspi_probe(struct platform_device *pdev)
nxp_fspi_default_setup(f);
ret = pm_runtime_put_sync(dev);
- if (ret < 0)
- return dev_err_probe(dev, ret, "Failed to disable clock");
+ if (ret < 0) {
+ ret = dev_err_probe(dev, ret, "Failed to disable clock");
+ goto err_disable_pm;
+ }
init_completion(&f->c);
ret = devm_request_irq(dev, irq,
nxp_fspi_irq_handler, 0, pdev->name, f);
- if (ret)
- return dev_err_probe(dev, ret, "Failed to request irq\n");
+ if (ret) {
+ ret = dev_err_probe(dev, ret, "Failed to request irq\n");
+ goto err_disable_pm;
+ }
ret = devm_mutex_init(dev, &f->lock);
- if (ret)
- return dev_err_probe(dev, ret, "Failed to initialize lock\n");
+ if (ret) {
+ ret = dev_err_probe(dev, ret, "Failed to initialize lock\n");
+ goto err_disable_pm;
+ }
ctlr->bus_num = -1;
ctlr->num_chipselect = NXP_FSPI_MAX_CHIPSELECT;
@@ -1389,6 +1397,11 @@ static int nxp_fspi_probe(struct platform_device *pdev)
return ret;
return devm_spi_register_controller(&pdev->dev, ctlr);
+
+err_disable_pm:
+ pm_runtime_dont_use_autosuspend(dev);
+ pm_runtime_disable(dev);
+ return ret;
}
static int nxp_fspi_runtime_suspend(struct device *dev)
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] spi: fsl-dspi: clean up after failed suspend and resume
2026-06-20 8:39 ` [PATCH 3/4] spi: fsl-dspi: clean up after failed suspend and resume Jiawen Liu
@ 2026-06-20 8:53 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-06-20 8:53 UTC (permalink / raw)
To: Jiawen Liu; +Cc: Frank.Li, imx
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Deadlock and guaranteed 5-second suspend timeout due to incorrect lifecycle ordering in dspi_suspend().
- [High] Race condition and potential deadlock in dspi_resume() due to resuming the SPI queue before hardware initialization.
--
commit 98c391bd91fadd1b60a50d12e8e2491310c7b7ea
Author: Jiawen Liu <1298662399@qq.com>
spi: fsl-dspi: clean up after failed suspend and resume
This patch updates the suspend and resume routines to better handle failures.
It ensures the IRQ is re-enabled if spi_controller_suspend() fails, and
properly unwinds clock preparation if controller resume or hardware
initialization fails.
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 019d05cdefe69..c2d283876ef80 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -1464,10 +1464,18 @@ static int dspi_init(struct fsl_dspi *dspi)
> static int dspi_suspend(struct device *dev)
> {
> struct fsl_dspi *dspi = dev_get_drvdata(dev);
> + int ret;
>
> if (dspi->irq)
> disable_irq(dspi->irq);
> - spi_controller_suspend(dspi->ctlr);
> +
> + ret = spi_controller_suspend(dspi->ctlr);
[Severity: High]
This is a pre-existing issue, but does disabling the IRQ before suspending
the controller cause active transfers to hang?
Looking at dspi_suspend(), disable_irq() is called before
spi_controller_suspend(). When spi_controller_suspend() stops the queue, it
waits up to 5 seconds for the active transfer to finish.
However, if the active transfer requires the hardware interrupt handler to
complete, it will never finish because the IRQ is already disabled. This
seems like it would guarantee a 5-second timeout, causing
spi_controller_suspend() to return -EBUSY and abort the suspend.
Should the controller be suspended before the IRQ is disabled?
> + if (ret) {
> + if (dspi->irq)
> + enable_irq(dspi->irq);
> + return ret;
> + }
> +
> clk_disable_unprepare(dspi->clk);
[ ... ]
> @@ -1485,12 +1493,15 @@ static int dspi_resume(struct device *dev)
> ret = clk_prepare_enable(dspi->clk);
> if (ret)
> return ret;
> - spi_controller_resume(dspi->ctlr);
> +
> + ret = spi_controller_resume(dspi->ctlr);
> + if (ret)
> + goto disable_clk;
>
> ret = dspi_init(dspi);
[Severity: High]
This isn't a bug introduced by this patch, but does calling
spi_controller_resume() before dspi_init() and enable_irq() create a race
window?
spi_controller_resume() unblocks the message pump, allowing the worker
thread to immediately dispatch pending transfers. If a transfer starts
before dspi_init() initializes the hardware state, it might interact with
uninitialized registers.
Additionally, dspi_init() clears the status registers:
regmap_write(dspi->regmap, SPI_SR, SPI_SR_CLEAR);
If a racing transfer sets completion flags in the hardware right as
dspi_init() clears them, the interrupt could be permanently lost. The
worker thread would then deadlock waiting for the xfer_done completion
event.
Should the hardware and IRQs be fully restored before resuming the queue?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260620083931.1120616-1-1298662399@qq.com?part=3
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] spi: bcmbca-hsspi: return error from failed controller suspend
2026-06-20 8:39 ` [PATCH 2/4] spi: bcmbca-hsspi: " Jiawen Liu
@ 2026-06-20 8:54 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-06-20 8:54 UTC (permalink / raw)
To: Jiawen Liu; +Cc: Frank.Li, imx
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Propagating the error from `spi_controller_suspend()` leaves the SPI controller permanently wedged in a suspended state.
--
commit 4a5c38a8fabc327922f122498eb97084160abb1e
Author: Jiawen Liu <1298662399@qq.com>
spi: bcmbca-hsspi: return error from failed controller suspend
This commit updates bcmbca_hsspi_suspend to check the return value of
spi_controller_suspend. If stopping the queue fails, it propagates the error
back to the PM core to prevent disabling the clocks while transfers are active.
> diff --git a/drivers/spi/spi-bcmbca-hsspi.c b/drivers/spi/spi-bcmbca-hsspi.c
> index 09c1472ae4fa1..af88ce04948b7 100644
> --- a/drivers/spi/spi-bcmbca-hsspi.c
> +++ b/drivers/spi/spi-bcmbca-hsspi.c
> @@ -568,8 +568,12 @@ static int bcmbca_hsspi_suspend(struct device *dev)
> {
> struct spi_controller *host = dev_get_drvdata(dev);
> struct bcmbca_hsspi *bs = spi_controller_get_devdata(host);
> + int ret;
> +
> + ret = spi_controller_suspend(host);
> + if (ret)
> + return ret;
[Severity: High]
Does returning the error directly here leave the SPI controller permanently
stuck in a suspended state?
Looking at how spi_controller_suspend() handles errors, it unconditionally
marks the controller as suspended before returning:
drivers/spi/spi.c:spi_controller_suspend() {
if (ctlr->queued) {
ret = spi_stop_queue(ctlr);
if (ret)
dev_err(&ctlr->dev, "queue stop failed\n");
}
__spi_mark_suspended(ctlr);
return ret;
}
If bcmbca_hsspi_suspend() returns this error to the PM core, the system
suspend is aborted. Because the device's suspend failed, the PM core considers
the device to be active and will not invoke its resume callback during the
PM abort recovery phase.
Since spi_controller_resume() is never called, the controller remains
flagged as suspended, meaning all future SPI transfers will fail with
-ESHUTDOWN until a subsequent successful suspend/resume cycle.
Should we explicitly call spi_controller_resume(host) to unwind the
internal suspend state before returning the error?
>
> - spi_controller_suspend(host);
> clk_disable_unprepare(bs->pll_clk);
> clk_disable_unprepare(bs->clk);
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260620083931.1120616-1-1298662399@qq.com?part=2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] spi: atcspi200: return error from failed controller suspend
2026-06-20 8:39 ` [PATCH 1/4] spi: atcspi200: return error from failed controller suspend Jiawen Liu
@ 2026-06-20 8:57 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-06-20 8:57 UTC (permalink / raw)
To: Jiawen Liu; +Cc: Frank.Li, imx
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Using dmaengine_terminate_all() on timeout leads to memory corruption and completion race conditions.
- [High] Driver ignores -EPROBE_DEFER from DMA channel request, permanently disabling DMA.
- [High] Missing hardware reset after a transfer timeout leaves the SPI controller in a stuck state.
--
commit 68634bef983da7c31a412c03f17d17fa06e49838
Author: Jiawen Liu <1298662399@qq.com>
spi: atcspi200: return error from failed controller suspend
spi_controller_suspend() can fail when the SPI core cannot stop the
controller. atcspi_suspend() ignored that error and disabled the
controller clock anyway. Return the error before disabling the clock.
> diff --git a/drivers/spi/spi-atcspi200.c b/drivers/spi/spi-atcspi200.c
> index 6d4b6aeb3f5b1..e0fd101a62bc8 100644
> --- a/drivers/spi/spi-atcspi200.c
> +++ b/drivers/spi/spi-atcspi200.c
> @@ -599,8 +599,11 @@ static int atcspi_suspend(struct device *dev)
> {
> struct spi_controller *host = dev_get_drvdata(dev);
> struct atcspi_dev *spi = spi_controller_get_devdata(host);
> + int ret;
>
> - spi_controller_suspend(host);
> + ret = spi_controller_suspend(host);
> + if (ret)
> + return ret;
>
> clk_disable_unprepare(spi->clk);
>
While reviewing this patch, I noticed a few pre-existing issues in this file.
[Severity: High]
This is a pre-existing issue, but I noticed atcspi_dma_trans() uses the
asynchronous dmaengine_terminate_all() on timeout, followed immediately by
unmapping the buffers:
drivers/spi/spi-atcspi200.c:atcspi_dma_trans() {
...
dmaengine_terminate_all(dma_ch);
}
exit_unmap:
spi_controller_dma_unmap_mem_op_data(spi->host, op, &sgt);
...
}
Does this risk memory corruption? Because dmaengine_terminate_all() doesn't
guarantee the DMA engine has stopped accessing memory, unmapping it here
could cause IOMMU faults.
Additionally, if atcspi_dma_callback() fires after the unmap, could it
prematurely complete spi->dma_completion for the next transfer? Should this
use dmaengine_terminate_sync() instead?
[Severity: High]
This isn't a bug introduced by this patch, but it looks like atcspi_probe()
might defeat the probe deferral mechanism for the DMA controller:
drivers/spi/spi-atcspi200.c:atcspi_probe() {
...
ret = atcspi_configure_dma(spi);
if (ret)
dev_info(spi->dev,
"Failed to init DMA, fallback to PIO mode\n");
...
}
Since atcspi_configure_dma() uses devm_dma_request_chan(), what happens if
it returns -EPROBE_DEFER when the DMA controller hasn't probed yet?
By catching the error and silently dropping it to fall back to PIO mode, will
the device be permanently stuck in PIO instead of waiting for the DMA
controller to become available?
[Severity: High]
This is another pre-existing issue, but missing a hardware reset after a
transfer timeout in atcspi_exec_mem_op() seems problematic:
drivers/spi/spi-atcspi200.c:atcspi_exec_mem_op() {
...
if (ret) {
dev_info(spi->dev, "SPI transmission failed\n");
goto exec_mem_exit;
}
...
}
When jumping to exec_mem_exit on an error (like -ETIMEDOUT from
atcspi_dma_trans()), we skip waiting for the ATCSPI_ACTIVE bit to clear.
Does this leave the SPI controller's internal state machine stuck waiting for
the timed-out transfer? If a new transfer is initiated while the controller
is still active, could writing to the ATCSPI_CMD register lock up the
hardware or corrupt the new transaction? Should a hardware reset be performed
on this error path?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260620083931.1120616-1-1298662399@qq.com?part=1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 4/4] spi: nxp-fspi: disable runtime PM on probe failures
2026-06-20 8:39 ` [PATCH 4/4] spi: nxp-fspi: disable runtime PM on probe failures Jiawen Liu
@ 2026-06-20 8:57 ` sashiko-bot
2026-06-20 14:09 ` Frank Li
1 sibling, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-06-20 8:57 UTC (permalink / raw)
To: Jiawen Liu; +Cc: Frank.Li, imx
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Medium] Probe error paths leave the hardware powered on and clocks leaked because the newly added cleanup path cancels the autosuspend timer without suspending the device.
Pre-existing issues:
- [High] Unchecked pm_runtime_get_sync() failure in nxp_fspi_cleanup() can cause a synchronous external abort (bus fault).
- [High] The ignored return value of nxp_fspi_default_setup() corrupts the clock enable count and causes a kernel panic.
--
commit f84d61f342ed9ba0fef9a6eeb2fb3c7129154065
Author: Jiawen Liu <1298662399@qq.com>
spi: nxp-fspi: disable runtime PM on probe failures
nxp_fspi_probe() enables runtime PM and autosuspend before several operations
that can fail. Some failure paths returned directly before the devm cleanup
action was installed, leaving runtime PM enabled. Route those failures through
a common runtime PM cleanup path. Use pm_runtime_resume_and_get() for the
initial clock enable.
> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> index 1e36ae084dd86..d94a2a7b98d44 100644
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c
[ ... ]
> @@ -1362,18 +1364,24 @@ static int nxp_fspi_probe(struct platform_device *pdev)
> nxp_fspi_default_setup(f);
[Severity: High]
This is a pre-existing issue, but does nxp_fspi_probe() need to check the
return value of nxp_fspi_default_setup()?
If nxp_fspi_default_setup() fails, it leaves the clocks disabled. Since this
return value is ignored, the driver proceeds to register the SPI controller.
When the device autosuspends, nxp_fspi_runtime_suspend() will disable the
already-disabled clock, which drives the clock enable count negative.
A subsequent access to the SPI bus will increment the count back to 0 in
nxp_fspi_runtime_resume(), leaving the clock physically OFF.
Would this lead to reading/writing unclocked MMIO registers and trigger a
synchronous external abort?
>
> ret = pm_runtime_put_sync(dev);
[ ... ]
> @@ -1389,6 +1397,11 @@ static int nxp_fspi_probe(struct platform_device *pdev)
> return ret;
>
> return devm_spi_register_controller(&pdev->dev, ctlr);
> +
> +err_disable_pm:
> + pm_runtime_dont_use_autosuspend(dev);
> + pm_runtime_disable(dev);
[Severity: Medium]
When execution jumps here due to a failure (e.g., in devm_request_irq()),
the device is still in the RPM_ACTIVE state because pm_runtime_put_sync()
only scheduled an autosuspend timer.
Will calling pm_runtime_disable() cancel the pending autosuspend timer
and freeze the device in RPM_ACTIVE?
Since the devm cleanup action (nxp_fspi_cleanup) has not been registered yet,
does this leave the hardware powered on and permanently leak the clocks?
> + return ret;
> }
[Severity: High]
This is a pre-existing issue, but there appears to be another unchecked
clock enablement in nxp_fspi_cleanup():
drivers/spi/spi-nxp-fspi.c:nxp_fspi_cleanup() {
...
/* enable clock first since there is register access */
pm_runtime_get_sync(f->dev);
/* disable the hardware */
fspi_writel(f, FSPI_MCR0_MDIS, f->iobase + FSPI_MCR0);
...
}
If pm_runtime_get_sync() fails to enable the clocks, the hardware remains
unclocked.
Since the return value is ignored, the code unconditionally proceeds to
execute fspi_writel().
Can accessing unclocked MMIO registers trigger a synchronous external
abort and crash the system during teardown or module unload?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260620083931.1120616-1-1298662399@qq.com?part=4
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 4/4] spi: nxp-fspi: disable runtime PM on probe failures
2026-06-20 8:39 ` [PATCH 4/4] spi: nxp-fspi: disable runtime PM on probe failures Jiawen Liu
2026-06-20 8:57 ` sashiko-bot
@ 2026-06-20 14:09 ` Frank Li
1 sibling, 0 replies; 9+ messages in thread
From: Frank Li @ 2026-06-20 14:09 UTC (permalink / raw)
To: Jiawen Liu
Cc: broonie, cl634, william.zhang, kursad.oney, jonas.gorski,
bcm-kernel-feedback-list, anand.gore, f.fainelli, rafal, olteanv,
han.xu, haibo.chen, yogeshgaur.83, linux-spi, linux-kernel,
linux-arm-kernel, imx
On Sat, Jun 20, 2026 at 12:39:31PM +0400, Jiawen Liu wrote:
> [You don't often get email from 1298662399@qq.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> nxp_fspi_probe() enables runtime PM and autosuspend before
> several operations that can fail.
>
> Some failure paths returned directly before the devm cleanup
> action was installed, leaving runtime PM enabled.
>
> Route those failures through a common runtime PM cleanup path.
> Use pm_runtime_resume_and_get() for the initial clock enable.
>
> Signed-off-by: Jiawen Liu <1298662399@qq.com>
> ---
> drivers/spi/spi-nxp-fspi.c | 31 ++++++++++++++++++++++---------
> 1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> index 1e36ae084dd8..d94a2a7b98d4 100644
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c
> @@ -1350,9 +1350,11 @@ static int nxp_fspi_probe(struct platform_device *pdev)
> pm_runtime_use_autosuspend(dev);
>
> /* enable clock */
> - ret = pm_runtime_get_sync(f->dev);
> - if (ret < 0)
> - return dev_err_probe(dev, ret, "Failed to enable clock");
> + ret = pm_runtime_resume_and_get(f->dev);
> + if (ret < 0) {
> + ret = dev_err_probe(dev, ret, "Failed to enable clock");
> + goto err_disable_pm;
> + }
Use PM_RUNTIME_ACQUIRE help macro to avoid all goto
Frank
>
> /* Clear potential interrupts */
> reg = fspi_readl(f, f->iobase + FSPI_INTR);
> @@ -1362,18 +1364,24 @@ static int nxp_fspi_probe(struct platform_device *pdev)
> nxp_fspi_default_setup(f);
>
> ret = pm_runtime_put_sync(dev);
> - if (ret < 0)
> - return dev_err_probe(dev, ret, "Failed to disable clock");
> + if (ret < 0) {
> + ret = dev_err_probe(dev, ret, "Failed to disable clock");
> + goto err_disable_pm;
> + }
>
> init_completion(&f->c);
> ret = devm_request_irq(dev, irq,
> nxp_fspi_irq_handler, 0, pdev->name, f);
> - if (ret)
> - return dev_err_probe(dev, ret, "Failed to request irq\n");
> + if (ret) {
> + ret = dev_err_probe(dev, ret, "Failed to request irq\n");
> + goto err_disable_pm;
> + }
>
> ret = devm_mutex_init(dev, &f->lock);
> - if (ret)
> - return dev_err_probe(dev, ret, "Failed to initialize lock\n");
> + if (ret) {
> + ret = dev_err_probe(dev, ret, "Failed to initialize lock\n");
> + goto err_disable_pm;
> + }
>
> ctlr->bus_num = -1;
> ctlr->num_chipselect = NXP_FSPI_MAX_CHIPSELECT;
> @@ -1389,6 +1397,11 @@ static int nxp_fspi_probe(struct platform_device *pdev)
> return ret;
>
> return devm_spi_register_controller(&pdev->dev, ctlr);
> +
> +err_disable_pm:
> + pm_runtime_dont_use_autosuspend(dev);
> + pm_runtime_disable(dev);
> + return ret;
> }
>
> static int nxp_fspi_runtime_suspend(struct device *dev)
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-06-20 14:09 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260620083931.1120616-1-1298662399@qq.com>
2026-06-20 8:39 ` [PATCH 1/4] spi: atcspi200: return error from failed controller suspend Jiawen Liu
2026-06-20 8:57 ` sashiko-bot
2026-06-20 8:39 ` [PATCH 2/4] spi: bcmbca-hsspi: " Jiawen Liu
2026-06-20 8:54 ` sashiko-bot
2026-06-20 8:39 ` [PATCH 3/4] spi: fsl-dspi: clean up after failed suspend and resume Jiawen Liu
2026-06-20 8:53 ` sashiko-bot
2026-06-20 8:39 ` [PATCH 4/4] spi: nxp-fspi: disable runtime PM on probe failures Jiawen Liu
2026-06-20 8:57 ` sashiko-bot
2026-06-20 14:09 ` Frank Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.