From: Hans Zhang <18255117159@163.com>
To: sashiko@lists.linux.dev, Manikandan K Pillai <mpillai@cadence.com>
Cc: linux-pci@vger.kernel.org
Subject: Re: [PATCH v4 2/2] PCI: cadence: Add debugfs property to provide LTSSM status of the PCIe link
Date: Fri, 8 May 2026 12:31:12 +0800 [thread overview]
Message-ID: <10503cf8-2f2c-4a8d-8c24-f1056859edd8@163.com> (raw)
In-Reply-To: <20260508041956.C8F10C2BCB0@smtp.kernel.org>
On 5/8/26 12:19, sashiko-bot@kernel.org wrote:
> 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?
This is the reminder from Mani that I made the necessary modifications
for, because the Cadence PCIe driver is a support module.
>
> 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.
Hi Manikandan,
I have already confirmed this with Manikandan. You can ask Manikandan to
confirm it again.
>
> [ ... ]
>> +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?
Will change.
>
> 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 HPA driver does not yet provide the "stop the PCIe link" function.
More will be added later.
>
> 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?
Will change.
>
>> +}
>
> [ ... ]
>
>> 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?
Will change.
Best regards,
Hans
>
next prev parent reply other threads:[~2026-05-08 4:31 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-08 3:40 [PATCH v4 0/2] PCI: cadence: Add LTSSM debugfs Hans Zhang
2026-05-08 3:41 ` [PATCH v4 1/2] PCI: cadence: Add HPA architecture flag Hans Zhang
2026-05-08 3:41 ` [PATCH v4 2/2] PCI: cadence: Add debugfs property to provide LTSSM status of the PCIe link Hans Zhang
2026-05-08 4:19 ` sashiko-bot
2026-05-08 4:31 ` Hans Zhang [this message]
2026-05-13 5:23 ` Aksh Garg
2026-05-13 6:11 ` Manikandan Karunakaran Pillai
2026-05-13 6:39 ` Hans Zhang
2026-05-13 7:08 ` Manikandan Karunakaran Pillai
2026-05-13 8:34 ` Aksh Garg
2026-05-13 8:55 ` Hans Zhang
2026-05-13 11:30 ` Aksh Garg
2026-05-13 12:04 ` Hans Zhang
2026-05-14 1:39 ` Manikandan Karunakaran Pillai
2026-05-14 1:46 ` Hans Zhang
2026-05-14 4:25 ` Manikandan Karunakaran Pillai
2026-05-13 6:37 ` Hans Zhang
2026-05-13 8:25 ` Aksh Garg
2026-05-13 8:29 ` Hans Zhang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=10503cf8-2f2c-4a8d-8c24-f1056859edd8@163.com \
--to=18255117159@163.com \
--cc=linux-pci@vger.kernel.org \
--cc=mpillai@cadence.com \
--cc=sashiko@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).