Re: [PATCH v8 31/35] pinctrl: airoha: add support of en7523 SoC

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing pinconf mappings for dedicated interface pins (2-9) and incorrect register mappings for PCIe reset pins (40-41).

Pre-existing issues:
- [High] The IRQ handler remains active after the GPIO chip is removed, leading to a Use-After-Free of `gc->irq.domain`.
--

--- Patch [31]: [PATCH v8 31/35] pinctrl: airoha: add support of en7523 SoC ---
commit e474b38e912b2b107b5f734fda40d3805c427084
Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>

pinctrl: airoha: add support of en7523 SoC

This patch adds support of Airoha en7523 SoC pin controller.

> diff --git a/drivers/pinctrl/airoha/pinctrl-en7523.c b/drivers/pinctrl/airoha/pinctrl-en7523.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pinctrl/airoha/pinctrl-en7523.c
[ ... ]
> +#define REG_I2C_SDA_PU				0x0044
> +#define SPI_MISO_PU_MASK			BIT(13)
> +#define SPI_MOSI_PU_MASK			BIT(12)
> +#define SPI_CLK_PU_MASK				BIT(11)
> +#define SPI_CS0_PU_MASK				BIT(10)
> +#define PCIE1_RESET_PU_MASK			BIT(9)
> +#define PCIE0_RESET_PU_MASK			BIT(8)
> +#define UART1_RXD_PU_MASK			BIT(3)
> +#define UART1_TXD_PU_MASK			BIT(2)
> +#define I2C_SCL_PU_MASK				BIT(1)
> +#define I2C_SDA_PU_MASK				BIT(0)
[ ... ]
> +static const struct airoha_pinctrl_conf pinctrl_pullup_conf[] = {
> +	PINCTRL_CONF_DESC(12, REG_GPIO_L_PU, BIT(0)),
> +	PINCTRL_CONF_DESC(13, REG_GPIO_L_PU, BIT(1)),
[ ... ]
> +	PINCTRL_CONF_DESC(39, REG_GPIO_L_PU, BIT(27)),
> +	PINCTRL_CONF_DESC(40, REG_GPIO_L_PU, BIT(28)),
> +	PINCTRL_CONF_DESC(41, REG_GPIO_L_PU, BIT(29)),
> +};

[Severity: High]
Are the pinconf mappings for pins 2 through 9 missing from this array?

The pinctrl_pullup_conf array (as well as the pulldown and drive arrays)
starts at pin 12, which seems to omit the electrical configuration for the 
I2C, SPI, and UART pins defined earlier. If a device tree attempts to 
configure pull-up/pull-down or drive strength for these buses, will it fail?

Also, are pins 40 and 41 (pcie_reset0 and pcie_reset1) mapped to the wrong 
register here? They are currently mapped to REG_GPIO_L_PU bits 28 and 29, 
but the macro definitions above show PCIE0_RESET_PU_MASK and 
PCIE1_RESET_PU_MASK belonging to REG_I2C_SDA_PU instead. Will configuring 
the PCIe reset pins silently corrupt the GPIO pad configuration bits?

> drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_pinctrl_add_gpiochip()
>
> 	err = devm_request_irq(dev, irq, airoha_irq_handler, IRQF_SHARED,
> 				dev_name(dev), pinctrl);
> 	if (err) {
> 		dev_err(dev, "error requesting irq %d: %d\n", irq, err);
> 		return err;
> 	}
> 
> 	return devm_gpiochip_add_data(dev, gc, pinctrl);

[Severity: High]
This isn't a bug introduced by this patch, but does the devres registration
order in airoha_pinctrl_add_gpiochip() create a use-after-free risk during
teardown?

If devm_gpiochip_add_data() fails, or when the driver is unbound, devres
unwinds in reverse order. This means devm_gpiochip_add_data() is cleaned
up first, which destroys gc->irq.domain. 

Since devm_request_irq() was called earlier, the shared IRQ handler
remains registered after the domain is destroyed. If a shared interrupt
fires before the IRQ is finally freed, will airoha_irq_handler() execute
and access the freed girq->domain via generic_handle_domain_irq()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727174141.664933-1-mikhail.kshevetskiy@iopsys.eu?part=31