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

On Wed, Aug 19, 2026 at 10:27:42PM +0000, [email protected] wrote:
> 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.
>

For reference, it seems the bot applied the patch to the wrong branch.
The reported problems from the bot are not valid as those changes were
introduced by the last 2 patch present in linux-next and clk-next.

Code was compiled and tested with those 2 changes included.
 
> 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 <[email protected]>
> 
> 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/[email protected]?part=3

-- 
	Ansuel