Re: [PATCH v2 2/2] spi: qcom-geni: Use GPIO to notify master of SPI target activity

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver incorrectly swallows errors from `devm_gpiod_get_index_optional()`, breaking deferred probing (-EPROBE_DEFER).
- [High] The target ready GPIO is completely ignored when the SPI controller operates in GPI DMA mode (`GENI_GPI_DMA`).
- [High] A race condition between hardware initiation and GPIO assertion can leave the ready GPIO permanently stuck high.
--

--- Patch [2]: [PATCH v2 2/2] spi: qcom-geni: Use GPIO to notify master of SPI target activity ---
commit d3635e8d846f952a724421a12c51192475a16e09
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

spi: qcom-geni: Use GPIO to notify master of SPI target activity

When operating in SPI target mode, the GENI controller relies on an
external GPIO to notify the SPI master about the target's active state.

Add support for an optional ready GPIO that is asserted when a target
transfer begins and deasserted when the transfer completes, is aborted,
or hits a timeout. This allows the target to explicitly signal its
availability to the master and ensures the GPIO is released in all error
and completion paths, preventing the master from observing a stale or
incorrect target-ready indication.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 6c57f8309a3b2..bbe040383d530 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
[ ... ]
> @@ -938,8 +943,12 @@ static int spi_geni_transfer_one(struct spi_controller *spi,
>  	if (mas->cur_xfer_mode == GENI_SE_FIFO || mas->cur_xfer_mode == GENI_SE_DMA) {
>  		ret = setup_se_xfer(xfer, mas, slv->mode, spi);
>  		/* SPI framework expects +ve ret code to wait for transfer complete */
> -		if (!ret)
> +		if (!ret) {
> +			if (spi->target && mas->target_gpio)
> +				gpiod_set_value(mas->target_gpio, 1);

[Severity: High]
Is there a race condition here between hardware initiation and GPIO assertion
that could leave the ready GPIO permanently stuck high?

When setup_se_xfer() arms the hardware and drops the spinlock:

drivers/spi/spi-geni-qcom.c:setup_se_xfer() {
    ...
    spin_unlock_irq(&mas->lock);
    return ret;
}

If the SPI master immediately sends clocks, the hardware can complete the
transfer and trigger geni_spi_isr() on another CPU before we assert the GPIO
here. The ISR finalizes the transfer and deasserts the GPIO to 0.

Afterwards, this code resumes and unconditionally asserts the GPIO to 1,
overriding the completed state.

>  			ret = 1;
> +		}
> +
>  		return ret;
>  	}
>  	return setup_gsi_xfer(xfer, mas, slv, spi);

[Severity: High]
Does this bypass the GPIO assertion logic when the SPI controller operates in
GPI DMA mode (GENI_GPI_DMA)?

The GPIO assertion logic appears to be restricted to SE DMA and FIFO paths,
skipping the GPI DMA fallback path completely.

Furthermore, is the GPIO deassertion also missing in the GPI DMA completion
handler?

drivers/spi/spi-geni-qcom.c:spi_gsi_callback_result() {
    ...
    if (!result->residue) {
        spi->cur_msg->status = 0;
        dev_dbg(&spi->dev, "DMA txn completed\n");
    }
    ...
}

[ ... ]
> @@ -1131,8 +1144,16 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	init_completion(&mas->rx_reset_done);
>  	spin_lock_init(&mas->lock);
>  
> -	if (spi->target)
> +	if (spi->target) {
>  		spi->target_abort = spi_geni_target_abort;
> +		mas->target_gpio = devm_gpiod_get_index_optional(dev, "ready", 0,
> +								 GPIOD_OUT_LOW);
> +		if (IS_ERR(mas->target_gpio)) {
> +			dev_err(dev, "Failed to request GPIO: %ld\n",
> +				PTR_ERR(mas->target_gpio));
> +			mas->target_gpio = NULL;

[Severity: High]
Does this incorrectly swallow errors from devm_gpiod_get_index_optional(),
breaking deferred probing (-EPROBE_DEFER)?

If the GPIO provider probes after the SPI controller,
devm_gpiod_get_index_optional() will return -EPROBE_DEFER, but this catches
the error, logs it, and sets the pointer to NULL instead of propagating the
error to allow retrying later.

> +		}
> +	}
>  
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, 250);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-use_gpio_to_notify_master_of_spi_target_activity-v2-0-94d1eff3175f@oss.qualcomm.com?part=2