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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Single-message transfers on a multi-owner controller acquire the hardware lock but never release it, deadlocking the shared controller.
- [High] Extraneous lock command TRE is emitted on the RX DMA channel for read transfers, violating hardware protocol.
- [Medium] PM runtime usage counter is leaked when rejecting multi-owner configurations in non-GPI mode.
- [Low] Usage of `of_property_read_bool()` ignores ACPI configurations for multi-owner support.
--

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

i2c: qcom-geni: Support multi-owner controllers in GPI mode

This commit introduces support for sharing a QUP-based I2C controller with
another system processor. It enables multi-owner operation using the
qcom,qup-multi-owner property and serializes GPI mode transfers using
hardware lock/unlock commands.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 96dbf04138bee..757c2c8eb2071 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -832,6 +832,14 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>  		if (i < num - 1)
>  			peripheral.stretch = 1;
>  
> +		peripheral.lock_action = GPI_LOCK_NONE;
> +		if (gi2c->se.multi_owner) {
> +			if (i == 0)
> +				peripheral.lock_action = GPI_LOCK_ACQUIRE;
> +			else if (i == num - 1)
> +				peripheral.lock_action = GPI_LOCK_RELEASE;
> +		}
> +

[Severity: High]
Does this code deadlock the shared controller for single-message transfers?

If num == 1, i == 0 evaluates to true and peripheral.lock_action is set
to GPI_LOCK_ACQUIRE. The else if (i == num - 1) branch is skipped entirely,
meaning GPI_LOCK_RELEASE is never requested. Since lock_action is a single 
enum value, it doesn't appear possible to emit both lock and unlock commands
for a single message this way.

[Severity: High]
Can this cause an extraneous lock command TRE to be emitted on the RX DMA
channel?

Because config.peripheral_config points to peripheral, the lock_action is
passed to the TX channel when calling geni_i2c_gpi() for I2C_WRITE. However,
if this is a read transfer (msgs[i].flags & I2C_M_RD), the same config
structure is immediately reused to configure the RX channel. 

Because peripheral.lock_action is not cleared back to GPI_LOCK_NONE, the RX
channel will also receive a GPI_LOCK_ACQUIRE command TRE, which could violate
the hardware protocol expecting command TREs only on the TX channel.

>  		peripheral.addr = msgs[i].addr;
>  		if (i > 0 && (!(msgs[i].flags & I2C_M_RD)))
>  			peripheral.multi_msg = false;

[ ... ]

> @@ -1028,6 +1040,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 direct return leak the PM runtime usage counter?

At the beginning of geni_i2c_init(), pm_runtime_resume_and_get() is called. 
By returning directly here instead of using the goto err; cleanup path, 
the driver bypasses pm_runtime_put(), leaving the device power domain active.

>  		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
>  
>  		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */

[ ... ]

> @@ -1098,6 +1115,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  		gi2c->clk_freq_out = I2C_MAX_STANDARD_MODE_FREQ;
>  	}
>  
> +	if (of_property_read_bool(pdev->dev.of_node, "qcom,qup-multi-owner")) {
> +		gi2c->se.multi_owner = true;
> +		dev_dbg(&pdev->dev, "I2C controller is shared with another system processor\n");
> +	}
> +

[Severity: Low]
Does this hardcode the multi-owner feature strictly to DeviceTree?

The driver supports ACPI companions (checked right below this via
has_acpi_companion(dev)). Using of_property_read_bool() ignores ACPI
platforms. Could this use device_property_read_bool(dev, "qcom,qup-multi-owner")
to uniformly support both ACPI _DSD and DeviceTree?

>  	if (has_acpi_companion(dev))
>  		ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
>

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