From: sashiko-bot@kernel.org
To: "Long Zhao" <longzhao@ambarella.com>, linux-gpio@vger.kernel.org
Cc: devicetree@vger.kernel.org, robh@kernel.org, conor+dt@kernel.org
Subject: Re: [PATCH v11 8/9] gpio: pl061: switch data path to gpio-regmap
Date: Tue, 20 Oct 2026 09:54:36 +0000 [thread overview]
Message-ID: <sashiko-outbox-178772@kernel.org> (raw)
In-Reply-To: <20261020-cv75-gpio-v11-8-e058bae19571@ambarella.com>
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
next prev parent reply other threads:[~2026-10-20 9:54 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-20 7:54 [PATCH v11 0/9] gpio: add Ambarella CV75 PL061 support Long Zhao via B4 Relay
2026-10-20 7:54 ` [PATCH v11 1/9] dt-bindings: gpio: pl061: add Ambarella CV75 variant Long Zhao via B4 Relay
2026-10-20 7:54 ` [PATCH v11 2/9] gpiolib: regmap: add GPIO_REGMAP_QUIRK_SET_AFTER_DIR Long Zhao via B4 Relay
2026-10-20 8:57 ` sashiko-bot
2026-10-20 7:54 ` [PATCH v11 3/9] gpio: pl061: drop redundant pending check in the IRQ handler Long Zhao via B4 Relay
2026-10-20 7:54 ` [PATCH v11 4/9] gpio: pl061: convert register access to regmap Long Zhao via B4 Relay
2026-10-20 7:54 ` [PATCH v11 5/9] gpio: pl061: use IRQ_TYPE_LEVEL_MASK and IRQ_TYPE_EDGE_BOTH Long Zhao via B4 Relay
2026-10-20 7:54 ` [PATCH v11 6/9] gpio: pl061: use cleanup helpers for locks Long Zhao via B4 Relay
2026-10-20 7:54 ` [PATCH v11 7/9] gpio: pl061: keep the parent device in a local Long Zhao via B4 Relay
2026-10-20 7:54 ` [PATCH v11 8/9] gpio: pl061: switch data path to gpio-regmap Long Zhao via B4 Relay
2026-10-20 9:54 ` sashiko-bot [this message]
2026-10-20 7:54 ` [PATCH v11 9/9] gpio: pl061: add Ambarella register-layout variant Long Zhao via B4 Relay
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=sashiko-outbox-178772@kernel.org \
--to=sashiko-bot@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=longzhao@ambarella.com \
--cc=robh@kernel.org \
--cc=sashiko-reviews@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox