Re: [PATCH v4 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 4 potential issue(s) to consider:
- [High] The driver masks errors from `devm_gpiod_get_optional()`, breaking probe deferral when the GPIO provider is not yet loaded.
- [High] A race condition between starting the hardware transfer and asserting `ready_gpio` can leave the ready signal permanently stuck high.
- [High] The `ready_gpio` signaling is completely bypassed when the SPI controller operates in GSI DMA mode (`GENI_GPI_DMA`), causing the SPI master to hang indefinitely.
- [Medium] The driver manipulates `ready_gpio` in hardirq context without checking if the provided GPIO can sleep.
--

--- Patch [2]: [PATCH v4 2/2] spi: qcom-geni: Use GPIO to notify master of SPI target activity ---
commit 8e9921c3fb958996ba0f3bd409f4db2936232e27
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.
This adds support for an optional ready GPIO that is asserted when a target
transfer begins and deasserted when the transfer completes.

> 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)
> +				gpiod_set_value(mas->ready_gpio, 1);
>  			ret = 1;
> +		}
> +
>  		return ret;
>  	}

[Severity: High]
Does this introduce a race condition with the interrupt handler that could
leave the ready GPIO permanently asserted?

In setup_se_xfer(), the transfer is started and the mas->lock is dropped.
If the master clocks the transfer immediately, it could complete and trigger
geni_spi_isr() before we reach the GPIO assertion here in
spi_geni_transfer_one().

The interrupt handler would deassert the GPIO on completion, but then this
function would unconditionally assert it again, leaving it stuck high.

[Severity: High]
Are we missing the ready signal assertion when the controller operates in
GSI DMA mode?

If the hardware is configured to use GENI_GPI_DMA, spi_geni_transfer_one()
routes the transfer to setup_gsi_xfer() directly below, which bypasses the
ready_gpio assertion entirely. 

Looking at setup_gsi_xfer(), it doesn't appear to assert the ready signal
before initiating DMA, and the completion callback spi_gsi_callback_result()
does not deassert it.

>  	return setup_gsi_xfer(xfer, mas, slv, spi);
>  }
[ ... ]
> @@ -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)
> +					gpiod_set_value(mas->ready_gpio, 0);

[Severity: Medium]
Will this cause a 'sleeping in atomic context' splat if the ready GPIO is
backed by a sleeping controller?

Since geni_spi_isr() runs in hardirq context, gpiod_set_value() could sleep
if the GPIO provider is an I2C or SPI expander. Should we verify
!gpiod_cansleep() during probe before using it here?

>  				/*
>  				 * If this happens, then a CMD_DONE came before all the
>  				 * Tx buffer bytes were sent out. This is unusual, log
[ ... ]
> @@ -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->ready_gpio = devm_gpiod_get_optional(dev, "ready",
> +							  GPIOD_OUT_LOW);
> +		if (IS_ERR(mas->ready_gpio)) {
> +			dev_err(dev, "Failed to request GPIO: %ld\n",
> +				PTR_ERR(mas->ready_gpio));
> +			mas->ready_gpio = NULL;
> +		}
> +	}

[Severity: High]
Does overwriting mas->ready_gpio with NULL break probe deferral?

If devm_gpiod_get_optional() returns -EPROBE_DEFER because the GPIO
controller isn't loaded yet, this error is masked and the SPI driver will
successfully probe but permanently operate without the required readiness
signaling.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-use_gpio_to_notify_master_of_spi_target_activity-v4-0-fde2ebf043db@oss.qualcomm.com?part=2