Re: [PATCH v13 10/13] gpio: pl061: switch data path to gpio-regmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpio: pl061: dropping IRQ resource helpers breaks IRQ locking and unbind
--

commit 074c89211872cf47652e8bf1f251d42902b7bc3b
Author: Long Zhao <longzhao@ambarella.com>

gpio: pl061: switch data path to gpio-regmap

This commit switches the PL061 GPIO data path callbacks (get, set, and
direction) to use the gpio-regmap helper library while keeping a custom
irq_chip and instantiating an external irq_domain for gpio-regmap to attach.

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 27ed759486caf..cb1ef0f3774c1 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c

[ ... ]

> @@ -229,54 +198,47 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)

[ ... ]

>  static void pl061_irq_mask(struct irq_data *d)
>  {
> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> -	struct pl061 *pl061 = gpiochip_get_data(gc);
> -	u8 mask = BIT(irqd_to_hwirq(d) % PL061_GPIO_NR);
> +	struct pl061 *pl061 = irq_data_get_irq_chip_data(d);
> +	unsigned int mask = pl061_line_mask(pl061, irqd_to_hwirq(d));
>  
> -	scoped_guard(raw_spinlock, &pl061->lock)
> -		regmap_clear_bits(pl061->regmap, GPIOIE, mask);
> -
> -	gpiochip_disable_irq(gc, d->hwirq);
> +	regmap_clear_bits(pl061->regmap, pl061->data->regs->ie, mask);
>  }
>  
>  static void pl061_irq_unmask(struct irq_data *d)
>  {
> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> -	struct pl061 *pl061 = gpiochip_get_data(gc);
> -	u8 mask = BIT(irqd_to_hwirq(d) % PL061_GPIO_NR);
> -
> -	gpiochip_enable_irq(gc, d->hwirq);
> +	struct pl061 *pl061 = irq_data_get_irq_chip_data(d);
> +	unsigned int mask = pl061_line_mask(pl061, irqd_to_hwirq(d));
>  
> -	scoped_guard(raw_spinlock, &pl061->lock)
> -		regmap_set_bits(pl061->regmap, GPIOIE, mask);
> +	regmap_set_bits(pl061->regmap, pl061->data->regs->ie, mask);
>  }

[ ... ]

> @@ -320,67 +280,169 @@ static const struct irq_chip pl061_irq_chip = {
>  	.irq_set_wake		= pl061_irq_set_wake,
>  	.irq_print_chip		= pl061_irq_print_chip,
>  	.flags			= IRQCHIP_IMMUTABLE,
> -	GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};

[ ... ]

> +	if (irq) {
> +		const struct irq_domain_info info = {
> +			.fwnode		= dev_fwnode(dev),
> +			.size		= data->ngpio,
> +			.hwirq_max	= data->ngpio,
> +			.ops		= &pl061_irq_domain_ops,
> +			.host_data	= pl061,
> +		};
> +
> +		pl061->irq_domain = devm_irq_domain_instantiate(dev, &info);
> +		if (IS_ERR(pl061->irq_domain))
> +			return PTR_ERR(pl061->irq_domain);
> +
> +		/*
> +		 * Registered before the gpiochip so unbind runs it after
> +		 * gpiochip_free_remaining_irqs(), while the domain is still
> +		 * alive. devm removes the domain after this action.
> +		 */
> +		ret = devm_add_action_or_reset(dev, pl061_dispose_irq_mappings, pl061);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
As reported by Sashiko on v11 [1], does dropping GPIOCHIP_IRQ_RESOURCE_HELPERS
from pl061_irq_chip and removing gpiochip_disable_irq() and
gpiochip_enable_irq() from pl061_irq_mask() and pl061_irq_unmask() introduce
regressions during device unbind and GPIO direction changes?

Without gpiochip_reqres_irq() and gpiochip_relres_irq(),
GPIOD_FLAG_USED_AS_IRQ and GPIOD_FLAG_IRQ_IS_ENABLED are never set on the GPIO
descriptors.

On device unbind while an interrupt is requested (for example via userspace
cdev edge detection in edge_detector_setup() -> request_threaded_irq() or an
in-kernel consumer), devm_gpio_regmap_unregister() -> gpiochip_remove() ->
gpiochip_free_remaining_irqs() only frees descriptors that have
GPIOD_FLAG_USED_AS_IRQ set:

drivers/gpio/gpiolib.c:gpiochip_free_remaining_irqs() {
	for_each_gpio_desc_with_flag(gc, desc, GPIOD_FLAG_USED_AS_IRQ) {
		...
	}
}

Because that flag is never set, gpiochip_free_remaining_irqs() becomes a no-op
despite the comment in pl061_probe() above devm_add_action_or_reset(). Next,
pl061_dispose_irq_mappings() calls irq_dispose_mapping() -> irq_free_desc()
while desc->action and any threaded handler are still active. This triggers a
warning in remove_proc_entry() on /proc/irq/<virq> and frees the irq_desc.
When the consumer later calls free_irq() (such as on closing the GPIO character
device file descriptor), irq_to_desc(virq) returns NULL, leaking the irqaction
and leaving the edge_irq_thread kthread running with a dangling dev_id pointer
to the freed struct line.

In addition, without these helpers:
- gpiochip_lock_as_irq() is not called when an interrupt is requested, so
  requesting an IRQ on a line configured as a push-pull output is no longer
  rejected.
- gpiod_direction_output() no longer prevents switching a line with an active
  interrupt to output mode.
- Module reference counting in gpiochip_reqres_irq() and gpiochip_relres_irq()
  is bypassed.

Although the commit message compares this with gpio-idio-16,
gpio-pcie-idio-24, and gpio-104-idi-48, those drivers use fixed-direction
inputs for their interrupt-capable lines and did not previously use
GPIOCHIP_IRQ_RESOURCE_HELPERS, whereas PL061 lines have software-configurable
direction.

[1] https://lore.kernel.org/r/20261020-cv75-gpio-v11-8-e058bae19571@ambarella.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-cv75-gpio-v13-0-4f93114ea204@ambarella.com?part=10