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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The macro `REG_PCIE_HB_RST` is used but never defined, causing a build failure.
- [High] Requesting PCIC resets on AN7583 causes an out-of-bounds read on the `en7581_rst_ofs` array, leading to undefined hardware behavior or memory corruption.

Pre-existing issues:
- [High] Probe error paths cause a use-after-free by mixing `devm_kzalloc` with non-devres `clk_hw_register()` for the PCIe clock.
- [High] Read-modify-write operations on shared registers in the reset and clock controllers lack spinlocks, causing data races and lost writes.
- [High] The clock `.disable` callback calls `usleep_range()`, which sleeps while holding an IRQ-disabled spinlock.
--

--- Patch [3]: [PATCH v6 3/3] clk: en7523: add support for Airoha AN7583 clock ---
commit e4983d60cb386b35f51bc20a5f0912039bf92780
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 087ff4568124f..493812c683f53 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
> @@ -505,6 +646,64 @@ static const u16 en751221_rst_map[] = {
>  	[EN751221_USB_PHY_P1_RST]	= 3 * RST_NR_PER_BANK + 7,
>  };
>  
> +static const u16 an7583_rst_map[] = {
> +	/* RST_CTRL2 */
> +	[AN7583_XPON_PHY_RST]		= 0,
> +	[AN7583_GPON_OLT_RST]		= 1,
[ ... ]
> +	/* RST_PCIC */
> +	[AN7583_PCIC_PERSTOUT0_RST]	= 2 * RST_NR_PER_BANK + 29,
> +	[AN7583_PCIC_PERSTOUT1_RST]	= 2 * RST_NR_PER_BANK + 26,
> +};

[Severity: High]
Does this out-of-bounds array access corrupt memory when requesting PCIC resets?

The an7583_rst_map defines these PCIC resets in bank index 2. 

When en7581_reset_register() is called later in this patch, it is passed
the en7581_rst_ofs array:

	static const u16 en7581_rst_ofs[] = {
		REG_RST_CTRL2,
		REG_RST_CTRL1,
	};

Since en7581_rst_ofs only has 2 elements, when en7523_reset_update()
dynamically calculates the bank index via id / RST_NR_PER_BANK, it evaluates
to 2. Accessing bank_ofs[2] will read out of bounds.

[ ... ]
> +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;
> +
> +	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 the probe error path leave
us with a use-after-free for the PCIe clock?

In en7523_register_pcie_clk() which is called by en7581_register_clocks(),
the clk_hw structure is allocated via devm_kzalloc(), but registered with the
non-devres clk_hw_register():

	cg = devm_kzalloc(dev, sizeof(*cg), GFP_KERNEL);
	...
	if (clk_hw_register(dev, &cg->hw))

If a subsequent initialization step fails in en7523_clk_probe() (like hw_init 
returning an error), the driver core automatically frees the memory via devres,
but the clock framework retains dangling pointers to the freed clk_hw.

> +
> +	val = readl(base + REG_RST_CTRL1);
> +	val |= REG_PCIE_HB_RST;

[Severity: Critical]
Will this break the build due to an undefined macro?

The REG_PCIE_HB_RST macro is not defined anywhere. The existing code seems
to use REG_RESET_CONTROL_PCIEHB for this bit instead.

[Severity: High]
This is a pre-existing issue, but could this unlocked read-modify-write race
with the reset framework?

en7523_pci_prepare() modifies REG_RESET_CONTROL1 (0x834) without any locks:

	val = readl(np_base + REG_RESET_CONTROL1);
	mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2 |
	       REG_RESET_CONTROL_PCIEHB;
	writel(val & ~mask, np_base + REG_RESET_CONTROL1);

Concurrently, en7523_reset_update() performs an unlocked read-modify-write
on the same register (REG_RST_CTRL1 is also 0x834):

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

Since .prepare() runs in process context and resets can be triggered by
other probing devices concurrently, this seems likely to cause lost writes.

> +	writel(val, base + REG_RST_CTRL1);
> +	val &= ~REG_PCIE_HB_RST;
> +	writel(val, base + REG_RST_CTRL1);
> +
> +	return en7581_reset_register(dev, base, an7583_rst_map,
> +				     ARRAY_SIZE(an7583_rst_map),
> +				     en7581_rst_ofs);
> +}
[ ... ]
> +static const struct en_clk_soc_data an7583_data = {
> +	.base_clks = an7583_base_clks,
> +	/* We increment num_clocks by 1 to account for additional PCIe clock */
> +	.num_clocks = ARRAY_SIZE(an7583_base_clks) + 1,
> +	.pcie_ops = {
> +		.is_enabled = en7581_pci_is_enabled,
> +		.enable = en7581_pci_enable,
> +		.disable = en7581_pci_disable,
> +	},

[Severity: High]
This isn't a bug introduced by this patch, but does mapping en7581_pci_disable
to the .disable callback result in sleeping while in an atomic context?

The core clock framework invokes .disable callbacks with the enable_lock
spinlock held and hardware interrupts disabled.

However, en7581_pci_disable() explicitly yields and sleeps:

	writel(val & ~mask, np_base + REG_PCI_CONTROL);
	usleep_range(1000, 2000);

Could this cause a scheduling while atomic panic when the PCIe clock is
disabled?

> +	.hw_init = an7583_clk_hw_init,
> +};

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