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 <[email protected]> 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/[email protected]?part=3
