Re: [PATCH 2/2] gpio: eic-sprd: use raw_spinlock_t in the irq startup path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Potential NULL pointer dereference in `sprd_eic_toggle_trigger` for unmapped or spurious hardware interrupts.
- [High] `sprd_eic_toggle_trigger` fails to properly toggle polarity for `SPRD_EIC_LATCH` because `sprd_eic_get` returns `-ENOTSUPP`.
- [High] Potential hardirq livelock and console spam due to `dev_warn` inside a retry loop in `sprd_eic_toggle_trigger`.
- [High] Unbinding one EIC instance breaks all other EIC instances because they share the same parent IRQ chained handler.
- [High] sprd_eic_toggle_trigger modifies interrupt masks unconditionally without holding desc->lock, creating a race condition with disable_irq().
--

commit 69e3dda6e9ba1a662342eec6f6b0ec04beeadc64
Author: Runyu Xiao <runyu.xiao@xxxxxxxxxx>

gpio: eic-sprd: use raw_spinlock_t in the irq startup path

sprd_eic_irq_unmask() enables the GPIO IRQ and then updates controller
state through sprd_eic_update(), which takes sprd_eic->lock with
spin_lock_irqsave(). The callback can be reached from irq_startup()
while setting up a requested IRQ, which is not sleepable on PREEMPT_RT.
This patch converts the lock to raw_spinlock_t to avoid sleeping.

> diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
> index 50fafeda8d7e2..3b7ebcf12fe7f 100644
> --- a/drivers/gpio/gpio-eic-sprd.c
> +++ b/drivers/gpio/gpio-eic-sprd.c
[ ... ]
> @@ -149,7 +149,7 @@ static void sprd_eic_update(struct gpio_chip *chip, unsigned int offset,
>  	unsigned long flags;
>  	u32 tmp;
>  
> -	spin_lock_irqsave(&sprd_eic->lock, flags);
> +	raw_spin_lock_irqsave(&sprd_eic->lock, flags);
>  	tmp = readl_relaxed(base + reg);
>  
>  	if (val)
> @@ -158,7 +158,7 @@ static void sprd_eic_update(struct gpio_chip *chip, unsigned int offset,
>  		tmp &= ~BIT(SPRD_EIC_BIT(offset));
>  
>  	writel_relaxed(tmp, base + reg);
> -	spin_unlock_irqrestore(&sprd_eic->lock, flags);
> +	raw_spin_unlock_irqrestore(&sprd_eic->lock, flags);
>  }

[Severity: Critical]
This is a pre-existing issue, but can sprd_eic_handle_one_type() cause a
NULL pointer dereference for unmapped or spurious hardware interrupts?

If an interrupt is unmapped, irq_find_mapping() returns 0:

drivers/gpio/gpio-eic-sprd.c:sprd_eic_handle_one_type() {
    ...
    girq = irq_find_mapping(chip->irq.domain, offset);
    generic_handle_irq(girq);
    sprd_eic_toggle_trigger(chip, girq, offset);
}

The girq value 0 is then passed into sprd_eic_toggle_trigger():

drivers/gpio/gpio-eic-sprd.c:sprd_eic_toggle_trigger() {
    ...
    struct irq_data *data = irq_get_irq_data(irq);
    u32 trigger = irqd_get_trigger_type(data);
    ...
}

Calling irq_get_irq_data(0) returns NULL, which causes irqd_get_trigger_type()
to dereference NULL. Can this path be triggered by the bootloader leaving an
interrupt unmasked?

[Severity: High]
This is a pre-existing issue, but does sprd_eic_toggle_trigger() race with
disable_irq() since it alters the hardware mask without holding the
descriptor lock?

Looking at sprd_eic_toggle_trigger(), it unconditionally calls
sprd_eic_irq_unmask() after returning from an interrupt handler:

drivers/gpio/gpio-eic-sprd.c:sprd_eic_toggle_trigger() {
    ...
    sprd_eic_irq_mask(data);
    ...
    sprd_eic_irq_unmask(data);
}

If a concurrent thread calls disable_irq(), which holds desc->lock and masks
the hardware, sprd_eic_toggle_trigger() could execute immediately afterward and
unmask it again without checking irqd_irq_disabled(data). Can this leave the
system vulnerable to unhandled interrupt storms?

[Severity: High]
This is a pre-existing issue, but does sprd_eic_toggle_trigger() have a
potential hardirq livelock and console spam issue?

If the line state changes, the code logs a warning and retries endlessly:

drivers/gpio/gpio-eic-sprd.c:sprd_eic_toggle_trigger() {
    ...
retry:
    ...
    post_state = sprd_eic_get(chip, offset);
    if (state != post_state) {
        dev_warn(chip->parent, "EIC level was changed.\n");
        state = post_state;
        goto retry;
    }
}

Since this runs in the context of a cascaded interrupt handler, if the input
line oscillates rapidly, will the dev_warn() overhead inside a tight loop
cause lockups and watchdog panics without any retry bounds?

[Severity: High]
This is a pre-existing issue, but does SPRD_EIC_LATCH fail to toggle polarity?

In sprd_eic_toggle_trigger(), the state is used to set the latch polarity:

drivers/gpio/gpio-eic-sprd.c:sprd_eic_toggle_trigger() {
    ...
    state = sprd_eic_get(chip, offset);
    ...
    case SPRD_EIC_LATCH:
        if (state)
            sprd_eic_update(chip, offset, SPRD_EIC_LATCH_INTPOL, 0);
        else
            sprd_eic_update(chip, offset, SPRD_EIC_LATCH_INTPOL, 1);
        break;
}

However, sprd_eic_get() doesn't implement reading for SPRD_EIC_LATCH
and returns -ENOTSUPP. This negative value evaluates as true, meaning the
code will always execute the true branch and never toggle to 1. Will this
cause LATCH EICs to become permanently stuck?

[ ... ]
> @@ -628,7 +628,7 @@ static int sprd_eic_probe(struct platform_device *pdev)
>  	if (!sprd_eic)
>  		return -ENOMEM;
>  
> -	spin_lock_init(&sprd_eic->lock);
> +	raw_spin_lock_init(&sprd_eic->lock);
>  	sprd_eic->type = pdata->type;
>  
>  	sprd_eic->irq = platform_get_irq(pdev, 0);

[Severity: High]
This is a pre-existing issue, but will unbinding one EIC instance break all
other EIC instances?

During probe, each instance sets its parent handler to sprd_eic_irq_handler
on the shared parent IRQ:

drivers/gpio/gpio-eic-sprd.c:sprd_eic_probe() {
    ...
    irq->parent_handler = sprd_eic_irq_handler;
    irq->parent_handler_data = sprd_eic;
    irq->num_parents = 1;
    irq->parents = &sprd_eic->irq;
    ...
}

If any single EIC instance is unbound, gpiolib's cleanup routine calls
irq_set_chained_handler_and_data(parent, NULL, NULL), clearing the shared
parent handler entirely. Will this leave the remaining EIC instances
completely non-functional?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617154035.1199948-1-runyu.xiao@xxxxxxxxxx?part=2