Re: [PATCH 6/6] spi: spi-geni-qcom: Add QSPI support over GPI DMA

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] spi: spi-geni-qcom: dummy_data len treated as cycles in
  setup_gsi_xfer()
- [Medium] spi: spi-geni-qcom: uncombined dummy_data sent as TX in
  setup_gsi_xfer()
- [Medium] spi: spi-geni-qcom: unreset started flag in
  spi_geni_transfer_one_message()
- [Medium] spi: geni-qcom: unvalidated tx_nbits and phase split in
  setup_gsi_xfer()
--

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

spi: spi-geni-qcom: Add QSPI support over GPI DMA

This commit adds QSPI controller support over GPI DMA for the
qcom,geni-qspi compatible, combining TX, optional dummy, and RX transfers
into a single SPI_TX_RX command.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index c2c3400b24c41..26dad85a590e3 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
[ ... ]
> @@ -412,27 +421,50 @@ spi_gsi_callback_result(void *cb, const struct dmaengine_result *result)
[ ... ]
>  static int setup_gsi_xfer(struct spi_transfer *xfer, struct spi_geni_master *mas,
> -			  struct spi_device *spi_slv, struct spi_controller *spi)
> +			  struct spi_device *spi_slv, struct spi_controller *spi,
> +			  struct spi_transfer *tx_rx_xfer)
>  {
>  	unsigned long flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
>  	struct dma_slave_config config = {};
>  	struct gpi_spi_config peripheral = {};
> -	struct dma_async_tx_descriptor *tx_desc, *rx_desc;
> +	struct dma_async_tx_descriptor *tx_desc, *rx_desc = NULL;
> +	struct spi_transfer *last = tx_rx_xfer ?: xfer;
>  	int ret;
>  
> +	/* QSPI receives data only as part of a combined TX-then-RX command. */
> +	if (mas->dev_data->is_qspi && xfer->rx_buf && !tx_rx_xfer)
> +		return -EOPNOTSUPP;
> +
>  	config.peripheral_config = &peripheral;
>  	config.peripheral_size = sizeof(peripheral);
>  	peripheral.set_config = true;
> @@ -443,20 +475,21 @@ static int setup_gsi_xfer(struct spi_transfer *xfer, struct spi_geni_master *mas
>  		mas->cur_speed_hz = xfer->speed_hz;
>  	}
>  
> -	if (xfer->tx_buf && xfer->rx_buf) {
> +	if (mas->dev_data->is_qspi && tx_rx_xfer) {
> +		/* Combine the command and data phases into one hardware command. */
> +		peripheral.cmd = SPI_TX_RX;
> +		peripheral.rx_len = get_xfer_len_in_words(tx_rx_xfer, mas);
> +	} else if (xfer->tx_buf && xfer->rx_buf) {
>  		peripheral.cmd = SPI_DUPLEX;
>  	} else if (xfer->tx_buf) {
>  		peripheral.cmd = SPI_TX;
>  		peripheral.rx_len = 0;
>  	} else if (xfer->rx_buf) {
>  		peripheral.cmd = SPI_RX;
> -		if (!(mas->cur_bits_per_word % MIN_WORD_LEN)) {
> -			peripheral.rx_len = ((xfer->len << 3) / mas->cur_bits_per_word);
> -		} else {
> -			int bytes_per_word = (mas->cur_bits_per_word / BITS_PER_BYTE) + 1;
> -
> -			peripheral.rx_len = (xfer->len / bytes_per_word);
> -		}
> +		peripheral.rx_len = get_xfer_len_in_words(xfer, mas);
> +	} else {
> +		dev_err(mas->dev, "Invalid transfer: no tx_buf or rx_buf\n");
> +		return -EINVAL;
>  	}

[Severity: Medium]
Can an uncombined dummy_data transfer or a bufferless transfer be sent on
the bus as an active SPI_TX DMA transfer here?

In GENI_GPI_DMA mode, spi->flags has SPI_CONTROLLER_MUST_TX set, so
spi_map_msg() sets xfer->tx_buf = spi->dummy_tx on every non-empty
transfer without a tx_buf (and callers such as spi_mem_exec_op() also
populate tx_buf on dummy_data transfers).  If a dummy_data transfer is not
merged into a [TX, dummy, RX] sequence in spi_geni_transfer_one_message(),
or if a transfer has no caller-supplied tx_buf or rx_buf, xfer->tx_buf is
still non-NULL.  It bypasses the bufferless check in
spi_geni_transfer_one_message() and takes the SPI_TX branch in
setup_gsi_xfer() with dummy_cycles = 0, driving dummy_tx bytes onto the
data lines instead of tri-stating or returning an error.  Because
xfer->tx_buf is always populated in GPI DMA mode, the SPI_RX and -EINVAL
branches here are also unreachable.

> @@ -466,6 +499,17 @@ static int setup_gsi_xfer(struct spi_transfer *xfer, struct spi_geni_master *mas
>  	peripheral.pack_en = true;
>  	peripheral.word_len = xfer->bits_per_word - MIN_WORD_LEN;
>  
> +	/* TRE_SPI_C0_DUMMY_CYCLES in the CONFIG0 TRE is an 8-bit field */
> +	if (mas->dummy_len > U8_MAX) {
> +		dev_err(mas->dev, "Dummy cycles %u exceed hardware limit %u\n",
> +			mas->dummy_len, (unsigned int)U8_MAX);
> +		return -EINVAL;
> +	}
> +	peripheral.dummy_cycles = mas->dummy_len;
> +
> +	if (mas->dev_data->is_qspi)
> +		peripheral.qspi_mode = spi_geni_qspi_mode(last);
[ ... ]
> @@ -1058,6 +1153,54 @@ static int spi_geni_transfer_one_message(struct spi_controller *spi,
>  	spi_geni_set_cs(spi_slv, !xfer->cs_off);
>  
>  	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		mas->dummy_len = 0;
> +		last_xfer = xfer;
> +
> +		/*
> +		 * Combine a TX phase, optional dummy phase and RX phase into
> +		 * one QSPI command.
> +		 */
> +		if (mas->dev_data->is_qspi && mas->cur_xfer_mode == GENI_GPI_DMA &&
> +		    xfer->len && xfer->tx_buf && xfer->tx_buf != spi->dummy_tx &&
> +		    !xfer->rx_buf && !xfer->dummy_data &&
> +		    !list_is_last(&xfer->transfer_list, &msg->transfers)) {
> +			next_xfer = list_next_entry(xfer, transfer_list);
> +
> +			if (next_xfer->dummy_data &&
> +			    !list_is_last(&next_xfer->transfer_list, &msg->transfers)) {
> +				/* The current client supplies the dummy length in cycles. */
> +				mas->dummy_len = next_xfer->len;
> +				next_xfer = list_next_entry(next_xfer, transfer_list);
> +			}

[Severity: High]
Does assigning next_xfer->len directly to mas->dummy_len program the wrong
number of dummy clock cycles for standard SPI clients?

In struct spi_transfer, len is always in bytes: the SPI core validates
word alignment, allocates dummy_tx, DMA-maps the buffer, and accounts
msg->actual_length (below in this function) in bytes, and callers such as
spi_mem_exec_op() set len = op->dummy.nbytes and tx_nbits =
op->dummy.buswidth.  Passing next_xfer->len directly into
peripheral.dummy_cycles without converting bytes to cycles via
(next_xfer->len * BITS_PER_BYTE) / next_xfer->tx_nbits programs too few
dummy cycles (for example, 8x too few in 1-bit mode), corrupting QSPI read
data.

> +
> +			/* Ignore the TX buffer inserted by SPI_CONTROLLER_MUST_TX. */
> +			if (next_xfer->len && next_xfer->rx_buf && !next_xfer->dummy_data &&
> +			    (!next_xfer->tx_buf || next_xfer->tx_buf == spi->dummy_tx)) {
> +				last_xfer = next_xfer;
> +			} else {
> +				mas->dummy_len = 0;
> +			}
> +		}
> +
> +		if (last_xfer != xfer) {
> +			next_xfer = xfer;
> +			for (;;) {
> +				/* A combined command cannot change timing or CS between phases. */
> +				if (next_xfer->speed_hz != xfer->speed_hz ||
> +				    next_xfer->bits_per_word != xfer->bits_per_word ||
> +				    next_xfer->cs_off || next_xfer->dtr_mode ||
> +				    next_xfer->word_delay.value ||
> +				    (next_xfer != last_xfer &&
> +				     (next_xfer->cs_change || next_xfer->delay.value))) {
> +					ret = -EOPNOTSUPP;
> +					goto out_err;
> +				}
> +				if (next_xfer == last_xfer)
> +					break;
> +				next_xfer = list_next_entry(next_xfer, transfer_list);
> +			}
> +		}

[Severity: Medium]
Does combining transfers into SPI_TX_RX drop the bus width of the TX and
dummy phases and fail to combine split command/address transfers?

In setup_gsi_xfer(), peripheral.qspi_mode is derived only from
spi_geni_qspi_mode(last), ignoring xfer->tx_nbits on the TX transfer and
next_xfer->tx_nbits on the dummy transfer, and this validation loop does
not check whether the phase bus widths are compatible with the single GO
TRE mode.  In addition, because spi_geni_probe() sets SPI_TX_DUAL/QUAD and
SPI_RX_DUAL/QUAD without implementing ctlr->mem_ops, spi_mem_exec_op()
splits reads with an address into four transfers ([CMD TX, ADDR TX, DUMMY,
DATA RX]); the combiner above only merges a single TX transfer before
DUMMY and DATA RX, sending CMD TX as a separate SPI_TX command.

> +
>  		if ((xfer->tx_buf || xfer->rx_buf) && xfer->len) {
>  			reinit_completion(&mas->xfer_done);
>  
> @@ -1066,16 +1209,17 @@ static int spi_geni_transfer_one_message(struct spi_controller *spi,
>  				     ((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);
> +				spi_geni_dma_sync(spi, xfer, last_xfer, true);
>  
> -			ret = spi_geni_transfer_one(spi, spi_slv, xfer);
> +			ret = spi_geni_transfer_one(spi, spi_slv, xfer,
> +						    last_xfer == xfer ? NULL : last_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);
> +			ret = spi_geni_wait_for_xfer(spi, xfer, last_xfer);
>  			if (ret)
>  				goto out_err;
>  
> @@ -1085,7 +1229,7 @@ static int spi_geni_transfer_one_message(struct spi_controller *spi,
>  				goto out_err;
>  			}
>  			if (dma_synced) {
> -				spi_geni_dma_sync(spi, xfer, false);
> +				spi_geni_dma_sync(spi, xfer, last_xfer, false);
>  				dma_synced = false;
>  			}
>  		} else if (xfer->len) {

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

If one transfer in a message completes normally and a subsequent transfer
fails validation before hardware submission (such as the combined-phase
check above or the -EOPNOTSUPP / -EINVAL checks in setup_gsi_xfer()),
out_err sees started == true from the earlier transfer and calls
spi_geni_handle_err() on an idle controller, resetting the GPI DMA
channels or issuing cancel/abort commands that time out.

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