[PATCH v7 0/2] i2c: i2c-qcom-geni: serve transfers during early resume

[PATCH v7 0/2] i2c: i2c-qcom-geni: serve transfers during early resume

[Mukesh Kumar Savaliya]

I2C transfers issued during the resume_noirq phase can fail on systems
using GENI I2C controllers with GPI DMA.

Some devices require I2C communication before their resume sequence can
complete. One example is a USB Ethernet device attached through PCIe,
where device configuration must be restored over I2C before PCIe link
initialization can proceed. Since such accesses may occur from
resume_noirq(), the I2C controller and its DMA backend must be capable
of servicing transfers at that stage.

GENI I2C transfers depend on interrupt-driven completion. During system
resume, both the GENI controller interrupt and the GPI DMA interrupt may
remain unavailable until the normal interrupt resume phase, preventing
transfer completion during early resume. Additionally, runtime PM may
still be disabled when the I2C transfer path is entered, causing
pm_runtime_get_sync() to return -EACCES and preventing controller
resources from being enabled.

Address these issues by:
  - Allowing the GPI DMA interrupt to resume early and remain active
    across system suspend/resume transitions.
  - Allowing the GENI I2C interrupt to operate during early resume and
    restoring runtime PM when necessary from resume_noirq().

With these changes, GENI I2C transfers can complete successfully during
the resume_noirq phase, allowing dependent devices to finish their
resume sequence without waiting for the regular interrupt resume stage.

Co-developed-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
Signed-off-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Acked-by: Aniket Randive <aniketrandive@oss.qualcomm.com>
----
v6->v7 :
- Separated gpi.c file change into separate patch due to dma engine subsystem.
- Added cover letter as patches increased to two from one.
- Added acked-by tag recieved in V6 patch.
Link to V6: https://lore.kernel.org/all/b7404cdb-7c67-40b0-8124-d4977a8ed3cf@oss.qualcomm.com/

---
v5->v6 :
- Modified commit log to start with problem description as suggested by Bjorn.
- Moved to new implementation of the logic while earlier replied to comments on
  older design and considers latest fix added recently.
- Made change generic to I2C including GPI mode transfer, this was not done earlier.
- Changed email address to oss.qualcomm.com domain.
Link to V5: https://lore.kernel.org/lkml/20241227130236.755794-1-quic_msavaliy@quicinc.com/

---
v4->v5:
- Commit log enhanced considering Bjorn's comments by explaining PCIe usecase.
- Enhanced comment with reason when using pm_runtime_force_resume().
- Corrected IS_ENABLED(CONFIG_PM) condition inside geni_i2c_xfer().
- Improved debug log as per Bjorn's suggestions during suspend, resume.
- Reverted back comment before devm_request_irq().
Link to V4: https://lore.kernel.org/lkml/bd699719-4958-445a-a685-4b5f6a8ad81f@quicinc.com/

---
v3->v4 :
 - Enhanced commit log by explaining client usecase scenario during early resume.
 - Covered 'usage_count' of 'struct dev_pm_info' under CONFIG_PM to compile non PM CONFIG.
Link to V3: https://lore.kernel.org/all/20241119143031.3331753-1-quic_msavaliy@quicinc.com/T/

---
v2 -> v3:
 - Updated exact usecase and scenario in the commit log description.
 - Removed bulleted points from technical description, added details in free flow.
 - Used pm_runtime_force_resume/suspend() instead customized local implementation.
 - Added debug log after pm_runtime_force_suspend().
Link to V2: https://lore.kernel.org/lkml/202410132233.P25W2vKq-lkp@intel.com/T/

---
v1 -> v2:
 - Changed gi2c->se.dev to dev during dev_dbg() calls.
 - Addressed review comments from Andi and Bjorn.
 - Returned 0 instead garbage inside geni_i2c_force_resume().
 - Added comments explaining forced resume transfer when runtime PM
   remains disabled.
Link to V1: https://patches.linaro.org/project/linux-i2c/patch/20240328123743.1713696-1-quic_msavaliy@quicinc.com/

----
Mukesh Kumar Savaliya (2):
  i2c: qcom-geni: Handle runtime PM disabled state during early resume
  dmaengine: qcom-gpi: Keep GPI interrupt active during system resume

 drivers/dma/qcom/gpi.c             |  3 ++-
 drivers/i2c/busses/i2c-qcom-geni.c | 12 +++++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v7 1/2] i2c: qcom-geni: Handle runtime PM disabled state during early resume

[Mukesh Kumar Savaliya]

During the noirq resume phase, the GENI I2C controller may receive
transfer requests before runtime PM has been fully restored. In this
window pm_runtime_get_sync() can return -EACCES, causing transfers to
fail even though the controller is in the process of resuming.

Treat -EACCES as a transient condition and allow the transfer path to
proceed while preserving existing error handling for other runtime PM
failures.

Also enable runtime PM in the resume_noirq callback when it remains
disabled and allow the controller interrupt to remain active across
system suspend/resume transitions by requesting it with
IRQF_NO_SUSPEND and IRQF_EARLY_RESUME.

These changes prevent spurious transfer failures during early resume
and allow the adapter to become operational before the normal resume
phase completes.

Co-developed-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
Signed-off-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Acked-by: Aniket Randive <aniketrandive@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 96dbf04138be..4bc00922cd97 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -917,6 +917,10 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
 	gi2c->err = 0;
 	reinit_completion(&gi2c->done);
 	ret = pm_runtime_get_sync(gi2c->se.dev);
+	if (ret == -EACCES) {
+		dev_warn(gi2c->se.dev, "Runtime PM is disabled:%d\n", ret);
+		ret = 0;
+	}
 	if (ret < 0) {
 		dev_err(gi2c->se.dev, "error turning SE resources:%d\n", ret);
 		pm_runtime_put_noidle(gi2c->se.dev);
@@ -1115,7 +1119,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		return ret;
 
 	/* Keep interrupts disabled initially to allow for low-power modes */
-	ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_NO_AUTOEN,
+	ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq,
+			       IRQF_NO_AUTOEN | IRQF_NO_SUSPEND | IRQF_EARLY_RESUME,
 			       dev_name(dev), gi2c);
 	if (ret)
 		return dev_err_probe(dev, ret,
@@ -1223,7 +1228,12 @@ static int __maybe_unused geni_i2c_resume_noirq(struct device *dev)
 	if (ret)
 		return ret;
 
+	/* Enforced disable_depth = 0 to actually enable runtime PM during noirq phase */
+	if (!pm_runtime_enabled(dev))
+		pm_runtime_enable(dev);
+
 	i2c_mark_adapter_resumed(&gi2c->adap);
+
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH v7 1/2] i2c: qcom-geni: Handle runtime PM disabled state during early resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Calling pm_runtime_enable() in the resume_noirq callback causes an unbalanced disable_depth, triggering a kernel warning during early resume.
- [High] Treating -EACCES from pm_runtime_get_sync() as success allows unclocked register accesses on powered-off hardware, risking fatal bus errors.
--

--- Patch [1]: [PATCH v7 1/2] i2c: qcom-geni: Handle runtime PM disabled state during early resume ---
commit 5b7e2f3ccf132073e5ef0e9e9ebe44fb7923a932
Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

i2c: qcom-geni: Handle runtime PM disabled state during early resume

During the noirq resume phase, the GENI I2C controller may receive transfer
requests before runtime PM has been fully restored. This patch treats the
resulting -EACCES error from pm_runtime_get_sync() as a transient condition
and allows the transfer path to proceed. It also manually enables runtime
PM in the resume_noirq callback.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 96dbf04138bee..4bc00922cd97b 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -917,6 +917,10 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>  	gi2c->err = 0;
>  	reinit_completion(&gi2c->done);
>  	ret = pm_runtime_get_sync(gi2c->se.dev);
> +	if (ret == -EACCES) {
> +		dev_warn(gi2c->se.dev, "Runtime PM is disabled:%d\n", ret);
> +		ret = 0;
> +	}

[Severity: High]
Does treating -EACCES as success risk causing fatal bus errors?

If an I2C client device issues a transfer during the system suspend late
or noirq phases before the controller's noirq resume callback runs,
pm_runtime_get_sync() returns -EACCES because the hardware is in
RPM_SUSPENDED (power domain and clocks are off).

By overriding the error and forcing ret = 0, the synchronization barrier
is bypassed. The driver will then proceed to write to unclocked hardware
registers, which can trigger a synchronous external abort.

[ ... ]

> @@ -1223,7 +1228,12 @@ static int __maybe_unused geni_i2c_resume_noirq(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/* Enforced disable_depth = 0 to actually enable runtime PM during noirq phase */
> +	if (!pm_runtime_enabled(dev))
> +		pm_runtime_enable(dev);
> +
>  	i2c_mark_adapter_resumed(&gi2c->adap);

[Severity: High]
Will this unbalance the disable_depth counter and trigger a kernel warning?

The PM core's device_suspend_late() disables runtime PM, incrementing
disable_depth. Calling pm_runtime_enable() here prematurely decrements
the counter to 0.

When the PM core subsequently calls pm_runtime_enable() in
device_resume_early(), it will detect an underflow and unconditionally
print the "Unbalanced pm_runtime_enable!" warning on every system
resume cycle.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709134623.1724212-1-mukesh.savaliya@oss.qualcomm.com?part=1

[PATCH v7 2/2] dmaengine: qcom-gpi: Keep GPI interrupt active during system resume

[Mukesh Kumar Savaliya]

GPI DMA clients may initiate transfers during the early stages of
system resume before the normal IRQ resume phase has completed.
However, the GPI interrupt is currently suspended during system
sleep, preventing transfer completion notifications from being
delivered until later in the resume sequence.

Request the GPI interrupt with IRQF_NO_SUSPEND and IRQF_EARLY_RESUME
to keep it available across suspend/resume transitions and allow
interrupt handling to resume during the noirq phase.

This ensures DMA completion events can be delivered to clients that
become operational before the normal resume phase completes

Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Acked-by: Aniket Randive <aniketrandive@oss.qualcomm.com>
---
 drivers/dma/qcom/gpi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index a5055a6273af..29872b6cb2c7 100644
--- a/drivers/dma/qcom/gpi.c
+++ b/drivers/dma/qcom/gpi.c
@@ -615,7 +615,8 @@ static int gpi_config_interrupts(struct gpii *gpii, enum gpii_irq_settings setti
 
 	if (!gpii->configured_irq) {
 		ret = devm_request_irq(gpii->gpi_dev->dev, gpii->irq,
-				       gpi_handle_irq, IRQF_TRIGGER_HIGH,
+				       gpi_handle_irq,
+				       IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND | IRQF_EARLY_RESUME,
 				       "gpi-dma", gpii);
 		if (ret < 0) {
 			dev_err(gpii->gpi_dev->dev, "error request irq:%d ret:%d\n",
-- 
2.43.0

Re: [PATCH v7 2/2] dmaengine: qcom-gpi: Keep GPI interrupt active during system resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] Adding IRQF_NO_SUSPEND leaves the interrupt unmasked during system sleep, causing fatal unclocked register accesses.
- [Low] The IRQF_EARLY_RESUME flag is functionally dead code and redundant when combined with IRQF_NO_SUSPEND.

Pre-existing issues:
- [High] A 3-way AB-BA deadlock occurs because gpi_ev_tasklet() acquires a read lock on pm_lock with local hardware interrupts enabled.
- [High] The error cleanup paths in gpi_ch_init() operate on the wrong channel pointer, causing leaks and multiple resets of the same channel.
- [High] Unregistered DMA device on the probe error path leads to a Use-After-Free and global list corruption.
- [High] tasklet_kill is called while the interrupt is still unmasked and enabled, risking a Use-After-Free on the event ring.
- [High] The interrupt handler returns IRQ_HANDLED when bailing out due to an invalid PM state, causing a permanent interrupt storm.
--

--- Patch [2]: [PATCH v7 2/2] dmaengine: qcom-gpi: Keep GPI interrupt active during system resume ---
commit a7024226a78be6fe1a44f0ec7dd85e67623b43ab
Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

dmaengine: qcom-gpi: Keep GPI interrupt active during system resume

GPI DMA clients may initiate transfers during the early stages of
system resume before the normal IRQ resume phase has completed.
Request the GPI interrupt with IRQF_NO_SUSPEND and IRQF_EARLY_RESUME
to keep it available across suspend/resume transitions.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..29872b6cb2c75 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -615,7 +615,8 @@ static int gpi_config_interrupts(struct gpii *gpii, enum gpii_irq_settings setti
>  
>  	if (!gpii->configured_irq) {
>  		ret = devm_request_irq(gpii->gpi_dev->dev, gpii->irq,
> -				       gpi_handle_irq, IRQF_TRIGGER_HIGH,
> +				       gpi_handle_irq,
> +				       IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND | IRQF_EARLY_RESUME,

[Severity: High]
Adding IRQF_NO_SUSPEND prevents the DMA interrupt from being masked by the
core IRQ subsystem during system suspend. Does this allow spurious or
delayed hardware interrupts to trigger while consumer driver hardware clocks
are disabled via runtime PM?

Looking at gpi_handle_irq(), the pm_state check may remain in ACTIVE_STATE
or PAUSE_STATE during suspend:

drivers/dma/qcom/gpi.c:gpi_handle_irq() {
    if (!REG_ACCESS_VALID(gpii->pm_state)) {
    ...
}

If this check passes while clocks are off, will the subsequent register
reads cause a synchronous external abort and crash the kernel?

[Severity: Low]
Is IRQF_EARLY_RESUME redundant here? When combined with IRQF_NO_SUSPEND,
IRQS_SUSPENDED is never set on the interrupt descriptor during
suspend_device_irqs(). This means resume_irq() won't do anything for this
interrupt during the early resume phase, making the flag functionally dead
code.

>  				       "gpi-dma", gpii);
>  		if (ret < 0) {

[ ... ]

[Severity: High]
This is a pre-existing issue, but can a 3-way AB-BA deadlock occur with
pm_lock?

gpi_ev_tasklet() running in softirq acquires a read lock on pm_lock without
disabling local hardware interrupts:

drivers/dma/qcom/gpi.c:gpi_ev_tasklet() {
    ...
    read_lock(&gpii->pm_lock);
    ...
}

A concurrent thread calls write_lock_irq():

drivers/dma/qcom/gpi.c:gpi_resume() {
    ...
    write_lock_irq(&gpii->pm_lock);
    ...
}

Due to fair rwlock semantics, the writer blocks new readers. If a hard
interrupt then fires on the tasklet's CPU, gpi_handle_irq() preempts the
tasklet and acquires the read lock:

drivers/dma/qcom/gpi.c:gpi_handle_irq() {
    ...
    read_lock_irqsave(&gpii->pm_lock, flags);
    ...
}

Will the hardirq reader spin waiting for the writer, while the writer spins
waiting for the preempted tasklet, creating a fatal deadlock?

[Severity: High]
This is a pre-existing issue, but do the error cleanup paths in
gpi_ch_init() operate on the wrong channel pointer?

If channel allocation fails, the error handlers iterate backwards but pass
the function argument gchan instead of the iterated element &gpii->gchan[i]:

drivers/dma/qcom/gpi.c:gpi_ch_init() {
    ...
error_start_chan:
    gpi_send_cmd(gpii, gchan, GPI_CH_CMD_RESET);
    ...
error_alloc_chan:
    gpi_reset_chan(gchan, GPI_CH_CMD_DE_ALLOC);
    ...
}

Does this repeatedly reset the same channel while failing to clean up the
newly allocated channels, leaking hardware resources and memory?

[Severity: High]
This is a pre-existing issue, but does gpi_probe() leak the DMA device on
failure?

If of_dma_controller_register() fails, the function returns directly:

drivers/dma/qcom/gpi.c:gpi_probe() {
    ...
    ret = of_dma_controller_register(...);
    if (ret)
        return ret;
    ...
}

Since dma_async_device_register() was already called earlier in the probe,
does this leave a dangling pointer in the global dma_device_list when
devres frees the gpi_dev structure?

[Severity: High]
This is a pre-existing issue, but is there a race condition in
gpi_free_chan_resources() that could cause a use-after-free?

The function kills the tasklet before disabling interrupts:

drivers/dma/qcom/gpi.c:gpi_free_chan_resources() {
    ...
    tasklet_kill(&gpii->ev_task);
    ...
    gpi_disable_interrupts(gpii);
    ...
}

If a pending or spurious IEOB interrupt arrives immediately after
tasklet_kill returns, could gpi_handle_irq() reschedule the tasklet,
allowing it to execute concurrently with or after
gpi_free_ring(&gpii->ev_ring, gpii), and access the freed event ring memory?

[Severity: High]
This is a pre-existing issue, but does returning IRQ_HANDLED when bailing
out cause a permanent interrupt storm?

If the pm_state check fails, gpi_handle_irq() jumps to exit_irq:

drivers/dma/qcom/gpi.c:gpi_handle_irq() {
    if (!REG_ACCESS_VALID(gpii->pm_state)) {
        ...
        goto exit_irq;
    }
    ...
exit_irq:
    return IRQ_HANDLED;
}

Since the interrupt is level-triggered, does falsely reporting IRQ_HANDLED
without servicing and clearing the device hardware condition cause the
interrupt controller to immediately re-assert the interrupt to the CPU,
creating an infinite loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709134623.1724212-1-mukesh.savaliya@oss.qualcomm.com?part=2