Linux Power Management development
 help / color / mirror / Atom feed
* [PATCH v3 0/2] PM: runtime: Fix sticky -EINVAL after resume callback failure
@ 2026-07-06  8:53 Praveen Talari
  2026-07-06  8:53 ` [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures Praveen Talari
  2026-07-06  8:53 ` [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync() Praveen Talari
  0 siblings, 2 replies; 17+ messages in thread
From: Praveen Talari @ 2026-07-06  8:53 UTC (permalink / raw)
  To: Mark Brown, Dilip Kota, Stephen Boyd, Girish Mahadevan,
	Alok Chauhan, bjorn.andersson, Konrad Dybcio, Rafael J. Wysocki,
	Len Brown, Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson
  Cc: linux-spi, linux-kernel, linux-arm-msm, Mukesh Kumar Savaliya,
	aniket.randive, chandana.chiluveru, jyothi.seerapu, linux-pm,
	driver-core, Praveen Talari

When a runtime PM resume callback returns an error, rpm_callback() sets
power.runtime_error on the device.  All subsequent rpm_resume() calls
then return -EINVAL immediately at the top of the function, permanently
blocking any future resume attempt — including those triggered by
consumers trying to power up their suppliers — until runtime PM is
explicitly re-initialized.

Unlike suspend failures, resume failures should be retryable.  The first
patch fixes this in the core by only setting power.runtime_error when a
suspend callback fails, leaving resume failures transient by nature.

The second patch fixes a pre-existing issue in the spi-geni-qcom driver
that this scenario exposed: pm_runtime_get_sync() was called in
spi_geni_init() without checking the return value, so a resume failure
would silently proceed to access hardware registers on a device that was
not powered up.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
Changes in v3:
- Moved power.runtime_error assignment from rpm_callback() into
  rpm_suspend() at the fail label, per maintainer feedback.

- Link to v2: https://patch.msgid.link/20260703-fix_sticky_-einval_after_pm_runtime_api_failure-v2-0-578b78a0cf46@oss.qualcomm.com

Changes in v2:
- Reworked the fix per maintainer feedback: instead of calling
  pm_runtime_set_suspended() in rpm_get_suppliers(), fix the root cause
  in rpm_callback() by not setting power.runtime_error on resume
  callback failures.
- Link to v1: https://patch.msgid.link/20260702-fix_sticky_-einval_after_pm_runtime_api_failure-v1-0-6ddc317011c0@oss.qualcomm.com

---
Praveen Talari (2):
      PM: runtime: Only set runtime_error on suspend callback failures
      spi: qcom-geni: Fix missing error check on pm_runtime_get_sync()

 drivers/base/power/runtime.c |  6 +++---
 drivers/spi/spi-geni-qcom.c  | 18 ++++++++++--------
 2 files changed, 13 insertions(+), 11 deletions(-)
---
base-commit: 4e5dfb7c84012007c3c7061126491bbc92d71bf1
change-id: 20260625-fix_sticky_-einval_after_pm_runtime_api_failure-6797d0a5c4d0

Best regards,
--  
Praveen Talari <praveen.talari@oss.qualcomm.com>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures
  2026-07-06  8:53 [PATCH v3 0/2] PM: runtime: Fix sticky -EINVAL after resume callback failure Praveen Talari
@ 2026-07-06  8:53 ` Praveen Talari
  2026-07-06 11:49   ` Rafael J. Wysocki (Intel)
  2026-07-09 16:35   ` Mark Brown
  2026-07-06  8:53 ` [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync() Praveen Talari
  1 sibling, 2 replies; 17+ messages in thread
From: Praveen Talari @ 2026-07-06  8:53 UTC (permalink / raw)
  To: Mark Brown, Dilip Kota, Stephen Boyd, Girish Mahadevan,
	Alok Chauhan, bjorn.andersson, Konrad Dybcio, Rafael J. Wysocki,
	Len Brown, Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson
  Cc: linux-spi, linux-kernel, linux-arm-msm, Mukesh Kumar Savaliya,
	aniket.randive, chandana.chiluveru, jyothi.seerapu, linux-pm,
	driver-core, Praveen Talari

When a runtime resume callback returns an error, rpm_callback() sets
power.runtime_error on the device.  This causes all subsequent calls to
rpm_resume() to return -EINVAL immediately at the top of the function
without invoking the callback again, making the failure permanent until
runtime PM is explicitly re-initialized.

Unlike suspend failures, resume failures should be retryable.  If a
device's resume callback fails, there is no reason to permanently block
future resume attempts on that device and all of its consumers.

Fix this by moving the power.runtime_error assignment out of the generic
rpm_callback() and into rpm_suspend() at its fail label, where suspend
callback failures are handled.  Resume callback failures now return the
error to the caller but leave power.runtime_error clear, allowing the
next resume attempt to invoke the callback normally.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/base/power/runtime.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 335288e8b5b3..fab38bc98113 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -469,9 +469,6 @@ static int rpm_callback(int (*cb)(struct device *), struct device *dev)
 	if (retval == -EACCES)
 		retval = -EAGAIN;
 
-	if (retval != -EAGAIN && retval != -EBUSY)
-		dev->power.runtime_error = retval;
-
 	return retval;
 }
 
@@ -751,6 +748,9 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 	dev->power.deferred_resume = false;
 	wake_up_all(&dev->power.wait_queue);
 
+	if (retval != -EAGAIN && retval != -EBUSY)
+		dev->power.runtime_error = retval;
+
 	/*
 	 * On transient errors, if the callback routine failed an autosuspend,
 	 * and if the last_busy time has been updated so that there is a new

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync()
  2026-07-06  8:53 [PATCH v3 0/2] PM: runtime: Fix sticky -EINVAL after resume callback failure Praveen Talari
  2026-07-06  8:53 ` [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures Praveen Talari
@ 2026-07-06  8:53 ` Praveen Talari
  2026-07-06 13:04   ` Konrad Dybcio
                     ` (2 more replies)
  1 sibling, 3 replies; 17+ messages in thread
From: Praveen Talari @ 2026-07-06  8:53 UTC (permalink / raw)
  To: Mark Brown, Dilip Kota, Stephen Boyd, Girish Mahadevan,
	Alok Chauhan, bjorn.andersson, Konrad Dybcio, Rafael J. Wysocki,
	Len Brown, Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson
  Cc: linux-spi, linux-kernel, linux-arm-msm, Mukesh Kumar Savaliya,
	aniket.randive, chandana.chiluveru, jyothi.seerapu, linux-pm,
	driver-core, Praveen Talari

spi_geni_init() calls pm_runtime_get_sync() to power up the device
before accessing hardware registers, but never checks the return value.
If the runtime resume fails, the function silently proceeds to read and
write hardware registers on a device that may not be powered up, leading
to register access faults.

Fix this by replacing pm_runtime_get_sync() with the
PM_RUNTIME_ACQUIRE_IF_ENABLED() macro and checking the result via
PM_RUNTIME_ACQUIRE_ERR(), propagating any error back to the caller
immediately before any hardware access occurs.

Since the macro handles its own cleanup on failure, the out_pm label and
the corresponding pm_runtime_put() call are no longer needed. Replace
all goto out_pm paths with direct return ret statements and remove the
label entirely.

Fixes: 561de45f72bd ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/spi/spi-geni-qcom.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 26e723cfea61..392accfd8515 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -613,25 +613,30 @@ static int spi_geni_init(struct spi_geni_master *mas)
 	u32 spi_tx_cfg, fifo_disable;
 	int ret = -ENXIO;
 
-	pm_runtime_get_sync(mas->dev);
+	PM_RUNTIME_ACQUIRE_IF_ENABLED(mas->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret < 0) {
+		dev_err(mas->dev, "Failed to resume and get %d\n", ret);
+		return ret;
+	}
 
 	proto = geni_se_read_proto(se);
 
 	if (spi->target) {
 		if (proto != GENI_SE_SPI_SLAVE) {
 			dev_err(mas->dev, "Invalid proto %d\n", proto);
-			goto out_pm;
+			return ret;
 		}
 		spi_slv_setup(mas);
 	} else if (proto == GENI_SE_INVALID_PROTO) {
 		ret = geni_load_se_firmware(se, GENI_SE_SPI);
 		if (ret) {
 			dev_err(mas->dev, "spi master firmware load failed ret: %d\n", ret);
-			goto out_pm;
+			return ret;
 		}
 	} else if (proto != GENI_SE_SPI) {
 		dev_err(mas->dev, "Invalid proto %d\n", proto);
-		goto out_pm;
+		return ret;
 	}
 	mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
 
@@ -664,7 +669,7 @@ static int spi_geni_init(struct spi_geni_master *mas)
 			dev_dbg(mas->dev, "Using GPI DMA mode for SPI\n");
 			break;
 		} else if (ret == -EPROBE_DEFER) {
-			goto out_pm;
+			return ret;
 		}
 		/*
 		 * in case of failure to get gpi dma channel, we can still do the
@@ -693,10 +698,7 @@ static int spi_geni_init(struct spi_geni_master *mas)
 		writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
 	}
 
-out_pm:
-	pm_runtime_put(mas->dev);
 	return ret;
-}
 
 static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
 {

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures
  2026-07-06  8:53 ` [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures Praveen Talari
@ 2026-07-06 11:49   ` Rafael J. Wysocki (Intel)
  2026-07-06 13:21     ` Mark Brown
  2026-07-09 16:35   ` Mark Brown
  1 sibling, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki (Intel) @ 2026-07-06 11:49 UTC (permalink / raw)
  To: Praveen Talari
  Cc: Mark Brown, Dilip Kota, Stephen Boyd, Girish Mahadevan,
	Alok Chauhan, bjorn.andersson, Konrad Dybcio, Rafael J. Wysocki,
	Len Brown, Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson, linux-spi, linux-kernel, linux-arm-msm,
	Mukesh Kumar Savaliya, aniket.randive, chandana.chiluveru,
	jyothi.seerapu, linux-pm, driver-core

On Mon, Jul 6, 2026 at 10:53 AM Praveen Talari
<praveen.talari@oss.qualcomm.com> wrote:
>
> When a runtime resume callback returns an error, rpm_callback() sets
> power.runtime_error on the device.  This causes all subsequent calls to
> rpm_resume() to return -EINVAL immediately at the top of the function
> without invoking the callback again, making the failure permanent until
> runtime PM is explicitly re-initialized.
>
> Unlike suspend failures, resume failures should be retryable.  If a
> device's resume callback fails, there is no reason to permanently block
> future resume attempts on that device and all of its consumers.
>
> Fix this by moving the power.runtime_error assignment out of the generic
> rpm_callback() and into rpm_suspend() at its fail label, where suspend
> callback failures are handled.  Resume callback failures now return the
> error to the caller but leave power.runtime_error clear, allowing the
> next resume attempt to invoke the callback normally.
>
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
>  drivers/base/power/runtime.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 335288e8b5b3..fab38bc98113 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -469,9 +469,6 @@ static int rpm_callback(int (*cb)(struct device *), struct device *dev)
>         if (retval == -EACCES)
>                 retval = -EAGAIN;
>
> -       if (retval != -EAGAIN && retval != -EBUSY)
> -               dev->power.runtime_error = retval;
> -
>         return retval;
>  }
>
> @@ -751,6 +748,9 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>         dev->power.deferred_resume = false;
>         wake_up_all(&dev->power.wait_queue);
>
> +       if (retval != -EAGAIN && retval != -EBUSY)
> +               dev->power.runtime_error = retval;
> +
>         /*
>          * On transient errors, if the callback routine failed an autosuspend,
>          * and if the last_busy time has been updated so that there is a new
>
> --

This is fine with me and I think that it's a quite significant change
for runtime PM, so I'd prefer it to go in through my tree.

I can take the other patch too if it gets ACKed for me.

Thanks!

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync()
  2026-07-06  8:53 ` [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync() Praveen Talari
@ 2026-07-06 13:04   ` Konrad Dybcio
  2026-07-09 14:54   ` Rafael J. Wysocki (Intel)
  2026-07-09 18:29   ` Mark Brown
  2 siblings, 0 replies; 17+ messages in thread
From: Konrad Dybcio @ 2026-07-06 13:04 UTC (permalink / raw)
  To: Praveen Talari, Mark Brown, Dilip Kota, Stephen Boyd,
	Girish Mahadevan, Alok Chauhan, bjorn.andersson,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Greg Kroah-Hartman,
	Danilo Krummrich, Douglas Anderson
  Cc: linux-spi, linux-kernel, linux-arm-msm, Mukesh Kumar Savaliya,
	aniket.randive, chandana.chiluveru, jyothi.seerapu, linux-pm,
	driver-core

On 7/6/26 10:53 AM, Praveen Talari wrote:
> spi_geni_init() calls pm_runtime_get_sync() to power up the device
> before accessing hardware registers, but never checks the return value.
> If the runtime resume fails, the function silently proceeds to read and
> write hardware registers on a device that may not be powered up, leading
> to register access faults.
> 
> Fix this by replacing pm_runtime_get_sync() with the
> PM_RUNTIME_ACQUIRE_IF_ENABLED() macro and checking the result via
> PM_RUNTIME_ACQUIRE_ERR(), propagating any error back to the caller
> immediately before any hardware access occurs.
> 
> Since the macro handles its own cleanup on failure, the out_pm label and
> the corresponding pm_runtime_put() call are no longer needed. Replace
> all goto out_pm paths with direct return ret statements and remove the
> label entirely.
> 
> Fixes: 561de45f72bd ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures
  2026-07-06 11:49   ` Rafael J. Wysocki (Intel)
@ 2026-07-06 13:21     ` Mark Brown
  2026-07-06 14:10       ` Rafael J. Wysocki (Intel)
  0 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2026-07-06 13:21 UTC (permalink / raw)
  To: Rafael J. Wysocki (Intel)
  Cc: Praveen Talari, Dilip Kota, Stephen Boyd, Girish Mahadevan,
	Alok Chauhan, bjorn.andersson, Konrad Dybcio, Len Brown,
	Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson, linux-spi, linux-kernel, linux-arm-msm,
	Mukesh Kumar Savaliya, aniket.randive, chandana.chiluveru,
	jyothi.seerapu, linux-pm, driver-core

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

On Mon, Jul 06, 2026 at 01:49:07PM +0200, Rafael J. Wysocki (Intel) wrote:
> On Mon, Jul 6, 2026 at 10:53 AM Praveen Talari

> >         /*
> >          * On transient errors, if the callback routine failed an autosuspend,
> >          * and if the last_busy time has been updated so that there is a new
> >
> > --

> This is fine with me and I think that it's a quite significant change
> for runtime PM, so I'd prefer it to go in through my tree.

> I can take the other patch too if it gets ACKed for me.

I've got a bunch of other stuff in flight for that driver - I'll need to
check but just in case perhaps a tag I could pull in?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures
  2026-07-06 13:21     ` Mark Brown
@ 2026-07-06 14:10       ` Rafael J. Wysocki (Intel)
  2026-07-06 17:30         ` Mark Brown
  0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki (Intel) @ 2026-07-06 14:10 UTC (permalink / raw)
  To: Mark Brown
  Cc: Rafael J. Wysocki (Intel), Praveen Talari, Dilip Kota,
	Stephen Boyd, Girish Mahadevan, Alok Chauhan, bjorn.andersson,
	Konrad Dybcio, Len Brown, Pavel Machek, Greg Kroah-Hartman,
	Danilo Krummrich, Douglas Anderson, linux-spi, linux-kernel,
	linux-arm-msm, Mukesh Kumar Savaliya, aniket.randive,
	chandana.chiluveru, jyothi.seerapu, linux-pm, driver-core

On Mon, Jul 6, 2026 at 3:21 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jul 06, 2026 at 01:49:07PM +0200, Rafael J. Wysocki (Intel) wrote:
> > On Mon, Jul 6, 2026 at 10:53 AM Praveen Talari
>
> > >         /*
> > >          * On transient errors, if the callback routine failed an autosuspend,
> > >          * and if the last_busy time has been updated so that there is a new
> > >
> > > --
>
> > This is fine with me and I think that it's a quite significant change
> > for runtime PM, so I'd prefer it to go in through my tree.
>
> > I can take the other patch too if it gets ACKed for me.
>
> I've got a bunch of other stuff in flight for that driver - I'll need to
> check but just in case perhaps a tag I could pull in?

I'm wondering though if the patches in the series really depend on each other.

There doesn't appear to be any code dependency between them.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures
  2026-07-06 14:10       ` Rafael J. Wysocki (Intel)
@ 2026-07-06 17:30         ` Mark Brown
  2026-07-09 14:52           ` Rafael J. Wysocki (Intel)
  0 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2026-07-06 17:30 UTC (permalink / raw)
  To: Rafael J. Wysocki (Intel)
  Cc: Praveen Talari, Dilip Kota, Stephen Boyd, Girish Mahadevan,
	Alok Chauhan, bjorn.andersson, Konrad Dybcio, Len Brown,
	Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson, linux-spi, linux-kernel, linux-arm-msm,
	Mukesh Kumar Savaliya, aniket.randive, chandana.chiluveru,
	jyothi.seerapu, linux-pm, driver-core

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

On Mon, Jul 06, 2026 at 04:10:34PM +0200, Rafael J. Wysocki (Intel) wrote:
> On Mon, Jul 6, 2026 at 3:21 PM Mark Brown <broonie@kernel.org> wrote:

> > I've got a bunch of other stuff in flight for that driver - I'll need to
> > check but just in case perhaps a tag I could pull in?

> I'm wondering though if the patches in the series really depend on each other.

> There doesn't appear to be any code dependency between them.

That would also work, a bit annoying for any bisects possibly but so
long as we land both in the same release we should be good.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures
  2026-07-06 17:30         ` Mark Brown
@ 2026-07-09 14:52           ` Rafael J. Wysocki (Intel)
  0 siblings, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki (Intel) @ 2026-07-09 14:52 UTC (permalink / raw)
  To: Mark Brown, Praveen Talari
  Cc: Dilip Kota, Stephen Boyd, Girish Mahadevan, Alok Chauhan,
	bjorn.andersson, Konrad Dybcio, Len Brown, Pavel Machek,
	Greg Kroah-Hartman, Danilo Krummrich, Douglas Anderson, linux-spi,
	linux-kernel, linux-arm-msm, Mukesh Kumar Savaliya,
	aniket.randive, chandana.chiluveru, jyothi.seerapu, linux-pm,
	driver-core

On Mon, Jul 6, 2026 at 7:30 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jul 06, 2026 at 04:10:34PM +0200, Rafael J. Wysocki (Intel) wrote:
> > On Mon, Jul 6, 2026 at 3:21 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > I've got a bunch of other stuff in flight for that driver - I'll need to
> > > check but just in case perhaps a tag I could pull in?
>
> > I'm wondering though if the patches in the series really depend on each other.
>
> > There doesn't appear to be any code dependency between them.
>
> That would also work, a bit annoying for any bisects possibly but so
> long as we land both in the same release we should be good.

So I've applied the first patch only as 7.3 material.

If you need it to be exposed on an immutable branch, please let me know.

Thanks!

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync()
  2026-07-06  8:53 ` [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync() Praveen Talari
  2026-07-06 13:04   ` Konrad Dybcio
@ 2026-07-09 14:54   ` Rafael J. Wysocki (Intel)
  2026-07-09 18:29   ` Mark Brown
  2 siblings, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki (Intel) @ 2026-07-09 14:54 UTC (permalink / raw)
  To: Praveen Talari
  Cc: Mark Brown, Dilip Kota, Stephen Boyd, Girish Mahadevan,
	Alok Chauhan, bjorn.andersson, Konrad Dybcio, Rafael J. Wysocki,
	Len Brown, Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson, linux-spi, linux-kernel, linux-arm-msm,
	Mukesh Kumar Savaliya, aniket.randive, chandana.chiluveru,
	jyothi.seerapu, linux-pm, driver-core

On Mon, Jul 6, 2026 at 10:53 AM Praveen Talari
<praveen.talari@oss.qualcomm.com> wrote:
>
> spi_geni_init() calls pm_runtime_get_sync() to power up the device
> before accessing hardware registers, but never checks the return value.
> If the runtime resume fails, the function silently proceeds to read and
> write hardware registers on a device that may not be powered up, leading
> to register access faults.
>
> Fix this by replacing pm_runtime_get_sync() with the
> PM_RUNTIME_ACQUIRE_IF_ENABLED() macro and checking the result via
> PM_RUNTIME_ACQUIRE_ERR(), propagating any error back to the caller
> immediately before any hardware access occurs.
>
> Since the macro handles its own cleanup on failure, the out_pm label and
> the corresponding pm_runtime_put() call are no longer needed. Replace
> all goto out_pm paths with direct return ret statements and remove the
> label entirely.
>
> Fixes: 561de45f72bd ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>

Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>

(no issues found)

> ---
>  drivers/spi/spi-geni-qcom.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 26e723cfea61..392accfd8515 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -613,25 +613,30 @@ static int spi_geni_init(struct spi_geni_master *mas)
>         u32 spi_tx_cfg, fifo_disable;
>         int ret = -ENXIO;
>
> -       pm_runtime_get_sync(mas->dev);
> +       PM_RUNTIME_ACQUIRE_IF_ENABLED(mas->dev, pm);
> +       ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +       if (ret < 0) {
> +               dev_err(mas->dev, "Failed to resume and get %d\n", ret);
> +               return ret;
> +       }
>
>         proto = geni_se_read_proto(se);
>
>         if (spi->target) {
>                 if (proto != GENI_SE_SPI_SLAVE) {
>                         dev_err(mas->dev, "Invalid proto %d\n", proto);
> -                       goto out_pm;
> +                       return ret;
>                 }
>                 spi_slv_setup(mas);
>         } else if (proto == GENI_SE_INVALID_PROTO) {
>                 ret = geni_load_se_firmware(se, GENI_SE_SPI);
>                 if (ret) {
>                         dev_err(mas->dev, "spi master firmware load failed ret: %d\n", ret);
> -                       goto out_pm;
> +                       return ret;
>                 }
>         } else if (proto != GENI_SE_SPI) {
>                 dev_err(mas->dev, "Invalid proto %d\n", proto);
> -               goto out_pm;
> +               return ret;
>         }
>         mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
>
> @@ -664,7 +669,7 @@ static int spi_geni_init(struct spi_geni_master *mas)
>                         dev_dbg(mas->dev, "Using GPI DMA mode for SPI\n");
>                         break;
>                 } else if (ret == -EPROBE_DEFER) {
> -                       goto out_pm;
> +                       return ret;
>                 }
>                 /*
>                  * in case of failure to get gpi dma channel, we can still do the
> @@ -693,10 +698,7 @@ static int spi_geni_init(struct spi_geni_master *mas)
>                 writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>         }
>
> -out_pm:
> -       pm_runtime_put(mas->dev);
>         return ret;
> -}
>
>  static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
>  {
>
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures
  2026-07-06  8:53 ` [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures Praveen Talari
  2026-07-06 11:49   ` Rafael J. Wysocki (Intel)
@ 2026-07-09 16:35   ` Mark Brown
  2026-07-09 17:35     ` Rafael J. Wysocki (Intel)
  1 sibling, 1 reply; 17+ messages in thread
From: Mark Brown @ 2026-07-09 16:35 UTC (permalink / raw)
  To: Praveen Talari
  Cc: Dilip Kota, Stephen Boyd, Girish Mahadevan, Alok Chauhan,
	bjorn.andersson, Konrad Dybcio, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson, linux-spi, linux-kernel, linux-arm-msm,
	Mukesh Kumar Savaliya, aniket.randive, chandana.chiluveru,
	jyothi.seerapu, linux-pm, driver-core

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

On Mon, Jul 06, 2026 at 02:23:09PM +0530, Praveen Talari wrote:
> When a runtime resume callback returns an error, rpm_callback() sets
> power.runtime_error on the device.  This causes all subsequent calls to
> rpm_resume() to return -EINVAL immediately at the top of the function
> without invoking the callback again, making the failure permanent until
> runtime PM is explicitly re-initialized.
> 
> Unlike suspend failures, resume failures should be retryable.  If a
> device's resume callback fails, there is no reason to permanently block
> future resume attempts on that device and all of its consumers.

runtime_pm.rst still says

  * If the resume callback returns an error code, the PM core regards this as a
    fatal error and will refuse to run the helper functions described in Section
    4 for the device, until its status is directly set to either 'active', or
    'suspended' (by means of special helper functions provided by the PM core
    for this purpose).

which needs an update now?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures
  2026-07-09 16:35   ` Mark Brown
@ 2026-07-09 17:35     ` Rafael J. Wysocki (Intel)
  2026-07-09 17:38       ` Mark Brown
  0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki (Intel) @ 2026-07-09 17:35 UTC (permalink / raw)
  To: Mark Brown
  Cc: Praveen Talari, Stephen Boyd, Girish Mahadevan, Alok Chauhan,
	bjorn.andersson, Konrad Dybcio, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson, linux-spi, linux-kernel, linux-arm-msm,
	Mukesh Kumar Savaliya, aniket.randive, chandana.chiluveru,
	jyothi.seerapu, linux-pm, driver-core

On Thu, Jul 9, 2026 at 6:35 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jul 06, 2026 at 02:23:09PM +0530, Praveen Talari wrote:
> > When a runtime resume callback returns an error, rpm_callback() sets
> > power.runtime_error on the device.  This causes all subsequent calls to
> > rpm_resume() to return -EINVAL immediately at the top of the function
> > without invoking the callback again, making the failure permanent until
> > runtime PM is explicitly re-initialized.
> >
> > Unlike suspend failures, resume failures should be retryable.  If a
> > device's resume callback fails, there is no reason to permanently block
> > future resume attempts on that device and all of its consumers.
>
> runtime_pm.rst still says
>
>   * If the resume callback returns an error code, the PM core regards this as a
>     fatal error and will refuse to run the helper functions described in Section
>     4 for the device, until its status is directly set to either 'active', or
>     'suspended' (by means of special helper functions provided by the PM core
>     for this purpose).
>
> which needs an update now?

Yeah, it would be good to update this one.

That can be done in a separate patch though as far as I'm concerned.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures
  2026-07-09 17:35     ` Rafael J. Wysocki (Intel)
@ 2026-07-09 17:38       ` Mark Brown
  0 siblings, 0 replies; 17+ messages in thread
From: Mark Brown @ 2026-07-09 17:38 UTC (permalink / raw)
  To: Rafael J. Wysocki (Intel)
  Cc: Praveen Talari, Stephen Boyd, Girish Mahadevan, Alok Chauhan,
	bjorn.andersson, Konrad Dybcio, Len Brown, Pavel Machek,
	Greg Kroah-Hartman, Danilo Krummrich, Douglas Anderson, linux-spi,
	linux-kernel, linux-arm-msm, Mukesh Kumar Savaliya,
	aniket.randive, chandana.chiluveru, jyothi.seerapu, linux-pm,
	driver-core

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

On Thu, Jul 09, 2026 at 07:35:18PM +0200, Rafael J. Wysocki (Intel) wrote:
> On Thu, Jul 9, 2026 at 6:35 PM Mark Brown <broonie@kernel.org> wrote:

> > runtime_pm.rst still says

> >   * If the resume callback returns an error code, the PM core regards this as a
> >     fatal error and will refuse to run the helper functions described in Section
> >     4 for the device, until its status is directly set to either 'active', or
> >     'suspended' (by means of special helper functions provided by the PM core
> >     for this purpose).

> > which needs an update now?

> Yeah, it would be good to update this one.

> That can be done in a separate patch though as far as I'm concerned.

Oh, totally - didn't mean to suggest we should block the original
change.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync()
  2026-07-06  8:53 ` [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync() Praveen Talari
  2026-07-06 13:04   ` Konrad Dybcio
  2026-07-09 14:54   ` Rafael J. Wysocki (Intel)
@ 2026-07-09 18:29   ` Mark Brown
  2026-07-10  4:11     ` Praveen Talari
  2 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2026-07-09 18:29 UTC (permalink / raw)
  To: Praveen Talari
  Cc: Dilip Kota, Stephen Boyd, Girish Mahadevan, Alok Chauhan,
	bjorn.andersson, Konrad Dybcio, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson, linux-spi, linux-kernel, linux-arm-msm,
	Mukesh Kumar Savaliya, aniket.randive, chandana.chiluveru,
	jyothi.seerapu, linux-pm, driver-core

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

On Mon, Jul 06, 2026 at 02:23:10PM +0530, Praveen Talari wrote:
> spi_geni_init() calls pm_runtime_get_sync() to power up the device
> before accessing hardware registers, but never checks the return value.

> @@ -693,10 +698,7 @@ static int spi_geni_init(struct spi_geni_master *mas)
>  		writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>  	}
>  
> -out_pm:
> -	pm_runtime_put(mas->dev);
>  	return ret;
> -}
>  
>  static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)

The above breaks the build due to removing the closing brace for the
function:

/build/stage/linux/drivers/spi/spi-geni-qcom.c: In function ‘spi_geni_init’:
/build/stage/linux/drivers/spi/spi-geni-qcom.c:712:21: error: invalid storage class for function ‘geni_byte_per_fifo_word’
  712 | static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
      |                     ^~~~~~~~~~~~~~~~~~~~~~~
/build/stage/linux/drivers/spi/spi-geni-qcom.c:726:13: error: invalid storage class for function ‘geni_spi_handle_tx’
  726 | static bool geni_spi_handle_tx(struct spi_geni_master *mas)
      |             ^~~~~~~~~~~~~~~~~~
/build/stage/linux/drivers/spi/spi-geni-qcom.c:764:13: error: invalid storage class for function ‘geni_spi_handle_rx’
  764 | static void geni_spi_handle_rx(struct spi_geni_master *mas)
      |             ^~~~~~~~~~~~~~~~~~
/build/stage/linux/drivers/spi/spi-geni-qcom.c:808:12: error: invalid storage class for function ‘setup_se_xfer’
  808 | static int setup_se_xfer(struct spi_transfer *xfer,

and so on.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync()
  2026-07-09 18:29   ` Mark Brown
@ 2026-07-10  4:11     ` Praveen Talari
  2026-07-10  9:23       ` Mark Brown
  0 siblings, 1 reply; 17+ messages in thread
From: Praveen Talari @ 2026-07-10  4:11 UTC (permalink / raw)
  To: Mark Brown
  Cc: Dilip Kota, Stephen Boyd, Girish Mahadevan, Alok Chauhan,
	bjorn.andersson, Konrad Dybcio, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson, linux-spi, linux-kernel, linux-arm-msm,
	Mukesh Kumar Savaliya, aniket.randive, chandana.chiluveru,
	jyothi.seerapu, linux-pm, driver-core

Hi Mark,

On 09-07-2026 23:59, Mark Brown wrote:
> On Mon, Jul 06, 2026 at 02:23:10PM +0530, Praveen Talari wrote:
>> spi_geni_init() calls pm_runtime_get_sync() to power up the device
>> before accessing hardware registers, but never checks the return value.
>> @@ -693,10 +698,7 @@ static int spi_geni_init(struct spi_geni_master *mas)
>>   		writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>>   	}
>>   
>> -out_pm:
>> -	pm_runtime_put(mas->dev);
>>   	return ret;
>> -}
>>   
>>   static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
> The above breaks the build due to removing the closing brace for the
> function:
Its my bad. It appears this was inadvertently removed in v3, although 
there were no issues up to v2.

Please let me know if you can add it while picking up this change, or if 
you would prefer that I send a v4 with the update included.

[PATCH v2 2/2] spi: qcom-geni: Fix missing error check on 
pm_runtime_get_sync() - Praveen Talari


Thanks,

Praveen Talari

>
> /build/stage/linux/drivers/spi/spi-geni-qcom.c: In function ‘spi_geni_init’:
> /build/stage/linux/drivers/spi/spi-geni-qcom.c:712:21: error: invalid storage class for function ‘geni_byte_per_fifo_word’
>    712 | static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
>        |                     ^~~~~~~~~~~~~~~~~~~~~~~
> /build/stage/linux/drivers/spi/spi-geni-qcom.c:726:13: error: invalid storage class for function ‘geni_spi_handle_tx’
>    726 | static bool geni_spi_handle_tx(struct spi_geni_master *mas)
>        |             ^~~~~~~~~~~~~~~~~~
> /build/stage/linux/drivers/spi/spi-geni-qcom.c:764:13: error: invalid storage class for function ‘geni_spi_handle_rx’
>    764 | static void geni_spi_handle_rx(struct spi_geni_master *mas)
>        |             ^~~~~~~~~~~~~~~~~~
> /build/stage/linux/drivers/spi/spi-geni-qcom.c:808:12: error: invalid storage class for function ‘setup_se_xfer’
>    808 | static int setup_se_xfer(struct spi_transfer *xfer,
>
> and so on.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync()
  2026-07-10  4:11     ` Praveen Talari
@ 2026-07-10  9:23       ` Mark Brown
  2026-07-10  9:43         ` Praveen Talari
  0 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2026-07-10  9:23 UTC (permalink / raw)
  To: Praveen Talari
  Cc: Dilip Kota, Stephen Boyd, Girish Mahadevan, Alok Chauhan,
	bjorn.andersson, Konrad Dybcio, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson, linux-spi, linux-kernel, linux-arm-msm,
	Mukesh Kumar Savaliya, aniket.randive, chandana.chiluveru,
	jyothi.seerapu, linux-pm, driver-core

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

On Fri, Jul 10, 2026 at 09:41:26AM +0530, Praveen Talari wrote:
> On 09-07-2026 23:59, Mark Brown wrote:

> > >   static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
> > The above breaks the build due to removing the closing brace for the
> > function:

> Its my bad. It appears this was inadvertently removed in v3, although there
> were no issues up to v2.

> Please let me know if you can add it while picking up this change, or if you
> would prefer that I send a v4 with the update included.

Please send a version that builds.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync()
  2026-07-10  9:23       ` Mark Brown
@ 2026-07-10  9:43         ` Praveen Talari
  0 siblings, 0 replies; 17+ messages in thread
From: Praveen Talari @ 2026-07-10  9:43 UTC (permalink / raw)
  To: Mark Brown
  Cc: Dilip Kota, Stephen Boyd, Girish Mahadevan, Alok Chauhan,
	bjorn.andersson, Konrad Dybcio, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Greg Kroah-Hartman, Danilo Krummrich,
	Douglas Anderson, linux-spi, linux-kernel, linux-arm-msm,
	Mukesh Kumar Savaliya, aniket.randive, chandana.chiluveru,
	jyothi.seerapu, linux-pm, driver-core

Hi Mark,

On 10-07-2026 14:53, Mark Brown wrote:
> On Fri, Jul 10, 2026 at 09:41:26AM +0530, Praveen Talari wrote:
>> On 09-07-2026 23:59, Mark Brown wrote:
>>>>    static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
>>> The above breaks the build due to removing the closing brace for the
>>> function:
>> Its my bad. It appears this was inadvertently removed in v3, although there
>> were no issues up to v2.
>> Please let me know if you can add it while picking up this change, or if you
>> would prefer that I send a v4 with the update included.
> Please send a version that builds.

Sure, will send latest version.

Thanks,

Praveen Talari


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2026-07-10  9:43 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-06  8:53 [PATCH v3 0/2] PM: runtime: Fix sticky -EINVAL after resume callback failure Praveen Talari
2026-07-06  8:53 ` [PATCH v3 1/2] PM: runtime: Only set runtime_error on suspend callback failures Praveen Talari
2026-07-06 11:49   ` Rafael J. Wysocki (Intel)
2026-07-06 13:21     ` Mark Brown
2026-07-06 14:10       ` Rafael J. Wysocki (Intel)
2026-07-06 17:30         ` Mark Brown
2026-07-09 14:52           ` Rafael J. Wysocki (Intel)
2026-07-09 16:35   ` Mark Brown
2026-07-09 17:35     ` Rafael J. Wysocki (Intel)
2026-07-09 17:38       ` Mark Brown
2026-07-06  8:53 ` [PATCH v3 2/2] spi: qcom-geni: Fix missing error check on pm_runtime_get_sync() Praveen Talari
2026-07-06 13:04   ` Konrad Dybcio
2026-07-09 14:54   ` Rafael J. Wysocki (Intel)
2026-07-09 18:29   ` Mark Brown
2026-07-10  4:11     ` Praveen Talari
2026-07-10  9:23       ` Mark Brown
2026-07-10  9:43         ` Praveen Talari

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox