Re: [PATCH v4 4/4] pinctrl: ambarella: add CV75 pin controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] PINCTRL_AMB depends on an undefined Kconfig symbol (ARCH_AMBARELLA) and lacks HAS_IOMEM dependency.
- [High] Atomic IOMUX commit mechanism is defeated by executing commits per-pin, exposing partial hardware states.
- [Medium] Kernel stack information leak via uninitialized stack variables in amb_pinconf_dbg_show().
--

--- Patch [4]: [PATCH v4 4/4] pinctrl: ambarella: add CV75 pin controller ---
commit 839ad92b1e2074392112c76fcb7c264408262830
Author: Long Zhao <longzhao@ambarella.com>

pinctrl: ambarella: add CV75 pin controller
    
Add an Ambarella pinmux-only pinctrl driver with CV75 function/group
tables. GPIO is handled by the PL061 driver.

> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index eda54aa5fde6f..8436815bf816c 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -37,6 +37,19 @@ config DEBUG_PINCTRL
>  	help
>  	  Say Y here to add some extra checks and diagnostics to PINCTRL calls.
>  
> +config PINCTRL_AMB
> +	bool "Ambarella pin controller support"
> +	depends on ARCH_AMBARELLA || COMPILE_TEST

[Severity: High]
Does ARCH_AMBARELLA exist in the Kconfig tree? If not, this driver can only
be enabled when COMPILE_TEST is set, making it inaccessible to production
configurations.

Also, since this driver selects GPIO_PL061 and MFD_SYSCON and uses IOMEM ops,
would it cause unmet dependency warnings and build failures on architectures
lacking IOMEM (like UML) under COMPILE_TEST without a HAS_IOMEM dependency?

> +	select GENERIC_PINCONF

[ ... ]

> diff --git a/drivers/pinctrl/pinctrl-ambarella.c b/drivers/pinctrl/pinctrl-ambarella.c
> new file mode 100644
> index 0000000000000..fb79a8b02209a
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-ambarella.c

[ ... ]

> +static void amb_pinmux_set_altfunc(struct amb_pinctrl_soc_data *soc,
> +				   u32 bank, u32 offset, u32 altfunc)
> +{
> +	u32 i, data;
> +
> +	if (!amb_iomux_accessible(soc))
> +		return;
> +
> +	for (i = 0; i < 3; i++) {
> +		data = readl_relaxed(soc->iomux_base + IOMUX_OFFSET(bank, i));
> +		data &= (~(0x1 << offset));
> +		data |= (((altfunc >> i) & 0x1) << offset);
> +		writel_relaxed(data, soc->iomux_base + IOMUX_OFFSET(bank, i));
> +	}
> +
> +	amb_iomux_commit(soc);

[Severity: High]
Does calling amb_iomux_commit() here defeat the hardware's atomic commit
mechanism when configuring a group of pins?

Looking at amb_pinmux_set_mux(), it iteratively configures each pin:

static int amb_pinmux_set_mux(...) {
    ...
    for (i = 0; i < grp->num_pins; i++) {
        ...
        amb_pinmux_set_altfunc(soc, bank, offset, alt);
    }
}

Committing after each individual pin exposes intermediate electrical states
for multi-pin interfaces (like Ethernet RGMII), which could cause bus
contention or interface glitching. Would it be safer to commit once after
all pins in the group are configured?

> +}

[ ... ]

> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +static void amb_pinconf_dbg_show(struct pinctrl_dev *pctldev,
> +				 struct seq_file *s, unsigned int pin)
> +{
> +	struct amb_pinctrl_soc_data *soc = pinctrl_dev_get_drvdata(pctldev);
> +	u32 pull_en, pull_dir, ds0, ds1, ds2, ds;
> +	u32 bank, offset;
> +	int strength;
> +
> +	bank = PINID_TO_BANK(pin);
> +	if (bank >= soc->data->nr_banks) {
> +		seq_puts(s, " (no pinconf)");
> +		return;
> +	}
> +
> +	offset = PINID_TO_OFFSET(pin);
> +
> +	regmap_read(soc->pull_regmap, soc->data->pull_en[bank], &pull_en);
> +	pull_en = (pull_en >> offset) & 1;

[Severity: Medium]
If regmap_read() fails (e.g. due to a syscon read error), won't pull_en
and other variables remain uninitialized?

The uninitialized stack values are then masked and later printed via
seq_printf(), which leaks kernel stack memory to userspace. Should the
return values of these regmap_read() calls be checked before using the
variables?

> +	regmap_read(soc->pull_regmap, soc->data->pull_dir[bank], &pull_dir);
> +	pull_dir = (pull_dir >> offset) & 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814-cv75-v4-b-v4-0-ef2e83ec0eac@ambarella.com?part=4