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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing Initialization / Dead Code
- [High] Incomplete Fix
- [High] Buffer Overflow / API Misuse
--

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

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

This patch updates the GENI Serial Engine driver to avoid selecting the
"sleep" pinctrl state when the engine is shared with another processor. It
introduces a multi_owner flag to conditionally skip the pinctrl state change
during shutdown.

> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 15636a8dc907a..3441ae3431e6a 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -607,9 +607,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;
> +	}

[Severity: High]
Does this regression also affect geni_se_resources_deactivate()?

Shared I2C and SPI buses use geni_se_resources_deactivate() as their active
power management callback. If a shared I2C or SPI controller initiates runtime
suspend, that function still unconditionally changes the pinctrl state:

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

Could this leave the primary protocol drivers vulnerable to the exact
disruption described in the commit message?

> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
> index c5e6ab85df099..9571da2c51a88 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;
>  };

[Severity: High]
Does this missing initialization cause a regression where the fix fails to
take effect?

The commit message mentions it is described via DeviceTree with
qcom,qup-multi-owner, but it doesn't appear that of_property_read_bool()
is called anywhere to populate multi_owner.

Without this, will the flag always remain false, preventing the fix from
taking effect for shared controllers?

[Severity: High]
Can this regression cause the driver to queue a truncated or corrupted
transfer chain to the DMA engine under heavy IO traffic?

In drivers/dma/qcom/gpi.c, gpi_prep_slave_sg() statically calculates the
required number of TREs without accounting for lock_action, which can add up
to 2 extra TREs (total 5):

drivers/dma/qcom/gpi.c:gpi_prep_slave_sg() {
    ...
    nr_tre = 3;
    set_config = *(u32 *)gchan->config;
    if (!set_config)
        nr_tre = 2;
    ...
}

If the transfer ring has 3 or 4 slots available, validation passes against the
smaller bound. However, when ring space runs out, gpi_queue_xfer() silently
drops the TRE:

drivers/dma/qcom/gpi.c:gpi_queue_xfer() {
    ...
    ret = gpi_ring_add_element(&gchan->ch_ring, (void **)&ch_tre);
    if (unlikely(ret)) {
        dev_err(gpii->gpi_dev->dev, "Error adding ring element to xfer ring\n");
        return;
    }
    ...
}

Will this cause the caller to submit a truncated chain, leading to hardware
hangs or data corruption?

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