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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Concurrent access to indirect SD-EQ registers is not protected by a lock, creating a race condition that can corrupt the captured diagnostic state.
--

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

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

This commit adds support for dumping key PCIe controller registers when a Link 
Down event occurs to aid in root-causing failures. The dump is output directly
to dmesg or devcoredump, and is also exposed on-demand via a regdump debugfs 
file.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index b58a607b713f5..ba41a0eb43dc5 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

[ ... ]

> @@ -1760,6 +1819,287 @@ static int qcom_pcie_set_max_opp(struct device *dev)

[ ... ]

> +static size_t qcom_pcie_fill_dump_buf(struct qcom_pcie *pcie, char *buf,
> +				      size_t buf_size)
> +{
> +	struct dw_pcie *pci = pcie->pci;

[ ... ]

> +	/*
> +	 * Gen5 (32 GT/s) local/remote pre/post cursor status, read through the
> +	 * SD-EQ registers inside the DWC RAS-DES vendor-specific extended
> +	 * capability. For each lane, write the lane selector into
> +	 * PCIE_RAS_DES_SD_EQ_CONTROL1 then read back
> +	 * PCIE_RAS_DES_SD_EQ_STATUS2/3, which carry the local/remote pre/post
> +	 * cursor values.
> +	 */
> +	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++) {
> +				dw_pcie_writel_dbi(pci,
> +						   rasdes_cap + PCIE_RAS_DES_SD_EQ_CONTROL1,
> +						   lane & PCIE_RAS_DES_SD_EQ_CONTROL1_LANE_SEL);
> +
> +				val = dw_pcie_readl_dbi(pci,
> +							rasdes_cap + PCIE_RAS_DES_SD_EQ_STATUS2);

[Severity: Medium]
Are these indirect SD-EQ register accesses in qcom_pcie_fill_dump_buf() safe 
from concurrent access?

If multiple processes read the regdump debugfs file simultaneously, or if a 
user reads the debugfs file concurrently with a PCIe Link Down event (which 
triggers the same dump logic from qcom_pcie_global_irq_thread()), these 
write-then-read sequences could interleave.

This race could cause one or both threads to read the status registers for the
wrong lane, corrupting the captured diagnostic state. Should this access be
protected by a lock?

> +				len += qcom_pcie_dump_lane_reg_val(buf + len, buf_size - len, &col,
> +							lane,
> +							rasdes_cap + PCIE_RAS_DES_SD_EQ_STATUS2,
> +							val);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905-regdump-v2-1-f7d34f9d4f46@oss.qualcomm.com?part=1