Re: [PATCH v2 3/4] gpio: davinci: Add support for level-triggered interrupts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] irq: unbanked level interrupts fail to update flow handler and overwrite parent irq_set_type
- [High] gpio: davinci: missing irq_ack causes spurious level interrupts
- [High] gpio: davinci: out-of-bounds array access due to unvalidated ti,ngpio
- [Medium] gpio: TOCTOU race and massive overhead due to gpiochip_[dis|en]able_irq in mask/unmask callbacks
- [Medium] gpio: davinci: W1S registers not cleared on resume
--

commit f6da5fa1d3ffb73b515603cd9539eceb04b2addc
Author: Aniket Limaye <a-limaye@ti.com>
Subject: gpio: davinci: Add support for level-triggered interrupts

This patch adds support for IRQ_TYPE_LEVEL_HIGH and IRQ_TYPE_LEVEL_LOW triggers to the DaVinci GPIO driver by utilizing extended registers. It clears level enables on setup and only sets them when a level trigger is explicitly requested to preserve the default both-edges trigger.

Link: https://lore.kernel.org/r/20260923-upstream-tda54-gpio-v1-2-d1a0a199c836@ti.com

[Severity: High]
In drivers/gpio/gpio-davinci.c, around line 623 in davinci_gpio_irq_setup, the code overwrites the parent controller's irq_set_type callback with gpio_irq_type_unbanked.

Then, around line 472 in gpio_irq_type_unbanked, it configures the new level registers but returns 0 without calling irq_set_handler_locked to set handle_level_irq, and without chaining to the parent's original irq_set_type callback.

Because unbanked GPIO interrupts bypass the local IRQ domain and use the parent controller directly, won't this leave the parent controller unaware of the level trigger configuration? Does this mean the flow handler remains handle_edge_irq, meaning we miss the required mask/ack sequence and risk an immediate interrupt storm when the line is held active?

[Severity: Medium]
In drivers/gpio/gpio-davinci.c, handle_level_irq is used for the new level-triggered interrupts, which means the irq_mask and irq_unmask callbacks are invoked on every single interrupt.

Around line 302 in gpio_irq_mask and line 315 in gpio_irq_unmask, the driver calls gpiochip_disable_irq and gpiochip_enable_irq. Since these gpiolib functions take spinlocks, won't this add massive overhead on every level interrupt?

Furthermore, clearing the GPIOD_FLAG_IRQ_IS_ENABLED flag during the interrupt allows a concurrent userspace thread to successfully change the GPIO direction to output. When the interrupt finishes and gpiochip_enable_irq is called, could this trigger a WARN_ON because the pin is now an output? Should these gpiolib calls be moved to irq_enable and irq_disable instead?

[Severity: High]
In drivers/gpio/gpio-davinci.c around line 364, the newly added gpio_irqchip_ext irq_chip lacks an irq_ack or irq_mask_ack callback.

In the chained handler gpio_irq_handler around line 404, it reads and clears intstat before calling generic_handle_domain_irq. For level-triggered interrupts, the hardware will immediately re-latch the intstat bit since the external line is still active.

When handle_level_irq masks the interrupt, it cannot clear this re-latched intstat due to the missing ack callback. Even after the device ISR runs and de-asserts the line, intstat remains latched. Could this cause the while loop in gpio_irq_handler to see a stale intstat bit and fire a spurious interrupt upon return, eventually causing the spurious interrupt detector to permanently disable the IRQ line?

[Severity: Medium]
In drivers/gpio/gpio-davinci.c around line 757, davinci_gpio_restore_context attempts to restore the extension registers set_high and set_low upon system resume.

Since these appear to be Write-1-to-Set registers in hardware, writing a 0 to a bit position has no effect. If firmware enabled a bit during suspend and it is 0 in the saved context, writing the 0 fails to clear it.

Could this leave level-triggered interrupts improperly enabled after resume? Would it be better to write the negated context to the corresponding clr_high and clr_low registers to properly clear bits?

[Severity: High]
In drivers/gpio/gpio-davinci.c around line 262 in davinci_gpio_probe, the initialization loop iterates nbank times to initialize chips->regs_ext and reads from offset_array_ext.

If a malformed Device Tree provides a ti,ngpio value greater than 160, nbank will exceed MAX_REGS_BANKS which is 5.

Could this cause an out-of-bounds read past the static offset_array_ext and a heap buffer overflow when writing past the end of the chips->regs_ext array? Since the only sanity check on nirq uses gpio_unbanked rather than ngpio, should we validate ngpio against MAX_REGS_BANKS before iterating?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-upstream-tda54-gpio-v2-0-5c4f8b10cdbc@ti.com?part=3