Re: [PATCH v10 5/6] pinctrl: s32cc: implement GPIO functionality

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Removal of error checks for `s32_regmap_read()` causes uninitialized kernel stack memory to be saved during suspend and printed via debugfs.
- [High] The driver does not save and restore the PGPDO (Parallel GPIO Pad Data Out) registers across suspend and resume, causing output GPIOs to lose their state.
- [Medium] `PINCTRL_S32CC` selects `GPIO_REGMAP` without fulfilling its dependency on `GPIOLIB`, potentially causing build failures.
- [Medium] The virtual `s32-gpio` regmap forces GPIO operations to be sleepable, preventing usage from interrupt handlers.
--

commit 065b0eb13249298984a661238d8ddad54d7f31a4
Author: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
Subject: pinctrl: s32cc: implement GPIO functionality

The patch updates the S32CC SIUL2 driver to operate as a monolithic
pinctrl/GPIO driver. It adds GPIO data access and direction handling using
the gpio-regmap library backed by a virtual regmap. Support is also added
for both legacy pinctrl-only and extended bindings.

Link: https://lore.kernel.org/linux-gpio/20260120115923.3463866-4-khristineandreea.barbulescu@oss.nxp.com/T/#m543c9edbdde74bdc68b6a2364e8b975356c33043
Link: https://lore.kernel.org/all/20260504131148.3622697-7-khristineandreea.barbulescu@oss.nxp.com/

> diff --git a/drivers/pinctrl/nxp/Kconfig b/drivers/pinctrl/nxp/Kconfig
> index abca7ef97003d..59fc6adf5b0bb 100644
> --- a/drivers/pinctrl/nxp/Kconfig
> +++ b/drivers/pinctrl/nxp/Kconfig
> @@ -5,6 +5,7 @@ config PINCTRL_S32CC
>  	select GENERIC_PINCTRL_GROUPS
>  	select GENERIC_PINMUX_FUNCTIONS
>  	select GENERIC_PINCONF
> +	select GPIO_REGMAP
>  	select REGMAP_MMIO

[Severity: Medium]
Does PINCTRL_S32CC also need to select or depend on GPIOLIB here?

Since GPIO_REGMAP is enclosed in an "if GPIOLIB" block in drivers/gpio/Kconfig,
it implicitly depends on GPIOLIB. If a user disables GPIOLIB, Kconfig will
report an unmet direct dependency warning. 

Because drivers/gpio/Makefile is gated by CONFIG_GPIOLIB, gpio-regmap.c
won't compile in that case, potentially leading to linker errors for
devm_gpio_regmap_register() in the pinctrl driver.

> diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
> index 89a4eb2000ee7..8843926345ec8 100644
> --- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
> +++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
[ ... ]
> @@ -662,15 +717,13 @@ static void s32_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
>  	struct s32_pin_group *grp;
>  	unsigned int config;
>  	const char *name;
> -	int i, ret;
> +	int i;
>  
>  	seq_puts(s, "\n");
>  	grp = &info->groups[selector];
>  	for (i = 0; i < grp->data.npins; i++) {
>  		name = pin_get_name(pctldev, grp->data.pins[i]);
> -		ret = s32_regmap_read(pctldev, grp->data.pins[i], &config);
> -		if (ret)
> -			return;
> +		s32_regmap_read(pctldev, grp->data.pins[i], &config);
>  		seq_printf(s, "%s: 0x%x\n", name, config);
>  	}
>  }

[Severity: High]
Is it safe to remove the return value check for s32_regmap_read() here?

If s32_regmap_read() fails, it returns a negative error code and leaves
config unmodified. Because config is an uninitialized local variable, this
could leak kernel stack memory to userspace via seq_printf().

[ ... ]
> +static const struct regmap_config s32_gpio_regmap_config = {
> +	.name = "s32-gpio",
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 1,
> +	.max_register = S32_GPIO_OP_SET | S32_GPIO_BANK_MASK | S32_GPIO_REG_MASK,
> +	.cache_type = REGCACHE_NONE,
> +};

[Severity: Medium]
Should this virtual regmap configuration specify .disable_locking = true?

Without .fast_io = true or .disable_locking = true, devm_regmap_init()
protects the map with a mutex. When gpio-regmap registers this, it detects
the mutex and unconditionally sets chip->can_sleep = true.

Consumers typically expect an on-SoC MMIO GPIO controller to be fast and
non-sleeping for use in interrupt contexts. Using a mutex breaks this
assumption and might trigger might_sleep() warnings.

[ ... ]
> @@ -710,7 +1207,6 @@ int s32_pinctrl_suspend(struct device *dev)
>  	const struct s32_pinctrl_soc_info *info = ipctl->info;
>  	struct s32_pinctrl_context *saved_context = &ipctl->saved_context;
>  	int i;
> -	int ret;
>  	unsigned int config;
>  
>  	for (i = 0; i < info->soc_data->npins; i++) {
> @@ -719,9 +1215,7 @@ int s32_pinctrl_suspend(struct device *dev)
>  		if (!s32_pinctrl_should_save(ipctl, pin->number))
>  			continue;
>  
> -		ret = s32_regmap_read(ipctl->pctl, pin->number, &config);
> -		if (ret)
> -			return -EINVAL;
> +		s32_regmap_read(ipctl->pctl, pin->number, &config);
>  
>  		saved_context->pads[i] = config;
>  	}

[Severity: High]
Can this save uninitialized stack garbage into the suspend context?

By removing the error check, if s32_regmap_read() fails, the uninitialized
config variable is stored in saved_context->pads[i] and will be written
back to the hardware during system resume.

[Severity: High]
Does this suspend/resume implementation also need to save and restore the
PGPDO (Parallel GPIO Pad Data Out) registers?

Currently, only the MSCR configuration (direction/pull state) via
mem_pin_ranges appears to be saved. If power is lost during suspend, the
PGPDO registers will reset to 0. Any GPIO configured as an output and driven
HIGH before suspend might glitch LOW upon resume since its output line
value is lost.

> @@ -736,7 +1230,7 @@ int s32_pinctrl_resume(struct device *dev)
>  	const struct s32_pinctrl_soc_info *info = ipctl->info;
>  	const struct pinctrl_pin_desc *pin;
>  	struct s32_pinctrl_context *saved_context = &ipctl->saved_context;
> -	int ret, i;
> +	int i;
>  
>  	for (i = 0; i < info->soc_data->npins; i++) {
>  		pin = &info->soc_data->pins[i];
> @@ -744,10 +1238,8 @@ int s32_pinctrl_resume(struct device *dev)
>  		if (!s32_pinctrl_should_save(ipctl, pin->number))
>  			continue;
>  
> -		ret = s32_regmap_write(ipctl->pctl, pin->number,
> -					 saved_context->pads[i]);
> -		if (ret)
> -			return ret;
> +		s32_regmap_write(ipctl->pctl, pin->number,
> +				 saved_context->pads[i]);
>  	}
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602080132.3256239-1-khristineandreea.barbulescu@oss.nxp.com?part=5