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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The GPIO output data registers (PGPDO) are not saved or restored during system suspend/resume, which will cause active GPIO outputs to glitch or lose their state upon resume.
- [High] The pinctrl device is exposed to the system via `pinctrl_enable()` before the newly added GPIO initialization is complete, creating a race condition if probe fails.
--

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

The commit implements GPIO data access and direction handling using the
gpio-regmap library backed by a virtual regmap. It translates the
gpio-regmap register model to the underlying SIUL2 registers (MSCR,
PGPDI, PGPDO) and uses existing pinctrl GPIO callbacks for request/free
paths. Both legacy and extended DT layouts are supported.

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/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
> index b9b757e28bff4..35e2f8a18ef23 100644
> --- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
> +++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c

[ ... ]

> @@ -721,7 +1249,7 @@ int s32_pinctrl_suspend(struct device *dev)
>  
>  		ret = s32_regmap_read(ipctl->pctl, pin->number, &config);
>  		if (ret)
> -			return -EINVAL;
> +			return ret;
>  
>  		saved_context->pads[i] = config;
>  	}

[Severity: High]
Does this code miss saving the GPIO output data registers (PGPDO)?

Currently, only the MSCR pad configuration registers are saved. When the
system enters deep sleep and the SIUL2 hardware block loses context, the
GPIO output states would be lost.

[ ... ]

> @@ -745,7 +1273,7 @@ int s32_pinctrl_resume(struct device *dev)
>  			continue;
>  
>  		ret = s32_regmap_write(ipctl->pctl, pin->number,
> -					 saved_context->pads[i]);
> +				 saved_context->pads[i]);
>  		if (ret)
>  			return ret;
>  	}

[Severity: High]
Will restoring the pad configurations without restoring the PGPDO registers
cause active GPIO outputs to glitch?

Since restoring the MSCR registers enables the output buffer, but the PGPDO
registers were not restored, GPIOs configured as outputs might drive a logic
level of 0 upon resume regardless of their previous state. This could
potentially break external peripherals.

[ ... ]

> @@ -995,7 +1535,42 @@ int s32_pinctrl_probe(struct platform_device *pdev,
>  		return dev_err_probe(&pdev->dev, ret,
>  				     "Failed to enable pinctrl\n");

[Severity: High]
Could exposing the pinctrl device before completing GPIO initialization cause
a use-after-free race condition if probe fails?

pinctrl_enable() makes the pinctrl device globally visible in pinctrldev_list.
If a concurrent consumer acquires a reference to it, but a subsequent GPIO
initialization step fails:

> -	dev_info(&pdev->dev, "Initialized S32 pinctrl driver\n");
> +	/* Setup GPIO if GPIO ranges are defined */
> +	if (!soc_data->gpio_ranges || !soc_data->num_gpio_ranges)
> +		return 0;
> +
> +	ret = s32_gpio_get_ngpio(soc_data, &ngpio);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Invalid GPIO ranges\n");

[ ... ]

> +	ipctl->gpio_rgm = devm_gpio_regmap_register(&pdev->dev, &gpio_cfg);
> +	if (IS_ERR(ipctl->gpio_rgm))
> +		return dev_err_probe(&pdev->dev,
> +				     PTR_ERR(ipctl->gpio_rgm),
> +				     "Unable to add gpio_regmap chip\n");

Could we be left with a dangling pointer when devres cleans up the unregistered
pinctrl device on the error return path?

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