Arithmetic underflow in SPI read routines

[michaelstoops]

I've identified an arithmetic underflow in the SPI read routines which seems to lead a deadlock, at least in my application.

The repeatability code is raspberrypi/pico-examples#101, in the case that the SPI slave starts while the SPI master is in the middle of a transmission. The slave correctly transmits 8 bytes and then deadlocks.

The flaw is in

pico-sdk/src/rp2_common/hardware_spi/spi.c

Line 79 in fc10a97

if (tx_remaining && spi_is_writable(spi) && rx_remaining - tx_remaining < fifo_depth) {

and similar lines below. In the error condition, tx_remaining is 248 while rx_remaining is 247. The operation underflows and gives a result of 0xFF FF FF FF FF FF FF FF.

I'm not 100% clear on what happens next, and it probably depends on the timing of signals coming in anyway. Either way, I think the arithmetic underflow is not intended.

[kilograham]

presumably you mean hang not deadlock

[michaelstoops]

presumably you mean hang not deadlock

Fair. :)

[michaelstoops]

Submitted proposed fix at #338.

[Wren6991]

I have merged #338, since the unsigned wrapping in the comparison was not intentional!

AFAIK it is an invariant for master mode that rx_remaining >= tx_remaining at any point in time, and it seems like slave mode breaks this invariant in some or all cases.

in the case that the SPI slave starts while the SPI master is in the middle of a transmission.

Could you expand on that please? Is the timing of the start of the slave transfer vs the start of the master transfer significant?

[michaelstoops]

Yes. Are you able to read Saleae Logic data files?

[michaelstoops]

Yes. Are you able to read Saleae Logic data files?

I guess you can download the application here, whether or not you have a Saleae device. https://www.saleae.com/downloads/

[kilograham]

i don't have enough context, but it would seem sensible that a slave might receive before transmitting? However whatever the outcome in that case, I worry about an off by one error on the other end of things (i.e. buffer being full)

[michaelstoops]

Here is the happy path. Slave starts first and waits for clock pulses from the master.

Happy_path.logicdata.zip

Here is the error path: master starts first, and slave starts while master is transmitting. The slave transmits only 8 bytes and stops.

master_first_slave_second.logicdata.zip

[Wren6991]

Yes. Are you able to read Saleae Logic data files?

I do have an old version of Logic installed yes. The trace attached to the PR does not seem to show this lockup case.

I'm not sure exactly what the circumstances are for rx_remaining becoming less than tx_remaining, but almost certainly to do with the fact that the push to the RX FIFO precedes the pop from the TX FIFO when in slave mode, whilst the reverse is true for master mode. Also noting that this check is likely not useful at all in slave mode, since you are not able to control the flow into the RX shift register at all, and if you get interrupted too heavily then the RX FIFO will simply overflow due to master TX being too fast.

[Wren6991]

So there is an underlying logic error in the SPI code, and the arithmetic unsigned wrapping issue was just a symptom of that. The FIFO depth clause should be disabled in slave mode.

[kilograham]

can this issue be closed?

[michaelstoops]

I opened this issue with the point to discuss the bug which was addressed through #338. I'm fine if you want to close this.

If there's a reason to keep this open, it's this:

i don't have enough context, but it would seem sensible that a slave might receive before transmitting? However whatever the outcome in that case, I worry about an off by one error on the other end of things (i.e. buffer being full)

On one hand, it seems like an audit is in order. On the other hand, I don't plan on doing it. Maybe nobody else does either, so it's up to you and your process.

Cheers,
Michael

[kilograham]

yeah, that was just me pontificating; if it works for you, i will close.