Re: [PATCH] gpio: ml-ioh: use raw_spinlock_t for the register lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Suspend and resume functions bypass spinlocks for channels 1-7, causing data races on hardware registers.
- [High] The driver uses `handle_simple_irq` for all interrupts, which causes an interrupt storm for level-triggered threaded interrupts.
--

--- Patch [1]: [PATCH] gpio: ml-ioh: use raw_spinlock_t for the register lock ---
commit c58bab978628257e8b70587775813cefa999a4d6
Author: Junjie Cao <junjie.cao@intel.com>

gpio: ml-ioh: use raw_spinlock_t for the register lock

This patch converts the driver's register lock from spinlock_t to
raw_spinlock_t. This fixes an issue on PREEMPT_RT where irqchip callbacks
invoked with hardirqs disabled would attempt to acquire a sleepable lock.

> diff --git a/drivers/gpio/gpio-ml-ioh.c b/drivers/gpio/gpio-ml-ioh.c
> index 6576e5dcb0eef..0a9d34b7636ef 100644
> --- a/drivers/gpio/gpio-ml-ioh.c
> +++ b/drivers/gpio/gpio-ml-ioh.c

[ ... ]

> @@ -256,7 +256,7 @@ static int ioh_irq_type(struct irq_data *d, unsigned int type)
>  	dev_dbg(chip->dev, "%s:irq=%d type=%d ch=%d pos=%d type=%d\n",
>  		__func__, irq, type, ch, im_pos, type);
>  
> -	spin_lock_irqsave(&chip->spinlock, flags);
> +	raw_spin_lock_irqsave(&chip->spinlock, flags);
>  
>  	switch (type) {
>  	case IRQ_TYPE_EDGE_RISING:

[Severity: High]
This is a pre-existing issue, but does this driver fail to assign the
correct flow handler for level-triggered interrupts?

During initialization, ioh_gpio_alloc_generic_chip() in
drivers/gpio/gpio-ml-ioh.c assigns handle_simple_irq to all interrupts:

gc = devm_irq_alloc_generic_chip(chip->dev, "ioh_gpio", 1, irq_start,
                                 chip->base, handle_simple_irq);

When a client driver requests a level-triggered threaded interrupt,
shouldn't ioh_irq_type() call irq_set_handler_locked() to switch the
flow handler to handle_level_irq?

Because handle_simple_irq does not mask the interrupt before waking the
thread, returning from the primary handler will immediately re-assert the
interrupt if the external hardware hasn't been serviced yet, leading to a
hardirq storm.

[ ... ]

> @@ -484,9 +484,9 @@ static int ioh_gpio_suspend(struct device *dev)
>  	struct ioh_gpio *chip = dev_get_drvdata(dev);
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&chip->spinlock, flags);
> +	raw_spin_lock_irqsave(&chip->spinlock, flags);
>  	ioh_gpio_save_reg_conf(chip);

[Severity: High]
This isn't a bug introduced by this patch, but does this lock only protect
the first of the eight GPIO chips?

In ioh_gpio_suspend(), we retrieve the base pointer to an array of 8 chips
and acquire the lock for chip[0]. However, ioh_gpio_save_reg_conf()
iterates through and saves registers for all 8 chips without acquiring
their respective locks:

static void ioh_gpio_save_reg_conf(struct ioh_gpio *chip)
{
    int i;
    for (i = 0; i < 8; i++, chip++) {
        chip->ioh_gpio_reg.po_reg =
                    ioread32(&chip->reg->regs[chip->ch].po);
        ...

If a concurrent operation accesses channels 1-7 during this time, it
will acquire the lock for that specific channel (like chip[1].spinlock),
which isn't held by the suspend path, potentially causing a data race.

> -	spin_unlock_irqrestore(&chip->spinlock, flags);
> +	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
>  
>  	return 0;
>  }
> @@ -496,11 +496,11 @@ static int ioh_gpio_resume(struct device *dev)
>  	struct ioh_gpio *chip = dev_get_drvdata(dev);
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&chip->spinlock, flags);
> +	raw_spin_lock_irqsave(&chip->spinlock, flags);
>  	iowrite32(0x01, &chip->reg->srst);
>  	iowrite32(0x00, &chip->reg->srst);
>  	ioh_gpio_restore_reg_conf(chip);

[Severity: High]
This is a pre-existing issue, but does the same data race occur here
during resume?

ioh_gpio_restore_reg_conf() also loops over all 8 chips and blindly
restores their registers, protected solely by chip[0]'s spinlock. Could
this overwrite concurrent changes to channels 1-7?

> -	spin_unlock_irqrestore(&chip->spinlock, flags);
> +	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731032747.2987292-1-junjie.cao@intel.com?part=1