Re: [PATCH 1/6] spi: spi-geni-qcom: Add transfer_one_message() support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] spi: spi-geni-qcom: unreset started flag in
  spi_geni_transfer_one_message()
- [Medium] spi: spi-geni-qcom: double handle_se_timeout() call on target
  abort
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] spi: geni-qcom: incorrect DMA timeout handling in
  spi_geni_target_abort() [1]
--

commit 3683cd703233401c535fc67323251b79d0c02313
Author: Jyothi Kumar Seerapu <jyothi.seerapu@oss.qualcomm.com>

spi: spi-geni-qcom: Add transfer_one_message() support

This commit replaces the SPI core's default transfer_one_message() path in
the Qualcomm GENI SPI driver with a driver-specific implementation that
handles message sequencing, GPIO chip-select control, transfer delays, DMA
buffer synchronization, and error recovery.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 6c57f8309a3b2..25c03096ee23f 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c

[ ... ]

> @@ -935,16 +940,206 @@ static int spi_geni_transfer_one(struct spi_controller *spi,

[ ... ]

> +static int spi_geni_transfer_one_message(struct spi_controller *spi,
> +					 struct spi_message *msg)
> +{
> +	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
> +	struct spi_device *spi_slv = msg->spi;
> +	struct spi_transfer *xfer, *next_xfer;
> +	bool keep_cs = false;
> +	bool dma_synced = false;
> +	bool started = false;
> +	int ret = 0;
> +
> +	xfer = list_first_entry(&msg->transfers, struct spi_transfer, transfer_list);
> +	spi_geni_set_cs(spi_slv, !xfer->cs_off);
> +
> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		if ((xfer->tx_buf || xfer->rx_buf) && xfer->len) {
> +			reinit_completion(&mas->xfer_done);
> +
> +			/* Synchronize buffers only for DMA; the SE FIFO path uses CPU access. */
> +			dma_synced = mas->cur_xfer_mode == GENI_GPI_DMA ||
> +				     ((xfer->tx_sg.nents || xfer->rx_sg.nents) &&
> +				      xfer->tx_sg.nents <= 1 && xfer->rx_sg.nents <= 1);
> +			if (dma_synced)
> +				spi_geni_dma_sync(spi, xfer, true);
> +
> +			ret = spi_geni_transfer_one(spi, spi_slv, xfer);
> +			if (ret < 0) {
> +				dev_err(mas->dev, "Transfer failed: %d\n", ret);
> +				goto out_err;
> +			}
> +			started = true;
> +
> +			ret = spi_geni_wait_for_xfer(spi, xfer);
> +			if (ret)
> +				goto out_err;
> +
> +			/* DMA errors and target aborts are reported through msg->status. */
> +			if (msg->status != -EINPROGRESS) {
> +				ret = msg->status;
> +				goto out_err;
> +			}
> +			if (dma_synced) {
> +				spi_geni_dma_sync(spi, xfer, false);
> +				dma_synced = false;
> +			}
> +		} else if (xfer->len) {
> +			dev_err(&spi_slv->dev,
> +				"Bufferless transfer has length %u\n", xfer->len);
> +			ret = -EINVAL;
> +			goto out_err;
> +		}
> +
> +		msg->actual_length += xfer->len;
> +		ret = spi_transfer_delay_exec(xfer);
> +		if (ret)
> +			goto out_err;

[ ... ]

> +out_err:
> +	if (!ret && msg->status != -EINPROGRESS)
> +		ret = msg->status;
> +
> +	if (ret && started)
> +		spi_geni_handle_err(spi, msg);

[Severity: Medium]
Should started be reset to false after each transfer completes, just like
dma_synced?

In a multi-transfer message, if one transfer finishes normally and a
subsequent transfer fails before starting hardware (for example, failing
the bufferless transfer check or set_rate() in setup_se_xfer()), started
remains true from the earlier transfer.  The out_err path then calls
spi_geni_handle_err() on an idle controller, where handle_se_timeout()
issues cancel and abort commands with no active transfer, waits up to two
seconds on completion timeouts, and sets mas->abort_failed = true, causing
later transfers to fail with -EBUSY.

[ ... ]

> @@ -1057,11 +1253,14 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
>  
>  static int spi_geni_target_abort(struct spi_controller *spi)
>  {
> +	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
> +
>  	if (!spi->cur_msg)
>  		return 0;
>  
> +	spi->cur_msg->status = -ECANCELED;
>  	handle_se_timeout(spi);

[Severity: High]
This problem wasn't introduced by this patch, but spi_geni_target_abort()
unconditionally calls handle_se_timeout() regardless of mas->cur_xfer_mode
[1].  When operating in GENI_GPI_DMA mode, this skips
handle_gpi_timeout() and dmaengine_terminate_sync(), and instead writes SE
registers such as SE_GENI_TX_WATERMARK_REG while the GPI DMA engine is
still active, leaving the controller in an unrecoverable state.  Should
this call spi_geni_handle_err() or dispatch on mas->cur_xfer_mode?

> -	spi_finalize_current_transfer(spi);
> +	complete(&mas->xfer_done);
>  
>  	return 0;
>  }

[Severity: Medium]
Does setting spi->cur_msg->status = -ECANCELED here cause
handle_se_timeout() to run twice on every target abort?

spi_geni_target_abort() calls handle_se_timeout() and then completes
mas->xfer_done.  When spi_geni_transfer_one_message() wakes up, it sees
msg->status != -EINPROGRESS, sets ret = -ECANCELED, and jumps to out_err
with started == true, which calls spi_geni_handle_err() ->
handle_se_timeout() a second time on the already-aborted hardware.  Because
mas->cur_xfer is already NULL on the second call, handle_se_timeout()
issues a redundant abort command that can time out after 1 second and set
mas->abort_failed = true.

In addition, if wait_for_completion_interruptible() in
spi_geni_wait_for_xfer() is interrupted by a signal at the same time as
spi_geni_target_abort(), both threads run handle_se_timeout() concurrently
without serialization on mas->abort_done, mas->tx_reset_done, and
mas->rx_reset_done.

[1] https://sashiko.dev/#/bug/linux-9f164df4-358b-4ab0-bbe7-3843965ed2e8

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-qspi_upstream_series-v1-0-21f43042dd60@oss.qualcomm.com?part=1