Re: [PATCH v3 2/4] clk: en7523: add support for dedicated PCIe PERSTOUT reset

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added `REG_NP_SCU_PCIC` reset bank shares register `0x88` with the clock framework, causing a cross-subsystem data race.

Pre-existing issues:
- [High] Missing spinlock in reset controller update function leads to intra-bank read-modify-write races between concurrent resets.
- [High] In `en7523_pci_prepare`, a stale local variable is reused for multiple delayed register writes, which will silently overwrite any concurrent changes made by the reset framework.
- [High] The clock `.disable` callback (`en7581_pci_disable`) calls `usleep_range()`, which sleeps while holding the clock framework's IRQ-disabled spinlock.
--

commit 1a24107acd835b1f50aa873616efba12fb4d03d2
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: clk: en7523: add support for dedicated PCIe PERSTOUT reset

This commit adds support for resetting the PCIe lines using the PERSTOUT
reset, controlled by the PCIC register. It implements inverted logic for
these bits within the reset function. The PCI enable and disable functions
are also updated to drop PERSTOUT bits in favor of this dedicated reset
handling.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 1ab0e2eca5d32..c9b21d9bf2f3f 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
[ ... ]
> @@ -650,9 +654,7 @@ static void en7581_pci_disable(struct clk_hw *hw)
>  	void __iomem *np_base = cg->base;
>  	u32 val, mask;
>  
> -	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |
> -	       REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
> -	       REG_PCI_CONTROL_PERSTOUT;
> +	mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1;
>  	val = readl(np_base + REG_PCI_CONTROL);
>  	writel(val & ~mask, np_base + REG_PCI_CONTROL);
>  	usleep_range(1000, 2000);

[Severity: High]
This is a pre-existing issue, but does this sleep cause a bug when called from
atomic context?

The clock framework core invokes .disable callbacks with the global enable_lock
raw spinlock held and local interrupts disabled. Inside en7581_pci_disable,
there is a call to usleep_range().

Could this lead to a system panic or deadlock by sleeping inside an atomic
context? Should this either use an atomic delay like udelay() or be moved to
the .unprepare callback?

[ ... ]
> @@ -754,14 +756,21 @@ static int en7523_reset_update(struct reset_controller_dev *rcdev,
>  			       unsigned long id, bool assert)
>  {
>  	struct en_rst_data *rst_data = container_of(rcdev, struct en_rst_data, rcdev);
> -	void __iomem *addr = rst_data->base + rst_data->bank_ofs[id / RST_NR_PER_BANK];
> +	u32 offset = rst_data->bank_ofs[id / RST_NR_PER_BANK];
> +	void __iomem *addr = rst_data->base + offset;
> +	bool inverted = false;
>  	u32 val;
>  
> +	/* For PCIC reset logic is inverted, 0:assert 1:deassert*/
> +	if (offset == REG_NP_SCU_PCIC)
> +		inverted = true;
> +
>  	val = readl(addr);
> +	val &= ~BIT(id % RST_NR_PER_BANK);
>  	if (assert)
> -		val |= BIT(id % RST_NR_PER_BANK);
> +		val |= inverted ? 0 : BIT(id % RST_NR_PER_BANK);
>  	else
> -		val &= ~BIT(id % RST_NR_PER_BANK);
> +		val |= inverted ? BIT(id % RST_NR_PER_BANK) : 0;
>  	writel(val, addr);

[Severity: High]
This isn't a bug introduced by this patch, but does this read-modify-write
sequence need a spinlock?

The en7523_reset_update function modifies MMIO registers locklessly. If
different drivers concurrently call reset_control_assert or deassert on the
same reset bank, wouldn't they race and overwrite each other's register
changes?

[Severity: High]
Is there a cross-subsystem data race introduced here by sharing register 0x88
between the clock and reset frameworks?

This patch maps REG_NP_SCU_PCIC (0x88) as a reset controller bank. However,
this same register is mapped as REG_PCI_CONTROL and modified by the clock
framework in en7581_pci_enable.

Since en7581_pci_enable runs under the clock core's enable_lock spinlock, and
en7523_reset_update runs locklessly, will concurrent accesses from these two
paths result in one silently overwriting the other's bit modifications?

[Severity: High]
This is a pre-existing issue, but does en7523_pci_prepare overwrite concurrent
register changes made by the reset framework?

In en7523_pci_prepare, a local variable is reused for multiple delayed register
writes:

drivers/clk/clk-en7523.c:en7523_pci_prepare() {
    val = readl(np_base + REG_RESET_CONTROL1);
    ...
    writel(val & ~mask, np_base + REG_RESET_CONTROL1);
    usleep_range(1000, 2000);
    writel(val | mask, np_base + REG_RESET_CONTROL1);
    msleep(100);
    writel(val & ~mask, np_base + REG_RESET_CONTROL1);
}

If a device requests a reset on bank 1 (REG_RESET_CONTROL1) during the ~115ms
sleep window, won't the final writel() in en7523_pci_prepare silently overwrite
those state changes using the stale val?

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