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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Concurrent read-modify-write accesses to the shared register 0x88 between the clock and reset subsystems will cause lost updates.
- [Medium] Removing the PERSTOUT reset deassertion from the clock enable path breaks PCIe initialization for existing Device Trees.

Pre-existing issues:
- [High] Missing spinlock in the reset controller to serialize read-modify-write operations.
- [High] Call to the sleeping function usleep_range() inside the atomic clock disable callback.
- [High] Stale register state written back after sleeping, clobbering concurrent hardware updates.
--

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

clk: en7523: add support for dedicated PCIe PERSTOUT reset

This commit adds support for resetting the PCIe lines using the PERSTOUT reset
by updating the reset map and moving the handling of these bits from the clock
enable and disable paths to the reset controller framework. The reset logic is
inverted for these specific bits and handled internally.

> 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
[ ... ]
> @@ -635,9 +641,7 @@ static int en7581_pci_enable(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;

[Severity: Medium]
Does removing the PERSTOUT reset deassertion from the clock enable path
break PCIe initialization for existing Device Trees?

When booting with an older DTB built before the resets property was updated
to explicitly list the PERSTOUT lines, the clock driver will no longer
automatically clear these resets in en7581_pci_enable(). Since the PCIe
driver also lacks references to them in the older DTB, it seems the PCIe
controller will remain in reset.

>  	val = readl(np_base + REG_PCI_CONTROL);
>  	writel(val | mask, np_base + REG_PCI_CONTROL);

[Severity: High]
Can concurrent read-modify-write accesses to this shared register between
the clock and reset subsystems cause lost updates?

The clock API protects operations with its global enable_lock, while the
reset framework operates lockless at the driver level. If PCIe host 0 enables
its clock in en7581_pci_enable() concurrently with PCIe host 1 toggling its
reset in en7523_reset_update(), their read-modify-write operations on the
exact same physical register (REG_PCI_CONTROL / REG_NP_SCU_PCIC at offset
0x88) will race and silently overwrite each other.

[ ... ]
> @@ -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 is it safe to call the sleeping function
usleep_range() inside the atomic clock disable callback?

The clk_ops->disable callback executes in atomic context with the clock
framework's enable_lock spinlock held and local interrupts disabled. Calling
usleep_range() here attempts a context switch, which will result in a
scheduling while atomic bug.

[ ... ]
> @@ -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 function need a
spinlock to serialize the read-modify-write operations?

The en7523_reset_update() callback performs unguarded read-modify-writes on
shared reset banks. If two distinct device drivers (such as I2C and PCIe)
probe concurrently and assert or deassert their respective resets, the
lockless read-modify-write cycles will race, overwriting one of the
operations and leading to silent hardware state corruption.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but could the caching of register
state in en7523_pci_prepare() clobber concurrent hardware updates?

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);
usleep_range(1000, 2000);
writel(val | mask, np_base + REG_RESET_CONTROL1);

The cached register state val is re-used to write after a sleep. If any
other driver toggles a reset bit in the same register (which controls up to
32 different resets) during the sleep duration, their reset state will be
unconditionally reverted and corrupted by this stale writeback.

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