Fix #750 by maintaining the Rx tail pointer #1493
Conversation
Analagous to #1493, we should maintain the Tx tail pointer properly, as well. Note that since the device won't examine a Tx descriptor until the tail pointer is set appropriately, we can relax stores and rely on the barrier in `tx_notify` to flush everything to RAM properly before writing the tail pointer and kicking off a DMA.
Analagous to #1493, we should maintain the Tx tail pointer properly, as well. Note that since the device won't examine a Tx descriptor until the tail pointer is set appropriately, we can relax stores and rely on the barrier in `tx_notify` to flush everything to RAM properly before writing the tail pointer and kicking off a DMA.
There was a problem hiding this comment.
This is a very good fix. The previous mental model was that the OWN bit was the key concept driving IO progress and that the tail pointer was just used to tell the HW to look at descriptors again.
In fact, without proper use of the tail pointer, it is probable that the HW is constantly polling the descriptor for a change in the OWN bit, thereby wasting power and memory bandwidth. Dan observes that the HW was never entering the SUSPEND state.
Analagous to #1493, we should maintain the Tx tail pointer properly, as well. Note that since the device won't examine a Tx descriptor until the tail pointer is set appropriately, we can relax stores and rely on the barrier in `tx_notify` to flush everything to RAM properly before writing the tail pointer and kicking off a DMA.
Analagous to #1493, we should maintain the Tx tail pointer properly, as well. Note that since the device won't examine a Tx descriptor until the tail pointer is set appropriately, we can relax stores and rely on the barrier in `tx_notify` to flush everything to RAM properly before writing the tail pointer and kicking off a DMA.
|
tl;dr: I disagree about a few specific observations (and have seen contrary behavior on hardware) but agree with the fix, with one remaining concern.
I have observed differently on actual hardware, which appears to enter the suspended state when it reads a descriptor without the OWN bit set. This is also shown in Fig 784: 
In our existing code (where the tail pointer is always left at the one-past-the-end position), failing to service the DMA ring will eventually result in the hardware entering the suspended state (i.e. You can test this by enabling the commented-out code here (which logs Values here are ordered based on
I've seen evidence to the contrary on actual hardware. If I set the tail pointer to Here's a trivial diff to show this behavior: diff --git a/drv/stm32h7-eth/src/ring.rs b/drv/stm32h7-eth/src/ring.rs
index 733e6a5f..23fed467 100644
--- a/drv/stm32h7-eth/src/ring.rs
+++ b/drv/stm32h7-eth/src/ring.rs
@@ -453,7 +453,7 @@ impl RxRing {
/// section of the ring is initialized and can be read. (The answer is "all
/// of it.")
pub fn tail_ptr(&self) -> *const RxDesc {
- self.storage.as_ptr_range().end
+ self.storage.as_ptr_range().end.wrapping_sub(1)
}
/// Returns the count of entries in the descriptor ring / buffers in theIn the mental model where rewriting the pointer to the same value does not trigger a receive poll demand, we'd expect this to accept 3 frames, then stop forever (because the peripheral pointer is at That being said, these changes line up with my current mental model and I think they should work! There remains the potential for one subtle race condition, which may not be possible in practice. Consider the following situation: The typical behavior upon receiving a packet would be something like
The possible race condition is the following ordering of operations:
I'm not sure if this race condition is feasible in practice: it requires user code to handle the IRQ incredibly quickly. It certainly won't happen in our user code, which copies packet data out of the DMA buffers. |
This conflates two things. I see what you're saying about polling on the descriptor, and the diagram seems clear enough; I confess, once it became clear that the current operation was inherently racy, I didn't worry about that side of it too much anymore and instead concentrated on proper maintenance of the tail pointers. But note that the failure case is due to the DMA engine not entering the suspended state; that's because application code on the host is resetting the descriptor and sets the OWN bit before it's (re)observed by the peripheral, but after the peripheral has read the other bits of the descriptor structure. If one were to set the initial value of the tail pointer to just after the end of the ring in this change, the code would still be susceptible to the race condition until the first packet was fully processed and the tail pointer updated.
That is a reasonable interpretation, but in the list of states you linked earlier, there's a "stopped" state that parenthetically mentions "Reset or Stop Receive Command issued", and it may also be that going from the stopped state to running takes a different path than going from suspended to running, handling this case differently. This is something I think it would be useful to actually get some clarity from ST on.
Given that there may well be a difference when transitioning from stopped to running states and from suspended to running, and that the theory behind this race is predicated on their similarity, I'd urge caution here in that we haven't validated the base assumptions (that I'm aware of, anyway). If this were, indeed, possible that seems like a major oversight in ring handling in ST's hardware. While not impossible, looking again at their suite of states, given how they differentiate several different running states with respect to handling of the current (and next) descriptor, I would be very surprised if they did not test whether the tail pointer register had been written before suspending. At any rate, I do think we should follow up with ST, amending the earlier report with revised understanding and questions for them. |
|
I've updated the change description to hopefully reflect the present understanding a little bit better. |
dancrossnyc
force-pushed
the
fix750
branch
2 times, most recently
from
ec2fa58
to
0dca982
Compare
Analagous to #1493, we should maintain the Tx tail pointer properly, as well. Note that since the device won't examine a Tx descriptor until the tail pointer is set appropriately, we can relax stores and rely on the barrier in `tx_notify` to flush everything to RAM properly before writing the tail pointer and kicking off a DMA.
Agreed on both points!
Also agreed! To spell stuff out explicitly, it seems like you can't get from stopped → running if the peripheral's head and tail pointers are the same; but you can get from suspended → running (as discussed above).
I don't understand this comment. This race condition that I'm positing doesn't depend on transitions out of stopped / suspended states; the peripheral is running when the race window occurs. Perhaps you're questioning the assertion that the DMA peripheral will "never escape" once suspended? That's because user code will never receive another packet (all packets have been handled, we've poked the tail pointer, and the peripheral is suspended), so user code will never again poke the tail pointer to kick the peripheral back into action. |
Yes, that fits the observed behavior, though it may be worth noting that this seems to contradict the datasheet in section 58.10.2.
No. Let me see if I can explain a bit better. Earlier you wrote:
This is based on the observation that writing tail, where tail==head, to the tail pointer register kicks it out of suspended state and into running. I think what this means is that the decision to transition from running to suspended state happens only when we're actively processing the descriptor ring and transitioning from one descriptor to another (that is, when we're running), but by focusing on the pointer increment here we are making an assumption: that the decision to suspend ignores concurrent writes to the tail pointer register. This assumption seems to underlie this hypothetical race. However, I see no evidence that the decision to transition to the suspended state relies solely on the equality test after increment; it may well be that the device latches indicators of writes to the tail pointer register, resets that before the increment and test, and checks the latch again after, thus detecting a write to the register that would negate the Suspension criteria (interrupt controllers have to do things like this to all the time to avoid missing interrupts, so I don't think it would be too unusual, and it is a rather obvious race condition). Alternatively, they may make the decision to suspend before generating an interrupt. Of course, it may also be that it doesn't do any of these things, but we just don't know. Absent evidence that there's actually a race here, I'm leery of assuming that there is one. In any event, it's certainly worth asking ST. |
Yup! For those following along at home, here's the relevant section: 
(this is already trivially contradicted by ST's example code, which always writes the tail pointer to 0; the
I understand your point now, thanks for the explanation! |
Analagous to #1493, we should maintain the Tx tail pointer properly, as well. Note that since the device won't examine a Tx descriptor until the tail pointer is set appropriately, we can relax stores and rely on the barrier in `tx_notify` to flush everything to RAM properly before writing the tail pointer and kicking off a DMA.
Analagous to #1493, we should maintain the Tx tail pointer properly, as well. Note that since the device won't examine a Tx descriptor until the tail pointer is set appropriately, we can relax stores and rely on the barrier in `tx_notify` to flush everything to RAM properly before writing the tail pointer and kicking off a DMA.
At its root, #750 is about a race condition between the DMA engine and the STM32 Ethernet, and the host CPU. We can prevent this race by properly maintaining the Rx tail pointer. Some background information to contextual this change: The device and host participate in a simple protocol to drive a state machine: the host gives descriptors to the device that specify buffers to DMA incoming Ethernet frames into. The host gives a descriptor to the device by setting the ownership bit and setting the tail pointer: this driver handles the former properly, but does not handle the latter correctly. The device consumes these descriptors as frames arrive and it DMAs them host memory. In turn, the device lets the host know that it is done with a descriptor by doing two things: 1. Resetting the ownership bit in the descriptor, and 2. Sending an interrupt to the host. Internally, the device _also_ increments its "head" pointer in the Rx ring (modulo the size of the ring). If the device hits the end of the ring (as specified by the value in the Rx tail pointer register), it goes into a "Suspended" state and stops DMAing packets to the host; it resumes when the Rx tail pointer register is written again (note: writing a value that is the same as the head value on the _first_ write doesn't seem to start the device). Both device and host maintain (or, perhaps more precisely, should maintain) two pointers into the ring: a producer pointer, and a consumer pointer. One may think of this as the host producing available descriptors that are consumed by the device, and the device producing received frames that are consumed by the host. The tail pointer sets a demarkation point between the host and device: descriptors before the tail pointer are owned by the device, those after are owned by the host. The ownership bit is used to communicate to the host that the device is done with a given descriptor. Put another way, the tail pointer represents the host's producer pointer; the host's consumer pointer is simply the next descriptor in the ring after the last one that it processed. The device's consumer pointer is whatever the host set the tail pointer to, and it's producer pointer is its head pointer. If the device encounters a situation where _its_ head and tail pointer are equal, the ring is full, and the DMA engine enters the suspended state until the tail pointer is rewritten. An invariant that both sides of the protocol must maintain is that host software ensures that it's producer pointer (that is, what it sets the tail pointer to) is always equal to or (ie, the ring is full), or greater than (ie, there is space available in the ring) to the devices's producer pointer (that is, the devices's "head" pointer). Of course, we talk about an ordering relationship here, but all of this is happening modulo the size of the ring, but that's a detail. Note that there are two cases here: 1. The first time you write to the tail pointer to set things in motion at the beginning of time, and 2. Every other time. For the first time, this change writes the address of the last valid descriptor in the ring into the tail pointer register (not one beyond the last!), as we have observed there is no non-racy way to provide the full ring to the device immediately after reset: writing the beginning of the ring to the tail pointer does not start the peripheral, and writing just beyond the end of it open us up to a race against updating the first descriptor (until the first time the tail pointer is reset). After consuming and resetting a descriptor on the host, we write the address of the next descriptor in the ring to the tail pointer, indicating that the just-processed descriptor is available to the device. As we do this for every received frame we do not starve the ring. In this scenario, the device follows the host around the ring; the problem with the first write is solved since, after the first frame reception, the device's head pointer will be somewhere beyond the first descriptor in the ring. Thus, writing the tail pointer to the address of (say) the 0'th element is fine, since that won't be equal to the head pointer on the peripheral. Similarly, all available descriptors can be consumed by the device. Finally, this allows for some simplification with respect to barriers: since the tail pointer indicates which descriptors are owned by the device and which by the host, the host can batch updates to the descriptors and do a single flush at the end of its processing, write before writing to the tail pointer. This was already done in `rx_notify`. A similar change for the Tx ring has also been sent. I have validated that, whereas I could reproduce this issue in a demo branch prior to this change, I cannot reproduce it with this change.
Analagous to #1493, we should maintain the Tx tail pointer properly, as well. Note that since the device won't examine a Tx descriptor until the tail pointer is set appropriately, we can relax stores and rely on the barrier in `tx_notify` to flush everything to RAM properly before writing the tail pointer and kicking off a DMA.
Analagous to #1493, we should maintain the Tx tail pointer properly, as well. Note that since the device won't examine a Tx descriptor until the tail pointer is set appropriately, we can relax stores and rely on the barrier in `tx_notify` to flush everything to RAM properly before writing the tail pointer and kicking off a DMA.
Analagous to #1493, we should maintain the Tx tail pointer properly, as well. Note that since the device won't examine a Tx descriptor until the tail pointer is set appropriately, we can relax stores and rely on the barrier in `tx_notify` to flush everything to RAM properly before writing the tail pointer and kicking off a DMA.
Analagous to #1493, we should maintain the Tx tail pointer properly, as well. Note that since the device won't examine a Tx descriptor until the tail pointer is set appropriately, we can relax stores and rely on the barrier in `tx_notify` to flush everything to RAM properly before writing the tail pointer and kicking off a DMA.
At its root, #750 is about a race condition between the DMA engine and the STM32 Ethernet, and the host CPU. We can prevent this race by properly maintaining the Rx tail pointer.
Some background information to contextual this change:
The device and host participate in a simple protocol to drive a state machine: the host gives descriptors to the device that specify buffers to DMA incoming Ethernet frames into. The host gives a descriptor to the device by setting the ownership bit and setting the tail pointer: this driver handles the former properly, but does not handle the latter correctly.
The device consumes these descriptors as frames arrive and it DMAs them host memory. In turn, the device lets the host know that it is done with a descriptor by doing two things:
Internally, the device also increments its "head" pointer in the Rx ring (modulo the size of the ring). If the device hits the end of the ring (as specified by the value in the Rx tail pointer register), it goes into a "Suspended" state and stops DMAing packets to the host; it nominally resumes when the Rx tail pointer register is written again (if the newly written tail register value is the same as the old value, it doesn't appear to continue: note this implies it is not possible to transition from a full ring directly to an empty ring).
Both device and host maintain (or, perhaps more precisely, should maintain) two pointers into the ring: a producer pointer, and a consumer pointer.
One may think of this as the host producing available descriptors that are consumed by the device, and the device producing received frames that are consumed by the host. The tail pointer sets a demarkation point between the host and device: descriptors before the tail pointer are owned by the device, those after are owned by the host. The ownership bit is used to communicate to the host that the device is done with a given descriptor.
Put another way, the tail pointer represents the host's producer pointer; the host's consumer pointer is simply the next descriptor in the ring after the last one that it processed. The device's consumer pointer is whatever the host set the tail pointer to, and it's producer pointer is its head pointer. If the device encounters a situation where its head and tail pointer are equal, the ring is full, and the DMA engine enters the suspended state until the tail pointer is rewritten.
An invariant that both sides of the protocol must maintain is that host software ensures that it's producer pointer (that is, what it sets the tail pointer to) is always equal to or (ie, the ring is full), or greater than (ie, there is space available in the ring) to the devices's producer pointer (that is, the devices's "head" pointer).
Of course, we talk about an ordering relationship here, but all of this is happening modulo the size of the ring, but that's a detail.
Note that there are two cases here:
For the first time, this change writes the address of the last valid descriptor in the ring into the tail pointer register (not one beyond the last!). Then after consuming and resetting a descriptor on the host, we write the address of the next valid descriptor in the ring to the tail pointer, indicating that the just-processed descriptor is available to the device. As we do this for every received frame, we can never starve the ring (at least, not due to this).
In this scenario, the device will follows the host around the ring; the problem with the first write is solved since, after the first frame reception, the device's head pointer will be somewhere beyond the first descriptor in the ring. Thus, writing the tail pointer to the address of (say) the 0'th element is fine, since that won't be equal to the head pointer on the peripheral. Similarly, all available descriptors can be consumed by the device.
Finally, this allows for some simplification with respect to barriers: since the tail pointer indicates which descriptors are owned by the device and which by the host, the host can batch up updates to the descriptors and do a single flush at the end of its processing, write before writing to the tail pointer. This was already done in
rx_notify.Note that something similar should probably be done for the transmission ring, which I suspect is vulnerable to a similar race between the DMA engine on the device and writes on the host.
I have validated that, whereas I could reproduce this issue in a demo branch prior to this change, I cannot reproduce it with this change.