Re: [PATCH 13/24] PCI: dwc: Add ZTE ZX279128S host controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] pci: race condition and state corruption in shared CRM2_RST register
- [Medium] pci: LNKCTL 32-bit read-modify-write clears LNKSTA W1C bits
- [Medium] pci: missing hardware cleanup on probe failure leaks clocks and PHY state
--

commit 41b496fd57c1fa24884637e3c63af32d3b98ad9f
Author: Navid Ghahremani <ghahramani.navid@gmail.com>
PCI: dwc: Add ZTE ZX279128S host controller driver

This commit adds a new PCIe controller driver for the ZTE ZX279128S SoC.
It supports bringing up both controller links using a vendor sequence,
handles shared board reset, and relies on legacy INTx interrupts.

> diff --git a/drivers/pci/controller/dwc/pcie-zx279128s.c b/drivers/pci/controller/dwc/pcie-zx279128s.c
> new file mode 100644
> index 0000000000000..3559beb35a142
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-zx279128s.c
[ ... ]
> +static void zx279128s_pcie_force_gen1(struct dw_pcie *pci)
> +{
> +	u8 cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u32 val;
> +
> +	if (WARN_ON(!cap))
> +		return;
> +
> +	dw_pcie_dbi_ro_wr_en(pci);
> +
> +	val = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCTL2);
> +	val &= ~PCI_EXP_LNKCTL2_TLS;
> +	val |= PCI_EXP_LNKCTL2_TLS_2_5GT;
> +	dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCTL2, val);
> +
> +	val = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCTL);
> +	dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCTL, val | PCI_EXP_LNKCTL_RL);

[Severity: Medium]
Does using 32-bit accessors (dw_pcie_readl_dbi and dw_pcie_writel_dbi) for
the 16-bit PCI_EXP_LNKCTL and PCI_EXP_LNKCTL2 registers inadvertently clear
Write-1-to-Clear (W1C) bits in the adjacent Link Status registers?

Because the Link Status registers are adjacent to the Link Control registers,
a 32-bit read-modify-write includes them. If any W1C bits (like Link
Autonomous Bandwidth Status) were read as 1, writing them back as 1 clears
them, which can result in lost link events. Should this use 16-bit accessors
or dw_pcie_read_dbi() with a size of 2?

> +
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +}
> +
> +static int zx279128s_pcie_init_crm(struct zx279128s_pcie *pcie)
> +{
> +	const struct zx279128s_pcie_port *port = pcie->port;
> +	struct regmap *crm1 = pcie->crm[ZX_CRM1];
> +	struct regmap *crm2 = pcie->crm[ZX_CRM2];
> +	int ret, i;
[ ... ]
> +	ret = regmap_write_bits(crm2, CRM2_RST, port->rst, port->rst);
> +	if (ret)
> +		return ret;
> +	ret = regmap_write_bits(crm2, CRM2_CLK, port->clk_off, 0);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(crm2, CRM2_RST, CRM2_RST_ALL);

[Severity: High]
Does an unconditional regmap_write() of CRM2_RST_ALL completely overwrite
the shared CRM2_RST register, potentially corrupting the reset state of a
sibling PCIe controller or other SoC peripherals?

> +}
> +
> +static int zx279128s_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct zx279128s_pcie *pcie = to_zx279128s_pcie(pci);
> +	int ret;
> +
> +	/*
> +	 * The board lines reset a device that can sit behind both controllers
> +	 * (the MT7915 of the H3600 uses both). The first controller to start
> +	 * resets and releases it; the second must not reset it again.
> +	 */
> +	mutex_lock(&zx279128s_board_lock);
> +	if (!zx279128s_board_released && (pcie->reset || pcie->enable)) {
[ ... ]
> +		msleep(200);
> +		zx279128s_board_released = true;
> +	}
> +	mutex_unlock(&zx279128s_board_lock);
> +
> +	ret = zx279128s_pcie_init_crm(pcie);

[Severity: High]
Since zx279128s_board_lock is released right before calling
zx279128s_pcie_init_crm(), can two concurrently probing controllers race
during initialization?

One controller might still be inside its 50ms fsleep() window after asserting
its reset bit, while the other completes its sequence and overwrites the
entire CRM2_RST register with CRM2_RST_ALL, prematurely deasserting the first
controller's reset.

> +	if (ret)
> +		return dev_err_probe(pci->dev, ret, "failed to initialize CRM\n");
> +
> +	/* Enable the controller, which starts link training */
[ ... ]
> +static const struct dw_pcie_host_ops zx279128s_pcie_host_ops = {
> +	.init = zx279128s_pcie_host_init,
> +	.msi_init = zx279128s_pcie_msi_init,
> +};

[Severity: Medium]
If the core dw_pcie_host_init() fails after successfully calling the driver's
.init callback, does the lack of a .deinit callback here cause the driver to
leak enabled clocks and leave PHYs powered on permanently?

Also, if any register access fails mid-sequence inside
zx279128s_pcie_init_crm(), the function returns immediately without rolling
back the already-enabled clocks and PHYs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010130024.177503-1-ghahramani.navid@gmail.com?part=13