Re: [PATCH 3/5] pinctrl: samsung: Add Exynos8855 pinctrl configuration

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Assigning `.eint_gpio_init = exynos_eint_gpio_init` for the ALIVE and CMGP pin controllers is incorrect and will trigger an unconditional `dev_err` on boot.

Pre-existing issues:
- [High] Out-of-bounds shift and integer truncation in `exynos_wkup_irq_set_wake` due to sparse `eint_offset` values in Exynos8855 pin banks.
--

commit 44d88e7478f7688359e44e48fa60a820ac3e8dfa
Author: Alim Akhtar <alim.akhtar@samsung.com>

pinctrl: samsung: Add Exynos8855 pinctrl configuration

Add pinctrl configuration for Exynos8855. The bank type macros are
reused from Exynos850 SoC.

> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> index fe9f92cb037ea..ba2a11f8e19d8 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c

[ ... ]

> +/* pin banks of exynos8855 pin-controller 1 (CMGP) */
> +static const struct samsung_pin_bank_data exynos8855_pin_banks1[] __initconst = {
> +	EXYNOS850_PIN_BANK_EINTW(1, 0x00,  "gpm0",  0x00),

[ ... ]

> +	EXYNOS850_PIN_BANK_EINTW(1, 0x280, "gpm20", 0x50),
> +	EXYNOS850_PIN_BANK_EINTW(1, 0x2a0, "gpm21", 0x54),
> +};

[Severity: High]
This is a pre-existing issue, but does the configuration of these eint_offset
values (up to 0x54) expose an out-of-bounds shift issue?

Looking at exynos_wkup_irq_set_wake() in
drivers/pinctrl/samsung/pinctrl-exynos.c, it sets wake mask bits using:

    unsigned long bit = 1UL << (2 * bank->eint_offset + irqd->hwirq);

If eint_offset is 0x54, the shift amount becomes 168, which invokes C
undefined behavior. Even for shifts between 32 and 63, the 64-bit value is
bitwise OR'd into a u32, dropping the upper bits and breaking the hardware
wakeup masks for these pins.

Will this logic need updating to support these sparse CMGP banks?

[ ... ]

> +static const struct samsung_pin_ctrl exynos8855_pin_ctrl[] __initconst = {
> +	{
> +		/* pin-controller instance 0 ALIVE data */
> +		.pin_banks	= exynos8855_pin_banks0,
> +		.nr_banks	= ARRAY_SIZE(exynos8855_pin_banks0),
> +		.eint_wkup_init = exynos_eint_wkup_init,
> +		.eint_gpio_init = exynos_eint_gpio_init,
> +	}, {
> +		/* pin-controller instance 1 CMGP data */
> +		.pin_banks	= exynos8855_pin_banks1,
> +		.nr_banks	= ARRAY_SIZE(exynos8855_pin_banks1),
> +		.eint_wkup_init = exynos_eint_wkup_init,
> +		.eint_gpio_init = exynos_eint_gpio_init,
> +	}, {

[Severity: Medium]
Is it correct to assign exynos_eint_gpio_init to the eint_gpio_init
callback for the ALIVE and CMGP controllers?

Because these controllers only contain EINT_TYPE_WKUP and EINT_TYPE_NONE
banks, they do not support standard GPIO interrupts and typically lack an
interrupts property in the device tree.

When exynos_eint_gpio_init runs and sees no irq, won't it unconditionally
log an error during boot:

    dev_err(dev, "irq number not available\n");
    
and return -EINVAL? Does this initialization need to be omitted here,
similar to how it is handled in exynos850_pin_ctrl?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612163020.411761-1-alim.akhtar@samsung.com?part=3