Re: [PATCH v6 2/3] PCI: cadence: Add HPA IP debugfs for LTSSM status

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unprotected hardware register access in the debugfs read callback can cause a kernel panic if the device is runtime suspended.
- [High] Race condition during root bus teardown due to missing `pci_lock_rescan_remove()`.
- [Medium] Debugfs feature silently fails to initialize when compiled as a module due to incorrect Kconfig and preprocessor macro interaction.
- [Medium] The patch implicitly introduces critical behavioral changes (root bus teardown) not mentioned in the commit message.
--

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

PCI: cadence: Add HPA IP debugfs for LTSSM status

This commit adds debugfs support for HPA-based Cadence PCIe controllers to
allow users to read the current LTSSM state. It also introduces root bus
teardown logic during device removal.

> Add debugfs support for HPA-based Cadence PCIe controllers. A new file
> 'ltssm_status' is created under debugfs, allowing users to read the
> current LTSSM state as a string and raw value.

This isn't a bug, but the patch appears to introduce behavioral changes for
root bus teardown (adding pci_stop_root_bus() and pci_remove_root_bus() via
cdns_pcie_hpa_host_disable()) that aren't mentioned in the commit message.

Should this bus teardown logic be split into a separate patch, or documented
in the commit message?

> 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 cause silent failures if configured as a module?

If a user configures this as a module (=m), Kbuild will define
CONFIG_PCIE_CADENCE_DEBUGFS_MODULE instead of CONFIG_PCIE_CADENCE_DEBUGFS.

Because pcie-cadence.h guards the function declarations with
#ifdef CONFIG_PCIE_CADENCE_DEBUGFS, the core drivers will compile against
the empty static inline dummy functions instead.

Additionally, if we fix the macro to use IS_ENABLED(), would it cause linker
errors if the core driver is built-in (=y) but the debugfs is built as a
module (=m)?

Should this configuration option be changed to a bool?

[ ... ]

> 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..97c5deef2b1ab
> --- /dev/null
> +++ b/drivers/pci/controller/cadence/pcie-cadence-debugfs.c
[ ... ]
> +static int ltssm_status_show(struct seq_file *s, void *v)
> +{
> +	struct cdns_pcie *pci = s->private;
> +	enum cdns_pcie_hpa_ltssm hpa_ltssm;
> +	const char *str_ltssm;
> +	u32 val;
> +
> +	if (pci->is_hpa) {
> +		val = cdns_pcie_hpa_readl(pci, REG_BANK_IP_REG,
> +					  CDNS_PCIE_HPA_PHY_DBG_STS_REG0);

Could accessing this register cause a kernel panic if the device is
runtime suspended?

Cadence PCIe platform drivers rely on runtime power management and will gate
clocks and power domains when idle.

If a user reads this debugfs file while the controller is runtime suspended,
will reading the memory-mapped register of a powered-off IP block trigger
a synchronous external abort?

Should this hardware access be wrapped with pm_runtime_get_sync(pci->dev)
and pm_runtime_put(pci->dev)?

[ ... ]

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
> index 0f540bed58e88..abc1d0e58b981 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
[ ... ]
> +void cdns_pcie_hpa_host_disable(struct cdns_pcie_rc *rc)
> +{
> +	struct pci_host_bridge *bridge;
> +
> +	cdns_pcie_debugfs_deinit(&rc->pcie);
> +	bridge = pci_host_bridge_from_priv(rc);
> +	pci_stop_root_bus(bridge->bus);
> +	pci_remove_root_bus(bridge->bus);
> +}

Could this lead to a race condition during root bus teardown?

Calling pci_stop_root_bus() and pci_remove_root_bus() without holding the
PCI rescan lock allows race conditions with concurrent PCI rescan or hotplug
operations triggered via sysfs.

While this pattern might have been copied from a pre-existing function like
cdns_pcie_host_disable(), does it need to be fixed here to prevent
use-after-free or system crashes?

Should these operations be protected by acquiring the lock using
pci_lock_rescan_remove() and pci_unlock_rescan_remove()?

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