Re: [PATCH v11 3/5] clk: en7523: protect concurrent access to reset and PCI register

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] clk: en7523: NULL dereference in clk_hw_get_dev() before
  registration
--

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

clk: en7523: protect concurrent access to reset and PCI register

This commit adds spinlock protection around PCI control and reset register
accesses in the EN7523 clock and reset callbacks.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 2baec734df9fe..e534a5c91e618 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
[ ... ]
> @@ -623,12 +666,19 @@ static int en7523_pci_prepare(struct clk_hw *hw)
>  static void en7523_pci_unprepare(struct clk_hw *hw)
>  {
>  	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
> +	struct device *dev = clk_hw_get_dev(hw);
>  	void __iomem *np_base = cg->base;
> +	struct en_clk_priv *priv;
> +	unsigned long flags;
>  	u32 val;
>  
> +	priv = dev_get_drvdata(dev);
> +
> +	spin_lock_irqsave(&priv->lock, flags);
>  	val = readl(np_base + REG_PCI_CONTROL);
>  	val &= ~REG_PCI_CONTROL_REFCLK_EN1;
>  	writel(val, np_base + REG_PCI_CONTROL);
> +	spin_unlock_irqrestore(&priv->lock, flags);
>  }
[ ... ]
> @@ -683,12 +748,20 @@ static int en7581_pci_enable(struct clk_hw *hw)
>  static void en7581_pci_disable(struct clk_hw *hw)
>  {
>  	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
> +	struct device *dev = clk_hw_get_dev(hw);
>  	void __iomem *np_base = cg->base;
> +	struct en_clk_priv *priv;
> +	unsigned long flags;
>  	u32 val, mask;
>  
> +	priv = dev_get_drvdata(dev);
>  	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
>  	val = readl(np_base + REG_PCI_CONTROL);
>  	writel(val & ~mask, np_base + REG_PCI_CONTROL);
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
>  	usleep_range(1000, 2000);
>  }

[Severity: High]
Does clk_hw_get_dev(hw) dereference a NULL hw->core pointer when
en7523_register_pcie_clk() calls init.ops->unprepare(&cg->hw)?

drivers/clk/clk-en7523.c:en7523_register_pcie_clk() {
	...
	cg->base = np_base;
	cg->hw.init = &init;

	if (init.ops->unprepare)
		init.ops->unprepare(&cg->hw);

	if (devm_clk_hw_register(dev, &cg->hw))
		return NULL;
}

clk_hw_get_dev() returns hw->core->dev, but hw->core is only allocated and
assigned inside devm_clk_hw_register().  Calling init.ops->unprepare()
before registration causes a NULL pointer dereference during probe on SoCs
that set pcie_ops.unprepare to en7523_pci_unprepare() or
en7581_pci_disable().

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