[PATCH v9 0/4] Enable multi-owner I2C support for QCOM GENI controllers

[PATCH v9 0/4] Enable multi-owner I2C support for QCOM GENI controllers

[Mukesh Kumar Savaliya]

The QUP-based GENI I2C controller driver currently assumes exclusive
ownership of the controller by a single system processor. This prevents
safe use of a single I2C controller by multiple system processors
(e.g. APPS and a DSP) running the same or different operating systems.

One practical example is an EEPROM connected to an I2C controller that
needs to be accessed independently by firmware running on a DSP and by
Linux running on the application processor, without causing bus-level
interference during transfers.

This series adds support for operating a QUP GENI I2C Serial Engine in a
multi-owner configuration. Each system processor uses its own dedicated
GPI instance (GPII) as the data path between the Serial Engine and the
GSI DMA engine. As a result, controller sharing is supported only when
the I2C controller operates in GPI mode; FIFO/CPU DMA modes are not
supported for this configuration.

To serialize access at the hardware level, the GPI DMA engine emits lock
and unlock Transfer Ring Elements (TREs) around I2C transfers. The GPI
driver determines lock/unlock placement autonomously based on the DMA
engine API contract: a LOCK TRE is inserted when the first transfer of a
new transaction is prepared (detected via lock_pending state in gchan),
and an UNLOCK TRE is inserted after the final write transfer in the batch
(signalled by the DMA_PREP_INTERRUPT flag on the last descriptor).

This design keeps the locking mechanism as an internal GPI implementation
detail, consistent with the BAM-DMA approach. The I2C driver indicates
multi-owner mode once via the initial dmaengine_slave_config call; it does
not need to track message positions or explicitly request lock/unlock TREs.
The DMA client is fully unaware of the underlying locking mechanism.

In addition, when a controller is shared, the GENI common layer avoids
placing the associated GPIOs into the pinctrl "sleep" state during
runtime suspend. This prevents disruption of transfers that may still
be in progress on another system processor using the same controller
pins.

The multi-owner behavior is enabled via a DeviceTree property,
`qcom,qup-multi-owner`, on the I2C controller node. This property must be
used only when the hardware configuration requires controller sharing
and when GPI mode is enabled.

Patch overview:
  1. Document the `qcom,qup-multi-owner` DeviceTree property for GENI I2C.
  2. Extend the QCOM GPI DMA driver to autonomously insert lock and unlock
     TREs for multi-owner transfers, with the multi_owner flag latched from
     the initial config call and lock/unlock placement determined entirely
     within the GPI driver based on transaction boundaries.
  3. Update the GENI common layer to keep pinctrl active for shared
     controllers during runtime suspend.
  4. Enable multi-owner operation in the GENI I2C driver: set multi_owner
     in the initial GPI config and remove all per-message lock/unlock
     guidance.


Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

---
Link to V8: https://lore.kernel.org/all/20260708051023.2872304-1-mukesh.savaliya@oss.qualcomm.com/
Changes in V9:
- Patch #1: Updated the DT binding description for the pinctrl sleep state
  based on Bjorn's feedback.
- Patch #2: Added Konrad's Acked-by tag. 
- Patch #2: Moved LOCK/UNLOCK handling into the GPI DMA driver and removed
  the corresponding logic from the I2C client driver, suggested by Bartosz and Vinod.
- Patch #4: Switched to device_property_present() for multi-owner property
  detection and performed minor debug log cleanup.
- Patch #4: Addressed review comments and added Konrad's Reviewed-by tag.


Link to V7 : https://lore.kernel.org/all/20260423145705.545552-1-mukesh.savaliya@oss.qualcomm.com/
Changes in V8:
- Documented that multi-owner controllers must not assume exclusive ownership of GPIOs and
  therefore sleep pinctrl state must not be selected.
- Documented that each owner is responsible for maintaining required resource votes for
  shared-controller operation.
- Clarified commit message to state that lock/unlock sequencing applies only to
  GPI DMA mode transfers, as FIFO mode does not use the GPI engine.
- Fixed GPI lock/unlock handling for single-message transfer, ensure both ACQUIRE
  and RELEASE are issued when num == 1.
- Simplified DT property assignment by directly assigning of_property_read_bool()
  result to se.multi_owner.
- Improved error message to clearly indicate that multi-owner configuration requires
  GPI DMA mode.
- Updated code comments to follow kernel style guidelines and improved wording for clarity.
- Rebased patch on tip.
- Added ACked by tag for Patch 1 and reveiwed by tag for Patch 3.

 
Link to V6 : https://lore.kernel.org/all/20260331114742.2896317-1-mukesh.savaliya@oss.qualcomm.com/
Changes in V7:
 - Added Acked-by for dt-biding patch 1 given by Rob.
 - Minor description change for multi_owner variable in patch 3 and added RB tag from Konrad.
 - Removed description of multi_owner DT property from code as it's part of kernel doc.
 - Returned with dev_err_probe() in geni_i2c_probe() - Konrad's suggestion.


Link to V5 : https://lore.kernel.org/lkml/20241129144357.2008465-2-mukesh.savaliya@oss.qualcomm.com/
Changes in V6:
 - Addressed review feedback from Krzysztof Kozlowski and other reviewers, primarily
   around clarifying the feature semantics and improving the DeviceTree flag naming.
 - Renamed the DeviceTree property from qcom,shared-se to qcom,qup-multi-owner to
   better describe the multi-owner controller use case.
 - Updated the cover letter to clearly describe the multi-owner I2C design, the
   GPI-only limitation, and the role of the new qcom,qup-multi-owner flag.
 - Updated the DeviceTree binding documentation to reflect the new qcom,qup-multi-owner
   property and refined its description for clarity and correctness.
 - [Patch 2/4] Simplify the GPI I2C interface by replacing multiple shared SE related
   state flags with a single internal lock/unlock control managed entirely in the GPI
   driver - Suggested by Vinod Koul.
 - [Patch 3/4] Updated the GENI common layer to avoid selecting the pinctrl “sleep”
   state for multi-owner controllers, preventing disruption of transfers initiated by
   another system processor during runtime suspend.
 - [Patch 4/4] Updated the GENI I2C driver to: 
    - Detect the qcom,qup-multi-owner DeviceTree property.
	- Mark the underlying serial engine as shared.
	- Request GPI lock and unlock TRE sequencing around I2C transfers using the
	  simplified single field API.
 - Clarified commit messages across all patches to avoid ambiguous terminology
   (such as “subsystem”), expand abbreviations, and better explain functional
   requirements rather than optimizations.
 - Updated copyright headers across all files wherever applicable.
 - Renamed variable shared_geni_se to multi_owner to match the DT property naming.
 - Changed dev_err(print_log) during probe() to dev_err_probe().
 

Link to V4 : https://lore.kernel.org/lkml/20241113161413.3821858-1-quic_msavaliy@quicinc.com/
Changes in V5:
 - Corrected name as qcom,shared-se instead of qcom,is-shared.
 - Added description for the SE acronyms into yaml file and commit log.
 - Renamed TRE_I2C_UNLOCK to TRE_UNLOCK being generic.
 - Log an error and return if non GPI mode goes into shared usecase.


Link to V3: https://lore.kernel.org/lkml/20240927063108.2773304-4-quic_msavaliy@quicinc.com/T/
Changes in V4:
 - Fixed Typo to dt-bindings in subject line of PATCH 1.
 - Replaced SS (subsystem) as multiprocessor as per Bryan's suggestions.
 - Replied to Krzysztof's comments and replaced SS with Multiprocessor system.
 - Removed Abbreviations and also bullet point list from  PATCH 1.
 - Changed feature flag name from qcom,shared-se to qcom,is-shared.
 - Removed bullet points from example of usecase and explained in paragraph.
 - Changed title suffix to dmaengine from dma for Patch 2.
 - Rename TRE_I2C_LOCK to TRE_LOCK in PATCH 2.
 - Enhanced comments about not modifying the pin states on shared SE for PATCH 3.
 - Enhanced shared_geni_se struct member explanation as per Bjorn's comment in PATCH 3.
 - Moved GPIO unconfiguration description from patch 4 to patch 3 as pointed by Bjorn.
 - Removed debug log which was unrelated to this feature change.
 - Added usecase exmaple of shared SE in commit log.


Link to V2: https://lore.kernel.org/lkml/a88a16ff-3537-4396-b2ea-4ba02b4850e9@quicinc.com/T/
Changes in V3:
 - Added missing maintainers which i forgot to add.
 - Add cover letter with description of SS and EE for dt-bindings patch.
 - Added acronyms expansion to commit log.
 - [PATCH v2 3/4] : Removed exported symbol geni_se_clks_off(). 
   Instead added changes to bypass pinctrl sleep configuration from
   geni_se_resources_off() function.
 - Changed title name of [PATCH v2 3/4] to reflect the suggested changes.
 - [PATCH v2 4/4] kept geni_i2c_runtime_suspend() as is and removed 
   explicit call to geni_se_clks_off().
 - Removed is_shared variable from i2c driver and instead used common 
   shared_geni_se variable from qcom-geni-se.h so that other protocols
   can also extend for similar feature.
 - I2C driver log changed from dev_err() to dev_dbg() for timeout.
 - set gpi_mode = true if shared_geni_se is set for this usecase. Enhanced
   comments around code and commit log.


Link to V1: https://lore.kernel.org/lkml/cb7613d0-586e-4089-a1b6-2405f4dc4883@quicinc.com/T/
Changes in V2:
 - Enhanced commit log grammatically for PATCH v1 3/4 as suggested by Bryan.
 - Updated Cover letter along with acronyms expansion.
 - Added maintainers list from other subsystems for review, which was missing.
   Thanks to Krzysztof for pointing out.
 - Added cover letter with an example of Serial Engine sharing.
 - Addressed review comments for all the patches.

---
Mukesh Kumar Savaliya (4):
  dt-bindings: i2c: qcom,i2c-geni: Document multi-owner controller
    support
  dmaengine: qcom: gpi: Add lock/unlock TREs for multi-owner I2C
    transfers
  soc: qcom: geni-se: Keep pinctrl active for multi-owner controllers
  i2c: qcom-geni: Support multi-owner controllers in GPI mode

 .../bindings/i2c/qcom,i2c-geni-qcom.yaml      | 16 ++++
 drivers/dma/qcom/gpi.c                        | 77 ++++++++++++++++++-
 drivers/i2c/busses/i2c-qcom-geni.c            | 17 +++-
 drivers/soc/qcom/qcom-geni-se.c               | 14 +++-
 include/linux/dma/qcom-gpi-dma.h              |  4 +
 include/linux/soc/qcom/geni-se.h              |  2 +
 6 files changed, 125 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH v9 1/4] dt-bindings: i2c: qcom,i2c-geni: Document multi-owner controller support

[Mukesh Kumar Savaliya]

Document a DeviceTree property to describe QUP-based I2C controllers that
are shared with one or more other system processors.

On some Qualcomm platforms, a QUP-based I2C controller may be accessed by
multiple system processors (for example, APPS and DSP). In such
configurations, the operating system must not assume exclusive ownership
of the controller or its associated hardware resources.

The new qcom,qup-multi-owner property indicates that the controller is
externally shared and that the operating system must avoid operations
which rely on sole control of the hardware.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
---
 .../bindings/i2c/qcom,i2c-geni-qcom.yaml         | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
index 51534953a69c..ed9b029603fd 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
@@ -60,6 +60,22 @@ properties:
   power-domains:
     maxItems: 1
 
+  qcom,qup-multi-owner:
+    type: boolean
+    description:
+      Indicates that the QUP-based controller is shared with one or more
+      other system processors and must not be assumed to have exclusive
+      ownership by the operating system.
+
+      The associated GPIOs must not be reconfigured into a sleep state
+      during runtime suspend, as doing so may disrupt transactions
+      initiated by another owner of the controller.
+
+      Each owner is responsible for maintaining any resource votes
+      required for operation of the shared controller (for example clocks,
+      power domains, interconnect bandwidth, or other platform-specific
+      resources)
+
   reg:
     maxItems: 1
 
-- 
2.43.0

Re: [PATCH v9 1/4] dt-bindings: i2c: qcom,i2c-geni: Document multi-owner controller support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The schema documents a runtime restriction for `qcom,qup-multi-owner` but fails to enforce it programmatically, allowing invalid device trees to pass validation.
--

--- Patch [1]: [PATCH v9 1/4] dt-bindings: i2c: qcom,i2c-geni: Document multi-owner controller support ---
commit 744011d8e769def492430680cc665ca844b312d6
Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

dt-bindings: i2c: qcom,i2c-geni: Document multi-owner controller support

This commit documents a new qcom,qup-multi-owner property for QUP-based I2C
controllers that are shared with other system processors. It instructs the
operating system to avoid assuming exclusive ownership of the hardware.

> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> index 51534953a69cf..ed9b029603fdf 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml

[ ... ]

> @@ -60,6 +60,22 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  qcom,qup-multi-owner:
> +    type: boolean
> +    description:
> +      Indicates that the QUP-based controller is shared with one or more
> +      other system processors and must not be assumed to have exclusive
> +      ownership by the operating system.
> +
> +      The associated GPIOs must not be reconfigured into a sleep state
> +      during runtime suspend, as doing so may disrupt transactions
> +      initiated by another owner of the controller.

[Severity: Medium]
The prose correctly documents that the GPIOs must not be put into a sleep
state when this property is present, but should this be enforced
programmatically in the schema?

Without an allOf conditional block restricting pinctrl-names to maxItems: 1
and disallowing pinctrl-1, device trees that mistakenly provide a sleep
pinctrl state alongside qcom,qup-multi-owner will silently pass
dt_binding_check and dtbs_check.

At runtime, the power management core would then automatically apply the sleep
state during runtime suspend, which disrupts the hardware as warned against in
the description.

> +
> +      Each owner is responsible for maintaining any resource votes
> +      required for operation of the shared controller (for example clocks,
> +      power domains, interconnect bandwidth, or other platform-specific
> +      resources)
>
>    reg:
>      maxItems: 1

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

[PATCH v9 2/4] dmaengine: qcom: gpi: Add lock/unlock TREs for multi-owner I2C transfers

[Mukesh Kumar Savaliya]

Some platforms use a QUP-based I2C controller in a configuration where the
controller is shared with another system processor (described in DT using
qcom,qup-multi-owner). In such setups, GPI hardware lock/unlock TREs can be
used to serialize access to the controller.

Add support to emit lock and unlock TREs around I2C transfers and increase
the maximum TRE count to account for the additional elements.

The GPI driver determines lock/unlock TRE placement autonomously based on
transaction boundaries, with no guidance required from the DMA client:
- A LOCK TRE is inserted before the first transfer in a transaction. The
  driver detects the start of a new transaction by checking that no lock
  is currently held (lock_pending == false).
- An UNLOCK TRE is inserted after the final write transfer in a
  transaction. The DMA_PREP_INTERRUPT flag on the descriptor marks the
  last descriptor in the batch.

The multi-owner flag is communicated once via the initial dmaengine_slave_
config call (set_config == 1) through the multi_owner field of struct
gpi_i2c_config, and is latched in gchan->multi_owner for the lifetime of
the transfer sequence. This keeps the locking mechanism as an internal GPI
implementation detail, consistent with the BAM-DMA approach, so DMA
consumers remain unaware of the underlying locking hardware.

Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
---
 drivers/dma/qcom/gpi.c           | 77 +++++++++++++++++++++++++++++++-
 include/linux/dma/qcom-gpi-dma.h |  4 ++
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index a5055a6273af..25fe7410bee2 100644
--- a/drivers/dma/qcom/gpi.c
+++ b/drivers/dma/qcom/gpi.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved.
  * Copyright (c) 2020, Linaro Limited
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
  */
 
 #include <dt-bindings/dma/qcom-gpi.h>
@@ -67,6 +68,14 @@
 #define TRE_DMA_LEN		GENMASK(23, 0)
 #define TRE_DMA_IMMEDIATE_LEN	GENMASK(3, 0)
 
+/* Lock TRE */
+#define TRE_LOCK		BIT(0)
+#define TRE_MINOR_TYPE		GENMASK(19, 16)
+#define TRE_MAJOR_TYPE		GENMASK(23, 20)
+
+/* Unlock TRE */
+#define TRE_UNLOCK		BIT(8)
+
 /* Register offsets from gpi-top */
 #define GPII_n_CH_k_CNTXT_0_OFFS(n, k)	(0x20000 + (0x4000 * (n)) + (0x80 * (k)))
 #define GPII_n_CH_k_CNTXT_0_EL_SIZE	GENMASK(31, 24)
@@ -492,6 +501,8 @@ struct gchan {
 	u32 dir;
 	struct gpi_ring ch_ring;
 	void *config;
+	bool multi_owner;   /* controller is shared; insert lock/unlock TREs */
+	bool lock_pending;  /* LOCK TRE has been inserted, UNLOCK not yet emitted */
 };
 
 struct gpii {
@@ -518,7 +529,7 @@ struct gpii {
 	bool ieob_set;
 };
 
-#define MAX_TRE 3
+#define MAX_TRE 5
 
 struct gpi_desc {
 	struct virt_dma_desc vd;
@@ -1605,6 +1616,7 @@ static int
 gpi_peripheral_config(struct dma_chan *chan, struct dma_slave_config *config)
 {
 	struct gchan *gchan = to_gchan(chan);
+	struct gpi_i2c_config *i2c_cfg;
 	void *new_config;
 
 	if (!config->peripheral_config)
@@ -1617,6 +1629,17 @@ gpi_peripheral_config(struct dma_chan *chan, struct dma_slave_config *config)
 	gchan->config = new_config;
 	memcpy(gchan->config, config->peripheral_config, config->peripheral_size);
 
+	/*
+	 * Latch the multi_owner flag from the initial config call so the GPI
+	 * driver can autonomously insert LOCK/UNLOCK TREs without the client
+	 * having to track transfer boundaries.
+	 */
+	if (gchan->protocol == QCOM_GPI_I2C) {
+		i2c_cfg = gchan->config;
+		if (i2c_cfg->set_config)
+			gchan->multi_owner = i2c_cfg->multi_owner;
+	}
+
 	return 0;
 }
 
@@ -1627,10 +1650,33 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
 	struct gpi_i2c_config *i2c = chan->config;
 	struct device *dev = chan->gpii->gpi_dev->dev;
 	unsigned int tre_idx = 0;
+	bool is_last = !!(flags & DMA_PREP_INTERRUPT);
 	dma_addr_t address;
 	struct gpi_tre *tre;
 	unsigned int i;
 
+	/*
+	 * Insert a LOCK TRE before the first transfer of a multi-owner
+	 * transaction.  The GPI driver detects the transaction start
+	 * autonomously: if multi_owner is set and no lock has been issued
+	 * since the last unlock (lock_pending == false), this is the first
+	 * descriptor in a new transaction.
+	 */
+	if (chan->multi_owner && !chan->lock_pending) {
+		tre = &desc->tre[tre_idx];
+		tre_idx++;
+
+		tre->dword[0] = 0;
+		tre->dword[1] = 0;
+		tre->dword[2] = 0;
+		tre->dword[3] = u32_encode_bits(1, TRE_LOCK);
+		tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOB);
+		tre->dword[3] |= u32_encode_bits(0, TRE_MINOR_TYPE);
+		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
+
+		chan->lock_pending = true;
+	}
+
 	/* first create config tre if applicable */
 	if (i2c->set_config) {
 		tre = &desc->tre[tre_idx];
@@ -1690,6 +1736,35 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
 
 		if (!(flags & DMA_PREP_INTERRUPT))
 			tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_BEI);
+
+		/*
+		 * Chain the DMA TRE to the UNLOCK TRE when this is the last
+		 * descriptor in the transaction (DMA_PREP_INTERRUPT set) and
+		 * the channel is in multi-owner mode.
+		 */
+		if (chan->multi_owner && is_last && i2c->op != I2C_READ)
+			tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_CHAIN);
+	}
+
+	/*
+	 * Insert an UNLOCK TRE after the last write transfer of a multi-owner
+	 * transaction.  DMA_PREP_INTERRUPT marks the final descriptor in the
+	 * batch; reads carry their own completion event so the unlock follows
+	 * the write leg of each read message.
+	 */
+	if (chan->multi_owner && is_last && i2c->op != I2C_READ) {
+		tre = &desc->tre[tre_idx];
+		tre_idx++;
+
+		tre->dword[0] = 0;
+		tre->dword[1] = 0;
+		tre->dword[2] = 0;
+		tre->dword[3] = u32_encode_bits(1, TRE_UNLOCK);
+		tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOB);
+		tre->dword[3] |= u32_encode_bits(1, TRE_MINOR_TYPE);
+		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
+
+		chan->lock_pending = false;
 	}
 
 	for (i = 0; i < tre_idx; i++)
diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
index 332be28427e4..590ca7935f2b 100644
--- a/include/linux/dma/qcom-gpi-dma.h
+++ b/include/linux/dma/qcom-gpi-dma.h
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /*
  * Copyright (c) 2020, Linaro Limited
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
  */
 
 #ifndef QCOM_GPI_DMA_H
@@ -68,6 +69,8 @@ enum i2c_op {
  * @rx_len: receive length for buffer
  * @op: i2c cmd
  * @multi_msg: is part of multi i2c r-w msgs
+ * @multi_owner: controller is shared with another system processor;
+ *               the GPI driver will insert lock/unlock TREs automatically
  */
 struct gpi_i2c_config {
 	u8 set_config;
@@ -81,6 +84,7 @@ struct gpi_i2c_config {
 	u32 rx_len;
 	enum i2c_op op;
 	bool multi_msg;
+	bool multi_owner;
 };
 
 #endif /* QCOM_GPI_DMA_H */
-- 
2.43.0

Re: [PATCH v9 2/4] dmaengine: qcom: gpi: Add lock/unlock TREs for multi-owner I2C transfers

[sashiko-bot]

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

New issues:
- [Critical] Setting `TRE_FLAGS_IEOB` on LOCK and UNLOCK TREs causes multiple completion events per descriptor, leading to Use-After-Free and queue corruption.
- [High] The driver prematurely drops the hardware lock in Write-then-Read multi-message transfers due to misinterpreting `DMA_PREP_INTERRUPT`.
- [High] Failure to reset `gchan->lock_pending` during channel reset or termination permanently bypasses hardware locking for subsequent transactions.
- [High] `gpi_prep_slave_sg()` blindly hardcodes the maximum TRE requirement to 3, failing to account for the newly added LOCK and UNLOCK TREs, causing ring buffer overflow.

Pre-existing issues:
- [High] Dangling pointer in `gchan->config` leads to Use-After-Free during channel reallocation.
--

--- Patch [2]: [PATCH v9 2/4] dmaengine: qcom: gpi: Add lock/unlock TREs for multi-owner I2C transfers ---
commit b6883d50c999f402ed4f7be119614262119be22d
Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

dmaengine: qcom: gpi: Add lock/unlock TREs for multi-owner I2C transfers

This commit adds support for emitting lock and unlock Transfer Ring Elements
(TREs) around I2C transfers on QUP-based controllers shared with other
processors. The GPI driver autonomously determines the placement of these
TREs based on transaction boundaries and latches a multi-owner flag during
initial configuration.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..25fe7410bee2e 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c

[ ... ]

> @@ -518,7 +529,7 @@ struct gpii {
>  	bool ieob_set;
>  };
>  
> -#define MAX_TRE 3
> +#define MAX_TRE 5

[Severity: High]
While MAX_TRE is increased to 5 here, does gpi_prep_slave_sg() still
hardcode the maximum TRE requirement to 3 when checking if the ring buffer
has enough elements available via gpi_ring_num_elements_avail()?

If the ring buffer only has 3 or 4 slots available, could this pass the
check but fail to enqueue the remaining elements like the UNLOCK TRE,
potentially locking the hardware bus?

>  struct gpi_desc {

[ ... ]

> @@ -1617,6 +1629,17 @@ gpi_peripheral_config(struct dma_chan *chan, struct dma_slave_config *config)
>  	gchan->config = new_config;
>  	memcpy(gchan->config, config->peripheral_config, config->peripheral_size);
>  

[Severity: High]
This isn't a bug introduced by this patch, but when
gpi_free_chan_resources() frees this memory with kfree(gchan->config), it
leaves gchan->config as a dangling pointer.

Since gchan elements are statically allocated arrays reused across DMA
channel requests, if the channel is requested again, will this call to
krealloc() receive the freed pointer and cause a slab use-after-free?

> +	/*
> +	 * Latch the multi_owner flag from the initial config call so the GPI

[ ... ]

> @@ -1627,10 +1650,33 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
> +	/*
> +	 * Insert a LOCK TRE before the first transfer of a multi-owner
> +	 * transaction.  The GPI driver detects the transaction start
> +	 * autonomously: if multi_owner is set and no lock has been issued
> +	 * since the last unlock (lock_pending == false), this is the first
> +	 * descriptor in a new transaction.
> +	 */
> +	if (chan->multi_owner && !chan->lock_pending) {
> +		tre = &desc->tre[tre_idx];
> +		tre_idx++;
> +
> +		tre->dword[0] = 0;
> +		tre->dword[1] = 0;
> +		tre->dword[2] = 0;
> +		tre->dword[3] = u32_encode_bits(1, TRE_LOCK);
> +		tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOB);

[Severity: Critical]
Adding TRE_FLAGS_IEOB to the LOCK and UNLOCK TREs means a single transaction
produces an EOB event (from Lock), an EOT event (from DMA), and another EOB
event (from Unlock).

In gpi_process_xfer_compl_event(), EOB events fall through to the success
path which pops the active descriptor from gchan->vc and frees it. Since a
single descriptor generates multiple events, could the event handler pop and
free unrelated descriptors from the queue, causing a use-after-free?

> +		tre->dword[3] |= u32_encode_bits(0, TRE_MINOR_TYPE);
> +		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
> +
> +		chan->lock_pending = true;

[Severity: High]
If a transaction errors out after a LOCK TRE is generated and the client
calls gpi_terminate_all() to abort the transfer, gchan->lock_pending is not
reset to false.

On the next transaction, will gpi_create_i2c_tre() evaluate lock_pending as
true and skip inserting the necessary LOCK TRE, causing transactions to run
on the shared bus without acquiring the hardware lock?

> +	}
> +
>  	/* first create config tre if applicable */
>  	if (i2c->set_config) {

[ ... ]

> @@ -1690,6 +1736,35 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,

[ ... ]

> +	/*
> +	 * Insert an UNLOCK TRE after the last write transfer of a multi-owner
> +	 * transaction.  DMA_PREP_INTERRUPT marks the final descriptor in the
> +	 * batch; reads carry their own completion event so the unlock follows
> +	 * the write leg of each read message.
> +	 */
> +	if (chan->multi_owner && is_last && i2c->op != I2C_READ) {
> +		tre = &desc->tre[tre_idx];
> +		tre_idx++;
> +
> +		tre->dword[0] = 0;
> +		tre->dword[1] = 0;
> +		tre->dword[2] = 0;
> +		tre->dword[3] = u32_encode_bits(1, TRE_UNLOCK);

[Severity: High]
If the i2c-qcom-geni client driver executes Write-then-Read sequences by
setting DMA_PREP_INTERRUPT on the write leg to wait for its completion, will
this logic equate the flag with the absolute end of the I2C transaction?

Could this prematurely append an UNLOCK TRE between the write and read,
destroying the I2C bus stretch and allowing other owners to interleave
transactions and corrupt the bus state?

> +		tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOB);
> +		tre->dword[3] |= u32_encode_bits(1, TRE_MINOR_TYPE);
> +		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
> +
> +		chan->lock_pending = false;
>  	}
>  
>  	for (i = 0; i < tre_idx; i++)

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

[PATCH v9 3/4] soc: qcom: geni-se: Keep pinctrl active for multi-owner controllers

[Mukesh Kumar Savaliya]

On platforms where a GENI Serial Engine is shared with another system
processor, selecting the "sleep" pinctrl state can disrupt ongoing
transfers initiated by the other processor.

Teach geni_se_resources_off() to skip selecting the pinctrl sleep state
when the Serial Engine is marked as shared, while still allowing the
rest of the resource shutdown sequence to proceed.

This is required for multi-owner configurations (described via DeviceTree
with qcom,qup-multi-owner on the protocol controller node).

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
---
 drivers/soc/qcom/qcom-geni-se.c  | 14 +++++++++++---
 include/linux/soc/qcom/geni-se.h |  2 ++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 873bfbd6b2b7..a4da75d54d6c 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -610,9 +610,17 @@ int geni_se_resources_off(struct geni_se *se)
 	if (has_acpi_companion(se->dev))
 		return 0;
 
-	ret = pinctrl_pm_select_sleep_state(se->dev);
-	if (ret)
-		return ret;
+	/*
+	 * Select the "sleep" pinctrl state only when the serial engine is
+	 * exclusively owned by this system processor. For shared controller
+	 * configurations, another system processor may still be using the pins,
+	 * and switching them to "sleep" can disrupt ongoing transfers.
+	 */
+	if (!se->multi_owner) {
+		ret = pinctrl_pm_select_sleep_state(se->dev);
+		if (ret)
+			return ret;
+	}
 
 	geni_se_clks_off(se);
 	return 0;
diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index 29a53bbc0dd4..725f89a44551 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -66,6 +66,7 @@ struct geni_icc_path {
  * @icc_paths:		Array of ICC paths for SE
  * @pd_list:		Power domain list for managing power domains
  * @has_opp:		Indicates if OPP is supported
+ * @multi_owner:	True if SE is shared between multiple owners.
  */
 struct geni_se {
 	void __iomem *base;
@@ -78,6 +79,7 @@ struct geni_se {
 	struct geni_icc_path icc_paths[3];
 	struct dev_pm_domain_list *pd_list;
 	bool has_opp;
+	bool multi_owner;
 };
 
 /* Common SE registers */
-- 
2.43.0

Re: [PATCH v9 3/4] soc: qcom: geni-se: Keep pinctrl active for multi-owner controllers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `qcom,qup-multi-owner` DeviceTree property is never parsed, making the `multi_owner` feature completely non-functional.
- [High] The runtime PM suspend callback `geni_se_resources_deactivate()` lacks the `se->multi_owner` check, breaking multi-owner controller operation during runtime suspend.
- [High] The GPI DMA driver prematurely releases the hardware lock during multi-message I2C read transactions, destroying bus atomicity.
--

--- Patch [3]: [PATCH v9 3/4] soc: qcom: geni-se: Keep pinctrl active for multi-owner controllers ---
commit ceae3d1ac31371a041b92e87207badc3969f7e87
Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

soc: qcom: geni-se: Keep pinctrl active for multi-owner controllers

On platforms where a GENI Serial Engine is shared with another system
processor, selecting the "sleep" pinctrl state can disrupt ongoing
transfers initiated by the other processor. This patch skips selecting
the sleep state when the engine is marked as shared.

> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 873bfbd6b2b7b..a4da75d54d6c3 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -610,9 +610,17 @@ int geni_se_resources_off(struct geni_se *se)
>  	if (has_acpi_companion(se->dev))
>  		return 0;
>  
> -	ret = pinctrl_pm_select_sleep_state(se->dev);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * Select the "sleep" pinctrl state only when the serial engine is
> +	 * exclusively owned by this system processor. For shared controller
> +	 * configurations, another system processor may still be using the pins,
> +	 * and switching them to "sleep" can disrupt ongoing transfers.
> +	 */
> +	if (!se->multi_owner) {

[Severity: High]
Where is se->multi_owner populated from the DeviceTree?

I don't see any parsing for the "qcom,qup-multi-owner" property in this
patch, which seems to leave the multi_owner flag permanently false. Does
this code require a device property read somewhere else to function?

[Severity: High]
Are runtime PM transitions handled correctly for shared controllers?

Looking at geni_se_resources_deactivate(), which is used by protocol drivers
like I2C for runtime suspend, it lacks a similar check:

drivers/soc/qcom/qcom-geni-se.c:geni_se_resources_deactivate() {
...
	ret = pinctrl_pm_select_sleep_state(se->dev);
	if (ret)
		return ret;
...
}

If a shared controller goes into local runtime suspend, won't it still force
the pins into the sleep state and disrupt ongoing transfers on the other
processor?

> +		ret = pinctrl_pm_select_sleep_state(se->dev);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	geni_se_clks_off(se);
>  	return 0;

[ ... ]

> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
> index 29a53bbc0dd49..725f89a445511 100644
> --- a/include/linux/soc/qcom/geni-se.h
> +++ b/include/linux/soc/qcom/geni-se.h
> @@ -78,6 +79,7 @@ struct geni_se {
>  	struct geni_icc_path icc_paths[3];
>  	struct dev_pm_domain_list *pd_list;
>  	bool has_opp;
> +	bool multi_owner;
>  };

[Severity: High]
Does this multi-owner configuration break transaction atomicity for
multi-message I2C reads?

Looking at the I2C driver, it disables multi-descriptor batching for reads:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_gpi_xfer() {
...
				gi2c->is_tx_multi_desc_xfer = false;
				break;
...
}

This causes DMA_PREP_INTERRUPT to be set on the write leg of the read
transaction.

The GPI DMA driver appears to interpret this flag as the end of the entire
batch and inserts an unlock command:

drivers/dma/qcom/gpi.c:gpi_create_i2c_tre() {
...
	if (chan->multi_owner && is_last && i2c->op != I2C_READ) {
...
}

Will this prematurely release the hardware lock before the read leg executes,
allowing another processor to inject traffic and corrupt the bus?

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

[PATCH v9 4/4] i2c: qcom-geni: Support multi-owner controllers in GPI mode

[Mukesh Kumar Savaliya]

Some platforms use a QUP-based I2C controller in a configuration where the
controller is shared with another system processor. In this setup the
operating system must not assume exclusive ownership of the controller or
its associated pins.

Add support for enabling multi-owner operation when DeviceTree specifies
qcom,qup-multi-owner. When enabled, mark the underlying serial engine as
shared so the common GENI resource handling avoids selecting the "sleep"
pinctrl state, which could disrupt transfers initiated by the other
processor.

For GPI mode transfers, indicate multi-owner mode to the GPI driver by
setting the multi_owner field in the initial dmaengine_slave_config call.
The GPI driver then autonomously inserts lock and unlock TREs around the
transfer sequence without any per-message guidance from the I2C driver.
This keeps the locking mechanism as an internal GPI implementation
detail; the I2C driver does not need to track transfer boundaries or
message positions for lock/unlock purposes.

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 658636c1ee0e..850fb0516a87 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -829,6 +829,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
 	peripheral.clk_div = itr->clk_div;
 	peripheral.set_config = 1;
 	peripheral.multi_msg = false;
+	peripheral.multi_owner = gi2c->se.multi_owner;
 
 	trace_geni_i2c_bus_setup(gi2c->se.dev, gi2c->clk_freq_out,
 				 itr->clk_div, itr->t_high_cnt,
@@ -1066,7 +1067,11 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
 	}
 
 	if (fifo_disable) {
-		/* FIFO is disabled, so we can only use GPI DMA */
+		/*
+		 * FIFO is disabled, so only GPI DMA can be used.
+		 * In multi-owner configurations, the SE may be shared between subsystems,
+		 * with each subsystem owning a separate GPII.
+		 */
 		gi2c->gpi_mode = true;
 		ret = setup_gpi_dma(gi2c);
 		if (ret)
@@ -1075,6 +1080,11 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
 		dev_dbg(gi2c->se.dev, "Using GPI DMA mode for I2C\n");
 	} else {
 		gi2c->gpi_mode = false;
+
+		if (gi2c->se.multi_owner)
+			return dev_err_probe(gi2c->se.dev, -EINVAL,
+					     "I2C sharing not supported in non-GSI mode\n");
+
 		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
 
 		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
@@ -1145,6 +1155,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		gi2c->clk_freq_out = I2C_MAX_STANDARD_MODE_FREQ;
 	}
 
+	if (device_property_present(&pdev->dev, "qcom,qup-multi-owner")) {
+		gi2c->se.multi_owner = true;
+		dev_dbg(&pdev->dev, "I2C controller is shared with another system processor\n");
+	}
+
 	if (has_acpi_companion(dev))
 		ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
 
-- 
2.43.0

Re: [PATCH v9 4/4] i2c: qcom-geni: Support multi-owner controllers in GPI mode

[sashiko-bot]

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

New issues:
- [High] Premature hardware lock release breaks multi-owner transaction atomicity
- [Medium] PM runtime usage counter leak on `geni_i2c_init` error path

Pre-existing issues:
- [Critical] Double free and use-after-free of DMA bounce buffers in `geni_i2c_gpi_xfer()`
- [High] Memory leak of DMA tracking arrays on multi-descriptor transfer error paths
--

--- Patch [4]: [PATCH v9 4/4] i2c: qcom-geni: Support multi-owner controllers in GPI mode ---
commit e932d5c4bcfb356d1e0cbaf5c247b0bd50b4e7e8
Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

i2c: qcom-geni: Support multi-owner controllers in GPI mode
    
Some platforms use a QUP-based I2C controller in a configuration where the
controller is shared with another system processor. In this setup the
operating system must not assume exclusive ownership of the controller or
its associated pins.
    
Add support for enabling multi-owner operation when DeviceTree specifies
qcom,qup-multi-owner.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -829,6 +829,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>  	peripheral.clk_div = itr->clk_div;
>  	peripheral.set_config = 1;
>  	peripheral.multi_msg = false;
> +	peripheral.multi_owner = gi2c->se.multi_owner;

[Severity: High]
Does this setup cause premature hardware lock releases, breaking the
multi-owner atomicity?

In geni_i2c_gpi(), the interrupt flag is unconditionally set for sequential
transfers:

geni_i2c_gpi() {
    ...
    } else {
        flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
    }
    ...
}

Since the GPI driver inserts an UNLOCK TRE when DMA_PREP_INTERRUPT is set,
would a multi-message sequence like Write-then-Read drop the hardware lock
mid-transaction while the bus is stretched?

[Severity: Critical]
This is a pre-existing issue, but does geni_i2c_gpi_xfer() have a double free
bug on its DMA bounce buffers?

The tx_buf and rx_buf variables are initialized outside the loop, but are not
reset at the top of each iteration:

geni_i2c_gpi_xfer() {
    void *tx_buf = NULL, *rx_buf = NULL;
    ...
    for (i = 0; i < num; i++) {
        ...
        if (msgs[i].flags & I2C_M_RD) {
            geni_i2c_gpi(..., &rx_buf, ...);
        }
        ...
        geni_i2c_gpi_unmap(..., rx_buf, ...);
    }
}

If a message sequence contains a Read followed by a Write, rx_buf is mapped
in the Read iteration and then freed in geni_i2c_gpi_unmap(). In the next
Write iteration, rx_buf is not reset to NULL and not overwritten, causing
geni_i2c_gpi_unmap() to use and free the stale pointer again:

geni_i2c_gpi_unmap() {
    ...
    if (rx_buf) {
        dma_unmap_single(...);
        i2c_put_dma_safe_msg_buf(rx_buf, ...);
    }
}

[Severity: High]
This is a pre-existing issue, but is there a memory leak of the DMA tracking
arrays on the multi-descriptor transfer error paths?

If a multi-descriptor transfer encounters an error (like a device NACK),
gi2c->err is set and the cleanup loop in geni_i2c_gpi_multi_desc_unmap()
aborts early:

geni_i2c_gpi_multi_desc_unmap() {
    ...
    if (tx_multi_xfer->unmap_msg_cnt == gi2c->num_msgs - 1) {
        kfree(tx_multi_xfer->dma_buf);
        kfree(tx_multi_xfer->dma_addr);
        break;
    }
    ...
}

Since unmap_msg_cnt never reaches num_msgs - 1 on an early error path, are
dma_buf and dma_addr permanently leaked?

[ ... ]

> @@ -1075,6 +1080,11 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
>  		dev_dbg(gi2c->se.dev, "Using GPI DMA mode for I2C\n");
>  	} else {
>  		gi2c->gpi_mode = false;
> +
> +		if (gi2c->se.multi_owner)
> +			return dev_err_probe(gi2c->se.dev, -EINVAL,
> +					     "I2C sharing not supported in non-GSI mode\n");
> +

[Severity: Medium]
Does this early return leak the PM runtime usage counter?

By returning directly here, it seems to bypass the pm_runtime_put() cleanup
step at the err: label that the rest of the function uses on failure.

>  		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);

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