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

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

[Junjie Cao]

ioh_irq_type() is registered as the irq_chip .irq_set_type callback and
takes chip->spinlock with spin_lock_irqsave().  This callback is reached
from __setup_irq() -> __irq_set_trigger() -> chip->irq_set_type() while
the caller holds desc->lock, a raw_spinlock_t, with hardirqs disabled.
That context is not sleepable, but on PREEMPT_RT a regular spinlock_t is
an rtmutex-backed sleeping lock, so acquiring it there is invalid.
ioh_irq_enable() and ioh_irq_disable() take the same lock from the
.irq_enable/.irq_disable callbacks, which are likewise invoked with
desc->lock held.

Convert the register lock to raw_spinlock_t.  The same lock also
serializes the GPIO direction/value callbacks and the suspend/resume
register save/restore, and those critical sections only perform short
sequences of MMIO register accesses (ioread32()/iowrite32()); the
.irq_set_type callback additionally emits a dev_warn() on an unsupported
type.  None of these are sleepable operations, so keeping this register
lock non-sleeping is appropriate for the irqchip callbacks and does not
change the GPIO-side locking contract.

This is the same fix as commit a02b8950d619 ("gpio: pch: use
raw_spinlock_t for the register lock"); this driver shares the same
structure as gpio-pch.

Fixes: 54be566317b6 ("gpio-ml-ioh: Support interrupt function")
Cc: stable@vger.kernel.org
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 drivers/gpio/gpio-ml-ioh.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpio-ml-ioh.c b/drivers/gpio/gpio-ml-ioh.c
index 6576e5dcb0ee..0a9d34b7636e 100644
--- a/drivers/gpio/gpio-ml-ioh.c
+++ b/drivers/gpio/gpio-ml-ioh.c
@@ -84,7 +84,7 @@ struct ioh_gpio {
 	u32 gpio_use_sel;
 	int ch;
 	int irq_base;
-	spinlock_t spinlock;
+	raw_spinlock_t spinlock;
 };
 
 static const int num_ports[] = {6, 12, 16, 16, 15, 16, 16, 12};
@@ -95,7 +95,7 @@ static int ioh_gpio_set(struct gpio_chip *gpio, unsigned int nr, int val)
 	struct ioh_gpio *chip =	gpiochip_get_data(gpio);
 	unsigned long flags;
 
-	spin_lock_irqsave(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&chip->spinlock, flags);
 	reg_val = ioread32(&chip->reg->regs[chip->ch].po);
 	if (val)
 		reg_val |= BIT(nr);
@@ -103,7 +103,7 @@ static int ioh_gpio_set(struct gpio_chip *gpio, unsigned int nr, int val)
 		reg_val &= ~BIT(nr);
 
 	iowrite32(reg_val, &chip->reg->regs[chip->ch].po);
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 
 	return 0;
 }
@@ -123,7 +123,7 @@ static int ioh_gpio_direction_output(struct gpio_chip *gpio, unsigned nr,
 	u32 reg_val;
 	unsigned long flags;
 
-	spin_lock_irqsave(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&chip->spinlock, flags);
 	pm = ioread32(&chip->reg->regs[chip->ch].pm);
 	pm &= BIT(num_ports[chip->ch]) - 1;
 	pm |= BIT(nr);
@@ -136,7 +136,7 @@ static int ioh_gpio_direction_output(struct gpio_chip *gpio, unsigned nr,
 		reg_val &= ~BIT(nr);
 	iowrite32(reg_val, &chip->reg->regs[chip->ch].po);
 
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 
 	return 0;
 }
@@ -147,12 +147,12 @@ static int ioh_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
 	u32 pm;
 	unsigned long flags;
 
-	spin_lock_irqsave(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&chip->spinlock, flags);
 	pm = ioread32(&chip->reg->regs[chip->ch].pm);
 	pm &= BIT(num_ports[chip->ch]) - 1;
 	pm &= ~BIT(nr);
 	iowrite32(pm, &chip->reg->regs[chip->ch].pm);
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 
 	return 0;
 }
@@ -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:
@@ -296,7 +296,7 @@ static int ioh_irq_type(struct irq_data *d, unsigned int type)
 	ien = ioread32(&chip->reg->regs[chip->ch].ien);
 	iowrite32(ien | BIT(ch), &chip->reg->regs[chip->ch].ien);
 end:
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 
 	return 0;
 }
@@ -326,11 +326,11 @@ static void ioh_irq_disable(struct irq_data *d)
 	unsigned long flags;
 	u32 ien;
 
-	spin_lock_irqsave(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&chip->spinlock, flags);
 	ien = ioread32(&chip->reg->regs[chip->ch].ien);
 	ien &= ~BIT(d->irq - chip->irq_base);
 	iowrite32(ien, &chip->reg->regs[chip->ch].ien);
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 }
 
 static void ioh_irq_enable(struct irq_data *d)
@@ -340,11 +340,11 @@ static void ioh_irq_enable(struct irq_data *d)
 	unsigned long flags;
 	u32 ien;
 
-	spin_lock_irqsave(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&chip->spinlock, flags);
 	ien = ioread32(&chip->reg->regs[chip->ch].ien);
 	ien |= BIT(d->irq - chip->irq_base);
 	iowrite32(ien, &chip->reg->regs[chip->ch].ien);
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 }
 
 static irqreturn_t ioh_gpio_handler(int irq, void *dev_id)
@@ -440,7 +440,7 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
 		chip->base = base;
 		chip->reg = chip->base;
 		chip->ch = i;
-		spin_lock_init(&chip->spinlock);
+		raw_spin_lock_init(&chip->spinlock);
 		ioh_gpio_setup(chip, num_ports[i]);
 		ret = devm_gpiochip_add_data(dev, &chip->gpio, chip);
 		if (ret) {
@@ -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);
-	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);
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 
 	return 0;
 }

base-commit: a02b8950d619123da64f69b70fe1dadef217dfe4
-- 
2.43.0

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

[sashiko-bot]

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