[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

[Linus Walleij]

On Fri, Jul 31, 2026 at 5:30 AM Junjie Cao <junjie.cao@intel.com> wrote:

> 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>

Reviewed-by: Linus Walleij <linusw@kernel.org>

If you have time, look into the issue reported by sashiko about suspend/resume
ignoring the spinlock.

Thanks!
Linus Walleij

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

[Junjie Cao]

Hi Linus,

Thanks for the review.

I checked it, and the report is correct. The PM callbacks take
chip[0].spinlock while the save and restore helpers walk all eight
channels, each with its own lock.

I have a follow-up that uses one lock shared by all channels. I'll send
it separately on top of this patch.

Thanks,
Junjie

[PATCH] gpio: ml-ioh: share the register lock across channels

[Junjie Cao]

Suspend and resume hold channel 0's lock while saving and restoring
registers for all eight channels. Code paths using the other seven locks
can therefore run concurrently with PM.

Use one controller-wide lock shared by all channels.

Fixes: b490fa0bf86e ("gpio-ml-ioh: Fix suspend/resume issue")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/r/20260731033956.EE6F61F000E9@smtp.kernel.org
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 drivers/gpio/gpio-ml-ioh.c | 69 ++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 32 deletions(-)

diff --git a/drivers/gpio/gpio-ml-ioh.c b/drivers/gpio/gpio-ml-ioh.c
index 0a9d34b7636e..391188c0f2c2 100644
--- a/drivers/gpio/gpio-ml-ioh.c
+++ b/drivers/gpio/gpio-ml-ioh.c
@@ -73,7 +73,7 @@ struct ioh_gpio_reg_data {
  * @gpio_use_sel:		Save GPIO_USE_SEL1~4 register for PM
  * @ch:				Indicate GPIO channel
  * @irq_base:		Save base of IRQ number for interrupt
- * @spinlock:		Used for register access protection
+ * @spinlock:		Shared register access lock
  */
 struct ioh_gpio {
 	void __iomem *base;
@@ -84,7 +84,12 @@ struct ioh_gpio {
 	u32 gpio_use_sel;
 	int ch;
 	int irq_base;
+	raw_spinlock_t *spinlock;
+};
+
+struct ioh_gpio_device {
 	raw_spinlock_t spinlock;
+	struct ioh_gpio chip[8];
 };
 
 static const int num_ports[] = {6, 12, 16, 16, 15, 16, 16, 12};
@@ -95,7 +100,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;
 
-	raw_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 +108,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);
-	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(chip->spinlock, flags);
 
 	return 0;
 }
@@ -123,7 +128,7 @@ static int ioh_gpio_direction_output(struct gpio_chip *gpio, unsigned nr,
 	u32 reg_val;
 	unsigned long flags;
 
-	raw_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 +141,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);
 
-	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(chip->spinlock, flags);
 
 	return 0;
 }
@@ -147,12 +152,12 @@ static int ioh_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
 	u32 pm;
 	unsigned long flags;
 
-	raw_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);
-	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(chip->spinlock, flags);
 
 	return 0;
 }
@@ -256,7 +261,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);
 
-	raw_spin_lock_irqsave(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(chip->spinlock, flags);
 
 	switch (type) {
 	case IRQ_TYPE_EDGE_RISING:
@@ -296,7 +301,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:
-	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(chip->spinlock, flags);
 
 	return 0;
 }
@@ -326,11 +331,11 @@ static void ioh_irq_disable(struct irq_data *d)
 	unsigned long flags;
 	u32 ien;
 
-	raw_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);
-	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(chip->spinlock, flags);
 }
 
 static void ioh_irq_enable(struct irq_data *d)
@@ -340,11 +345,11 @@ static void ioh_irq_enable(struct irq_data *d)
 	unsigned long flags;
 	u32 ien;
 
-	raw_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);
-	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(chip->spinlock, flags);
 }
 
 static irqreturn_t ioh_gpio_handler(int irq, void *dev_id)
@@ -407,8 +412,8 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
 	int ret;
 	int i, j;
 	struct ioh_gpio *chip;
+	struct ioh_gpio_device *priv;
 	void __iomem *base;
-	void *chip_save;
 	int irq_base;
 
 	ret = pcim_enable_device(pdev);
@@ -429,18 +434,18 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
 		return -ENOMEM;
 	}
 
-	chip_save = devm_kcalloc(dev, 8, sizeof(*chip), GFP_KERNEL);
-	if (chip_save == NULL) {
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
 		return -ENOMEM;
-	}
 
-	chip = chip_save;
+	raw_spin_lock_init(&priv->spinlock);
+	chip = priv->chip;
 	for (i = 0; i < 8; i++, chip++) {
 		chip->dev = dev;
 		chip->base = base;
 		chip->reg = chip->base;
 		chip->ch = i;
-		raw_spin_lock_init(&chip->spinlock);
+		chip->spinlock = &priv->spinlock;
 		ioh_gpio_setup(chip, num_ports[i]);
 		ret = devm_gpiochip_add_data(dev, &chip->gpio, chip);
 		if (ret) {
@@ -449,7 +454,7 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
 		}
 	}
 
-	chip = chip_save;
+	chip = priv->chip;
 	for (j = 0; j < 8; j++, chip++) {
 		irq_base = devm_irq_alloc_descs(dev, -1, IOH_IRQ_BASE,
 						num_ports[j], NUMA_NO_NODE);
@@ -466,7 +471,7 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
 			return ret;
 	}
 
-	chip = chip_save;
+	chip = priv->chip;
 	ret = devm_request_irq(dev, pdev->irq, ioh_gpio_handler,
 			       IRQF_SHARED, KBUILD_MODNAME, chip);
 	if (ret != 0) {
@@ -474,33 +479,33 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
 		return ret;
 	}
 
-	pci_set_drvdata(pdev, chip);
+	pci_set_drvdata(pdev, priv);
 
 	return 0;
 }
 
 static int ioh_gpio_suspend(struct device *dev)
 {
-	struct ioh_gpio *chip = dev_get_drvdata(dev);
+	struct ioh_gpio_device *priv = dev_get_drvdata(dev);
 	unsigned long flags;
 
-	raw_spin_lock_irqsave(&chip->spinlock, flags);
-	ioh_gpio_save_reg_conf(chip);
-	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&priv->spinlock, flags);
+	ioh_gpio_save_reg_conf(priv->chip);
+	raw_spin_unlock_irqrestore(&priv->spinlock, flags);
 
 	return 0;
 }
 
 static int ioh_gpio_resume(struct device *dev)
 {
-	struct ioh_gpio *chip = dev_get_drvdata(dev);
+	struct ioh_gpio_device *priv = dev_get_drvdata(dev);
 	unsigned long flags;
 
-	raw_spin_lock_irqsave(&chip->spinlock, flags);
-	iowrite32(0x01, &chip->reg->srst);
-	iowrite32(0x00, &chip->reg->srst);
-	ioh_gpio_restore_reg_conf(chip);
-	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&priv->spinlock, flags);
+	iowrite32(0x01, &priv->chip->reg->srst);
+	iowrite32(0x00, &priv->chip->reg->srst);
+	ioh_gpio_restore_reg_conf(priv->chip);
+	raw_spin_unlock_irqrestore(&priv->spinlock, flags);
 
 	return 0;
 }

base-commit: a02b8950d619123da64f69b70fe1dadef217dfe4
prerequisite-patch-id: cebc220b508b7d9216be1fe01348b9989f0b957e
-- 
2.43.0

Re: [PATCH] gpio: ml-ioh: share the register lock across channels

[Linus Walleij]

On Tue, Aug 4, 2026 at 12:00 PM Junjie Cao <junjie.cao@intel.com> wrote:

> Suspend and resume hold channel 0's lock while saving and restoring
> registers for all eight channels. Code paths using the other seven locks
> can therefore run concurrently with PM.
>
> Use one controller-wide lock shared by all channels.
>
> Fixes: b490fa0bf86e ("gpio-ml-ioh: Fix suspend/resume issue")
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/r/20260731033956.EE6F61F000E9@smtp.kernel.org
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>

Reviewed-by: Linus Walleij <linusw@kernel.org>

Yours,
Linus Walleij