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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The CMGP pin controller (instance 1) is missing the `.eint_wkup_init` callback, but its pin banks are configured for wakeup interrupts (`EINT_TYPE_WKUP`).
- [Low] The ALIVE and CMGP controllers erroneously configure `eint_gpio_init`, triggering spurious boot errors.

Pre-existing issues:
- [High] Calculating the PMU wakeup mask using the newly introduced, extremely sparse `eint_offset` values triggers C undefined behavior by shifting beyond 63 bits.
--

commit 9e3fddb96151cd82aa6084b7afbcf1a84a9843ef
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..db120ae4d847a 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
[ ... ]
> +/* pin banks of exynos8855 pin-controller 0 (ALIVE) */
> +static const struct samsung_pin_bank_data exynos8855_pin_banks0[] __initconst = {
> +	/* Must start with EINTG banks, ordered by EINT group number. */
> +	EXYNOS850_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00),
[ ... ]
> +	EXYNOS850_PIN_BANK_EINTW(1, 0x260, "gpj0", 0x4c),
> +	EXYNOS850_PIN_BANK_EINTW(1, 0x280, "gpj1", 0x50),
> +	EXYNOS850_PIN_BANK_EINTW(1, 0x2a0, "gpj2", 0x54),

[Severity: High]
This isn't a bug introduced by this patch, but the new Exynos8855 pin
definitions expose it. The new pin layout introduces extremely sparse
eint_offset values up to 0x54 (like gpj2 here).

In exynos_wkup_irq_set_wake(), the wake mask bit is calculated as:

unsigned long bit = 1UL << (2 * bank->eint_offset + irqd->hwirq);
...
eint_wake_mask_values[0] |= bit;

When computing the shift for an offset of 0x54 (84), the expression evaluates
to 1UL << 168. Since 1UL is 64 bits wide on ARM64, this left shift invokes
C undefined behavior.

Even ignoring the undefined behavior, the result is implicitly cast to u32
when OR'd into eint_wake_mask_values[0], effectively truncating it to 0.

Does this completely break PMU wakeup configuration for all pins with an
eint_offset >= 0x10?

> +};
[ ... ]
> +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,
> +	}, {

[Severity: Low]
Does the ALIVE controller need to configure eint_gpio_init?

Neither the ALIVE nor the CMGP instances contain any EINT_TYPE_GPIO banks
(they only contain EINT_TYPE_WKUP and EINT_TYPE_NONE). Because wake-up only
controllers typically do not have a generic hardware IRQ mapped for standard
GPIOs in the device tree, exynos_eint_gpio_init unconditionally attempts to
retrieve it, prints a spurious error, and returns -EINVAL:

dev_err(dev, "irq number not available\n");

Could this cause unnecessary boot log pollution and attempts to initialize
GPIO IRQ domains for zero eligible banks?

> +		/* pin-controller instance 1 CMGP data */
> +		.pin_banks	= exynos8855_pin_banks1,
> +		.nr_banks	= ARRAY_SIZE(exynos8855_pin_banks1),
> +		.eint_gpio_init = exynos_eint_gpio_init,
> +	}, {

[Severity: High]
Is the .eint_wkup_init callback missing here for the CMGP pin controller?

The CMGP pin controller banks (exynos8855_pin_banks1) are exclusively defined
using the EXYNOS850_PIN_BANK_EINTW macro, which sets .eint_type to
EINT_TYPE_WKUP. However, this instance only configures .eint_gpio_init and
omits .eint_wkup_init.

During initialization, exynos_eint_gpio_init() skips any banks that are not
EINT_TYPE_GPIO. Because .eint_wkup_init is missing, it looks like the wakeup
interrupts for gpm0 through gpm21 will never be registered, leaving them
without functional external wakeup interrupts.

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