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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clk: en7523: build failure due to undeclared 'priv' in an7583_clk_hw_init
- [High] clk: en7523: missing spinlock in reset controller update
- [Low] clk: en7523: undeclared 'priv' variable breaks build in an7583_clk_hw_init
--

commit bdbd93bdb15e9361a92abee5427344b8ccd03042
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.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index ec66a504d4c55..e8c61474c1f98 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
[ ... ]
> @@ -1088,6 +1287,42 @@ 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);
> +
> +	priv->base = base;

[Severity: High]
Does this code break the build by using an undeclared identifier?

It appears the pointer priv is never declared, initialized, or fetched
(e.g., via platform_get_drvdata(pdev)) in an7583_clk_hw_init().

[Severity: Low]
Does this function also unintentionally omit the PCIe SCU register
initialization (REG_NP_SCU_SSTR and REG_NP_SCU_PCIC)?

These were initialized in en7581_clk_hw_init(), and it looks like their
omission here stems from the same adaptation that left the priv pointer
undeclared.

[ ... ]
> +
> +	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);

[Severity: High]
Can this mapping cause lost reset updates due to concurrent driver access?

By registering these reset lines, different drivers will manage their IP block
resets independently. However, the shared reset bank registers appear to be
modified without any locks:

static int en7523_reset_update(...)
{
	...
	val = readl(addr);
	if (assert ^ inverted)
		val |= BIT(id % RST_NR_PER_BANK);
	else
		val &= ~BIT(id % RST_NR_PER_BANK);
	writel(val, addr);
	...
}

If two different drivers assert or deassert resets concurrently mapped to the
same bank, could this unprotected read-modify-write sequence lead to a race
condition where a hardware IP is left incorrectly in reset or unexpectedly
pulled out of reset?

> +	if (ret)
> +		return ret;
> +
> +	return en7581_serdes_phy_register(dev);
> +}

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