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] gpio: amdptpci: hardware access occurs after failed device resume
- [High] gpio: amdptpci: pending wakeup interrupts are destroyed during resume
- [High] pm: ISR accesses suspended MMIO due to irq_pm_held flag race
- [High] gpio: interrupt storm due to registering GPIO chip before parent IRQ
- [Medium] gpio: amdptpci: invalid bitmask writes to reserved bits during suspend
- [Medium] gpio: amdptpci: device can never runtime suspend if wake IRQ is configured
- [Medium] gpio: amdptpci: S3 wakeups broken if device autosuspends before system suspend
- [Low] gpio: amdptpci: missing pm_runtime_dont_use_autosuspend() on probe error path
--

commit 84767db1836e202d0ec481c787ac2cf3407a669a
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..c63e9b6dcf7ad
> --- /dev/null
> +++ b/drivers/gpio/gpio-amdptpci.c
[ ... ]
> +static void pt27_restore_regs(struct pt27_gpio *chip)
> +{
[ ... ]
> +	pt27_wr16(chip, PT27_REG_WAKE_LVL,  chip->saved.wake_lvl);
> +	pt27_wr16(chip, PT27_REG_WAKE_MODE, chip->saved.wake_mode);
> +	pt27_wr16(chip, PT27_REG_WAKE_EN,   chip->saved.wake_en);
> +
> +	pt27_wr32(chip, PT27_REG_INT_STAT,  PT27_GPIO_MASK);
> +	pt27_wr16(chip, PT27_REG_WAKE_STAT, PT27_WAKE_MASK);

[Severity: High]
Does writing the mask to the status register here clear all pending interrupts
unconditionally before the ISR can service them? 

When the system wakes up due to a GPIO event, the hardware logs the event in
the status register. If pt27_restore_regs() clears them assuming W1C
semantics, won't the core IRQ handler read 0 pending interrupts and return
IRQ_NONE, permanently losing the wakeup interrupt?

> +
> +	pt27_wr32(chip, PT27_REG_INT_EN,   chip->saved.int_en);
> +	pt27_wr32(chip, PT27_REG_INT_MASK, chip->saved.int_mask);
> +
> +}
[ ... ]
> +static bool pt27_irq_regs_accessible(struct pt27_gpio *chip)
> +{
> +
> +	return READ_ONCE(chip->irq_bus_active) ||
> +	       READ_ONCE(chip->irq_pm_held);
> +}

[Severity: High]
Will this OR condition cause the device to be accessed even if it failed to
resume? 

In pt27_irq_bus_lock() below, a failed resume sets chip->irq_bus_active to
false, but chip->irq_pm_held is already set to true by
pt27_irq_request_resources(). This means pt27_irq_regs_accessible() will
evaluate to true, and subsequent IRQ operations will attempt to write to MMIO
registers of a powered-down device.

[ ... ]
> +static int pt27_irq_request_resources(struct irq_data *d)
> +{
[ ... ]
> +	raw_spin_lock_irqsave(&chip->lock, flags);
> +	chip->requested_irq_mask |= BIT(hwirq);
> +	if (!chip->irq_pm_held) {
> +		chip->irq_pm_held = true;
> +
> +		pm_runtime_get_noresume(chip->dev);
> +	}
> +	raw_spin_unlock_irqrestore(&chip->lock, flags);

[Severity: High]
Is there a race condition here where the ISR might access suspended MMIO
hardware? 

This sets chip->irq_pm_held to true and calls pm_runtime_get_noresume(),
which only increments the usage counter but does not wake the device from D3.
The device is not actually woken until pt27_irq_bus_lock() runs. 

If a shared PCI INTx interrupt fires in this window, the ISR pt27_gpio_isr()
will see chip->irq_pm_held is true and bypass its suspend safeguard, leading
to an invalid MMIO read. Should this use pm_runtime_get_if_active() instead?

[ ... ]
> +static void pt27_irq_bus_lock(struct irq_data *d)
> +{
> +	struct gpio_chip *gc   = irq_data_get_irq_chip_data(d);
> +	struct pt27_gpio *chip = gpiochip_get_data(gc);
> +	int ret;
> +
> +	mutex_lock(&chip->irq_bus_lock);
> +
> +	ret = pm_runtime_resume_and_get(chip->dev);
> +	if (ret < 0) {
> +		WRITE_ONCE(chip->irq_bus_active, false);
> +		dev_err_ratelimited(chip->dev,
> +				    "%s: cannot resume for IRQ configuration: %d\n",
> +				     __func__, ret);
> +		return;
> +	}

[Severity: High]
If pm_runtime_resume_and_get() fails here, it correctly records that the bus
is inactive. Since the core IRQ operation is not aborted, and the static flag
forces pt27_irq_regs_accessible() to evaluate to true, will the outer IRQ
code proceed to interact with the device despite this failure?

[ ... ]
> +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);
> +	/*
> +	 * No requested IRQ means nothing to dispatch, and the device may be
> +	 * runtime-suspended (D3): a shared INTx line can still call us, so do
> +	 * not touch MMIO unless the PM hold keeps the device in D0.
> +	 */
> +	if (!chip->irq_pm_held) {
> +		raw_spin_unlock_irqrestore(&chip->lock, flags);
> +		return IRQ_NONE;
> +	}
> +	pending = pt27_pending_irqs_locked(chip);

[Severity: High]
Could this ISR bypass its suspend safeguard based on the static flag race
discussed earlier? If the hardware is suspended but the flag evaluates to
true, could this lead to a bus error or spurious interrupt?

[ ... ]
> +static int pt27_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
[ ... ]
> +	ret = devm_gpiochip_add_data(dev, &chip->gc, chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register GPIO chip\n");
> +
> +	ret = devm_request_irq(dev, chip->irq, pt27_gpio_isr, irq_flags | IRQF_NO_THREAD, dev_name(dev), chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to request IRQ %d\n", chip->irq);

[Severity: High]
Can this initialization sequence lead to an unhandled interrupt storm? 

By calling devm_gpiochip_add_data() before devm_request_irq(), the GPIO chip
is exposed to consumers before its parent ISR is registered. If a consumer
immediately requests and unmasks a GPIO IRQ, the hardware will assert the
shared PCI IRQ line, but pt27_gpio_isr() will not yet be registered to handle
it.

Reversing the order is safe because if a spurious shared interrupt occurs
before the domain is initialized, chip->irq_pm_held will be false, and the
ISR will safely return IRQ_NONE without dereferencing anything.

[Severity: Low]
If either of these registration steps fail, the probe function returns
directly. Does this leak the PM configuration by missing a call to
pm_runtime_dont_use_autosuspend(dev) to revert the PM state?

[ ... ]
> +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;
> +	}

[Severity: Medium]
Will returning early here break S3 wakeups if the device autosuspends before
system suspend? 

pt27_gpio_runtime_suspend() masks all interrupts, configuring no wake events.
By skipping the resume preparation when already suspended, this function
never unmasks wake-capable pins, completely breaking the controller's ability
to wake the system.

> +
> +	raw_spin_lock_irqsave(&chip->lock, flags);
> +	pt27_save_regs(chip);
> +	pt27_wr32(chip, PT27_REG_INT_EN,    0);
> +	int_mask = 0xFFFFFFFF & ~chip->wake_mask;
> +	//int_mask = PT27_REG_INT_MASK & ~chip->wake_mask;
> +	pt27_wr32(chip, PT27_REG_INT_MASK,  int_mask);

[Severity: Medium]
Does this write invalid bitmasks to reserved hardware bits? 

The controller supports 24 GPIOs (PT27_GPIO_MASK is 0x00FFFFFF). By using
0xFFFFFFFF, the bitwise NOT operation will write 1s to bits 24-31 of the
PT27_REG_INT_MASK register, which could trigger unintended hardware faults.

> +	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);
> +
> +	synchronize_irq(chip->irq);
> +	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->requested_irq_mask) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +
> +	if (chip->wake_mask && !device_may_wakeup(dev)) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}

[Severity: Medium]
Does the first check unintentionally prevent the device from ever entering
runtime suspend when a wake IRQ is configured? 

A wake IRQ is inherently a requested IRQ, meaning chip->requested_irq_mask
will always be non-zero if chip->wake_mask is non-zero. The first block will
therefore unconditionally abort the suspend with -EBUSY, making the
subsequent wake_mask check dead code and preventing the PCI device from
reaching D3.

[ ... ]

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