Re: [PATCH v2 1/2] clk: eyeq: Use devm_platform_ioremap_resource()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Switching to devm_platform_ioremap_resource causes the MMIO region to be unmapped on probe failure, leaving dangling pointers in registered clocks.
--

commit d16beda6863fd4b206a647746c109b76b72334fa
Author: Benoît Monin <benoit.monin@bootlin.com>

clk: eyeq: Use devm_platform_ioremap_resource()

Convert eqc_probe() from the open-coded platform_get_resource() +
ioremap() sequence to devm_platform_ioremap_resource(). Besides less
code, this requests the memory region so the OLB registers are properly
reserved in the iomem_resource tree.

Move devm_platform_ioremap_resource() before checking for device match
data, so OLBs bound without match data also get their memory region
mapped and reserved.

> diff --git a/drivers/clk/clk-eyeq.c b/drivers/clk/clk-eyeq.c
> index cf37feccc734f..3662e6990bf7f 100644
> --- a/drivers/clk/clk-eyeq.c
> +++ b/drivers/clk/clk-eyeq.c
> @@ -513,21 +513,16 @@ static int eqc_probe(struct platform_device *pdev)
>  	const struct eqc_match_data *data;
>  	struct clk_hw_onecell_data *cells;
>  	unsigned int i, clk_count;
> -	struct resource *res;
>  	void __iomem *base;
>  	int ret;
>  
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +

[Severity: High]
Does this change introduce a regression if of_clk_add_hw_provider() fails
later in eqc_probe()?

If the provider registration fails, the probe function aborts and triggers
devres cleanup, which unmaps this base memory region.

However, the clocks are registered earlier in eqc_probe() using non-devm
functions:

drivers/clk/clk-eyeq.c:eqc_probe() {
    ...
    for (i = 0; i < data->clk_count; i++) {
        ...
        ret = clk->probe(dev, NULL, clk, base, cells);
        ...
    }
    return of_clk_add_hw_provider(np, of_clk_hw_onecell_get, cells);
}

drivers/clk/clk-eyeq.c:eqc_probe_divider() {
    ...
    hw = clk_hw_register_divider_table_parent_data(dev, ...,
                                                   base + clk->div.reg, ...);
    ...
}

Because the clocks are registered with non-devm functions, they remain in
the clock framework on probe failure. Since base is now managed by devres,
the clock framework will hold dangling pointers to the unmapped base memory.

Prior to this change, the manual ioremap() was not unmapped on failure,
which avoided a use-after-free.

Would it be safer to convert the clock registrations to their devm
equivalents, or explicitly unregister them on probe failure to prevent
these dangling pointers?

>  	data = device_get_match_data(dev);
>  	if (!data)
> -		return 0; /* No clocks nor auxdevs, we are done. */
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res)
> -		return -ENODEV;
> -
> -	base = ioremap(res->start, resource_size(res));
> -	if (!base)
> -		return -ENOMEM;
> +		return 0; /* No clocks nor auxdevs, stop here but keep resource reserved */
>  
>  	/* Init optional auxiliary devices. */
>  	eqc_auxdev_create_optional(dev, base, data->reset_auxdev_name);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-clk-eyeq-res-v2-0-687e5de3aba7@bootlin.com?part=1