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

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 <[email protected]>

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/[email protected]?part=3