Re: [PATCH v4 2/2] PCI: cadence: Add debugfs property to provide LTSSM status of the PCIe link

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] PCIE_CADENCE_DEBUGFS tristate configuration causes build failure and module linkage issues.
- [High] Race condition in debugfs deinitialization leads to unprotected hardware access.
- [High] Missing Runtime PM reference in debugfs read handler causes system crash.
- [Medium] Missing link teardown in `cdns_pcie_hpa_host_disable`.
- [Medium] Incorrect bitmask `CDNS_PCIE_LGA_LTSSM_STATUS_MASK` truncates LTSSM states.
--

commit b032928b3c42715d7bb97e0bd26a99d252cd98d9
Author: Hans Zhang <18255117159@163.com>

PCI: cadence: Add debugfs property to provide LTSSM status of the PCIe link

Add the debugfs property to provide a view of the current link's LTSSM
status from the Root Port device.

> diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
> index 9e651d5459735..cb010bc97aad3 100644
> --- a/drivers/pci/controller/cadence/Kconfig
> +++ b/drivers/pci/controller/cadence/Kconfig
> @@ -6,6 +6,15 @@ menu "Cadence-based PCIe controllers"
>  config PCIE_CADENCE
>  	tristate
>  
> +config PCIE_CADENCE_DEBUGFS
> +	tristate "Cadence PCIe debugfs entries"

Could this Kconfig option be changed to a bool?

When configured as a module (=m), Kbuild defines
CONFIG_PCIE_CADENCE_DEBUGFS_MODULE, not CONFIG_PCIE_CADENCE_DEBUGFS. As a
result, the preprocessor check in pcie-cadence.h evaluates to false, causing
the header to declare empty static inline stubs. When pcie-cadence-debugfs.c
defines the non-static versions, the build fails with a function redefinition
error.

Furthermore, changing the check to IS_ENABLED(CONFIG_PCIE_CADENCE_DEBUGFS)
would cause linkage errors if the core drivers are built-in while the debugfs
feature is a module, since built-in code cannot call module functions.

[ ... ]

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-debugfs.c b/drivers/pci/controller/cadence/pcie-cadence-debugfs.c
> new file mode 100644
> index 0000000000000..a4f0c4f6f98f4
> --- /dev/null
> +++ b/drivers/pci/controller/cadence/pcie-cadence-debugfs.c
> @@ -0,0 +1,208 @@
[ ... ]
> +#define CDNS_PCIE_LGA_LTSSM_STATUS_MASK	GENMASK(29, 24)

Does this mask need to be extended to cover all possible states?

This mask covers only 6 bits, allowing it to extract values up to 63.
However, the cdns_pcie_ltssm enum defines states up to 127. If the
hardware enters a state 64 or higher, FIELD_GET will silently truncate
the value, making state 65 appear as state 1.

[ ... ]
> +static int ltssm_status_show(struct seq_file *s, void *v)
> +{
> +	struct cdns_pcie *pci = s->private;
> +	enum cdns_pcie_ltssm ltssm;
> +	u32 reg;
> +
> +	if (pci->is_hpa) {
> +		reg = cdns_pcie_hpa_readl(pci, REG_BANK_IP_REG,
> +					CDNS_PCIE_HPA_PHY_DBG_STS_REG0);
> +		ltssm = FIELD_GET(CDNS_PCIE_HPA_LTSSM_STATUS_MASK, reg);
> +	} else {
> +		reg = cdns_pcie_readl(pci, CDNS_PCIE_LM_BASE);
> +		ltssm = FIELD_GET(CDNS_PCIE_LGA_LTSSM_STATUS_MASK, reg);
> +	}

Is a runtime PM reference required here before accessing the hardware?

Since the driver supports power management, the controller might be in a
low-power, clock-gated state when this debugfs file is read. Reading the
registers without calling pm_runtime_get_sync() first could cause a bus fault
and crash the system.

[ ... ]

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index c0e1194a936b3..38a0157b60dc7 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -659,6 +659,7 @@ void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep)
>  	pci_epc_mem_free_addr(epc, ep->irq_phys_addr, ep->irq_cpu_addr,
>  			      SZ_128K);
>  	pci_epc_mem_exit(epc);
> +	cdns_pcie_debugfs_deinit(&ep->pcie);
>  }

Should cdns_pcie_debugfs_deinit() be called earlier in the teardown process?

If a userspace process is currently reading the debugfs file while the module
is being removed, ltssm_status_show() could attempt to read registers after
the hardware has been deactivated, potentially resulting in a bus fault.
Removing the debugfs entries first ensures no new reads can begin while the
hardware is shut down.

[ ... ]

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
> index 0f540bed58e88..8bf7cc106413b 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
> @@ -309,6 +309,18 @@ int cdns_pcie_hpa_host_link_setup(struct cdns_pcie_rc *rc)
>  }
>  EXPORT_SYMBOL_GPL(cdns_pcie_hpa_host_link_setup);
>  
> +void cdns_pcie_hpa_host_disable(struct cdns_pcie_rc *rc)
> +{
> +	struct pci_host_bridge *bridge;
> +
> +	bridge = pci_host_bridge_from_priv(rc);
> +	pci_stop_root_bus(bridge->bus);
> +	pci_remove_root_bus(bridge->bus);
> +

Is it necessary to stop the PCIe link here?

The setup path explicitly starts the link with cdns_pcie_start_link(),
but this teardown function omits stopping it or disabling PTM responses.
Leaving the link active after the bus is unregistered could lead to
spurious interrupts or unhandled DMA requests.

> +	cdns_pcie_debugfs_deinit(&rc->pcie);

Similar to the endpoint path, should debugfs teardown happen before
the hardware is disabled to avoid concurrent access faults?

> +}

[ ... ]

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 0bc9e6e90e0e0..873d496c440f0 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -370,6 +370,7 @@ void cdns_pcie_host_disable(struct cdns_pcie_rc *rc)
>  
>  	cdns_pcie_host_deinit(rc);
>  	cdns_pcie_host_link_disable(rc);
> +	cdns_pcie_debugfs_deinit(&rc->pcie);
>  }

Should this debugfs teardown be moved to the beginning of the function
for the same reason mentioned above?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508034101.1910036-1-18255117159@163.com?part=2