Re: [PATCH v3 4/4] pinctrl: rockchip: Add RV1106 pinctrl support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Hardware register aliasing in RV1106 Bank 0 due to incorrect pin count and missing bounds checks.

Pre-existing issues:
- [Critical] State corruption and out-of-bounds access due to static `rockchip_pin_ctrl` mutation on probe deferral.
--

--- Patch [4]: [PATCH v3 4/4] pinctrl: rockchip: Add RV1106 pinctrl support ---
commit 5db525b718839289847048c3ede5984a9fb138b8
Author: Simon Glass <sjg@chromium.org>

pinctrl: rockchip: Add RV1106 pinctrl support

Add pinctrl support for the Rockchip RV1106. Each GPIO bank has its own
IO control (IOC) register block, referenced by the rockchip,grf phandle
of the bank node; the register offsets are relative to the bank's own block.

Link: https://github.com/LuckfoxTECH/luckfox-pico

> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index 08a46a04a8157..4e5e4434ba4e7 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c

[ ... ]

> @@ -4505,6 +4594,44 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
>  	/* try to find the optional reference to the ioc1 syscon */
>  	info->regmap_ioc1 = syscon_regmap_lookup_by_phandle_optional(np, "rockchip,ioc1");
>  
> +	/*
> +	 * On SoCs where each GPIO bank has its own IOC block, the bank nodes
> +	 * carry a rockchip,grf phandle pointing at it. The bank number comes
> +	 * from the gpio alias, as used by the gpio driver, falling back to
> +	 * the node position for devicetrees without aliases. The fallback is
> +	 * wrong when an SoC variant omits a bank, so aliases are needed
> +	 * there.
> +	 */
> +	i = 0;
> +	for_each_child_of_node_scoped(np, child) {
> +		struct rockchip_pin_bank *bank = NULL;
> +		int id, j;

[ ... ]

> +		bank->regmap_ioc = syscon_regmap_lookup_by_phandle_optional(
> +							child, "rockchip,grf");
> +		if (IS_ERR(bank->regmap_ioc))
> +			return dev_err_probe(dev, PTR_ERR(bank->regmap_ioc),
> +					     "%pOFn: failed to look up bank ioc\n",
> +					     child);
> +	}

[Severity: Critical]
This is a pre-existing issue, but does this new syscon lookup reliably
trigger state corruption across probe deferrals?

Looking at rockchip_pinctrl_get_soc_data() called earlier in probe, it
mutates the static rockchip_pin_ctrl structure:

drivers/pinctrl/pinctrl-rockchip.c:rockchip_pinctrl_get_soc_data() {
    ...
    bank->pin_base = ctrl->nr_pins;
    ctrl->nr_pins += bank->nr_pins;
    ...
}

If syscon_regmap_lookup_by_phandle_optional() returns -EPROBE_DEFER, the
probe fails without resetting this static state. On subsequent probes,
ctrl->nr_pins accumulates, inflating bank->pin_base.

When a pin is later accessed, does this inflated pin_base cause
pin_to_bank() to return the wrong bank, leading to a large negative
integer for the pin?

drivers/pinctrl/pinctrl-rockchip.c:pin_to_bank() {
    ...
    while (pin >= (b->pin_base + b->nr_pins))
        b++;
    return b;
}

Would this negative pin value act as a negative array index for
bank->iomux[iomux_num], reading garbage offsets and corrupting arbitrary
registers?

> +
>  	ret = rockchip_pinctrl_register(pdev, info);
>  	if (ret)
>  		return ret;

[ ... ]

> @@ -4623,6 +4750,69 @@ static struct rockchip_pin_ctrl rv1103b_pin_ctrl __maybe_unused = {
>  	.schmitt_calc_reg	= rv1103b_calc_schmitt_reg_and_bit,
>  };
>  
> +static struct rockchip_pin_bank rv1106_pin_banks[] = {
> +	PIN_BANK_IOMUX_FLAGS_OFFSET_DRV_FLAGS(0, 32, "gpio0",
> +				    IOMUX_WIDTH_4BIT,
> +				    IOMUX_WIDTH_4BIT,

[Severity: High]
Since only pins 0-6 of GPIO0 have drive-strength registers, does declaring
bank 0 with 32 pins expose hardware register aliasing?

Without bounds checking for higher pins, if pin 16 is accessed, the offset
calculation in rv1106_calc_pull_reg_and_bit() evaluates to 0x40:

drivers/pinctrl/pinctrl-rockchip.c:rv1106_calc_pull_reg_and_bit() {
    ...
    *reg = rv1106_pull_offsets[bank->bank_num];
    *reg += ((pin_num / RV1106_PULL_PINS_PER_REG) * 4);
    ...
}

Because rv1106_smt_offsets[0] is also 0x40, wouldn't setting the pull
configuration for pin 16 silently overwrite the schmitt trigger registers
for pins 0-7?

> +				    IOMUX_WIDTH_4BIT,
> +				    IOMUX_WIDTH_4BIT,
> +				    0, -1, -1, -1,
> +				    DRV_TYPE_IO_LEVEL_8_BIT,
> +				    DRV_TYPE_IO_LEVEL_8_BIT,
> +				    DRV_TYPE_IO_LEVEL_8_BIT,
> +				    DRV_TYPE_IO_LEVEL_8_BIT),

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729132736.3807082-1-sjg@chromium.org?part=4