Re: [PATCH v10 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 2 potential issue(s) to consider:
- [High] clk: en7523: separate spinlocks fail to serialize shared SCU registers
- [Medium] clk: en7523: stale REG_PCI_CONTROL write after sleep in en7523_pci_prepare()
--

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

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

This commit adds spinlocks to struct en_clk_gate and struct en_rst_data to
serialize concurrent read-modify-write accesses to the PCI control and reset
registers.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 2baec734df9fe..c05d0bfa23400 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
> @@ -123,6 +123,8 @@ struct en_clk_desc {
>  
>  struct en_clk_gate {
>  	void __iomem *base;
> +	/* protect SCU register */
> +	spinlock_t lock;
>  	struct clk_hw hw;
>  };
>  
> @@ -130,6 +132,8 @@ struct en_rst_data {
>  	const u16 *bank_ofs;
>  	const u16 *idx_map;
>  	void __iomem *base;
> +	/* protect SCU register */
> +	spinlock_t lock;
>  	struct reset_controller_dev rcdev;
>  };

[Severity: High]
Since struct en_clk_gate and struct en_rst_data each embed their own separate
spinlock_t instance (initialized independently in en7523_register_pcie_clk()
and en7581_reset_register()), do PCIe clock operations and reset controller
operations still race with each other on the shared SCU registers?

Specifically, both structures point at the same SCU MMIO base and perform
read-modify-write updates on the same physical registers:

  - Offset 0x88 (REG_PCI_CONTROL / REG_NP_SCU_PCIC) is modified under
    &cg->lock in en7523_pci_prepare(), en7523_pci_unprepare(),
    en7581_pci_enable(), and en7581_pci_disable(), while
    en7523_reset_update() modifies the same register under &rst_data->lock.

  - Offset 0x834 (REG_RESET_CONTROL1 / REG_RST_CTRL1) is modified under
    &cg->lock in en7523_pci_prepare(), while en7523_reset_update() modifies
    the same register under &rst_data->lock.

Should struct en_clk_gate and struct en_rst_data share a single spinlock (or
a pointer to one) so that concurrent clock and reset operations actually
exclude each other when accessing these shared registers?

> @@ -577,44 +581,78 @@ static u32 en7523_get_div(const struct en_clk_desc *desc, u32 val)
[ ... ]
>  static int en7523_pci_prepare(struct clk_hw *hw)
>  {
>  	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
>  	void __iomem *np_base = cg->base;
> +	unsigned long flags;
>  	u32 val, mask;
>  
>  	/* Need to pull device low before reset */
> +	spin_lock_irqsave(&cg->lock, flags);
>  	val = readl(np_base + REG_PCI_CONTROL);
>  	val &= ~(REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT);
>  	writel(val, np_base + REG_PCI_CONTROL);
> +	spin_unlock_irqrestore(&cg->lock, flags);
> +
>  	usleep_range(1000, 2000);
>  
>  	/* Enable PCIe port 1 */
> +	spin_lock_irqsave(&cg->lock, flags);
>  	val |= REG_PCI_CONTROL_REFCLK_EN1;
>  	writel(val, np_base + REG_PCI_CONTROL);
> +	spin_unlock_irqrestore(&cg->lock, flags);

[Severity: Medium]
In en7523_pci_prepare(), after dropping &cg->lock across usleep_range(1000,
2000) and reacquiring it to set REG_PCI_CONTROL_REFCLK_EN1, could reusing the
cached val without re-reading np_base + REG_PCI_CONTROL overwrite concurrent
changes made to REG_PCI_CONTROL while the lock was released?

The subsequent critical sections in en7523_pci_prepare() re-read the register
after reacquiring &cg->lock, but this block still modifies the local val read
before the sleep.

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