Re: [PATCH v11 8/9] gpio: pl061: switch data path to gpio-regmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] gpio: pl061: dropping IRQ resource helpers breaks GPIO-IRQ locking and cleanup
- [High] gpio: pl061: switching to external irq_domain breaks ACPI _AEI events
- [Medium] gpio: pl061: missing irq_set_lockdep_class() in pl061_irq_domain_map()
- [Medium] gpio: pl061: pl061_remove_irq() uninstalls unset handler on probe failure
--

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

gpio: pl061: switch data path to gpio-regmap

This commit converts the PL061 GPIO driver data path (get, set, and direction
operations) to use gpio-regmap while switching the interrupt support to a
custom irq_domain attached via gpio_regmap_config.

>     gpio: pl061: switch data path to gpio-regmap
>     
>     Move get, set and direction onto gpio-regmap. ARM PL061 sets
>     GPIO_REGMAP_QUIRK_SET_AFTER_DIR so the output value is written
>     after the direction change.
>     
>     Keep a custom irqchip and irq domain. Create the domain with
>     the PL061 private data and let gpio-regmap attach it.
>     
>     Check the chained-handler regmap_read() return and always run
>     chained_irq_exit(). irq_type still ignores MMIO write returns.
>     
>     irq_set_parent() nests the GPIO irq_desc under the AMBA parent
>     for irq_set_wake() and lockdep; checked with PROVE_LOCKING on
>     CV75.
>     
>     Signed-off-by: Long Zhao <longzhao@ambarella.com>

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 585871ff25199..3d04f8c2a5c69 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c

[ ... ]

> @@ -231,54 +199,52 @@ 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);
> -
> -	scoped_guard(raw_spinlock, &pl061->lock)
> -		regmap_clear_bits(pl061->regmap, GPIOIE, mask);
> +	struct pl061 *pl061 = irq_data_get_irq_chip_data(d);
> +	unsigned int mask = pl061_line_mask(pl061, irqd_to_hwirq(d));
>  
> -	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);
> +	struct pl061 *pl061 = irq_data_get_irq_chip_data(d);
> +	unsigned int mask = pl061_line_mask(pl061, irqd_to_hwirq(d));
>  
> -	gpiochip_enable_irq(gc, d->hwirq);
> -
> -	scoped_guard(raw_spinlock, &pl061->lock)
> -		regmap_set_bits(pl061->regmap, GPIOIE, mask);
> +	regmap_set_bits(pl061->regmap, pl061->data->regs->ie, mask);
>  }

[ ... ]

> @@ -322,67 +286,162 @@ 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,
> +};

[Severity: High]
Does dropping GPIOCHIP_IRQ_RESOURCE_HELPERS from pl061_irq_chip and
removing gpiochip_disable_irq() / gpiochip_enable_irq() from
pl061_irq_mask() and pl061_irq_unmask() break GPIO-IRQ resource management
and unbind cleanup?

Attaching the custom domain through config.irq_domain in
devm_gpio_regmap_register() only calls gpiochip_irqchip_add_domain(), which
sets gc->to_irq and gc->irq.domain without installing IRQ resource
callbacks.

Without gpiochip_reqres_irq() and gpiochip_relres_irq() (and the
enable/disable helpers), requesting an interrupt on a PL061 line no longer
calls gpiochip_lock_as_irq() or takes a module reference on the provider:

- GPIOD_FLAG_USED_AS_IRQ and GPIOD_FLAG_IRQ_IS_ENABLED are never set on the
  GPIO descriptor.
- Requesting an IRQ on a line configured as a push-pull output is no longer
  rejected, and gpiod_direction_output() no longer prevents reconfiguring a
  line with an active IRQ to output mode.
- On unbind, pl061_probe() relies on gpiochip_free_remaining_irqs() running
  before pl061_remove_irq(), but in drivers/gpio/gpiolib.c:

static void gpiochip_free_remaining_irqs(struct gpio_chip *gc)
{
	struct gpio_desc *desc;

	for_each_gpio_desc_with_flag(gc, desc, GPIOD_FLAG_USED_AS_IRQ)
		gpiod_free_irqs(desc);
}

Because GPIOD_FLAG_USED_AS_IRQ is never set, gpiochip_free_remaining_irqs()
becomes a no-op, and pl061_remove_irq() calls irq_dispose_mapping(virq) ->
irq_free_desc(virq) while consumer irqactions, procfs entries, and threaded
IRQ kthreads are still active.

> +
> +static int pl061_irq_domain_map(struct irq_domain *d, unsigned int virq,
> +				irq_hw_number_t hwirq)
> +{
> +	struct pl061 *pl061 = d->host_data;
> +
> +	irq_set_chip_data(virq, pl061);
> +	irq_set_chip_and_handler(virq, &pl061_irq_chip, handle_bad_irq);
> +	irq_set_noprobe(virq);
> +	irq_set_parent(virq, pl061->parent_irq);
> +
> +	return 0;
> +}

[Severity: Medium]
The commit message states:

  "irq_set_parent() nests the GPIO irq_desc under the AMBA parent
   for irq_set_wake() and lockdep; checked with PROVE_LOCKING on
   CV75."

Does irq_set_parent() actually configure lockdep classes for
irq_set_irq_wake()?

Looking at kernel/irq/manage.c, irq_set_parent() only sets desc->parent_irq
when CONFIG_HARDIRQS_SW_RESEND is enabled (for software IRQ resending in
irq_sw_resend()), and is a no-op stub otherwise:

#ifdef CONFIG_HARDIRQS_SW_RESEND
int irq_set_parent(int irq, int parent_irq)
{
	scoped_irqdesc_get_and_lock(irq, 0) {
		scoped_irqdesc->parent_irq = parent_irq;
		return 0;
	}
	return -EINVAL;
}
EXPORT_SYMBOL_GPL(irq_set_parent);
#endif

Every irq_desc has its desc->lock initialized to the shared
&irq_desc_lock_class in init_desc(). Previously, gpiochip_irq_map() called
irq_set_lockdep_class() to place child GPIO interrupts into a separate
lockdep class from pl061->parent_irq.

Without irq_set_lockdep_class() in pl061_irq_domain_map(), won't calling
enable_irq_wake() or disable_irq_wake() on a mapped PL061 GPIO IRQ trigger
a recursive locking splat on &irq_desc_lock_class when
irq_set_irq_wake(virq) holds the child desc->lock and calls
pl061_irq_set_wake() -> irq_set_irq_wake(pl061->parent_irq)?

> +
> +static void pl061_irq_domain_unmap(struct irq_domain *d, unsigned int virq)
> +{
> +	irq_set_chip_and_handler(virq, NULL, NULL);
> +	irq_set_chip_data(virq, NULL);
> +}
> +
> +static const struct irq_domain_ops pl061_irq_domain_ops = {
> +	.map	= pl061_irq_domain_map,
> +	.unmap	= pl061_irq_domain_unmap,
> +	.xlate	= irq_domain_xlate_twothreecell,
> +};
> +
> +static void pl061_remove_irq(void *data)
> +{
> +	struct pl061 *pl061 = data;
> +
> +	irq_set_chained_handler_and_data(pl061->parent_irq, NULL, NULL);
> +
> +	for (unsigned int i = 0; i < pl061->data->ngpio; i++) {
> +		unsigned int virq;
> +
> +		virq = irq_find_mapping(pl061->irq_domain, i);
> +		if (virq)
> +			irq_dispose_mapping(virq);
> +	}
> +}

[ ... ]

> +	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_remove_irq, pl061);
> +		if (ret)
> +			return ret;
> +	}
>  
> -	ret = devm_gpiochip_add_data(dev, &pl061->gc, pl061);
> -	if (ret)
> -		return ret;
> +	config.parent = dev;
> +	config.regmap = pl061->regmap;
> +	config.ngpio = data->ngpio;
> +	config.reg_dat_base = GPIO_REGMAP_ADDR(regs->dat);
> +	config.reg_set_base = GPIO_REGMAP_ADDR(regs->dat);
> +	config.reg_dir_out_base = GPIO_REGMAP_ADDR(regs->dir);
> +	config.reg_mask_xlate = data->reg_mask_xlate;
> +	config.quirks = data->quirks;
> +	config.irq_domain = pl061->irq_domain;
> +	config.drvdata = pl061;
> +
> +	gpio_regmap = devm_gpio_regmap_register(dev, &config);
> +	if (IS_ERR(gpio_regmap))
> +		return PTR_ERR(gpio_regmap);
> +
> +	if (irq)
> +		irq_set_chained_handler_and_data(irq, pl061_irq_handler, pl061);

[Severity: Medium]
What happens in pl061_probe() if devm_gpio_regmap_register() fails (for
example with -EPROBE_DEFER when resolving pinctrl gpio-ranges or -ENOMEM)?

Because devm_add_action_or_reset(dev, pl061_remove_irq, pl061) is
registered before devm_gpio_regmap_register(), while
irq_set_chained_handler_and_data(irq, pl061_irq_handler, pl061) is only
called afterwards, a failure in devm_gpio_regmap_register() will run
pl061_remove_irq():

pl061_remove_irq()
  irq_set_chained_handler_and_data(pl061->parent_irq, NULL, NULL)
    __irq_do_set_handler(desc, NULL, 1, NULL)

In kernel/irq/chip.c:__irq_do_set_handler(), uninstalling a chained handler
that was never installed calls mask_ack_irq(desc) on the unstarted parent
IRQ, drops an unpaired irq_chip_pm_put() reference, sets desc->depth = 1,
and overwrites desc->handle_irq with handle_bad_irq.

Also, doesn't installing the chained handler after
devm_gpio_regmap_register() leave the parent chained handler active
throughout gpiochip_remove() and kfree(gpio) during devm teardown?

[Severity: High]
Furthermore, does switching from gpio_irq_chip to an external irq_domain in
devm_gpio_regmap_register() break ACPI GPIO-signaled events (_AEI) on ARM64
ACPI platforms using PL061 (ARMH0061)?

Previously, devm_gpiochip_add_data() called gpiochip_add_irqchip(), which
registered the domain and called acpi_gpiochip_request_interrupts(gc).

Now, gpio_regmap_register() calls gpiochip_add_data() while chip->irq.chip
is NULL (so gpiochip_add_irqchip() returns early), and then attaches
config.irq_domain via gpiochip_irqchip_add_domain():

gpio_regmap_register()
  gpiochip_add_data()
    gpiochip_add_irqchip() -> returns 0 early because !gc->irq.chip
  gpiochip_irqchip_add_domain()
    gpiochip_irqchip_add_allocated_domain()

Because gpiochip_irqchip_add_domain() never calls
acpi_gpiochip_request_interrupts(), ACPI _AEI interrupts (such as power
button events on SBSA / QEMU virt platforms) will no longer be requested.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-cv75-gpio-v11-0-e058bae19571@ambarella.com?part=8