Re: [PATCH v11 25/37] pinctrl: airoha: split driver on shared code and SoC specific drivers

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] Undocumented Feature Additions / Silent Fixes

Pre-existing issues:
- [High] Race Condition leading to Use-After-Free during teardown
- [High] Race Condition in IRQ handler dropping edge interrupts
- [High] Lock nesting violation on PREEMPT_RT due to syscon regmap operations in atomic context
--

--- Patch [25]: [PATCH v11 25/37] pinctrl: airoha: split driver on shared code and SoC specific drivers ---
commit 43427b8e37815dccb451d5575c676158386beaf8
Author: Mikhail Kshevetskiy <[email protected]>
pinctrl: airoha: split driver on shared code and SoC specific drivers

This patch refactors the Airoha pinctrl driver by splitting the combined
source file into shared common code and SoC-specific drivers. It extracts
the pin controller and GPIO logic into a common file and creates separate
driver files for the AN7581 and AN7583.

> diff --git a/drivers/pinctrl/airoha/pinctrl-an7583.c b/drivers/pinctrl/airoha/pinctrl-an7583.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pinctrl/airoha/pinctrl-an7583.c
> @@ -0,0 +1,1476 @@
> +// SPDX-License-Identifier: GPL-2.0-only
[ ... ]
> +static const int pon_pins[] = { 15, 16, 17, 18, 19, 20 };
> +static const int pon_alt_pins[] = { 36, 37, 38, 39, 40 };
> +static const int olt_pins[] = { 36, 37, 38, 39, 40 };
> +static const int pon_tod_1pps_pins[] = { 32 };

[Severity: Medium]
The commit message describes this patch purely as a structural split of the
combined driver into shared and SoC-specific parts. However, this appears
to add new functional features like the pon_alt and olt pin groups for
AN7583.

Could the commit message be updated to mention these functional additions,
or should they be split into a separate feature patch?

[Severity: High]
This is a pre-existing issue, but while looking at the initialization
flow in airoha_pinctrl_add_gpiochip() in
drivers/pinctrl/airoha/pinctrl-airoha.c:

	err = devm_request_irq(dev, irq, airoha_irq_handler, IRQF_SHARED,
				dev_name(dev), pinctrl);
	if (err) {
		...
	}

	return devm_gpiochip_add_data(dev, gc, pinctrl);

Does requesting the shared IRQ before adding the gpiochip create an
inverted cleanup order?

Since devres cleans up in reverse, the gpiochip (and its irq_domain) would
be freed before the IRQ is released.

If the shared interrupt fires during this window, could the handler
dereference a freed irq_domain pointer?

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition
in drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_irq_handler():

	generic_handle_domain_irq(girq->domain, offset);
	regmap_write(pinctrl->regmap,
		     pinctrl->gpio_regs->status[i], BIT(irq));

Since the interrupt status was already acknowledged inside the flow
handler before generic_handle_domain_irq() returns, does this second
unconditional regmap_write() risk silently clearing newly latched edge
interrupts that arrived in the meantime?

[Severity: High]
This isn't a bug introduced by this patch, but I observed a potential
locking violation on PREEMPT_RT systems in the irqchip callbacks
like airoha_irq_mask() and airoha_irq_ack() in pinctrl-airoha.c:

	regmap_clear_bits(pinctrl->regmap, gpio_regs->level[index], mask);

Since these callbacks execute with the raw_spinlock_t irq_desc->lock held,
and the syscon MMIO regmap operations use a sleepable spinlock_t, will
this cause a lock nesting violation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=25