Re: [PATCH 2/2] gpio: dwapb: Use gpiolib irq_set_affinity helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] gpio: dwapb: broken irq_set_affinity on multi-IRQ DT and ACPI platforms
- [High] gpio: unlocked parent call and state desync in gpiochip_irq_set_affinity()
- [Low] gpio: unguarded parent_data access in gpiochip_irq_set_affinity()
--

commit 72a8fcde432031570e3b16d2b7058ce326e44df1
Author: Liang Hao <haohlliang@gmail.com>

gpio: dwapb: Use gpiolib irq_set_affinity helper

This commit wires GPIOCHIP_IRQ_AFFINITY_HELPERS into dwapb_irq_chip so that
dwapb GPIO interrupts use the gpiolib irq_set_affinity helper.

> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index aa7c08e607074..9e63c3ecb5b45 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -412,6 +412,7 @@ static const struct irq_chip dwapb_irq_chip = {
>  	.irq_enable	= dwapb_irq_enable,
>  	.irq_disable	= dwapb_irq_disable,
>  	.irq_set_wake	= pm_sleep_ptr(dwapb_irq_set_wake),
> +	GPIOCHIP_IRQ_AFFINITY_HELPERS,

[Severity: High]
Does wiring GPIOCHIP_IRQ_AFFINITY_HELPERS unconditionally into dwapb_irq_chip
cause irq_set_affinity to fail with -EINVAL on multi-IRQ Device Tree
platforms and ACPI platforms?

In drivers/gpio/gpio-dwapb.c, dwapb_convert_irqs() compacts non-zero per-line
parent IRQs into pirq->irq[]:

drivers/gpio/gpio-dwapb.c:dwapb_convert_irqs() {
	for (i = 0; i < pp->ngpio; ++i) {
		if (!pp->irq[i])
			continue;

		pirq->irq[pirq->nr_irqs++] = pp->irq[i];
	}
}

And dwapb_configure_irqs() sets up girq differently depending on whether
there is an ACPI companion or Device Tree interrupts:

drivers/gpio/gpio-dwapb.c:dwapb_configure_irqs() {
	if (has_acpi_companion(gpio->dev)) {
		girq->num_parents = 0;
		girq->parents = NULL;
		girq->parent_handler = NULL;

		err = devm_request_irq(gpio->dev, pp->irq[0],
				       dwapb_irq_handler_mfd,
				       IRQF_SHARED, DWAPB_DRIVER_NAME, gpio);
		...
	} else {
		girq->num_parents = pirq->nr_irqs;
		girq->parents = pirq->irq;
		girq->parent_handler_data = gpio;
		girq->parent_handler = dwapb_irq_handler;
	}
}

On Device Tree platforms with multiple parent interrupts (where
pirq->nr_irqs > 1, such as platforms wiring dedicated per-GPIO interrupts on
Port A), girq->map is left NULL. In drivers/gpio/gpiolib.c,
gpiochip_irq_map() only calls irq_set_parent() when gc->irq.num_parents == 1
or gc->irq.map is non-NULL, leaving desc->parent_irq == 0:

drivers/gpio/gpiolib.c:gpiochip_irq_map() {
	if (gc->irq.num_parents == 1)
		ret = irq_set_parent(irq, gc->irq.parents[0]);
	else if (gc->irq.map)
		ret = irq_set_parent(irq, gc->irq.map[hwirq]);
}

When gpiochip_irq_set_affinity() -> gpiochip_chained_parent_virq() runs,
girq->num_parents != 1, girq->map is NULL, and desc->parent_irq is 0, so
gpiochip_chained_parent_virq() returns 0 and gpiochip_irq_set_affinity()
returns -EINVAL. Furthermore, if per-line parent IRQs are sparsely populated
such that only a single line j > 0 has a valid parent IRQ,
dwapb_convert_irqs() compacts it into pirq->irq[0] with pirq->nr_irqs == 1,
causing gpiochip_chained_parent_virq() to return line j's parent virq for any
GPIO line on Port A.

On ACPI platforms (has_acpi_companion(gpio->dev)), girq->parent_handler is
NULL, so gpiochip_chained_parent_virq() also immediately returns 0:

drivers/gpio/gpiolib.c:gpiochip_chained_parent_virq() {
	if (!girq->parent_handler)
		return 0;
	...
	if (girq->num_parents == 1)
		return girq->parents[0];

	if (girq->map) {
		if (!gpiochip_irqchip_irq_valid(gc, hwirq))
			return 0;
		return girq->map[hwirq];
	}

	desc = irq_to_desc(d->irq);
	if (desc && desc->parent_irq)
		return desc->parent_irq;

	return 0;
}

Because dwapb_irq_chip.irq_set_affinity is now non-NULL,
__irq_can_set_affinity() and irq_can_set_affinity_usr() evaluate to true.
Won't this cause irq_startup() -> irq_setup_affinity() to fail with -EINVAL
when requesting any GPIO IRQ, migrate_one_irq() in kernel/irq/cpuhotplug.c to
log ratelimited warnings during CPU hot-unplug, and writes to
/proc/irq/<N>/smp_affinity to fail on these platforms?

[Severity: High]
Can calling pdata->chip->irq_set_affinity() through
gpiochip_irq_set_affinity() on a chained dwapb irq_chip race on the parent
irq_chip without parent_desc->lock held and desynchronize parent and sibling
IRQ affinity state?

For chained dwapb GPIO interrupts, d->parent_data is NULL and the parent is a
separate virq with its own struct irq_desc. When the genirq core invokes
dwapb_irq_chip.irq_set_affinity, only the child GPIO descriptor lock is held,
yet gpiochip_irq_set_affinity() directly calls into the parent chip:

drivers/gpio/gpiolib.c:gpiochip_irq_set_affinity() {
	pdata = irq_get_irq_data(parent);
	if (!pdata || !pdata->chip || !pdata->chip->irq_set_affinity)
		return -EINVAL;

	ret = pdata->chip->irq_set_affinity(pdata, dest, force);
	switch (ret) {
	case IRQ_SET_MASK_OK:
	case IRQ_SET_MASK_OK_NOCOPY:
	case IRQ_SET_MASK_OK_DONE:
		irq_data_update_effective_affinity(d,
			irq_data_get_effective_affinity_mask(pdata));
		return IRQ_SET_MASK_OK;
}

Parent irq_chip callbacks such as gic_set_affinity() in
drivers/irqchip/irq-gic-v3.c or plic_set_affinity() in
drivers/irqchip/irq-sifive-plic.c expect their own parent_desc->lock to be
held while masking, updating routing registers, unmasking, and updating
pdata->common->effective_affinity. If two GPIO lines sharing a parent IRQ
update affinity concurrently (or race with operations on the parent virq),
can this corrupt hardware routing state and effective_affinity masks?

In addition, because calling pdata->chip->irq_set_affinity() directly
bypasses irq_do_set_affinity() on parent_desc,
parent_desc->irq_common_data.affinity and IRQD_AFFINITY_SET are never updated
on the parent descriptor. And when any sibling GPIO line sharing the same
parent IRQ is later started via request_threaded_irq() -> irq_startup() ->
irq_setup_affinity() in kernel/irq/manage.c:

kernel/irq/manage.c:irq_setup_affinity() {
	struct cpumask *set = irq_default_affinity;
	...
	if (!__irq_can_set_affinity(desc))
		return 0;
	...
	return irq_do_set_affinity(&desc->irq_data, &mask, false);
}

won't starting that second GPIO line overwrite the shared parent IRQ's
hardware routing back to irq_default_affinity while leaving the first GPIO
line's descriptor reporting stale affinity masks?

[Severity: Low]
Will gpiochip_irq_set_affinity() in drivers/gpio/gpiolib.c fail to build when
CONFIG_GPIOLIB_IRQCHIP is enabled without CONFIG_IRQ_DOMAIN_HIERARCHY?

In include/linux/irq.h, parent_data in struct irq_data and
irq_chip_set_affinity_parent() are only declared when
CONFIG_IRQ_DOMAIN_HIERARCHY is enabled:

include/linux/irq.h:struct irq_data {
#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
	struct irq_data		*parent_data;
#endif
}

However, gpiochip_irq_set_affinity() accesses d->parent_data and calls
irq_chip_set_affinity_parent() without a CONFIG_IRQ_DOMAIN_HIERARCHY guard:

drivers/gpio/gpiolib.c:gpiochip_irq_set_affinity() {
	if (d->parent_data)
		return irq_chip_set_affinity_parent(d, dest, force);
}

Since CONFIG_GPIO_DWAPB and CONFIG_GPIOLIB_IRQCHIP do not select
CONFIG_IRQ_DOMAIN_HIERARCHY, should this block in gpiochip_irq_set_affinity()
be guarded with CONFIG_IRQ_DOMAIN_HIERARCHY like the other parent_data users
in drivers/gpio/gpiolib.c and drivers/gpio/gpio-dwapb.c?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023151209.58772-1-haohlliang@gmail.com?part=2