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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Masking of legitimate errors (such as -EPROBE_DEFER) from devm_gpiod_get_index_optional() breaks probe deferral.
- [High] A race condition between hardware transfer setup and completion can leave the target ready GPIO permanently asserted.
- [High] The target_gpio is manipulated from hard IRQ handlers and under spinlocks without verifying it is safe for atomic access.
- [Medium] The target ready GPIO is never asserted or deasserted when the controller operates in GENI_GPI_DMA mode.
--

--- Patch [2]: [PATCH v3 2/2] spi: qcom-geni: Use GPIO to notify master of SPI target activity ---
commit 53485a3ab9e87d880b4b4561671fdbf7ecc3b9cf
Author: Praveen Talari <[email protected]>

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

This commit adds support for an optional ready GPIO to notify the SPI master
of target activity. The GPIO is asserted when a transfer begins and deasserted
on completion or error.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> --- 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 it possible for the hardware to complete the transfer and trigger the ISR
before this GPIO assertion occurs?

Since setup_se_xfer() drops the mas->lock spinlock before returning, a fast
SPI master or immediate hardware error could trigger the interrupt. If the ISR
runs first and deasserts the GPIO:

geni_spi_isr()
    if (mas->cur_xfer) {
        ...
        if (spi->target && mas->target_gpio)
            gpiod_set_value(mas->target_gpio, 0);
    }

Would this code then unconditionally overwrite the GPIO to 1, leaving it
permanently asserted and falsely signaling readiness to the master?

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

[Severity: Medium]
Does the target ready GPIO need to be managed when operating in GENI_GPI_DMA
mode?

If mas->cur_xfer_mode is GENI_GPI_DMA, this path is taken, bypassing the new
GPIO assertion logic entirely. Could this leave the external SPI master
waiting indefinitely for the readiness signal?

[ ... ]

> @@ -981,6 +990,8 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
>  			if (mas->cur_xfer) {
>  				spi_finalize_current_transfer(spi);
>  				mas->cur_xfer = NULL;
> +				if (spi->target && mas->target_gpio)
> +					gpiod_set_value(mas->target_gpio, 0);

[Severity: High]
Could this cause a scheduling while atomic panic if the GPIO assigned in the
device tree is behind a sleeping controller, such as an I2C or SPI expander?

The GPIO is requested during probe without checking gpiod_cansleep() to
explicitly reject sleeping GPIOs, and is being manipulated here directly
inside a hardirq handler.

[ ... ]

> @@ -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 error handling block probe deferral?

By catching all errors from devm_gpiod_get_index_optional() and replacing them
with NULL, legitimate errors like -EPROBE_DEFER will be masked.

If the GPIO provider has not probed yet, will the SPI controller permanently
fail to acquire the GPIO for this boot rather than properly deferring?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-use_gpio_to_notify_master_of_spi_target_activity-v3-0-e1c9e0dabbda@oss.qualcomm.com?part=2