Skip to content

Commit 5fc1836

Browse files
authored
Bug fix: race condition in I2C cr1 read-modify-write
The `modify` method executes a read-modify-write operation on the `cr1` control register, as can be verified with the following disassembly code. Notably, the `read` variable is assigned a constant `true` within the example, which results in the elimination of the if comparison due to compiler optimization. ``` 8004a5e: f645 4400 movw r4, #23552 @ 0x5c00 8004a62: 4605 mov r5, r0 8004a64: f2c4 0400 movt r4, #16384 @ 0x4000 // r4 holds cr1 address 8004a68: 4688 mov r8, r1 8004a6a: 6820 ldr r0, [r4, #0] // read cr1 [first read] 8004a6c: 466e mov r6, sp 8004a6e: f440 7080 orr.w r0, r0, #256 @ 0x100 // modify start bit 8004a72: 6020 str r0, [r4, #0] // write cr1 [first write] 8004a74: 6820 ldr r0, [r4, #0] // read cr1 [second read] /* Code breaks if IRQ here */ 8004a76: f440 6080 orr.w r0, r0, #1024 @ 0x400 // modify ack bit /* Code breaks if IRQ here */ 8004a7a: 6020 str r0, [r4, #0] // write cr1 [second write] ``` The issue arises when an IRQ occurs between the second read and write operations. Due to the CPU's faster execution speed compared to the peripheral, it is highly likely that the second read operation will still observe the `cr1` register with the `start` bit set. However, if an IRQ is serviced, the peripheral may have sufficient time to complete generating the start condition, resulting in the hardware clearing the `start` bit in the peripheral's `cr1` register. Nonetheless, the copy of the `cr1` register stored in `r0` during the second read operation will still retain the `start` bit set. Consequently, when the second write operation is executed on `cr1`, the `start` bit is once again set. As a result, the peripheral attempts to generate a second start condition, leading to a situation where the I²C peripheral becomes unresponsive in my specific case. The solution is to swap the two method calls. Once the `ack` bit is set, it will not be automatically cleared, thus we are safe even if being interrupted in between.
1 parent 6ab795a commit 5fc1836

File tree

1 file changed

+1
-1
lines changed

1 file changed

+1
-1
lines changed

src/i2c/dma.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -421,10 +421,10 @@ where
421421

422422
fn send_start(&mut self, read: bool) -> Result<(), super::Error> {
423423
let i2c = &self.hal_i2c.i2c;
424-
i2c.cr1.modify(|_, w| w.start().set_bit());
425424
if read {
426425
i2c.cr1.modify(|_, w| w.ack().set_bit());
427426
}
427+
i2c.cr1.modify(|_, w| w.start().set_bit());
428428

429429
// Wait until START condition was generated
430430
while self

0 commit comments

Comments
 (0)