[Coverity CID: 220305] Logically dead code in drivers/gpio/gpio_nrfx.c

[zephyrbot]

Static code scan issues found in file:

https://github.com/zephyrproject-rtos/zephyr/tree/169144afa1826511ee6ec3f53d590b2c0d39d3d4/drivers/gpio/gpio_nrfx.c#L448

Category: Control flow issues
Function: check_level_trigger_pins
Component: Drivers
CID: 220305

Details:

zephyr/drivers/gpio/gpio_nrfx.c

Line 448 in 169144a

if ((data->double_edge & bit) ||

442                 if (!(level_pins & bit)) {
443                     uint32_t sense = nrf_gpio_pin_sense_get(abs_pin);
444                     bool high = (sense == GPIO_PIN_CNF_SENSE_High);
445    
446                     if (port_latch & bit) {
447                         /* check if there was an interrupt */
>>>     CID 220305:  Control flow issues  (DEADCODE)
>>>     Execution cannot reach this statement: "if (data->double_edge & bit...".
448                         if ((data->double_edge & bit) ||
449                             ((!!data->int_active_level) == high)) {
450                             out |= bit;
451                         }
452    
453                         /* invert configured level */

Please fix or provide comments in coverity using the link:

https://scan9.coverity.com/reports.htm#v29271/p12996

Note: This issue was created automatically. Priority was set based on classification
of the file affected and the impact field in coverity. Assignees were set using the CODEOWNERS file.

[mniestroj]

False positive. Statement can be reached with CONFIG_GPIO_NRF_INT_EDGE_USING_SENSE=y.

[dleach02]

Given that port_latch is initialized to zero and only set within the CONFIG_GPIO_NRF_INT_EDGE_USING_SENSE would it make sense to put this whole 'if (port_latch & bit)' block within a similar conditional compile block as the assignment?

[mniestroj]

The idea was to avoid conditional compile blocks, as they obfuscate the code. The goal was to use if (IS_ENABLED(CONFIG_GPIO_NRF_INT_EDGE_USING_SENSE)) { instead of #if IS_ENABLED(CONFIG_GPIO_NRF_INT_EDGE_USING_SENSE) whenever possible. However cfg->port->LATCH; is not valid when NRF_GPIO_LATCH_PRESENT is not set, which is why there is a build error triggered if CONFIG_GPIO_NRF_INT_EDGE_USING_SENSE is used on platforms without LATCH register:

zephyr/drivers/gpio/gpio_nrfx.c

Lines 14 to 18 in 169144a

	#if IS_ENABLED(CONFIG_GPIO_NRF_INT_EDGE_USING_SENSE) && \
	!defined(NRF_GPIO_LATCH_PRESENT)
	#error "GPIO LATCH is required by edge interrupts using GPIO SENSE," \
	"but it is not supported by the platform."
	#endif

.

I would rather go in the other direction, by replacing

zephyr/drivers/gpio/gpio_nrfx.c

Lines 433 to 436 in 169144a

	#if IS_ENABLED(CONFIG_GPIO_NRF_INT_EDGE_USING_SENSE)
	/* Read LATCH, which will tell us which pin has changed its state. */
	port_latch = cfg->port->LATCH;
	#endif

with

static inline uint32_t get_latch(NRF_GPIO_Type *port)
{
#if defined(NRF_GPIO_LATCH_PRESENT)
    return cfg->port->LATCH;
#else
    return 0;
#endif
}

and

if (IS_ENABLED(CONFIG_GPIO_NRF_INT_EDGE_USING_SENSE)) {
	/* Read LATCH, which will tell us which pin has changed its state. */
	port_latch = get_latch(cfg->port);
}

However I am not sure how Coverity would treat that.

[dleach02]

I get it. I'm not a big fan of conditional compiled code either.

[dleach02]

Coverity might still be smart enough to figure out that in the particular test run, 0 will always be returned... I think you may still end up needed to conditional the code coverity is highlighting... And that might be the clearest, to the reader, way of addressing this if we want to deal with the dead code notice. Or we can mark it as a false positive and move on.

[anangl]

Marked as false positive.