Erroneous, though benign, bit operation for nrf5x gpio

[steverpalmer]

(FYI I am a RIOT newbie, but have 40 years of coding experience. Thanks for RIOT by-the-way.)

Description

Short version - replace NRF_GPIOTE->INTENSET |= (GPIOTE_INTENSET_IN0_Msk << i); with NRF_GPIOTE->INTENSET = (GPIOTE_INTENSET_IN0_Msk << i);

In file cpu/nrf5x_common/periph/gpio.c function gpio_irq_enable sets register INTENSET with a C bitwise OR assignment (|=) on or around line 216. My understanding that this reads the register, ORs in the appropriate mask, and then writes it back. However, the behaviour of the ARM INTENSET register means that one only the assignment is required.

A similar case occurs in the following function gpio_irq_disable, which correctly assigns the mask value directly to the INTENCLR register, rather than trying to do a bitwise AND assignment with a negated mask.

I've done a "grep" search through the code base and have found no other instances of using bitwise OR assignment on an INTENSET register.

I'm 95% confident that this "bug" is benign (as in has no impact). The only case that I'm not sure of is whether the bitwise OR assignment is atomic. I could be wrong, but there may be a case if two pieces of thread/ISRs code are trying to gpio_irq_enable on different pins, when one interrupts the other. Maybe one of the assignments to INTENSET could be "lost"?

Steps to reproduce the issue

I wouldn't know how to start writing code to demonstrate this "bug". I found the fault just by code inspection as I was trying to debug my code and understand the RIOT code.

Versions

I noticed this in 2024-04 release, but it is also present in the master git branch.

[steverpalmer]

Thinking further, my "not-atomic" worry is not a problem, since writing a zero bit to the INTENSET register is a NOP.

I can think of no cases where this fault is detectable. I'll adjust the title correspondingly.

[dylad]

Hello @steverpalmer and welcome ! Indeed the read-modify-write operation is completely useless here, thanks for spotting this. While this code pattern does not create bug in such case, it introduces unnecessary operations so better save a few instructions here. Would you like to submit a PR to fix this ? If not, I could submit one.