Re: [PATCH 1/2] gpiolib: Add irq_set_affinity helper for GPIO line IRQs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpiolib: unlocked parent affinity update in gpiochip_irq_set_affinity()
- [Low] gpiolib: missing hierarchy Kconfig guard in gpiochip_irq_set_affinity()
--

commit 6be16d8d1664dad3beb504901b90bb4ff317a568
Author: Liang Hao <haohlliang@gmail.com>

gpiolib: Add irq_set_affinity helper for GPIO line IRQs

This commit adds gpiochip_irq_set_affinity() and a helper macro so GPIO line
interrupts can forward affinity updates to upstream interrupt controllers in
both hierarchical and chained irqchip setups.

> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 2987051d99043..0a46265799631 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
[ ... ]
> +int gpiochip_irq_set_affinity(struct irq_data *d,
> +			      const struct cpumask *dest, bool force)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_data *pdata;
> +	unsigned int parent;
> +	int ret;
> +
> +	if (d->parent_data)
> +		return irq_chip_set_affinity_parent(d, dest, force);

[Severity: Low]
Will this fail to build when hierarchical irqdomains are not enabled in the
kernel configuration?

In drivers/gpio/Kconfig, the gpiolib irqchip option only selects the base
irqdomain support:

drivers/gpio/Kconfig:
	config GPIOLIB_IRQCHIP
		select IRQ_DOMAIN
		bool

However, both the parent_data member in struct irq_data and the declaration of
irq_chip_set_affinity_parent() in include/linux/irq.h are guarded by the
hierarchy configuration option:

include/linux/irq.h:
	struct irq_data {
		...
	#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
		struct irq_data		*parent_data;
	#endif
		void			*chip_data;
	};
	...
	#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
	...
	extern int irq_chip_set_affinity_parent(struct irq_data *data,
						const struct cpumask *dest,
						bool force);

Should the d->parent_data check and irq_chip_set_affinity_parent() call in
gpiochip_irq_set_affinity() be guarded by an #ifdef just like the other
hierarchical helpers in drivers/gpio/gpiolib.c?

> +
> +	parent = gpiochip_chained_parent_virq(gc, d);
> +	if (!parent)
> +		return -EINVAL;
> +
> +	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;
> +	default:
> +		return ret;
> +	}
> +}

[Severity: High]
Is it safe to invoke pdata->chip->irq_set_affinity() directly on a chained
parent interrupt without holding the parent irq_desc lock or updating the
parent descriptor's affinity state?

Unlike hierarchical domains where d and d->parent_data belong to the same
struct irq_desc, a chained parent virq has a separate struct irq_desc with its
own lock and its own struct irq_common_data. When the genirq core invokes
gpiochip_irq_set_affinity() through irq_do_set_affinity(), only the child
GPIO line's desc->lock is held.

Calling pdata->chip->irq_set_affinity(pdata, dest, force) unlocked from
gpiochip_irq_set_affinity() appears to introduce several issues:

1. Concurrent affinity updates on sibling GPIO lines sharing the same parent
virq (or concurrent operations on the parent virq itself, such as CPU hotplug
migration in migrate_one_irq()) will race inside the parent irq_chip callback
and on pdata's effective_affinity mask. Upstream irqchip drivers assume their
own irq_desc->lock serializes irq_set_affinity(), for example in
drivers/irqchip/irq-gic-v3.c:gic_set_affinity():

	enabled = gic_peek_irq(d, GICD_ISENABLER);
	if (enabled)
		gic_mask_irq(d);
	...
	gic_write_irouter(val, reg);

	if (enabled)
		gic_unmask_irq(d);

	irq_data_update_effective_affinity(d, cpumask_of(cpu));

and in drivers/irqchip/irq-sifive-plic.c:plic_set_affinity():

	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);

	irq_data_update_effective_affinity(d, cpumask_of(cpu));

	if (!irqd_irq_disabled(d))
		plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);

Without holding the parent descriptor's lock, concurrent calls can corrupt the
parent routing state or observe the parent temporarily masked and leave it
permanently disabled.

2. Because irq_do_set_affinity() only updates desc->irq_common_data.affinity
on the child descriptor:

kernel/irq/manage.c:irq_do_set_affinity()
	switch (ret) {
	case IRQ_SET_MASK_OK:
	case IRQ_SET_MASK_OK_DONE:
		cpumask_copy(desc->irq_common_data.affinity, mask);
		fallthrough;
	case IRQ_SET_MASK_OK_NOCOPY:
		irq_validate_effective_affinity(data);
		irq_set_thread_affinity(desc);
		ret = 0;
	}

bypassing irq_do_set_affinity() on the parent descriptor leaves the parent's
irq_common_data.affinity unmodified (and skips deferred affinity movement
checks). As a result, CPU hot-unplug in
kernel/irq/cpuhotplug.c:migrate_one_irq() will migrate the parent interrupt
using its stale affinity mask, and on configurations without effective
affinity masks, irq_data_get_effective_affinity_mask(pdata) will also return
that stale parent affinity mask:

include/linux/irq.h:irq_data_get_effective_affinity_mask()
	static inline
	const struct cpumask *irq_data_get_effective_affinity_mask(struct irq_data *d)
	{
		return irq_data_get_affinity_mask(d);
	}

3. When multiple GPIO lines share a chained parent virq, requesting and
starting up any sibling GPIO line later via
request_threaded_irq() -> __setup_irq() -> irq_startup() -> irq_setup_affinity()
will see a fresh child descriptor without a user-configured affinity mask and
call irq_do_set_affinity() with irq_default_affinity:

kernel/irq/manage.c:irq_setup_affinity()
	if (irqd_affinity_is_managed(&desc->irq_data) ||
	    irqd_has_set(&desc->irq_data, IRQD_AFFINITY_SET)) {
		if (cpumask_intersects(desc->irq_common_data.affinity,
				       cpu_online_mask))
			set = desc->irq_common_data.affinity;
		else
			irqd_clear(&desc->irq_data, IRQD_AFFINITY_SET);
	}
	...
	return irq_do_set_affinity(&desc->irq_data, &mask, false);

Won't starting a second GPIO line on the same parent virq silently reset the
shared parent interrupt's hardware affinity back to the default mask, leaving
any previously configured sibling GPIO line out of sync with the hardware?

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