Re: [PATCH v2 4/4] pinctrl: qcom: spmi-gpio: Add level-shifter function support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Writing the software-defined `LEVEL_SHIFTER` function index to hardware registers causes register corruption.
- [High] Out-of-bounds array access when displaying pinconf debugfs for a level-shifter pin.
- [High] Use-After-Free of level shifter resources during driver unbind due to reversed devm initialization order.
- [High] Failure to enforce mutual exclusivity between the bidirectional level shifter and physical GPIO functions.
- [Medium] Missing GET support for the newly added `qcom,1p2v-1p8v-ls-en` pin configuration.
--

commit febef2022fa1d171b552bf56874bdc04c2cb463b
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

pinctrl: qcom: spmi-gpio: Add level-shifter function support

This commit introduces bidirectional level shifter support for open-drain
signals. It adds a level-shifter function to the SPMI GPIO driver for specific
GPIO pairs managed via RPMh.

> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> index 268cfae706a82..741795ae261ef 100644
> --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> @@ -137,6 +144,7 @@ enum pmic_gpio_func_index {
>  	PMIC_GPIO_FUNC_INDEX_DTEST2,
>  	PMIC_GPIO_FUNC_INDEX_DTEST3,
>  	PMIC_GPIO_FUNC_INDEX_DTEST4,
> +	PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER,
>  };

[ ... ]

> @@ -183,25 +191,86 @@ struct pmic_gpio_state {
>  	{"qcom,analog-pass",		PMIC_GPIO_CONF_ANALOG_PASS,	0},
> -	{"qcom,dtest-buffer",           PMIC_GPIO_CONF_DTEST_BUFFER,    0},
> +	{"qcom,dtest-buffer",		PMIC_GPIO_CONF_DTEST_BUFFER,	0},
> +	{"qcom,1p2v-1p8v-ls-en",	PMIC_GPIO_CONF_LS_ENABLE,	0},
>  };

[Severity: Medium]
Since this commit adds the PMIC_GPIO_CONF_LS_ENABLE property and handles it
in pmic_gpio_pinconf_group_set(), does it also need corresponding read
support?

The driver uses generic pinconf dumps but lacks a pin_config_group_get
implementation, and pmic_gpio_pinconf_pin_get() lacks a case for the new
parameter. This appears to break the symmetry of the pinctrl API, which
might cause programmatic reads of this configuration to fail with -EINVAL.

[ ... ]

> @@ -214,16 +283,16 @@ static const char *const pmic_gpio_groups[] = {
>  };
>  
>  static const char *const pmic_gpio_functions[] = {
> -	[PMIC_GPIO_FUNC_INDEX_NORMAL]	= PMIC_GPIO_FUNC_NORMAL,
> -	[PMIC_GPIO_FUNC_INDEX_PAIRED]	= PMIC_GPIO_FUNC_PAIRED,
> -	[PMIC_GPIO_FUNC_INDEX_FUNC1]	= PMIC_GPIO_FUNC_FUNC1,
> -	[PMIC_GPIO_FUNC_INDEX_FUNC2]	= PMIC_GPIO_FUNC_FUNC2,
> -	[PMIC_GPIO_FUNC_INDEX_FUNC3]	= PMIC_GPIO_FUNC_FUNC3,
> -	[PMIC_GPIO_FUNC_INDEX_FUNC4]	= PMIC_GPIO_FUNC_FUNC4,
> -	[PMIC_GPIO_FUNC_INDEX_DTEST1]	= PMIC_GPIO_FUNC_DTEST1,
> -	[PMIC_GPIO_FUNC_INDEX_DTEST2]	= PMIC_GPIO_FUNC_DTEST2,
> -	[PMIC_GPIO_FUNC_INDEX_DTEST3]	= PMIC_GPIO_FUNC_DTEST3,
> -	[PMIC_GPIO_FUNC_INDEX_DTEST4]	= PMIC_GPIO_FUNC_DTEST4,
> +	[PMIC_GPIO_FUNC_INDEX_NORMAL]		= PMIC_GPIO_FUNC_NORMAL,
> +	[PMIC_GPIO_FUNC_INDEX_PAIRED]		= PMIC_GPIO_FUNC_PAIRED,
> +	[PMIC_GPIO_FUNC_INDEX_FUNC1]		= PMIC_GPIO_FUNC_FUNC1,
> +	[PMIC_GPIO_FUNC_INDEX_FUNC2]		= PMIC_GPIO_FUNC_FUNC2,
> +	[PMIC_GPIO_FUNC_INDEX_FUNC3]		= PMIC_GPIO_FUNC_FUNC3,
> +	[PMIC_GPIO_FUNC_INDEX_FUNC4]		= PMIC_GPIO_FUNC_FUNC4,
> +	[PMIC_GPIO_FUNC_INDEX_DTEST1]		= PMIC_GPIO_FUNC_DTEST1,
> +	[PMIC_GPIO_FUNC_INDEX_DTEST2]		= PMIC_GPIO_FUNC_DTEST2,
> +	[PMIC_GPIO_FUNC_INDEX_DTEST3]		= PMIC_GPIO_FUNC_DTEST3,
> +	[PMIC_GPIO_FUNC_INDEX_DTEST4]		= PMIC_GPIO_FUNC_DTEST4,
>  };

[Severity: High]
Should PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER be added to this array?

If pmic_gpio_pinconf_pin_dbg_show() formats the status of a level-shifter
pin, it dereferences pmic_gpio_functions[function]. For a level-shifter
pin, this would be index 10 (or potentially 12 for non-LV/MV pins), while
the array only has 10 elements.

Could this cause an out-of-bounds memory access that crashes the kernel when
reading pinctrl debugfs?

[ ... ]

> @@ -513,7 +639,8 @@ static int pmic_gpio_pinconf_pin_set(struct pinctrl_dev *pctldev,
>  
>  	pad = pctldev->desc->pins[pin].drv_data;
>  
> -	pad->is_enabled = true;
> +	if (pad->function != PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER)
> +		pad->is_enabled = true;

[Severity: High]
The commit message states that the level shifter must be mutually exclusive
with all existing functions and configurations. However, does this code
enforce that exclusivity?

If a device tree simultaneously assigns a normal function to a pin and
enables the level shifter on its associated group, pad->function will be
NORMAL.

Since the check above will pass, it will set pad->is_enabled = true,
potentially causing both the physical GPIO output buffer and the external
level shifter to drive the pin concurrently. Could this result in
electrical overstress?

[Severity: High]
Is there a risk of corrupting hardware registers with the LEVEL_SHIFTER
index?

When setting PMIC_GPIO_REG_MODE_CTL for non-LV/MV pins later in
pmic_gpio_pinconf_pin_set(), the driver does this:

        val = val << PMIC_GPIO_REG_MODE_DIR_SHIFT;
        val |= pad->function << PMIC_GPIO_REG_MODE_FUNCTION_SHIFT;

If pad->function is PMIC_GPIO_FUNC_INDEX_LEVEL_SHIFTER (10, 0b1010),
shifting it by 1 results in 20 (0x14). Since the hardware function field is
only 3 bits, this overflows and overwrites bit 4
(PMIC_GPIO_REG_MODE_DIR_SHIFT).

Since LEVEL_SHIFTER is a software-defined pseudo-function that physically
operates by keeping the pad disabled, should the driver explicitly bypass
hardware register writes for level-shifter pins?

[ ... ]

> @@ -1285,6 +1545,15 @@ static int pmic_gpio_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	/* Register level-shifter groups and function if hardware and DT both opt in */
> +	if (hw_data->ls_config && hw_data->num_ls &&
> +	    of_property_present(dev->of_node, "qcom,rpmh") &&
> +	    of_property_present(dev->of_node, "qcom,pmic-id")) {
> +		ret = pmic_gpio_register_level_shifters(state, hw_data);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: High]
Will this devres allocation order cause a Use-After-Free during driver
unbind?

Earlier in pmic_gpio_probe(), devm_pinctrl_register_and_init() is called to
register the pinctrl device. Then, pmic_gpio_register_level_shifters()
allocates the level shifter resources (ls_group_data, ls_group_names) using
devm_kcalloc and devm_kzalloc.

Because devres releases resources in LIFO order, the level shifter memory
will be freed before the pinctrl device is unregistered. Does this leave a
window where the pinctrl device remains active and exposed to userspace
(e.g. sysfs/debugfs) while its registered groups and functions point to
freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528-pinctrl-level-shifter-v2-0-3a6a025392bf@oss.qualcomm.com?part=4