Re: [PATCH 2/2] mux-controller: ti: add driver for event mux router

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Explicit iounmap() called on a devres-managed memory region on the error path.
- [High] Blocking memory allocation with GFP_KERNEL is used inside a PM suspend callback.
- [Medium] Disabling the mux (state 0) fails to clear the interrupt enable bit (MUX_ENABLE_INTR).
- [Medium] Brittle error check using modulo operator bypasses negative error codes that are multiples of 3.
- [Medium] Masking of original error code from devm_platform_ioremap_resource drops context like -EPROBE_DEFER.
- [Low] Driver enables COMPILE_TEST but fails to select REGMAP_MMIO, causing linker errors.
--

commit 4ce655ff4ffe60f566fd3744fe5da650083548ce
Author: Rahul Sharma <r-sharma3@ti.com>

mux-controller: ti: add driver for event mux router

The driver supports event muxing routers like gpio mux router and timesync
router. It allows configuration of hardware mux routers using memory-mapped
registers and supports a 3-field format for precise control.

> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index eb34457beaab3..58739a01f0d53 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
[ ... ]
> @@ -83,6 +83,21 @@ config MUX_RZV2H_VBENCTL
>  	  To compile the driver as a module, choose M here: the module will
>  	  be called mux-rzv2h-vbenctl.
>  
> +config MUX_TI_K3_EVENT_ROUTER
> +	tristate "TI Event Mux Router using MMIO registers"
> +	depends on OF && (REGMAP_MMIO || COMPILE_TEST)

[Severity: Low]
Does this dependency expose the driver to build failures? When compiling with
CONFIG_COMPILE_TEST=y and CONFIG_REGMAP_MMIO=n, this allows the driver to be
built, but devm_regmap_init_mmio() lacks an inline stub when REGMAP_MMIO is
disabled, which can cause linker errors. Should this select REGMAP_MMIO if
COMPILE_TEST is enabled, or depend directly on REGMAP_MMIO?

> +	help
> +	  This is extension of MMIO mux for  timesync router and gpiomux
[ ... ]
> diff --git a/drivers/mux/ti-k3-event-mux.c b/drivers/mux/ti-k3-event-mux.c
> new file mode 100644
> index 0000000000000..2469500d1b48c
> --- /dev/null
> +++ b/drivers/mux/ti-k3-event-mux.c
[ ... ]
> +static int mux_ti_k3_event_suspend(struct device *dev)
> +{
> +	struct mux_ti_k3_event_chip *chip = dev_get_drvdata(dev);
> +	int i, ret;
> +
> +	if (!chip->saved_states) {
> +		chip->saved_states = devm_kcalloc(dev, chip->num_fields,
> +						  sizeof(u32), GFP_KERNEL);

[Severity: High]
Can this blocking allocation cause a deadlock during system suspend? The
mux_ti_k3_event_suspend() callback dynamically allocates memory using
GFP_KERNEL on the first system suspend. Executing GFP_KERNEL allocations
during system suspend is unsafe because user processes are frozen and devices
may already be suspended, potentially triggering memory reclaim and writeback
that waits on a suspended device.

> +		if (!chip->saved_states)
> +			return -ENOMEM;
> +	}
[ ... ]
> +static int mux_ti_k3_event_set(struct mux_control *mux, int state)
> +{
> +	struct mux_ti_k3_event *fields = mux_chip_priv(mux->chip);
> +	struct mux_ti_k3_event *field = &fields[mux_control_get_index(mux)];
> +
> +	if (!state)
> +		return regmap_update_bits(field->regmap, field->reg, field->mask, 0);

[Severity: Medium]
Does transitioning to the inactive state (0) fail to clear the interrupt
enable bit? The active state (1) forces MUX_ENABLE_INTR to be set. However,
the inactive state updates the register using only field->mask with value 0.
Because MUX_ENABLE_INTR is not part of field->mask read from the device tree,
regmap_update_bits() will leave the hardware mux channel partially enabled.

> +
> +	return regmap_update_bits(field->regmap, field->reg, field->mask | MUX_ENABLE_INTR,
> +		field->value | MUX_ENABLE_INTR);
> +}
[ ... ]
> +static int mux_ti_k3_event_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base)) {
> +		return dev_err_probe(dev, -ENODEV,
> +				     "failed to get base address\n");

[Severity: Medium]
Does hardcoding the return value to -ENODEV break deferred probing? Masking
the original error code from devm_platform_ioremap_resource() discards
context like -EPROBE_DEFER, preventing the core subsystem from correctly
deferring the probe when resources are unavailable.

> +	} else {
> +		regmap = devm_regmap_init_mmio(dev, base, &mux_ti_k3_event_regmap_cfg);
> +	}
> +	if (IS_ERR(regmap)) {
> +		iounmap(base);

[Severity: High]
Does this explicit iounmap() cause a double-free of the ioremap mapping?
Because base was allocated using devm_platform_ioremap_resource(), the
devres framework will automatically attempt to unmap it again during the
probe rollback if devm_regmap_init_mmio() fails.

> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "failed to get regmap\n");
> +	}
> +
> +	ret = of_property_count_u32_elems(np, "ti,reg-mask-val");
> +	if (!ret || ret % 3) {

[Severity: Medium]
Could this error check bypass negative error codes that are multiples of 3?
If of_property_count_u32_elems() returns an error code like -EOVERFLOW (-75),
ret % 3 evaluates to false. This results in the driver calculating a negative
num_fields and proceeding to devm_mux_chip_alloc() with a wrapped size,
hiding the true configuration error and failing with -ENOMEM later.

> +		ret = -EINVAL;
> +		dev_err(dev, "ti,reg-mask-val property missing or invalid: %d\n",
> +			ret);
> +		return ret;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828100615.1700223-1-r-sharma3@ti.com?part=2