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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The PCIE_CADENCE_DEBUGFS Kconfig option is defined as a tristate but checked using #ifdef, causing silently disabled debugfs features when built as a module.
--

commit 42a8fcd88891f03bce5c72c19d8d785ffffc51ae
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 reading the current LTSSM state as a string and raw value.

> 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"

Should this be a bool instead of a tristate?

[ ... ]

> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index 9a464cbaf073d..2320319af83bc 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h

[ ... ]

> +#ifdef CONFIG_PCIE_CADENCE_DEBUGFS
> +void cdns_pcie_debugfs_deinit(struct cdns_pcie *pci);
> +void cdns_pcie_debugfs_init(struct cdns_pcie *pci);
> +#else
> +static inline void cdns_pcie_debugfs_deinit(struct cdns_pcie *pci)
> +{
> +}
> +static inline void cdns_pcie_debugfs_init(struct cdns_pcie *pci)
> +{
> +}
> +#endif

When configured as a module (=m), Kbuild defines
CONFIG_PCIE_CADENCE_DEBUGFS_MODULE instead of CONFIG_PCIE_CADENCE_DEBUGFS.
Does this #ifdef evaluate to false and silently use the dummy stubs instead
of initializing the debugfs features?

If this were fixed to use IS_ENABLED(), could it introduce linker errors
when the host driver is built-in (=y) but debugfs is built as a module (=m)?
Changing the Kconfig definition to bool would avoid this.

[ ... ]

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-debugfs.c b/drivers/pci/controller/cadence/pcie-cadence-debugfs.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pci/controller/cadence/pcie-cadence-debugfs.c

[ ... ]

> +void cdns_pcie_debugfs_init(struct cdns_pcie *pci)
> +{
> +	char dirname[CDNS_DEBUGFS_BUF_MAX];
> +	struct device *dev = pci->dev;
> +
> +	/* Create main directory for each platform driver. */
> +	snprintf(dirname, CDNS_DEBUGFS_BUF_MAX, "cdns_pcie_%s", dev_name(dev));
> +	pci->debug_dir = debugfs_create_dir(dirname, NULL);
> +
> +	cdns_pcie_ltssm_debugfs_init(pci, pci->debug_dir);
> +}
> +EXPORT_SYMBOL_GPL(cdns_pcie_debugfs_init);

Since this file can currently be built as a module, does it need a
MODULE_LICENSE() declaration? Without it, loading the module might fail
when trying to use GPL-only exported symbols like debugfs_create_dir().

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