Re: [PATCH v3] PCI: qcom: Add register dump support for PCIe Link Down events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] PCI: qcom: unlocked register dump in
  qcom_pcie_fill_dump_buf()
- [Medium] PCI: qcom: out-of-bounds PARF reads in
  qcom_pcie_fill_dump_buf()
- [Low] PCI: qcom: PARF_INT_ALL_STATUS cleared before
  qcom_pcie_dump_regs()
--

commit b30e1ed6bb64a53a1b0da2d0c7ce94bc1f80823d
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

PCI: qcom: Add register dump support for PCIe Link Down events

This commit dumps key DBI, PARF, and MHI controller registers to dmesg on a
PCIe Link Down interrupt and exposes the same register dump via a per-controller
"regdump" debugfs file.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index d903f10211c23..147aab6f4155b 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
[ ... ]
> @@ -207,11 +220,48 @@
>  #define PERST_DELAY_US				1000
>  #define FLUSH_TIMEOUT_US			100
>  
> +/* Dump buffer size for the link-down register dump */
> +#define QCOM_PCIE_DUMP_BUF_SIZE			SZ_8K
> +
>  #define QCOM_PCIE_CRC8_POLYNOMIAL		(BIT(2) | BIT(1) | BIT(0))
>  
>  #define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
>  		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_get_link_speed(speed)))
>  
> +#define QCOM_PCIE_DUMP_REGS_PER_LINE	4
> +
> +static const u32 qcom_pcie_parf_dump_regs[] = {
> +	PARF_LTSSM,
> +	PARF_INT_ALL_STATUS,
> +	PARF_SYS_CTRL,
> +};
> +
> +static const u32 qcom_pcie_ext_parf_dump_regs[] = {
> +	PARF_PM_STTS,
> +	PARF_PM_STTS_1,
> +	PARF_INT_ALL_STATUS,
> +	PARF_INT_ALL_2_STATUS,
> +	PARF_CLKREQ_OVERRIDE,
> +	PARF_L1SS_SLEEP_MODE_HANDLER_STATUS,
> +	PARF_L1SS_SLEEP_MODE_HANDLER_CFG,
> +	PARF_CORE_ERRORS,
> +	PARF_LINK_DOWN_AXI_ECAM_BLOCK_STATUS,
> +	PARF_STATUS,
> +	PARF_SYS_CTRL,
> +};
[ ... ]
> @@ -1855,6 +1912,260 @@ static int qcom_pcie_set_max_opp(struct device *dev)
>  	return ret;
>  }
[ ... ]
> +	len += scnprintf(buf + len, buf_size - len, "\nPARF registers:\n");
> +	if (pcie->cfg->has_ext_parf_regs)
> +		len += qcom_pcie_dump_reg_table(pcie->parf, qcom_pcie_ext_parf_dump_regs,
> +						ARRAY_SIZE(qcom_pcie_ext_parf_dump_regs),
> +						buf + len, buf_size - len);
> +	else
> +		len += qcom_pcie_dump_reg_table(pcie->parf, qcom_pcie_parf_dump_regs,
> +						ARRAY_SIZE(qcom_pcie_parf_dump_regs),
> +						buf + len, buf_size - len);

[Severity: Medium]
Can dumping qcom_pcie_parf_dump_regs[] read past the end of pcie->parf on
cfg_2_1_0 platforms?

qcom_pcie_probe() now registers the "regdump" debugfs file unconditionally on
all non-firmware-managed controllers.  On IP v2.1.0 platforms (apq8064 and
ipq8064), has_ext_parf_regs is false and the "parf" MMIO region is only 0x100
bytes:

arch/arm/boot/dts/qcom/qcom-apq8064.dtsi:
			reg = <0x1b500000 0x1000>,
			      <0x1b502000 0x80>,
			      <0x1b600000 0x100>,
			      <0x0ff00000 0x100000>;
			reg-names = "dbi", "elbi", "parf", "config";

Reading "regdump" on these platforms reads PARF_LTSSM (0x1b0) and
PARF_INT_ALL_STATUS (0x224) beyond the 0x100-byte mapping.  Additionally,
v1.0.0 and v2.1.0 controllers report LTSSM state in ELBI_SYS_STTS rather than
PARF_LTSSM, whereas newer controllers with has_ext_parf_regs set only dump
qcom_pcie_ext_parf_dump_regs[] and omit PARF_LTSSM altogether.

[ ... ]

> +	if (pcie_get_link_speed(pci->max_link_speed) == PCIE_SPEED_32_0GT) {
> +		u16 rasdes_cap = dw_pcie_find_rasdes_capability(pci);
> +
> +		if (rasdes_cap) {
> +			int num_lanes = dw_pcie_link_get_max_link_width(pci);
> +			int lane;
> +
> +			for (lane = 0; lane < num_lanes; lane++) {
> +				val = dw_pcie_readl_dbi(pci,
> +							rasdes_cap + PCIE_RAS_DES_SD_EQ_CONTROL1);
> +				val &= ~PCIE_RAS_DES_SD_EQ_CONTROL1_LANE_SEL;
> +				val |= lane & PCIE_RAS_DES_SD_EQ_CONTROL1_LANE_SEL;
> +				dw_pcie_writel_dbi(pci,
> +						   rasdes_cap + PCIE_RAS_DES_SD_EQ_CONTROL1,
> +						   val);
> +
> +				val = dw_pcie_readl_dbi(pci,
> +							rasdes_cap + PCIE_RAS_DES_SD_EQ_STATUS2);
> +				len += qcom_pcie_dump_lane_reg_val(buf + len, buf_size - len, &col,
> +							lane,
> +							rasdes_cap + PCIE_RAS_DES_SD_EQ_STATUS2,
> +							val);
> +
> +				val = dw_pcie_readl_dbi(pci,
> +							rasdes_cap + PCIE_RAS_DES_SD_EQ_STATUS3);
> +				len += qcom_pcie_dump_lane_reg_val(buf + len, buf_size - len, &col,
> +							lane,
> +							rasdes_cap + PCIE_RAS_DES_SD_EQ_STATUS3,
> +							val);
> +			}
> +		}
> +	}
[ ... ]
> +static int qcom_pcie_regdump_show(struct seq_file *s, void *data)
> +{
> +	struct qcom_pcie *pcie = (struct qcom_pcie *)dev_get_drvdata(s->private);
> +	char *buf;
> +	size_t len;
> +
> +	buf = kmalloc(QCOM_PCIE_DUMP_BUF_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	len = qcom_pcie_fill_dump_buf(pcie, buf, QCOM_PCIE_DUMP_BUF_SIZE);
> +	seq_write(s, buf, len);
> +
> +	kfree(buf);
> +
> +	return 0;
> +}

[Severity: Medium]
Can qcom_pcie_fill_dump_buf() access unclocked MMIO registers if it races with
qcom_pcie_reset_root_port()?

Neither qcom_pcie_regdump_show() nor qcom_pcie_dump_regs() holds
pcie->reset_lock while calling qcom_pcie_fill_dump_buf():

CPU0                                  CPU1
qcom_pcie_reset_root_port()           qcom_pcie_regdump_show()
  guard(mutex)(&pcie->reset_lock)
  qcom_pcie_host_deinit()
    pcie->cfg->ops->deinit()
      clk_bulk_disable_unprepare()
      regulator_bulk_disable()
                                        qcom_pcie_fill_dump_buf()
                                          dw_pcie_readl_dbi()
                                            <-- unclocked MMIO / SError

While qcom_pcie_reset_root_port() reinitializes the Root Port under
pcie->reset_lock, controller clocks and regulators are disabled and resets are
asserted, so concurrent DBI, PARF, or MHI reads and writes from
qcom_pcie_fill_dump_buf() can trigger an immediate bus abort (SError) and
kernel panic.  In addition, on 32 GT/s controllers, concurrent callers of
qcom_pcie_fill_dump_buf() can interleave writes to the banked lane selector
PCIE_RAS_DES_SD_EQ_CONTROL1 and read back PCIE_RAS_DES_SD_EQ_STATUS2/3 for the
wrong lane.

Acquiring pcie->reset_lock around the register accesses in both dump paths
would serialize them against Root Port resets and each other.

[ ... ]

> @@ -1983,6 +2297,8 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>  		qcom_pcie_enable_outbound_blocker(pcie);
>  		mutex_unlock(&pcie->reset_lock);
>  
> +		qcom_pcie_dump_regs(pcie);
> +
>  		for_each_pci_bridge(port, pp->bridge->bus) {
>  			if (pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
>  				pci_host_handle_link_down(port);

[Severity: Low]
Does qcom_pcie_dump_regs() read PARF_INT_ALL_STATUS after it has already been
cleared?

At the start of qcom_pcie_global_irq_thread(), the handler reads
PARF_INT_ALL_STATUS into status and immediately writes status to
PARF_INT_ALL_CLEAR:

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_global_irq_thread() {
	unsigned long status = readl_relaxed(pcie->parf + PARF_INT_ALL_STATUS);

	writel_relaxed(status, pcie->parf + PARF_INT_ALL_CLEAR);
    ...
}

When qcom_pcie_dump_regs() later dumps PARF_INT_ALL_STATUS from
qcom_pcie_parf_dump_regs[] or qcom_pcie_ext_parf_dump_regs[], the interrupt
status bits that triggered the Link Down event have already been cleared in
hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-regdump-v3-1-c85648af91ee@oss.qualcomm.com?part=1