analogdevicesinc / linux

Linux kernel variant from Analog Devices; see README.md for details
https://github.com/analogdevicesinc/linux
Other
445 stars 844 forks source link

AXI SPI Engine driver modifies SPI xfer length. #2299

Open dlech opened 1 year ago

dlech commented 1 year ago

https://github.com/analogdevicesinc/linux/commit/30707b090c25161e995ecdeebba70e8d5388f920 introduced a function spi_engine_update_xfer_len() that modifies the len field of each xfer in an SPI message. However, modifying the struct spi_transfer is problematic.

Since spi_engine_compile_message() actually gets called twice per message (once is a "dry" run to calculate the size, then again to actually write the commands to memory), spi_engine_update_xfer_len() is getting called twice and therefore modifies the length twice. This probably has gone unnoticed since the first call usually sets the length to 1 and the second call can't make it any smaller.

Modifying the members of struct spi_transfer in the SPI controller doesn't seem like a good idea in general since it doesn't own that memory. For example, a client driver may be reusing the struct spi_transfer and not write the length again before each transaction.

(The commit in question is not in the mainline kernel, but it fixes a potential issue that may still exist in the mainline kernel.)

dlech commented 1 year ago

Fixing this is likely going to break things that depend on this buggy behavoir. For example, in the ad_pulsar driver.

https://github.com/analogdevicesinc/linux/blob/b8d0e51373d107a62a364ec7c7abce9cad6ba7cd/drivers/iio/adc/ad_pulsar.c#L359-L382

For a single conversion (reading the _raw sysfs attribute), the xfer len is always 4 even when the ADC is 16 bits or less. Due to the double evaluation of spi_engine_update_xfer_len(), the len gets modified to 1 word which is correct for those ADCs. After fixing, this will instruct it to read two word instead when it only needs to read one.

nunojsa commented 1 year ago

I guess my comments in #2300 can also be applied in here