Re: [PATCH v2] gpio: amdptpci: add support for AMD PT PCI controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] pm: irq handler accesses hardware registers without runtime PM check
- [High] pm: missing synchronize_irq() in runtime suspend callback
- [High] gpio: system suspend erroneously unmasks all interrupts due to incorrect mask calculation
- [High] gpio: PREEMPT_RT lock nesting violation in pt27_irq_mask()
- [Medium] gpio: level-triggered interrupts permanently masked due to PM state drop
- [Medium] gpio: TOCTOU race in PM reference tracking between mask and unmask
- [Medium] gpio: teardown ordering violation due to mixed devm and manual cleanup
- [Medium] pci: runtime PM state leak in probe error path
--

commit 0c4bfe0b69469a3b6087533b773e271439cdda28
Author: Szuying Chen <Chloe_Chen@asmedia.com.tw>

gpio: amdptpci: add support for AMD PT PCI controller

This commit implements the new gpio-amdptpci driver to support the
GPIO functionality on AMD PT PCI platforms.

> diff --git a/drivers/gpio/gpio-amdptpci.c b/drivers/gpio/gpio-amdptpci.c
> new file mode 100644
> index 0000000000000..ac2a7b46c3cde
> --- /dev/null
> +++ b/drivers/gpio/gpio-amdptpci.c
> @@ -0,0 +1,891 @@

[ ... ]

> +static void pt27_irq_mask(struct irq_data *d)
> +{
> +	struct gpio_chip *gc   = irq_data_get_irq_chip_data(d);
> +	struct pt27_gpio *chip = gpiochip_get_data(gc);
> +	irq_hw_number_t   hwirq = irqd_to_hwirq(d);
> +	unsigned long flags;
> +	u32 val;
> +	bool drop_pm = false;
> +
> +	gpiochip_disable_irq(gc, hwirq);
> +
> +	if (!pt27_irq_regs_accessible(chip)) {
> +		dev_err_ratelimited(chip->dev,
> +				    "%s: GPIO%lu remains masked; PT27 is not in D0\n",
> +				     __func__, hwirq);
> +		return;
> +	}
> +
> +	raw_spin_lock_irqsave(&chip->lock, flags);
> +
> +	val  = pt27_rd32(chip, PT27_REG_INT_MASK);
> +	val |= BIT(hwirq);
> +	pt27_wr32(chip, PT27_REG_INT_MASK, val);
> +
> +	val  = pt27_rd32(chip, PT27_REG_INT_EN);
> +	val &= ~BIT(hwirq);
> +	pt27_wr32(chip, PT27_REG_INT_EN, val);
> +
> +	chip->enabled_irq_mask &= ~BIT(hwirq);
> +	if (!chip->enabled_irq_mask && chip->irq_pm_held) {
> +		chip->irq_pm_held = false;
> +		drop_pm = true;
> +	}
> +
> +	raw_spin_unlock_irqrestore(&chip->lock, flags);

[Severity: Medium]
Will this logic permanently mask level-triggered interrupts?

handle_level_irq() temporarily masks the interrupt by calling .irq_mask() and
unmasks it afterward with .irq_unmask(). If this is the only active interrupt,
this clears enabled_irq_mask to 0, setting chip->irq_pm_held to
false and dropping the PM reference. 

When handle_level_irq() subsequently calls pt27_irq_unmask(), it will fail the
pt27_irq_regs_accessible(chip) check because irq_pm_held is false and
irq_bus_active is false, leaving the interrupt permanently masked.

> +
> +	if (drop_pm)
> +		pt27_runtime_put_autosuspend(chip);

[Severity: High]
Does this code call a sleepable PM runtime function while running in atomic
context under the IRQ core's raw spinlock?

pt27_irq_mask() is called by the IRQ core with the descriptor lock held, which
is a raw_spinlock_t. If drop_pm is true, the driver calls
pt27_runtime_put_autosuspend(chip), which internally acquires dev->power.lock
(a spinlock_t).

On PREEMPT_RT, spinlock_t is a sleepable rt_mutex. Acquiring a sleepable lock
inside a raw_spinlock_t critical section is a strict lock nesting violation
and triggers a sleeping in atomic context BUG.

> +
> +}
> +
> +static void pt27_irq_unmask(struct irq_data *d)
> +{
> +	struct gpio_chip *gc   = irq_data_get_irq_chip_data(d);
> +	struct pt27_gpio *chip = gpiochip_get_data(gc);
> +	irq_hw_number_t   hwirq = irqd_to_hwirq(d);
> +	unsigned long flags;
> +	u32 val;
> +
> +	if (!pt27_irq_regs_accessible(chip)) {
> +		dev_err_ratelimited(chip->dev,
> +				    "%s: cannot unmask GPIO%lu; PT27 is not in D0\n",
> +				     __func__, hwirq);
> +		return;
> +	}
> +
> +	pt27_irq_take_pm_hold(chip);
> +
> +	gpiochip_enable_irq(gc, hwirq);
> +
> +	raw_spin_lock_irqsave(&chip->lock, flags);

[Severity: Medium]
Can a TOCTOU race occur here in PM reference tracking between mask and unmask?

If CPU 0 executes pt27_irq_unmask() for hwirq 0, it calls
pt27_irq_take_pm_hold(), which sees irq_pm_held == true, skips incrementing
the PM usage count, and drops the lock.

Before CPU 0 re-acquires the lock, CPU 1 could execute pt27_irq_mask() for
the only active hwirq 1, clear enabled_irq_mask, set irq_pm_held to false,
and call pm_runtime_put_autosuspend().

CPU 0 then re-acquires the lock and sets enabled_irq_mask |= BIT(0), but
irq_pm_held remains false. The PM usage count is now unbalanced and the device
may enter runtime suspend while hwirq 0 is actively relying on it.

[ ... ]

> +static irqreturn_t pt27_gpio_isr(int irq, void *data)
> +{
> +	struct pt27_gpio *chip = data;
> +	unsigned long pending;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&chip->lock, flags);
> +	pending = pt27_pending_irqs_locked(chip);
> +	raw_spin_unlock_irqrestore(&chip->lock, flags);

[Severity: High]
Does this ISR access hardware registers without checking if the device is
active?

The device supports shared INTx interrupts via IRQF_SHARED, which means the
interrupt can fire at any time, including when the AMD PT GPIO controller is
in a suspended PM state (D3hot/D3cold).

pt27_gpio_isr() unconditionally calls pt27_pending_irqs_locked(), which
performs MMIO reads. Reading MMIO from a suspended PCI device typically
returns 0xffffffff and can cause PCIe target aborts, system hangs, or
interrupt storms.

Could we use pm_runtime_get_if_active() before accessing hardware registers?

[ ... ]

> +static int pt27_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct device    *dev = &pdev->dev;
> +	struct pt27_gpio *chip;

[ ... ]

> +err_free_irq:
> +	devm_free_irq(dev, chip->irq, chip);
> +err_wakeup:
> +	device_init_wakeup(dev, false);
> +err_free_vectors:
> +	pci_free_irq_vectors(pdev);
> +err_clear_master:
> +	pci_clear_master(pdev);
> +	return ret;
> +}

[Severity: Medium]
Are we leaking the runtime PM state in the probe error paths?

Since pm_runtime_use_autosuspend(dev) is called earlier in probe, if probe
fails at devm_request_irq() or devm_gpiochip_add_data(), it will jump to these
error labels without calling pm_runtime_dont_use_autosuspend(dev).

This leaves the PCI device with use_autosuspend = true. If another driver
binds to this device later, it will unexpectedly inherit an active autosuspend
configuration.

> +static void pt27_gpio_remove(struct pci_dev *pdev)
> +{
> +	struct device     *dev  = &pdev->dev;
> +	struct pt27_gpio  *chip = pci_get_drvdata(pdev);
> +	unsigned long flags;
> +	bool drop_pm;

[ ... ]

> +	devm_free_irq(dev, chip->irq, chip);
> +	pci_free_irq_vectors(pdev);
> +	pci_clear_master(pdev);
> +
> +	mutex_unlock(&chip->irq_bus_lock);
> +}

[Severity: Medium]
Does manual cleanup of the IRQ vectors invert the teardown order?

Because the gpiochip was registered using devm_gpiochip_add_data(), it won't
be unregistered until after pt27_gpio_remove() returns. However,
pci_free_irq_vectors() is called manually here, destroying the backing
hardware resources while the logical device remains active and exposed.

Userspace could successfully request GPIO interrupts during this window that
will silently never fire, breaking the hardware-to-software dependency chain.
Could we use devm_add_action_or_reset() for freeing the IRQ vectors?

> +static int __maybe_unused pt27_gpio_suspend(struct device *dev)
> +{
> +	struct pt27_gpio *chip = dev_get_drvdata(dev);
> +	unsigned long flags;
> +	u32 int_mask;
> +
> +	if (pm_runtime_status_suspended(dev)) {
> +		dev_dbg(dev, "%s: already runtime-suspended, skipping save\n",
> +			__func__);
> +		return 0;
> +	}
> +
> +	raw_spin_lock_irqsave(&chip->lock, flags);
> +	pt27_save_regs(chip);
> +	pt27_wr32(chip, PT27_REG_INT_EN,    0);
> +	int_mask = PT27_REG_INT_MASK & ~chip->wake_mask;
> +	pt27_wr32(chip, PT27_REG_INT_MASK,  int_mask);

[Severity: High]
Does this bitwise operation use a register offset address instead of a valid
mask?

PT27_REG_INT_MASK is a register offset macro (0x0020), not the GPIO bitmask
(PT27_GPIO_MASK). This bitwise AND limits int_mask to only bit 5, forcing all
other 23 bits to 0.

Since writing 0 to the mask register enables/unmasks the interrupt, this writes
a corrupted state to the device that inadvertently unmasks almost all interrupts
during system sleep, potentially causing an interrupt storm and preventing
sleep.

> +	pt27_wr32(chip, PT27_REG_INT_STAT,  PT27_GPIO_MASK);
> +	pt27_wr16(chip, PT27_REG_WAKE_STAT, PT27_WAKE_MASK);
> +	raw_spin_unlock_irqrestore(&chip->lock, flags);
> +
> +	return 0;
> +}

[ ... ]

> +static int __maybe_unused pt27_gpio_runtime_suspend(struct device *dev)
> +{
> +	struct pt27_gpio *chip = dev_get_drvdata(dev);
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	raw_spin_lock_irqsave(&chip->lock, flags);
> +
> +	if (chip->irq_pm_held || chip->enabled_irq_mask) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +
> +	if (chip->wake_mask && !device_may_wakeup(dev)) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +
> +	if (pt27_pending_irqs_locked(chip)) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +
> +	pt27_save_regs(chip);
> +
> +	pt27_wr32(chip, PT27_REG_INT_EN,    0);
> +	pt27_wr32(chip, PT27_REG_INT_MASK,  PT27_GPIO_MASK);
> +	pt27_wr32(chip, PT27_REG_INT_STAT,  PT27_GPIO_MASK);
> +	pt27_wr16(chip, PT27_REG_WAKE_STAT, PT27_WAKE_MASK);
> +
> +out_unlock:
> +	raw_spin_unlock_irqrestore(&chip->lock, flags);
> +	return ret;
> +}

[Severity: High]
Should this function call synchronize_irq(chip->irq)?

The device can use shared interrupts (IRQF_SHARED). When drivers use shared
IRQs, the PM subsystem guidelines strictly require calling synchronize_irq()
in the runtime suspend callback.

This initiates power-down and masks interrupts without previously synchronizing
the shared IRQ line, allowing the hardware to be powered down while the shared
IRQ handler is still executing mid-flight on another CPU, leading to invalid
MMIO accesses and potential crashes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013074327.4961-1-Chloe_Chen@asmedia.com.tw?part=1