spi: sam0: fix txrx, NULL buffers, and add a stub async method. #5731
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5731 +/- ##
==========================================
+ Coverage 54.04% 54.05% +<.01%
==========================================
Files 461 461
Lines 43743 43743
Branches 8369 8369
==========================================
+ Hits 23642 23645 +3
+ Misses 19960 19957 -3
Partials 141 141
Continue to review full report at Codecov.
|
|
LGTM 👍 |
drivers/spi/spi_sam0.c
Outdated
| @@ -146,7 +146,9 @@ static void spi_sam0_shift_master(SercomSpi *regs, struct spi_sam0_data *data) | |||
| rx = regs->DATA.reg; | |||
|
|
|||
| if (spi_context_rx_on(&data->ctx)) { | |||
There was a problem hiding this comment.
Is this check needed anymore?
There was a problem hiding this comment.
Yip, it's needed. The SPI API supports discarding a block by setting .buf to NULL and .len to the block length. spi_context_rx_on returns true if .buf is not null or .len is not zero.
There was a problem hiding this comment.
If .buf is null and .len is zero, "nothing" will be executed inside the if block.
If .buf is null and .len is not zero, "only" spi_context_update_rx will be executed inside the if block.
If .buf is not null and .len is zero, *data->ctx.rx_buf = rx; will be executed and spi_context_update_rx will return inside the if block.
If .buf is not null and .len is not zero, both *data->ctx.rx_buf = rx; and spi_context_update_rx will be executed.
I think that
if (spi_context_rx_on(&data->ctx)) {
if (data->ctx.rx_buf != NULL) {
*data->ctx.rx_buf = rx;
}
spi_context_update_rx(&data->ctx, 1, 1);
}
is the same with
if (data->ctx.rx_buf != NULL) {
*data->ctx.rx_buf = rx;
}
spi_context_update_rx(&data->ctx, 1, 1);
Am I missing something?
There was a problem hiding this comment.
Original code is actually right.
Sounds like what I did with spi_context_rx_on() is a bit flawed.
If buf != NULL but len == 0: we really don't want to store anything (this could lead to a buffer overflow).
rx is on if only len > 0 in fact. (since the 2 valid cases are : (buf && len) || (!buf && len), factorized: testing len is sufficient).
[edit] Guys, check PR #5785
There was a problem hiding this comment.
Note: I'll wait on the result of #5785.
There was a problem hiding this comment.
@tbursztyka I've updated after #5785. PTAL.
galak
added
area: SPI
This patches fixes a few bugs with the SAM0 driver: - txrx was trasnmitting too many bytes - adds support for NULL buffers to the fast paths - fixes a NULL dereference on the rx buffer slow path The tests under tests/driver/spi/spi_loopback now pass. Signed-off-by: Michael Hope <[email protected]>
Used to test the recent driver updates. Signed-off-by: Michael Hope <[email protected]>
nzmichaelh
force-pushed
the
fix_spi_txrx
branch
from
ca5d497 to
9802bfe
Compare
This gives better coverage on the SAM0 by exercising the RX only
paths.
The benchmark is a simple transcive-big-blocks-in-a-loop. This is
useful for the SAM0 as the driver is CPU based and the benchmark shows
how close the driver gets to peak throughput.
Signed-off-by: Michael Hope [email protected]