Re: [PATCH] gpio: pch: use raw_spinlock_t for the register lock

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

Pre-existing issues:
- [High] The GPIO chip is registered via devm_gpiochip_add_data() before its irq_base is initialized, exposing incorrect IRQ mappings to consumers.
- [High] The shared interrupt handler is requested before the generic irqchip is fully initialized, which could lead to uninitialized descriptor accesses.
- [High] pch_gpio_to_irq() returns valid-looking bogus IRQ numbers when IRQ allocation fails during probe.
--

--- Patch [1]: [PATCH] gpio: pch: use raw_spinlock_t for the register lock ---
commit 90a7cbda6861f749a9ed833233971bb8a60d95dc
Author: Junjie Cao <[email protected]>

gpio: pch: use raw_spinlock_t for the register lock

This patch converts the spinlock_t register lock in the PCH GPIO driver to a
raw_spinlock_t. This prevents invalid wait context and scheduling while atomic
issues on PREEMPT_RT kernels, as the lock is acquired within the non-sleepable
irq_set_type callback.

> diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
> index 4ffa0955a9e3d..07a5617b314b3 100644
> --- a/drivers/gpio/gpio-pch.c
> +++ b/drivers/gpio/gpio-pch.c
[ ... ]
> @@ -374,7 +374,7 @@ static int pch_gpio_probe(struct pci_dev *pdev,
>  	chip->ioh = id->driver_data;
>  	chip->reg = chip->base;
>  	pci_set_drvdata(pdev, chip);
> -	spin_lock_init(&chip->spinlock);
> +	raw_spin_lock_init(&chip->spinlock);
>  	pch_gpio_setup(chip);
>  
>  	ret = devm_gpiochip_add_data(dev, &chip->gpio, chip);

[Severity: High]
This isn't a bug introduced by this patch, but in
drivers/gpio/gpio-pch.c:pch_gpio_probe(), does registering the GPIO chip before
initializing its IRQ base expose incorrect IRQ mappings to consumers?

If a consumer acquires the GPIO and calls gpiod_to_irq() in the race window
after devm_gpiochip_add_data() but before devm_irq_alloc_descs() sets
chip->irq_base, pch_gpio_to_irq() will return 0 + offset since the chip
structure is initially zeroed out.

[Severity: High]
This is a pre-existing issue, but in drivers/gpio/gpio-pch.c:pch_gpio_probe(),
if devm_irq_alloc_descs() fails, probe succeeds but leaves chip->irq_base
as -1:

	irq_base = devm_irq_alloc_descs(dev, -1, 0,
					gpio_pins[chip->ioh], NUMA_NO_NODE);
	if (irq_base < 0) {
		dev_warn(dev, "PCH gpio: Failed to get IRQ base num\n");
		chip->irq_base = -1;
		return 0;
	}

When a consumer later calls gpiod_to_irq(), pch_gpio_to_irq() calculates
chip->irq_base + offset:

static int pch_gpio_to_irq(struct gpio_chip *gpio, unsigned int offset)
{
	struct pch_gpio *chip = gpiochip_get_data(gpio);

	return chip->irq_base + offset;
}

For offsets > 0, does this compute a non-negative value (e.g. 0, 1, 2),
breaking the error-reporting contract of to_irq and potentially causing
consumers to steal essential system IRQs?

[Severity: High]
This is also a pre-existing issue, but later in
drivers/gpio/gpio-pch.c:pch_gpio_probe(), the shared interrupt handler is
requested before the generic irqchip is allocated:

	ret = devm_request_irq(dev, pdev->irq, pch_gpio_handler,
			       IRQF_SHARED, KBUILD_MODNAME, chip);
	if (ret)
		return dev_err_probe(dev, ret, "Failed to request IRQ\n");

	return pch_gpio_alloc_generic_chip(chip, irq_base, gpio_pins[chip->ioh]);

If an interrupt fires immediately on the shared line, could pch_gpio_handler()
try to dispatch the interrupt using uninitialized descriptors, leading to a
handle_bad_irq warning and potentially disabling the shared interrupt line?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1