Re: [PATCH 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] Data race between clock enable/disable and reset assert/deassert on the shared PCIC register (0x88).
- [Medium] The reset status callback does not account for inverted reset logic, reporting incorrect status for PCIC resets.

Pre-existing issues:
- [High] Missing synchronization in the reset controller allows concurrent reset operations to corrupt shared reset registers.
- [High] The `en7581_pci_disable` function calls `usleep_range()`, sleeping in an atomic context.
--

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

clk: en7523: add support for dedicated PCIe PERSTOUT reset

Add support for resetting the PCIe lines with the PERSTOUT reset. These
special reset are controlled by the PCIC register and are specific to each
of the 3 PCIe lines.

Notice that reset logic is inverted for these bit where 0 is assert and 1
deassert. This is intenrally handled in the reset function.

PCI enable/disable are updated to drop PERSTOUT bits in favor dedicated
reset handling.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 1ab0e2eca5d32..a33cf2e1b76f8 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 the Common Clock Framework calls the
.disable callback with the enable_lock spinlock held and hardware interrupts
disabled.

Does calling usleep_range() here sleep in an atomic context and lead to a 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;

[Severity: Medium]
It appears en7523_reset_status() wasn't updated to handle the inverted logic:

drivers/clk/clk-en7523.c:en7523_reset_status() {
    ...
    return !!(readl(addr) & BIT(id % RST_NR_PER_BANK));
}

Will callers of reset_control_status() receive incorrect status for these
resets, returning 0 for asserted and 1 for deasserted when querying PCIC
resets?

> +
>  	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 is a pre-existing issue, but multiple drivers could concurrently probe or
change power states, causing their reset operations on the same register bank
to interleave.

Could this lockless read-modify-write operation allow concurrent reset
operations to corrupt the shared reset registers?

[Severity: High]
en7581_pci_enable() modifies REG_PCI_CONTROL using a lockless
read-modify-write:

drivers/clk/clk-en7523.c:en7581_pci_enable() {
    ...
	val = readl(np_base + REG_PCI_CONTROL);
	writel(val | mask, np_base + REG_PCI_CONTROL);
    ...
}

If REG_NP_SCU_PCIC and REG_PCI_CONTROL point to the same register, a
concurrent clk_enable() and reset_control_assert() could silently corrupt
each other's bits since they belong to different lock domains.

Will this read-modify-write race with the clock framework?

>  
>  	return 0;
>  }

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