Bug fix: race condition in I2C cr1 read-modify-write

[zyma98]

Buggy code:

I2C.cr1.modify(|_, w| w.start().set_bit()); if read { I2C.cr1.modify(|_, w| w.ack().set_bit()); }

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.

Attached is the bug captured on a logic analyzer.

[burrbull]

I think there should be comment with link to this PR.

And add this to changelog.

[burrbull]

What about:

if read { I2C.cr1.modify(|_, w| w.ack().set_bit().start().set_bit()); } else { I2C.cr1.modify(|_, w| w.start().set_bit()); }

?

[zyma98]

What about:

if read { I2C.cr1.modify(|_, w| w.ack().set_bit().start().set_bit()); } else { I2C.cr1.modify(|_, w| w.start().set_bit()); }

?

Your suggestion is definitely better. It's combined into a single read-modify-write.

 8004a5e:	f645 4400	movw	r4, #23552	@ 0x5c00 8004a62:	4605	mov	r5, r0 8004a64:	f2c4 0400	movt	r4, #16384	@ 0x4000 8004a68:	4688	mov	r8, r1 8004a6a:	6820	ldr	r0, [r4, #0] 8004a6c:	466e	mov	r6, sp 8004a6e:	f440 60a0	orr.w	r0, r0, #1280	@ 0x500 8004a72:	6020	str	r0, [r4, #0] 8004a74:	4630	mov	r0, r6 

Should I update the pull request?

[burrbull]

Yes, please.

[zyma98]

Updated.