Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AXI SPI Engine driver modifies SPI xfer length. #2299

Open
dlech opened this issue Oct 20, 2023 · 2 comments
Open

AXI SPI Engine driver modifies SPI xfer length. #2299

dlech opened this issue Oct 20, 2023 · 2 comments

Comments

@dlech
Copy link
Collaborator

dlech commented Oct 20, 2023

30707b0 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
Copy link
Collaborator Author

dlech commented Oct 20, 2023

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

static int ad_pulsar_read_channel(struct ad_pulsar_adc *adc, unsigned int reg,
unsigned int *val)
{
struct spi_transfer xfer = {
.bits_per_word = adc->info->resolution,
.speed_hz = adc->info->sclk_rate,
.len = 4,
};
int ret;
adc->cfg = reg;
put_unaligned_be16(reg << 2, adc->spi_tx_data);
if (adc->info->cfg_register)
xfer.tx_buf = adc->spi_tx_data;
xfer.rx_buf = adc->spi_rx_data;
ret = spi_sync_transfer(adc->spi, &xfer, 1);
if (ret)
return ret;
*val = get_unaligned_le32(adc->spi_rx_data);
return ret;
}

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
Copy link
Collaborator

nunojsa commented Oct 23, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants