Skip to content

Erroneous, though benign, bit operation for nrf5x gpio #20736

@steverpalmer

Description

@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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions