Re: [PATCH v10 24/36] 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:
- [High] Hardcoded SCU compatible string in shared probe function prevents AN7583 and future SoCs from probing.

Pre-existing issues:
- [High] Parent IRQ requested before gpiochip initialization creates a race condition.
- [High] Redundant manual clearing of IRQ status bit leads to permanently lost edge interrupts.
- [High] Lock nesting violation in IRQ callbacks triggers "sleeping in atomic context" on RT kernels.
--

--- Patch [24]: [PATCH v10 24/36] pinctrl: airoha: split driver on shared code and SoC specific drivers ---
commit 5233d68e7af6a3826dd884bf3c42e656bc817865
Author: Mikhail Kshevetskiy <[email protected]>

pinctrl: airoha: split driver on shared code and SoC specific drivers

Split the combined an7581/an7583 source file into shared pinctrl code and
SoC-specific pinctrl drivers. The initialization code was also adapted to
work properly for both shared and SoC-specific cases.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 55a6efc5887bc..134c57a54b963 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
[ ... ]
> @@ -3070,6 +781,8 @@ static int airoha_pinctrl_probe(struct platform_device *pdev)
>  	int err, i;
>  
>  	data = device_get_match_data(dev);
> +	if (!data)
> +		return -EINVAL;
>  
>  	pinctrl = devm_kzalloc(dev, sizeof(*pinctrl), GFP_KERNEL);
>  	if (!pinctrl)
> @@ -3086,8 +799,8 @@ static int airoha_pinctrl_probe(struct platform_device *pdev)
>  	pinctrl->chip_scu = map;
>  
>  	/* Init pinctrl desc struct */
> -	pinctrl->desc.name = KBUILD_MODNAME;
> -	pinctrl->desc.owner = THIS_MODULE;
> +	pinctrl->desc.name = data->pinctrl_name;
> +	pinctrl->desc.owner = data->pinctrl_owner;

[Severity: High]
Does this shared probe function prevent other SoCs from probing due to a
hardcoded SCU compatible string?

In airoha_pinctrl_probe(), there appears to be a lookup that ignores the
SoC-specific match data:

	map = syscon_regmap_lookup_by_compatible("airoha,en7581-chip-scu");
	if (IS_ERR(map))
		return PTR_ERR(map);

If the SCU node for the newly added AN7583 uses a different compatible string
(such as "airoha,an7583-chip-scu"), this lookup will fail and return an error.
This would abort the probe and leave pinmux and GPIO functionality unavailable
for the new SoC.

[ ... ]
> @@ -3141,87 +854,10 @@ static int airoha_pinctrl_probe(struct platform_device *pdev)
>  	/* build gpio-chip */
>  	return airoha_pinctrl_add_gpiochip(pinctrl, pdev);
>  }

[Severity: High]
This is a pre-existing issue, but does requesting the parent IRQ before
gpiochip initialization create a race condition?

In airoha_pinctrl_add_gpiochip(), the IRQ is requested early:

	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);

If the shared IRQ line is asserted immediately by another device and the
Airoha hardware also has a pending interrupt, airoha_irq_handler() could
execute and call generic_handle_domain_irq(girq->domain, offset). Because
devm_gpiochip_add_data() hasn't run yet, girq->domain is still NULL, which
could cause a NULL pointer dereference or a silently dropped interrupt.

[Severity: High]
This is a pre-existing issue, but does the redundant manual clearing of the
IRQ status bit lead to permanently lost edge interrupts?

In airoha_irq_handler(), after servicing an interrupt, the status is cleared:

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

If a new edge interrupt arrives while the child handler is executing, the
hardware latches the new status. This manual write might blindly wipe the
newly latched status before it can be processed, meaning edge-triggered
interrupts could be silently cleared and lost forever under high interrupt
load.

[Severity: High]
This is a pre-existing issue, but could these regmap accesses violate lock
nesting rules in IRQ callbacks and trigger sleeping in atomic context panics
on RT kernels?

In airoha_irq_mask() and airoha_irq_unmask():

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

and

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

The generic IRQ core calls these functions while holding irq_desc->lock,
which is a raw_spinlock_t. Calling regmap_update_bits() and
regmap_clear_bits() on a syscon regmap internally takes a standard spinlock_t
or mutex. Acquiring a standard sleepable lock inside a raw spinlock protected
region is invalid. 

Should .irq_bus_lock and .irq_bus_sync_unlock be used to perform these regmap
operations outside the raw spinlock context?

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