Re: [PATCH v14 10/14] gpio: pl061: switch data path to gpio-regmap

[sashiko-bot@kernel.org]

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

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

gpio: pl061: switch data path to gpio-regmap

This commit converts the PL061 GPIO data path to gpio-regmap while keeping
a custom irq_chip and instantiating an external irq_domain.

> 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]
Does dropping GPIOCHIP_IRQ_RESOURCE_HELPERS and
gpiochip_enable_irq()/gpiochip_disable_irq() break IRQ teardown on unbind
and gpiolib's direction vs. IRQ protections?

Without those helpers, GPIOD_FLAG_USED_AS_IRQ and GPIOD_FLAG_IRQ_IS_ENABLED
are never set on the GPIO descriptors.  Although the comment above
pl061_dispose_irq_mappings() relies on gpiochip_free_remaining_irqs()
cleaning up active interrupts first during unbind,
gpiochip_free_remaining_irqs() only iterates over descriptors with
GPIOD_FLAG_USED_AS_IRQ set and becomes a no-op.

If a line has an active interrupt when the device is unbound (for example,
via GPIO_V2_GET_LINE_IOCTL edge detection), pl061_dispose_irq_mappings()
calls irq_dispose_mapping() -> free_desc() while desc->action and the
threaded IRQ handler are still installed, warning in remove_proc_entry()
and freeing the irq_desc; a later free_irq() then sees irq_to_desc(virq) ==
NULL and leaks the irqaction while leaving edge_irq_thread running with a
dangling dev_id pointer.

Leaving those flags unset also regressions direction and module lifetime
enforcement: requesting an IRQ on a line configured as an output is no
longer rejected by gpiochip_lock_as_irq(), switching a line with an active
enabled IRQ to output mode is no longer blocked by
gpiod_direction_output_nonotify(), and module refcounting in
gpiochip_reqres_irq()/gpiochip_relres_irq() is bypassed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-cv75-gpio-v14-0-e586aad0467a@ambarella.com?part=10