Re: [PATCH 09/11] spi: Add NVIDIA LSTP SPI driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] spi: lstp: missing set_cs causes CS assertion leak and bus contention
- [Medium] spi: lstp: xfer->delay executes after chip select deassertion
--

commit b315e8ac97c56dc30f3204c3748b56ba5483565f
Author: Adrian Ambrozewicz <aambrozewicz@nvidia.com>

spi: Add NVIDIA LSTP SPI driver

Add a driver for the SPI controller channels of NVIDIA LSTP USB adapters.
Each channel is registered as an SPI host controller, and devices behind it
can be created via device tree, ACPI, or userspace. The firmware reports
chip selects and bus speed.

> diff --git a/drivers/spi/spi-lstp.c b/drivers/spi/spi-lstp.c
> new file mode 100644
> index 0000000000000..7aa908e6c398e
> --- /dev/null
> +++ b/drivers/spi/spi-lstp.c

[ ... ]

> +static int lstp_spi_transfer_one_message(struct spi_controller *ctrl, struct spi_message *mesg)
> +{
> +	struct lstp_channel *ch = spi_controller_get_devdata(ctrl);
> +	struct spi_transfer *xfer;
> +	u8 chip_select = spi_get_chipselect(mesg->spi, 0);
> +	int ret = 0;
> +	bool cs_active = false;
> +
> +	if (chip_select >= ctrl->num_chipselect) {
> +		dev_err_ratelimited(ch->dev, "%s: ch_%d: Invalid chip select %u (max %u)\n",
> +				    __func__, ch->ch_id, chip_select, ctrl->num_chipselect - 1);
> +		ret = -EINVAL;
> +		goto finalize;
> +	}
> +
> +	u8 cs_bits = lstp_spi_cs_map[chip_select];
> +
> +	list_for_each_entry(xfer, &mesg->transfers, transfer_list) {
> +		u8 cs_flags = 0;
> +		bool is_last = list_is_last(&xfer->transfer_list, &mesg->transfers);
> +		bool no_data = !xfer->tx_buf && !xfer->rx_buf;
> +
> +		if (!cs_active) {
> +			cs_flags |= LSTP_SPI_CMD_CS_ASSERT;
> +			cs_active = true;
> +		}
> +
> +		if ((is_last && !xfer->cs_change) || (!is_last && xfer->cs_change)) {
> +			cs_flags |= LSTP_SPI_CMD_CS_DEASSERT;
> +			cs_active = false;
> +		}
> +
> +		/* A data transfer fuses delay and CS edge into one command; CS-only waits first. */
> +		if (no_data)
> +			spi_transfer_delay_exec(xfer);
> +
> +		ret = lstp_spi_do_transfer(ch, xfer, cs_bits, cs_flags);
> +		if (ret)
> +			break;

[Severity: High]
Does this approach to managing chip select state lead to bus contention?

Since lstp_spi_probe() intentionally omits the ctrl->set_cs callback, the core
SPI subsystem cannot forcibly deassert the chip select on bus idle or when
switching devices.

If lstp_spi_do_transfer() fails mid-message, the loop breaks early without
appending the LSTP_SPI_CMD_CS_DEASSERT flag, permanently leaking the asserted
CS state to the hardware.

Additionally, if a message intentionally ends with xfer->cs_change = 1 to hold
CS active across messages, the next message targeting a different SPI device
will start with a local cs_active = false. The driver will assert the new
chip select, but the old one is never deasserted, leaving the hardware with
multiple active CS lines.

> +
> +		mesg->actual_length += xfer->len;
> +
> +		if (!is_last && xfer->cs_change)
> +			spi_transfer_cs_change_delay_exec(mesg, xfer);
> +
> +		if (!no_data)
> +			spi_transfer_delay_exec(xfer);

[Severity: Medium]
Does the execution of spi_transfer_delay_exec() happen too late?

The driver bundles the DEASSERT flag into the data transfer command, meaning
the hardware drops the chip select immediately after clocking the data.

However, the host CPU executes spi_transfer_delay_exec() after
lstp_spi_do_transfer() returns, meaning the delay occurs after the hardware
has already completed the command and dropped the chip select.

Can this violate timing constraints for SPI peripherals that require a specific
pre-deassertion hold time after the final bit is clocked out?

> +	}
> +
> +finalize:
> +	mesg->status = ret;
> +	spi_finalize_current_message(ctrl);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-lstp-usb-adapter-v1-0-aambrozewicz@nvidia.com?part=9