Re: [PATCH v7 3/3] clk: en7523: add support for Airoha AN7583 clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: en7523: NULL pointer dereference in PHY ops due to missing priv->base initialization
--

commit 37bd1589683423a3a7795bb6de626bc15702ea39
Author: Christian Marangi <ansuelsmth@gmail.com>

clk: en7523: add support for Airoha AN7583 clock
    
Add support for Airoha AN7583 clock and reset.
    
Airoha AN7583 SoC have the same register address of EN7581 but implement
different bits and additional base clocks. Also reset are different with
the introduction of 2 dedicated MDIO line and drop of some reset lines.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index ec66a504d4c55..d3a273b46c8f0 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c

[ ... ]

> @@ -1088,6 +1287,40 @@ static int en7581_clk_hw_init(struct platform_device *pdev,
>  	return en7581_serdes_phy_register(&pdev->dev);
>  }
>  
> +static int an7583_clk_hw_init(struct platform_device *pdev,
> +			      struct clk_hw_onecell_data *clk_data)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct regmap *map;
> +	void __iomem *base;
> +	u32 val;
> +	int ret;
> +
> +	map = syscon_regmap_lookup_by_phandle(dev->of_node, "airoha,chip-scu");
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	en7581_register_clocks(dev, clk_data, map, base);

[Severity: High]
Are we missing a step to retrieve the driver data and assign priv->base = base
here, similar to what is done in en7581_clk_hw_init()?

Without this initialization, when the PHY is later configured,
en7581_serdes_phy_set_mode() will attempt to access the mapped resource:

drivers/clk/clk-en7523.c:en7581_serdes_phy_set_mode() {
    ...
	spin_lock_irqsave(&priv->lock, flags);
	val = readl(priv->base + reg);
	val &= ~mask;
    ...
}

Because priv was zero-allocated during probe, priv->base will be NULL here,
which could lead to a NULL pointer dereference when readl is called.

> +
> +	val = readl(base + REG_RST_CTRL1);
> +	val |= REG_PCIE_HB_RST;
> +	writel(val, base + REG_RST_CTRL1);
> +	val &= ~REG_PCIE_HB_RST;
> +	writel(val, base + REG_RST_CTRL1);
> +
> +	ret = en7581_reset_register(dev, base, an7583_rst_map,
> +				    ARRAY_SIZE(an7583_rst_map),
> +				    en7581_rst_ofs);
> +	if (ret)
> +		return ret;
> +
> +	return en7581_serdes_phy_register(dev);
> +}
> +

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