Re: [PATCH 2/2] gpio: davinci: Add level support via extended registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: davinci: interrupt storm due to incorrect default level triggers
- [High] gpio: davinci: infinite loop in chained handler for level IRQs
--

commit 31955d22d1eb969a680fcf19a5faaeb164ffab12
Author: Aniket Limaye <a-limaye@ti.com>

gpio: davinci: Add level support via extended registers

This commit adds support for level-triggered interrupts for the TDA54 GPIO
controller. It utilizes the extended register sets to allow handling of
IRQ_TYPE_LEVEL_HIGH and IRQ_TYPE_LEVEL_LOW triggers.

> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index cccbaea1dec24..e5a3abe6b9d3f 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
[ ... ]
> @@ -293,6 +317,18 @@ static void gpio_irq_unmask(struct irq_data *d)
[ ... ]
> +static int gpio_irq_type_ext(struct irq_data *d, unsigned int trigger)
> +{
> +	if (trigger & ~(IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_MASK))
> +		return -EINVAL;
> +
> +	return 0;
> +}

[Severity: High]
Does this code need to reconfigure the flow handler to handle_level_irq
when a level trigger is requested?

If a level trigger is requested, we need to assign the proper flow handler
using irq_set_handler_locked(). Since this doesn't happen, handle_simple_irq
will continue to be used, which does not mask the interrupt line while the
action handler runs.

If the external line remains asserted during a level-triggered interrupt,
could the chained handler (gpio_irq_handler) continuously detect the active
status in the intstat register?

Since the intstat bit immediately re-latches after being cleared, wouldn't
the while (1) loop inside gpio_irq_handler spin infinitely and cause a CPU
lockup?

[ ... ]
> @@ -418,8 +474,12 @@ davinci_gpio_irq_map(struct irq_domain *d, unsigned int irq,
>  	struct davinci_gpio_controller *chips =
>  				(struct davinci_gpio_controller *)d->host_data;
>  
> -	irq_set_chip_and_handler_name(irq, &gpio_irqchip, handle_simple_irq,
> -				"davinci_gpio");
> +	if (chips->regs_ext[0])
> +		irq_set_chip_and_handler_name(irq, &gpio_irqchip_ext, handle_simple_irq,
> +					      "davinci_gpio");

[Severity: High]
Is it safe to hardcode handle_simple_irq as the default handler for all banked
GPIO interrupts here, when level interrupts are supported?

As mentioned above in gpio_irq_type_ext(), using handle_simple_irq for level
interrupts can lead to an infinite loop in the chained handler because it
does not mask the line.

[ ... ]
> @@ -534,6 +595,11 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
[ ... ]
>  		/* default trigger: both edges */
>  		g = chips->regs[0];
>  		writel_relaxed(~0, &g->set_falling);
>  		writel_relaxed(~0, &g->set_rising);
> +		g_ext = chips->regs_ext[0];
> +		if (g_ext) {
> +			writel_relaxed(~0, &g_ext->set_high);
> +			writel_relaxed(~0, &g_ext->set_low);
> +		}

[Severity: High]
Will this unconditionally enable level interrupts for all unbanked GPIOs
during controller setup?

The code writes ~0 to both set_high and set_low registers, which enables
level-triggered interrupts for both high and low states on all unbanked pins
simultaneously.

Since a digital line must be either high or low, wouldn't this guarantee
the interrupt condition is continuously met? If an unbanked GPIO interrupt
is then unmasked without explicitly overriding the trigger type (for example,
relying on the default 'both edges' state), wouldn't this trigger an
unrecoverable interrupt storm?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-upstream-tda54-gpio-v1-0-d1a0a199c836@ti.com?part=2