Re: [PATCH v4 4/4] pinctrl: ambarella: add CV75 pin controller
flat view
From: sashiko-bot@kernel.org
Date: 2026-08-14 06:40:08
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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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?
+}
[ ... ]
quoted hunk ↗ jump to hunk
+#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