Re: [PATCH] pinctrl: mediatek: common-v1: Use a raw spinlock for the pinctrl regmap

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] pinctrl: mediatek: fallback to syscon bypasses lockdep fix
- [Medium] pinctrl: mediatek: integer underflow in max_register calculation
--

commit b8dcd543528b096e44b07a9f7d47936a1a0bdd08
Author: Ryan Brue <ryanbrue.dev@xxxxxxxxx>

pinctrl: mediatek: common-v1: Use a raw spinlock for the pinctrl regmap

This commit introduces a new regmap with a raw spinlock for Mediatek
pinctrl nodes to resolve invalid wait context splats in hard IRQ context.
It registers this regmap with syscon so other node users can share it,
and falls back to an existing syscon regmap if one is already registered.

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> index 1a977acd6883a..65b1e3096183d 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
[ ... ]
> +static struct regmap *mtk_pctrl_syscon_regmap(struct device_node *np)
> +{
> +	struct regmap_config config = mtk_pctrl_regmap_config;
> +	struct regmap *regmap;
> +	struct resource res;
> +	void __iomem *base;
> +	int ret;
> +
> +	ret = of_address_to_resource(np, 0, &res);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	base = ioremap(res.start, resource_size(&res));
> +	if (!base)
> +		return ERR_PTR(-ENOMEM);
> +
> +	config.name = kasprintf(GFP_KERNEL, "%pOFn@%pa", np, &res.start);
> +	if (!config.name) {
> +		iounmap(base);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	config.max_register = resource_size(&res) - config.reg_stride;

[Severity: Medium]
Can this calculation underflow if the mapped resource size in the device
tree is smaller than config.reg_stride? 

Because resource_size() returns an unsigned resource_size_t, a small
resource size (e.g., 1, 2, or 3) would underflow and set max_register to
an extremely large value. This seems to bypass regmap memory bounds checking.
The standard syscon code in drivers/mfd/syscon.c usually checks if the
resource size is smaller than the IO width to prevent this.

> +	regmap = regmap_init_mmio(NULL, base, &config);
> +	kfree(config.name);
> +	if (IS_ERR(regmap)) {
> +		iounmap(base);
> +		return regmap;
> +	}
> +
> +	ret = of_syscon_register_regmap(np, regmap);
> +	if (ret) {
> +		regmap_exit(regmap);
> +		iounmap(base);
> +		/* An earlier probe, or another user of the node, got there first. */
> +		return ret == -EEXIST ? syscon_node_to_regmap(np) : ERR_PTR(ret);

[Severity: Medium]
Does this fallback path silently inherit a non-raw spinlock regmap if another
driver (like ethernet) already triggered its creation?

If another driver sharing the syscon node probes before this pinctrl driver,
the standard syscon initialization will create a regmap with a normal,
sleepable spinlock_t. When the pinctrl driver subsequently probes,
of_syscon_register_regmap() returns -EEXIST, and we fall back to
syscon_node_to_regmap().

Would this result in the pinctrl driver using the existing non-raw
spinlock regmap, bypassing the intended lockdep fix and potentially causing
lockdep warnings on PREEMPT_RT when accessed from hard IRQ context?

> +	}
> +
> +	return regmap;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-rbrue-suez-upstreaming-mtk-pinctrl-raw-regmap-v1-1-db92943f42cb@xxxxxxxxx?part=1