Re: [PATCH 3/7] clk: mediatek: mt8167: Map apmixedsys iomem as shared

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clk: mediatek: IS_ERR() used to check of_iomap() return value instead of NULL check
- [High] clk: mediatek: missing platform_set_drvdata() causes silent failure in remove()
- [Medium] clk: mediatek: missing iounmap() for driver base mapping in remove()
--

commit 847eb51d406a77cda5e7387f55742129733ec7b6
Author: Luca Leonardo Scorcia <l.scorcia@gmail.com>

clk: mediatek: mt8167: Map apmixedsys iomem as shared

The hdmi_phy for mt8167 and the apmixedsys clocks share the same IO
memory region and they both map it using devm_platform_ioremap_resource,
which implies exclusive access. Switch the apmixedsys driver to of_iomap,
like it is done in the mt8173-apmixedsys driver. Also, implement the
remove function while we're at it.

> diff --git a/drivers/clk/mediatek/clk-mt8167-apmixedsys.c b/drivers/clk/mediatek/clk-mt8167-apmixedsys.c
> index fb6c21bbeef81..3e8611991de78 100644
> --- a/drivers/clk/mediatek/clk-mt8167-apmixedsys.c
> +++ b/drivers/clk/mediatek/clk-mt8167-apmixedsys.c
[ ... ]
> @@ -97,17 +98,19 @@ static int clk_mt8167_apmixed_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	int ret;
>  
> -	base = devm_platform_ioremap_resource(pdev, 0);
> +	base = of_iomap(node, 0);
>  	if (IS_ERR(base))

[Severity: High]
Does this code handle the of_iomap() return value correctly? of_iomap()
returns NULL on failure, rather than an ERR_PTR. If it fails, base will be
NULL, which bypasses this error check and propagates to the register
functions.

>  		return PTR_ERR(base);
>  
>  	clk_data = mtk_devm_alloc_clk_data(dev, MT8167_CLK_APMIXED_NR_CLK);
> -	if (!clk_data)
> -		return -ENOMEM;
> +	if (IS_ERR_OR_NULL(clk_data)) {

[Severity: High]
Does this code need to save clk_data to the device's driver data? It looks
like clk_mt8167_apmixed_probe() allocates clk_data but never calls
platform_set_drvdata() to store it before returning.

> +		ret = -ENOMEM;
> +		goto unmap_io;
> +	}
[ ... ]
> @@ -124,10 +127,22 @@ static int clk_mt8167_apmixed_probe(struct platform_device *pdev)
>  	mtk_clk_unregister_dividers(adj_divs, ARRAY_SIZE(adj_divs), clk_data);
>  unregister_plls:
>  	mtk_clk_unregister_plls(plls, ARRAY_SIZE(plls), clk_data);
> +unmap_io:
> +	iounmap(base);
>  
>  	return ret;
>  }
>  
> +static void clk_mt8167_apmixed_remove(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct clk_hw_onecell_data *clk_data = platform_get_drvdata(pdev);

[Severity: High]
Since platform_set_drvdata() was never called in the probe function, won't
platform_get_drvdata() evaluate to NULL here? This would render the
subsequent unregister calls as no-ops, failing to unregister the clocks.

[Severity: Medium]
Does this remove function leak the I/O memory mapping? The base mapping
created by of_iomap() in clk_mt8167_apmixed_probe() doesn't appear to be
unmapped via iounmap() here.

Since base is only a local variable in the probe function and isn't saved to
a device context, there is no way for clk_mt8167_apmixed_remove() to retrieve
the pointer to clean it up.

> +
> +	of_clk_del_provider(node);
> +	mtk_clk_unregister_dividers(adj_divs, ARRAY_SIZE(adj_divs), clk_data);
> +	mtk_clk_unregister_plls(plls, ARRAY_SIZE(plls), clk_data);
> +}

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