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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Clocks registered via clk_hw_register_fixed_rate() and clk_hw_register() are leaked and leave dangling pointers in the Common Clock Framework if probe fails.
- [High] Read-modify-write race condition in `en7523_reset_update` without a lock.
--

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

clk: en7523: add support for Airoha AN7583 clock

This commit adds support for the Airoha AN7583 clock and reset. It
introduces a new SoC that shares register addresses with EN7581 but
implements different bits, additional base clocks, and dedicated resets.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 087ff4568124f..217d5d5f932d6 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c

[ ... ]

> @@ -1057,6 +1057,28 @@ static int en7581_clk_hw_init(struct platform_device *pdev,
>  				     en7581_rst_ofs);
>  }
>  
> +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;
> +
> +	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]
This isn't a bug introduced by this patch, but does calling
en7581_register_clocks() here leak memory and leave dangling pointers if the
subsequent en7581_reset_register() fails?

en7581_register_clocks() registers clocks using clk_hw_register_fixed_rate()
and clk_hw_register() without their devm counterparts, but allocates the
inner structures (like struct en_clk_gate) using devm_kzalloc().

If en7581_reset_register() fails, the probe will fail. The devres framework
will automatically free the memory allocated by devm_kzalloc(), but the
clocks won't be unregistered from the Common Clock Framework, leaving it
with dangling pointers and memory leaks for the fixed rate clocks.

Could these be switched to the devm_clk_hw_register_* family of functions?

> +
> +	return en7581_reset_register(dev, base, an7583_rst_map,
> +				     ARRAY_SIZE(an7583_rst_map),
> +				     en7581_rst_ofs);
> +}

[Severity: High]
This is a pre-existing issue, but can concurrent resets trigger a race
condition since there is no locking during the reset updates?

en7581_reset_register() assigns en7581_reset_ops, which invokes
en7523_reset_update(). That function performs an unlocked read-modify-write:

    val = readl(addr);
    if (assert)
        val |= BIT(id % RST_NR_PER_BANK);
    else
        val &= ~BIT(id % RST_NR_PER_BANK);
    writel(val, addr);

Since multiple independent peripheral drivers (e.g., PCIe, USB, UART) can
trigger concurrent reset_control_assert() or reset_control_deassert() calls
for different bits within the same register bank, could one CPU overwrite
and drop another CPU's concurrent state change?

Could a spinlock be added to serialize access to these shared reset
registers?

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