Re: [PATCH 6/7] Input: edt-ft5x06 - add SPI transport and FT8722 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Input: edt-ft5x06 - unaligned DMA rx buffer sharing cacheline with tx
--

commit 6c4cc2125db365d716cb624219a2c7fd67fb674b
Author: Luka Panio <lukapanio@gmail.com>

Input: edt-ft5x06 - add SPI transport and FT8722 support

This commit adds an SPI transport driver for FocalTech touchscreen
controllers handled by the edt-ft5x06 driver, along with firmware
download support for the RAM-only FT8722 controller.

> diff --git a/drivers/input/touchscreen/edt-ft5x06-spi.c b/drivers/input/touchscreen/edt-ft5x06-spi.c
> new file mode 100644
> index 0000000000000..d0ae0d4b621dd
> --- /dev/null
> +++ b/drivers/input/touchscreen/edt-ft5x06-spi.c
> @@ -0,0 +1,475 @@

[ ... ]

> +static int fts_spi_xfer(struct spi_device *spi, const u8 *tx, u8 *rx,
> +			size_t len)
> +{
> +	struct spi_transfer xfer = {
> +		.tx_buf = tx,
> +		.rx_buf = rx,
> +		.len = len,
> +	};
> +	int ret;
> +
> +	ret = spi_sync_transfer(spi, &xfer, 1);
> +	fsleep(FTS_SPI_CS_HIGH_US);
> +
> +	return ret;
> +}
> +
> +static int fts_spi_write(struct spi_device *spi, u8 reg, const void *val,
> +			 size_t len)
> +{
> +	size_t xlen = FTS_SPI_HEADER_LEN + (len ? FTS_SPI_DUMMY_LEN + len : 0);
> +	u8 *tx, *rx;
> +	int ret, i;
> +
> +	if (len > U16_MAX)
> +		return -EINVAL;
> +
> +	tx = kzalloc(2 * xlen, GFP_KERNEL);
> +	if (!tx)
> +		return -ENOMEM;
> +	rx = tx + xlen;
> +
> +	tx[0] = reg;
> +	tx[1] = FTS_SPI_CTRL_WRITE;
> +	put_unaligned_be16(len, &tx[2]);
> +	if (len)
> +		memcpy(&tx[FTS_SPI_HEADER_LEN + FTS_SPI_DUMMY_LEN], val, len);
> +
> +	for (i = 0; i < FTS_SPI_RETRIES; i++) {
> +		ret = fts_spi_xfer(spi, tx, rx, xlen);
> +		if (!ret && !(rx[3] & FTS_SPI_STATUS_ERR))
> +			break;
> +		if (!ret)
> +			ret = -EIO;
> +	}
> +
> +	kfree(tx);
> +	return ret;
> +}
> +
> +static int fts_spi_read(struct spi_device *spi, u8 reg, void *val,
> +			size_t len)
> +{
> +	size_t dp = FTS_SPI_HEADER_LEN + FTS_SPI_DUMMY_LEN;
> +	size_t xlen = dp + len + FTS_SPI_CRC_LEN;
> +	u8 *tx, *rx;
> +	int ret, i;
> +
> +	if (!len || len > U16_MAX)
> +		return -EINVAL;
> +
> +	tx = kzalloc(2 * xlen, GFP_KERNEL);
> +	if (!tx)
> +		return -ENOMEM;
> +	rx = tx + xlen;

[Severity: High]
Does placing rx at tx + xlen in fts_spi_write() and fts_spi_read() cause
tx and rx to share a cacheline during full-duplex SPI DMA transfers?

In both functions, a single buffer of size 2 * xlen is allocated, with tx
at offset 0 and rx = tx + xlen. Both buffers are then passed to
fts_spi_xfer() -> spi_sync_transfer() in a single full-duplex
spi_transfer, where the SPI core maps tx as DMA_TO_DEVICE and rx as
DMA_FROM_DEVICE concurrently.

Because xlen is typically not aligned to ARCH_DMA_MINALIGN (for example,
xlen is 4 or 8 for control writes, 10 or 11 for short reads, 71 for
touch report reads in edt_ft5x06_ts_isr(), and 32759 for firmware RAM
writes in fts_boot_write_ram()), rx starts in the middle of the cacheline
where tx ends.

On systems with DMA-capable SPI controllers, this overlapping mapping in
opposite DMA directions triggers DMA-API debug EEXIST warnings and risks
cacheline corruption between tx and rx during cache maintenance on
non-coherent architectures (particularly when xlen is large enough to
bypass swiotlb bouncing under CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC, or
across retries in the FTS_SPI_RETRIES loop), as well as presenting an
unaligned rx_buf pointer to SPI controllers that require aligned buffers.

Could tx and rx be allocated separately, or could the offset of rx be
aligned with ALIGN(xlen, ARCH_DMA_MINALIGN)?

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017140827.290666-1-lukapanio@gmail.com?part=6