Re: [PATCH v6 5/7] input: keyboard: mtk-pmic-keys: Add MT6392 support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Out-of-bounds array access and incorrect parsing of disabled DT nodes in `mtk_pmic_keys_probe` due to a mismatch between child counting and iteration macros.
- [High] `mt6357_regs` incorrectly uses `MTK_PMIC_HOMEKEY_INDEX` instead of `MTK_PMIC_HOMEKEY_RST` for the home key reset mask.
--

commit 2b2bdf18b713288536d3d56d4b82a48e65594233
Author: Val Packett <val@packett.cool>

    input: keyboard: mtk-pmic-keys: Add MT6392 support
    
This commit adds support for the MT6392 PMIC to the mediatek pmic keys driver
by defining its specific register layout and adding the required compatible
string.

> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index c78d9f6d97c4f..8b4a89fce4fb1 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c

[ ... ]

> @@ -69,6 +70,19 @@ static const struct mtk_pmic_regs mt6397_regs = {
>  	.rst_lprst_mask = MTK_PMIC_RST_DU_MASK,
>  };
>  
> +static const struct mtk_pmic_regs mt6392_regs = {
> +	.keys_regs[MTK_PMIC_PWRKEY_INDEX] =
> +		MTK_PMIC_KEYS_REGS(MT6392_CHRSTATUS, 0x2,
> +				   MT6392_INT_MISC_CON, 0x10,
> +				   MTK_PMIC_PWRKEY_RST),
> +	.keys_regs[MTK_PMIC_HOMEKEY_INDEX] =
> +		MTK_PMIC_KEYS_REGS(MT6392_CHRSTATUS, 0x4,
> +				   MT6392_INT_MISC_CON, 0x8,
> +				   MTK_PMIC_HOMEKEY_RST),
> +	.pmic_rst_reg = MT6392_TOP_RST_MISC,
> +	.rst_lprst_mask = MTK_PMIC_RST_DU_MASK,
> +};
> +

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
out-of-bounds access in mtk_pmic_keys_probe() due to a mismatch in child node
parsing:

drivers/input/keyboard/mtk-pmic-keys.c:mtk_pmic_keys_probe() {
    ...
	keycount = of_get_available_child_count(node);
	if (keycount > MTK_PMIC_MAX_KEY_COUNT) {
		dev_err(dev, "too many keys defined (%d)\n", keycount);
		return -EINVAL;
	}

	for_each_child_of_node_scoped(node, child) {
		keys->keys[index].regs = &keys->mtk_regs->keys_regs[index];
    ...
}

The check uses of_get_available_child_count(), which skips nodes with
status="disabled", but the loop uses for_each_child_of_node_scoped(), which
iterates over all child nodes regardless of their availability.

If a device tree defines more than MTK_PMIC_MAX_KEY_COUNT nodes but disables
enough to pass the initial check, could the loop execute more than
MTK_PMIC_MAX_KEY_COUNT times?  Would this cause an out-of-bounds array access
on the keys->keys[] and irqnames[] arrays?

[Severity: High]
This is a pre-existing issue as well, but looking at the existing register
definitions, mt6357_regs appears to use the wrong macro for the home key
reset mask:

drivers/input/keyboard/mtk-pmic-keys.c:
static const struct mtk_pmic_regs mt6357_regs = {
	.keys_regs[MTK_PMIC_PWRKEY_INDEX] =
		MTK_PMIC_KEYS_REGS(MT6357_TOPSTATUS,
		0x2, MT6357_INT_MISC_CON, 0x10, MTK_PMIC_PWRKEY_RST),
	.keys_regs[MTK_PMIC_HOMEKEY_INDEX] =
		MTK_PMIC_KEYS_REGS(MT6357_TOPSTATUS,
		0x8, MT6357_INT_MISC_CON, 0x8, MTK_PMIC_HOMEKEY_INDEX),
    ...
};

Does this incorrectly use MTK_PMIC_HOMEKEY_INDEX instead of
MTK_PMIC_HOMEKEY_RST?

Because MTK_PMIC_HOMEKEY_INDEX is just an array index, would this result in
modifying the wrong bits in the reset register during
mtk_pmic_keys_lp_reset_setup() and failing to properly enable the long-press
hardware reset for the home key on MT6357?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612200717.361018-1-l.scorcia@gmail.com?part=5