Re: [PATCH 1/2] gpio: sch: use raw_spinlock_t in the irq startup path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Read-Modify-Write cycle on the Write-1-to-Clear (W1C) status register causes lost interrupts.
--

commit 088444fdf19ac3925d66a59ae9e74b003c72c066
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

gpio: sch: use raw_spinlock_t in the irq startup path

This patch converts the SCH controller lock from spinlock_t to
raw_spinlock_t to prevent a sleep-in-invalid-context bug on PREEMPT_RT.
The lock is acquired in the non-sleepable IRQ startup path.

> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index 966d16a6d5157..5e361742a11ad 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c

[ ... ]

> @@ -215,9 +215,9 @@ static void sch_irq_ack(struct irq_data *d)
>  	irq_hw_number_t gpio_num = irqd_to_hwirq(d);
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&sch->lock, flags);
> +	raw_spin_lock_irqsave(&sch->lock, flags);
>  	sch_gpio_reg_set(sch, gpio_num, GTS, 1);

[Severity: High]
This isn't a bug introduced by this patch, but does this cause lost
interrupts due to a read-modify-write on a write-1-to-clear register?

sch_irq_ack() acknowledges a GPIO interrupt by calling
sch_gpio_reg_set() for the GTS register. 

Looking at sch_gpio_reg_set():

drivers/gpio/gpio-sch.c:sch_gpio_reg_set() {
    ...
    reg_val = ioread8(sch->regs + offset);

    if (val)
        reg_val |= BIT(bit);
    else
        reg_val &= ~BIT(bit);

    iowrite8(reg_val, sch->regs + offset);
}

And sch_gpio_gpe_handler() demonstrates that GTS returns the status of
pending interrupts when read:

drivers/gpio/gpio-sch.c:sch_gpio_gpe_handler() {
    ...
    core_status = ioread32(sch->regs + CORE_BANK_OFFSET + GTS);
    ...
}

Because GTS is a write-1-to-clear status register, the ioread8() in
sch_gpio_reg_set() returns 1s for all currently pending interrupts.

When those 1s are written back via iowrite8(), won't it inadvertently
clear all pending interrupts in that bank, instead of just the specific
gpio_num being acknowledged?

> -	spin_unlock_irqrestore(&sch->lock, flags);
> +	raw_spin_unlock_irqrestore(&sch->lock, flags);
>  }

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