[PATCH v3 00/16] PCI: dwc: Cache PCIe capability offset and simplify drivers

[PATCH v3 00/16] PCI: dwc: Cache PCIe capability offset and simplify drivers

[Hans Zhang]

Hi,

The DWC PCIe core and its many platform drivers repeatedly call
dw_pcie_find_capability(pci, PCI_CAP_ID_EXP) to obtain the offset of the
PCI Express Capability structure. This is wasteful and makes the code
verbose. Some drivers even search for the offset in suspend/resume paths.

Add a cached pcie_cap field in struct dw_pcie and a helper
dw_pcie_get_pcie_cap() to initialize it once at the point when the
hardware is ready. Then replace all explicit capability searches with
the cached value across the entire dwc subtree.

**Safety analysis: DBI access timing**
The PCIe Capability offset is read from DBI configuration space. DBI
registers are only accessible after the controller's clocks, resets, and
power are enabled. The following call graph demonstrates that all
dw_pcie_find_capability() replacements occur only after hardware is ready:

- For Root Complex mode:
  dw_pcie_host_init()
    ...
    if (pp->ops->init)
      pp->ops->init   [enables clocks/resets]
    dw_pcie_get_pcie_cap()   [caches offset]
    ...
    dw_pcie_link_up
      pci->ops->link_up
    ...
    dw_pcie_start_link
      pci->ops->start_link
    ...
    pci_host_probe
    if (pp->ops->post_init)
      pp->ops->post_init
    ...

- For Endpoint mode:
  The core no longer automatically caches the offset in dw_pcie_ep_init().
  Instead, each EP driver must ensure hardware is enabled (clocks, power,
  PHY) before calling dw_pcie_get_pcie_cap(). Drivers like qcom-ep and
  tegra194 perform this in the PERST# deassert handler, where DBI is
  accessible. The helper will then cache the offset for subsequent uses.

- Some platform drivers (e.g., layerscape-ep) enable hardware themselves
  before calling dw_pcie_ep_init() and explicitly call
  dw_pcie_get_pcie_cap() to cache the offset early. This is safe.

Thus, no DBI access occurs before hardware is operational.

Changes in v3:
- Remove automatic caching from dw_pcie_ep_init() to prevent unclocked
  DBI access on EP platforms (qcom-ep, tegra194).
- Add explicit dw_pcie_get_pcie_cap() calls in qcom-ep and tegra194 EP
  paths after hardware enablement.
- Keep Host mode caching unchanged.
- Update commit messages accordingly.

Changes in v2:
- Move dw_pcie_get_pcie_cap() from the beginning of dw_pcie_host_init()
  and dw_pcie_ep_init() to after the hardware initialization callbacks
  (host_init/pre_init). This can prevent the reading of DBI when the clock
  is turned off or when the hardware is not ready.
- Convert all platform drivers to use the cached offset via
  dw_pcie_get_pcie_cap() or directly pci->pcie_cap where safe.
- Fix type mismatches (u16 -> u8).
- Properly split per-driver modifications.

v1: https://patchwork.kernel.org/project/linux-pci/cover/20260509135152.2241235-1-18255117159@163.com/

v2: https://patchwork.kernel.org/project/linux-pci/patch/20260530153101.695580-1-18255117159@163.com/

---
**Why splitting into per-driver patches**  
To facilitate review and potential bisection, each platform driver is
modified in its own patch. The core changes are in patches 1-2, and
follow-up patches convert individual glue drivers. If maintainers prefer
to squash them, please let me know and I will merge in v3.

This splitting patch of each controller was carried out solely for
the convenience of code review.
---

Hans Zhang (16):
  PCI: dwc: Add pcie_cap field and helper in designware header
  PCI: dwc: Use cached PCIe capability offset in core
  PCI: dwc: imx6: Use cached PCIe capability offset
  PCI: dwc: layerscape-ep: Use cached PCIe capability offset
  PCI: dwc: meson: Use cached PCIe capability offset
  PCI: dwc: rockchip: Use cached PCIe capability offset
  PCI: dwc: eswin: Use cached PCIe capability offset
  PCI: dwc: fu740: Use cached PCIe capability offset
  PCI: dwc: intel-gw: Use cached PCIe capability offset
  PCI: dwc: qcom-ep: Use cached PCIe capability offset
  PCI: dwc: qcom: Use cached PCIe capability offset
  PCI: dwc: sophgo: Use cached PCIe capability offset
  PCI: dwc: spacemit-k1: Use cached PCIe capability offset
  PCI: dwc: spear13xx: Use cached PCIe capability offset
  PCI: dwc: tegra194: Use cached PCIe capability offset
  PCI: dwc: ultrarisc: Use cached PCIe capability offset

 drivers/pci/controller/dwc/pci-imx6.c         |  6 +++---
 .../pci/controller/dwc/pci-layerscape-ep.c    |  9 +++-----
 drivers/pci/controller/dwc/pci-meson.c        |  4 ++--
 .../pci/controller/dwc/pcie-designware-ep.c   |  2 +-
 .../pci/controller/dwc/pcie-designware-host.c |  2 ++
 drivers/pci/controller/dwc/pcie-designware.c  | 15 ++++++-------
 drivers/pci/controller/dwc/pcie-designware.h  | 17 +++++++++++++++
 drivers/pci/controller/dwc/pcie-dw-rockchip.c |  2 +-
 drivers/pci/controller/dwc/pcie-eswin.c       |  3 +--
 drivers/pci/controller/dwc/pcie-fu740.c       |  2 +-
 drivers/pci/controller/dwc/pcie-intel-gw.c    |  2 +-
 drivers/pci/controller/dwc/pcie-qcom-ep.c     | 13 ++++++------
 drivers/pci/controller/dwc/pcie-qcom.c        | 21 ++++++++-----------
 drivers/pci/controller/dwc/pcie-sophgo.c      |  5 +++--
 drivers/pci/controller/dwc/pcie-spacemit-k1.c |  2 +-
 drivers/pci/controller/dwc/pcie-spear13xx.c   |  2 +-
 drivers/pci/controller/dwc/pcie-tegra194.c    |  6 ++----
 drivers/pci/controller/dwc/pcie-ultrarisc.c   |  2 +-
 18 files changed, 61 insertions(+), 54 deletions(-)


base-commit: 58717b2a1365d06c8c64b72aa948541b53fe31eb
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 01/16] PCI: dwc: Add pcie_cap field and helper in designware header

[Hans Zhang]

Add a pcie_cap field to struct dw_pcie to store the offset of the
PCI Express Capability structure. Provide a helper dw_pcie_get_pcie_cap()
which performs the capability search on first call and caches the result.

This is a preparatory step for replacing repetitive capability searches
in both core and platform drivers.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/controller/dwc/pcie-designware.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index de4b245b1758..8113efd33a24 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -591,6 +591,8 @@ struct dw_pcie {
 	 * use_parent_dt_ranges to true to avoid this warning.
 	 */
 	bool			use_parent_dt_ranges;
+
+	u8			pcie_cap;	/* PCIe capability offset */
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
@@ -829,6 +831,21 @@ static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci)
 	dw_pcie_writel_dbi(pci, reg, val);
 }
 
+/**
+ * dw_pcie_get_pcie_cap() - Return cached PCIe Capability offset
+ * @pci: DWC instance
+ *
+ * Finds and caches the offset of PCI_CAP_ID_EXP on first call.
+ * Returns 0 if the capability is not present.
+ */
+static inline u8 dw_pcie_get_pcie_cap(struct dw_pcie *pci)
+{
+	if (!pci->pcie_cap)
+		pci->pcie_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+
+	return pci->pcie_cap;
+}
+
 static inline int dw_pcie_start_link(struct dw_pcie *pci)
 {
 	if (pci->ops && pci->ops->start_link)
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 01/16] PCI: dwc: Add pcie_cap field and helper in designware header

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Data race and logically flawed caching in dw_pcie_get_pcie_cap() due to unprotected read-modify-write and failure to cache 'not found' state.
--

--- Patch [1]: [PATCH v3 01/16] PCI: dwc: Add pcie_cap field and helper in designware header ---
commit ab22dc6f534d77b34df13c99dd929771547f5ff1
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: Add pcie_cap field and helper in designware header

This commit adds a pcie_cap field to struct dw_pcie to cache the offset of
the PCI Express Capability structure. It introduces dw_pcie_get_pcie_cap()
to perform the capability search on the first call and cache the result.

> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
[ ... ]
> @@ -829,6 +831,21 @@ static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci)
>  	dw_pcie_writel_dbi(pci, reg, val);
>  }
>  
> +/**
> + * dw_pcie_get_pcie_cap() - Return cached PCIe Capability offset
> + * @pci: DWC instance
> + *
> + * Finds and caches the offset of PCI_CAP_ID_EXP on first call.
> + * Returns 0 if the capability is not present.
> + */
> +static inline u8 dw_pcie_get_pcie_cap(struct dw_pcie *pci)
> +{
> +	if (!pci->pcie_cap)
> +		pci->pcie_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);

[Severity: Medium]
Will this repeatedly search the configuration space if the capability is
not found? If dw_pcie_find_capability() returns 0 when the capability is
missing, pci->pcie_cap will remain 0, causing the search to be repeated
on every subsequent call instead of being cached.

Also, since this helper might be called concurrently by different threads,
could the unprotected read and write to pci->pcie_cap cause a data race?

> +
> +	return pci->pcie_cap;
> +}

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 01/16] PCI: dwc: Add pcie_cap field and helper in designware header

[Hans Zhang]

On 7/20/26 23:17, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Data race and logically flawed caching in dw_pcie_get_pcie_cap() due to unprotected read-modify-write and failure to cache 'not found' state.
> --
> 
> --- Patch [1]: [PATCH v3 01/16] PCI: dwc: Add pcie_cap field and helper in designware header ---
> commit ab22dc6f534d77b34df13c99dd929771547f5ff1
> Author: Hans Zhang <18255117159@163.com>
> 
> PCI: dwc: Add pcie_cap field and helper in designware header
> 
> This commit adds a pcie_cap field to struct dw_pcie to cache the offset of
> the PCI Express Capability structure. It introduces dw_pcie_get_pcie_cap()
> to perform the capability search on the first call and cache the result.
> 
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> [ ... ]
>> @@ -829,6 +831,21 @@ static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci)
>>   	dw_pcie_writel_dbi(pci, reg, val);
>>   }
>>   
>> +/**
>> + * dw_pcie_get_pcie_cap() - Return cached PCIe Capability offset
>> + * @pci: DWC instance
>> + *
>> + * Finds and caches the offset of PCI_CAP_ID_EXP on first call.
>> + * Returns 0 if the capability is not present.
>> + */
>> +static inline u8 dw_pcie_get_pcie_cap(struct dw_pcie *pci)
>> +{
>> +	if (!pci->pcie_cap)
>> +		pci->pcie_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> 
> [Severity: Medium]
> Will this repeatedly search the configuration space if the capability is
> not found? If dw_pcie_find_capability() returns 0 when the capability is
> missing, pci->pcie_cap will remain 0, causing the search to be repeated
> on every subsequent call instead of being cached.
> 
> Also, since this helper might be called concurrently by different threads,
> could the unprotected read and write to pci->pcie_cap cause a data race?
> 

Hi,

As mentioned in the previous reply, there is no need for us to worry 
about this.

https://lore.kernel.org/linux-pci/5cc6fbcc-98eb-4da5-b123-2c04c4d39326@163.com/


Best regards,
Hans


>> +
>> +	return pci->pcie_cap;
>> +}
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 02/16] PCI: dwc: Use cached PCIe capability offset in core

[Hans Zhang]

Modify the DWC core functions to use the cached pcie_cap offset instead
of calling dw_pcie_find_capability() each time.

In the DWC core, dw_pcie_find_capability() is called at several locations:
- dw_pcie_ep_init_non_sticky_registers()
- dw_pcie_wait_for_link()
- dw_pcie_link_set_max_speed()
- dw_pcie_link_get_max_link_width()
- dw_pcie_link_set_max_link_width()

The cached offset is initialized after hardware is ready:
- In host mode: dw_pcie_host_init() calls pp->ops->host_init() (enables
  clocks/resets), then dw_pcie_get_pcie_cap() caches the offset.
- In endpoint mode: the core no longer caches automatically. Instead,
  drivers must call dw_pcie_get_pcie_cap() after hardware is enabled
  (e.g., after PERST# deassert). dw_pcie_ep_init_non_sticky_registers()
  is called after that point, so it can safely use pci->pcie_cap.

dw_pcie_ep_init_non_sticky_registers() now assumes pci->pcie_cap is
valid. if not, it prints a warning and skips the operation. The other
functions run after probe, so pci->pcie_cap is already valid and can
be used directly.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c   |  2 +-
 drivers/pci/controller/dwc/pcie-designware-host.c |  2 ++
 drivers/pci/controller/dwc/pcie-designware.c      | 15 ++++++---------
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 7d2794945704..14228be8bbce 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -1246,7 +1246,7 @@ static void dw_pcie_ep_init_non_sticky_registers(struct dw_pcie *pci)
 	 * to all other functions as well.
 	 */
 	if (funcs > 1) {
-		offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+		offset = pci->pcie_cap;
 		func0_lnkcap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
 		func0_lnkcap = FIELD_GET(PCI_EXP_LNKCAP_MLW |
 					 PCI_EXP_LNKCAP_SLS, func0_lnkcap);
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 06722259d2e3..fee800ac56d4 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -593,6 +593,8 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 			goto err_free_ecam;
 	}
 
+	dw_pcie_get_pcie_cap(pci);
+
 	if (pci_msi_enabled()) {
 		pp->use_imsi_rx = !(pp->ops->msi_init ||
 				     of_property_present(np, "msi-parent") ||
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index ec4722ed9303..810729b91892 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -766,7 +766,7 @@ const char *dw_pcie_ltssm_status_string(enum dw_pcie_ltssm ltssm)
  */
 int dw_pcie_wait_for_link(struct dw_pcie *pci)
 {
-	u32 offset, val, ltssm;
+	u32 val, ltssm;
 	int retries;
 
 	/* Check if the link is up or not */
@@ -806,8 +806,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
 
 	pci_host_common_link_train_delay(pci->max_link_speed);
 
-	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
+	val = dw_pcie_readw_dbi(pci, pci->pcie_cap + PCI_EXP_LNKSTA);
 
 	dev_info(pci->dev, "PCIe Gen.%u x%u link up\n",
 		 FIELD_GET(PCI_EXP_LNKSTA_CLS, val),
@@ -843,7 +842,7 @@ EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
 static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
 {
 	u32 cap, ctrl2, link_speed;
-	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u8 offset = pci->pcie_cap;
 
 	cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
 
@@ -890,7 +889,7 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
 int dw_pcie_link_get_max_link_width(struct dw_pcie *pci)
 {
 	u8 cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	u32 lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
+	u32 lnkcap = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);
 
 	return FIELD_GET(PCI_EXP_LNKCAP_MLW, lnkcap);
 }
@@ -898,7 +897,6 @@ int dw_pcie_link_get_max_link_width(struct dw_pcie *pci)
 static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
 {
 	u32 lnkcap, lwsc, plc;
-	u8 cap;
 
 	if (!num_lanes)
 		return;
@@ -935,10 +933,9 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
 	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc);
 	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc);
 
-	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
+	lnkcap = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);
 	FIELD_MODIFY(PCI_EXP_LNKCAP_MLW, &lnkcap, num_lanes);
-	dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap);
+	dw_pcie_writel_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP, lnkcap);
 }
 
 void dw_pcie_iatu_detect(struct dw_pcie *pci)
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 02/16] PCI: dwc: Use cached PCIe capability offset in core

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized `pci->pcie_cap` used in Endpoint mode causes severe PCIe configuration space corruption.
- [Low] Unnecessary PCIe capability search and unused variable left intact.
--

--- Patch [2]: [PATCH v3 02/16] PCI: dwc: Use cached PCIe capability offset in core ---
commit 1e2ce6ece477d5f9c179f2ac4c13cd06cf764f59
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: Use cached PCIe capability offset in core
    
Modify the DWC core functions to use the cached pcie_cap offset instead
of calling dw_pcie_find_capability() each time.

[ ... ]

> dw_pcie_ep_init_non_sticky_registers() now assumes pci->pcie_cap is
> valid. if not, it prints a warning and skips the operation. The other
> functions run after probe, so pci->pcie_cap is already valid and can
> be used directly.

[Severity: High]
The commit message mentions adding a warning and skipping the operation if
pci->pcie_cap is not valid, but this check appears to be missing in the
code below.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 7d2794945704e..14228be8bbce7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -1246,7 +1246,7 @@ static void dw_pcie_ep_init_non_sticky_registers(struct dw_pcie *pci)
>  	 * to all other functions as well.
>  	 */
>  	if (funcs > 1) {
> -		offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +		offset = pci->pcie_cap;

[Severity: High]
Without the validation check mentioned in the commit message, if
pci->pcie_cap is uninitialized (0), will this lead to corrupting the
standard PCI configuration space header registers instead of the intended
capability registers?

[ ... ]

> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index ec4722ed93032..810729b918928 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -806,8 +806,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>  
>  	pci_host_common_link_train_delay(pci->max_link_speed);
>  
> -	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
> +	val = dw_pcie_readw_dbi(pci, pci->pcie_cap + PCI_EXP_LNKSTA);

[Severity: High]
If pci->pcie_cap is 0, this will read from 0 + PCI_EXP_LNKSTA (0x12) instead
of the actual capability offset. Could this lead to failed link status
checks?

[ ... ]

> @@ -890,7 +889,7 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
>  int dw_pcie_link_get_max_link_width(struct dw_pcie *pci)
>  {
>  	u8 cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	u32 lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
> +	u32 lnkcap = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);

[Severity: Low]
Is the cap variable still needed here? It looks like it is now unused, which
might cause an unnecessary MMIO read during the capability search and trigger
a compiler warning.

>  	return FIELD_GET(PCI_EXP_LNKCAP_MLW, lnkcap);
>  }
> @@ -898,7 +897,6 @@ int dw_pcie_link_get_max_link_width(struct dw_pcie *pci)
>  static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
>  {
>  	u32 lnkcap, lwsc, plc;
> -	u8 cap;
>  
>  	if (!num_lanes)
>  		return;
> @@ -935,10 +933,9 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
>  	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc);
>  	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc);
>  
> -	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
> +	lnkcap = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);
>  	FIELD_MODIFY(PCI_EXP_LNKCAP_MLW, &lnkcap, num_lanes);
> -	dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap);
> +	dw_pcie_writel_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP, lnkcap);
>  }

[Severity: High]
If pci->pcie_cap happens to be 0 here, this will perform a read-modify-write
at offset 0 + PCI_EXP_LNKCAP (0x0C). Could this destroy the standard
PCI Header Type and BIST registers?

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 03/16] PCI: dwc: imx6: Use cached PCIe capability offset

[Hans Zhang]

Thus, the cached offset is available when these functions are called, and
DBI access occurs only after hardware is enabled.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pci-imx6, dw_pcie_find_capability() is called in the following
call chain:

  static const struct dw_pcie_ops dw_pcie_ops = {
    .start_link = imx_pcie_start_link,
  };
  imx_pcie_start_link()
    -> dw_pcie_find_capability()
    -> imx_pcie_ltssm_enable()
      -> dw_pcie_find_capability()

Replace these calls with the cached pci->pcie_cap. The call chain after
modification becomes:

  dw_pcie_host_init()
    -> pp->ops->init() [imx6_pcie_host_init]
    -> dw_pcie_get_pcie_cap()  (caches offset)
    -> pci->ops->start_link = imx_pcie_start_link
      -> uses pci->pcie_cap
      -> imx_pcie_ltssm_enable()
        -> uses pci->pcie_cap
---
 drivers/pci/controller/dwc/pci-imx6.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 98e1db751132..74b28b4df484 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -966,10 +966,10 @@ static void imx_pcie_ltssm_enable(struct device *dev)
 {
 	struct imx_pcie *imx_pcie = dev_get_drvdata(dev);
 	const struct imx_pcie_drvdata *drvdata = imx_pcie->drvdata;
-	u8 offset = dw_pcie_find_capability(imx_pcie->pci, PCI_CAP_ID_EXP);
+	struct dw_pcie *pci = imx_pcie->pci;
 	u32 tmp;
 
-	tmp = dw_pcie_readl_dbi(imx_pcie->pci, offset + PCI_EXP_LNKCAP);
+	tmp = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);
 	phy_set_speed(imx_pcie->phy, FIELD_GET(PCI_EXP_LNKCAP_SLS, tmp));
 	if (drvdata->ltssm_mask)
 		regmap_update_bits(imx_pcie->iomuxc_gpr, drvdata->ltssm_off, drvdata->ltssm_mask,
@@ -995,7 +995,7 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
 {
 	struct imx_pcie *imx_pcie = to_imx_pcie(pci);
 	struct device *dev = pci->dev;
-	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u8 offset = pci->pcie_cap;
 	u32 tmp;
 	int ret;
 
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 03/16] PCI: dwc: imx6: Use cached PCIe capability offset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] In Endpoint (EP) mode, the cached `pci->pcie_cap` offset is uninitialized and defaults to 0, causing `imx_pcie_start_link` and `imx_pcie_ltssm_enable` to read and corrupt the standard PCI configuration space header instead of the PCIe Link Capabilities register.
--

--- Patch [3]: [PATCH v3 03/16] PCI: dwc: imx6: Use cached PCIe capability offset ---
commit e88cd77b027a125e9713fc310c612c5158a23be1
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: imx6: Use cached PCIe capability offset

This commit updates the i.MX6 PCIe driver to use the cached pcie_cap
offset instead of dynamically finding the PCI_CAP_ID_EXP capability
during link start and LTSSM enablement.

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 98e1db751132a..74b28b4df4846 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -966,10 +966,10 @@ static void imx_pcie_ltssm_enable(struct device *dev)
>  {
>  	struct imx_pcie *imx_pcie = dev_get_drvdata(dev);
>  	const struct imx_pcie_drvdata *drvdata = imx_pcie->drvdata;
> -	u8 offset = dw_pcie_find_capability(imx_pcie->pci, PCI_CAP_ID_EXP);
> +	struct dw_pcie *pci = imx_pcie->pci;
>  	u32 tmp;
>  
> -	tmp = dw_pcie_readl_dbi(imx_pcie->pci, offset + PCI_EXP_LNKCAP);
> +	tmp = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);

[Severity: High]
Is pci->pcie_cap guaranteed to be initialized here in Endpoint (EP) mode?

It looks like imx_add_pcie_ep() does not call dw_pcie_get_pcie_cap() after
the hardware is enabled. If pci->pcie_cap evaluates to 0 in EP mode, wouldn't
this read from offset 0x0C (Cache Line Size, Latency Timer, etc.) in the
standard PCI configuration space instead of the PCIe Link Capabilities register?

>  	phy_set_speed(imx_pcie->phy, FIELD_GET(PCI_EXP_LNKCAP_SLS, tmp));
>  	if (drvdata->ltssm_mask)

[ ... ]

> @@ -995,7 +995,7 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>  {
>  	struct imx_pcie *imx_pcie = to_imx_pcie(pci);
>  	struct device *dev = pci->dev;
> -	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u8 offset = pci->pcie_cap;
>  	u32 tmp;
>  	int ret;

[ ... ]

>  	dw_pcie_dbi_ro_wr_en(pci);
>  	tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>  	tmp &= ~PCI_EXP_LNKCAP_SLS;
>  	tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
>  	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);

[Severity: High]
Can this corrupt the standard PCI configuration space header in EP mode?

Since offset is derived from pci->pcie_cap, if it remains uninitialized
(0) for endpoints, this write would target offset 0x0C in the standard PCI
header. This could corrupt the Cache Line Size, Latency Timer, Header Type,
and BIST registers.

Does the endpoint initialization path need a call to dw_pcie_get_pcie_cap()
before the link is started via dw_pcie_ep_start()?

>  	dw_pcie_dbi_ro_wr_dis(pci);

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 04/16] PCI: dwc: layerscape-ep: Use cached PCIe capability offset

[Hans Zhang]

Replace these with dw_pcie_get_pcie_cap(). The hardware is already enabled
by the driver before ls_pcie_ep_probe() and before the interrupt handler
runs. dw_pcie_get_pcie_cap() will cache the offset on first call, and
subsequent calls (including inside dw_pcie_ep_init) will use the cached
value without re-searching.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pci-layerscape-ep, dw_pcie_find_capability() is called in:

  ls_pcie_ep_probe()
    -> offset = dw_pcie_find_capability()
    -> pcie->lnkcap = dw_pcie_readl_dbi(offset + PCI_EXP_LNKCAP)
    -> dw_pcie_ep_init()
    -> ls_pcie_ep_interrupt_init()
        -> devm_request_irq(..., ls_pcie_ep_event_handler)
          -> ls_pcie_ep_event_handler()
            -> dw_pcie_find_capability()
---
 drivers/pci/controller/dwc/pci-layerscape-ep.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
index 8936975ff104..b2d0b51df3c5 100644
--- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
+++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
@@ -74,7 +74,6 @@ static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
 	struct ls_pcie_ep *pcie = dev_id;
 	struct dw_pcie *pci = pcie->pci;
 	u32 val, cfg;
-	u8 offset;
 
 	val = ls_pcie_pf_lut_readl(pcie, PEX_PF0_PME_MES_DR);
 	ls_pcie_pf_lut_writel(pcie, PEX_PF0_PME_MES_DR, val);
@@ -83,9 +82,6 @@ static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	if (val & PEX_PF0_PME_MES_DR_LUD) {
-
-		offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-
 		/*
 		 * The values of the Maximum Link Width and Supported Link
 		 * Speed from the Link Capabilities Register will be lost
@@ -93,7 +89,8 @@ static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
 		 * that configured by the Reset Configuration Word (RCW).
 		 */
 		dw_pcie_dbi_ro_wr_en(pci);
-		dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, pcie->lnkcap);
+		dw_pcie_writel_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP,
+				   pcie->lnkcap);
 		dw_pcie_dbi_ro_wr_dis(pci);
 
 		cfg = ls_pcie_pf_lut_readl(pcie, PEX_PF0_CONFIG);
@@ -266,7 +263,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcie);
 
-	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	offset = dw_pcie_get_pcie_cap(pci);
 	pcie->lnkcap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
 
 	ret = dw_pcie_ep_init(&pci->ep);
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 04/16] PCI: dwc: layerscape-ep: Use cached PCIe capability offset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Read-modify-write data race on PCIE_MISC_CONTROL_1_OFF hardware register.
--

--- Patch [4]: [PATCH v3 04/16] PCI: dwc: layerscape-ep: Use cached PCIe capability offset ---
commit 702ab6e16bc3343bb66237e6ea619ecc9e086c7f
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: layerscape-ep: Use cached PCIe capability offset

This commit replaces dw_pcie_find_capability() with dw_pcie_get_pcie_cap()
to cache and reuse the PCIe capability offset. This avoids re-searching
the capability list during probe and interrupt handling.

> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> @@ -83,9 +82,6 @@ static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
>  		return IRQ_NONE;
>  
>  	if (val & PEX_PF0_PME_MES_DR_LUD) {
> -
> -		offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -
>  		/*
>  		 * The values of the Maximum Link Width and Supported Link
>  		 * Speed from the Link Capabilities Register will be lost
>  		 * during link down or hot reset. Restore initial value
>  		 * that configured by the Reset Configuration Word (RCW).
>  		 */
>  		dw_pcie_dbi_ro_wr_en(pci);
> -		dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, pcie->lnkcap);
> +		dw_pcie_writel_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP,
> +				   pcie->lnkcap);
>  		dw_pcie_dbi_ro_wr_dis(pci);

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential data race 
on the PCIE_MISC_CONTROL_1_OFF hardware register here?

The ls_pcie_ep_event_handler() function runs in hardirq context. When it
calls dw_pcie_dbi_ro_wr_en() and dw_pcie_dbi_ro_wr_dis(), it performs an
unlocked read-modify-write on the register and then unconditionally clears
the enable bit:

drivers/pci/controller/dwc/pcie-designware.h:dw_pcie_dbi_ro_wr_dis() {
    reg = PCIE_MISC_CONTROL_1_OFF;
    val = dw_pcie_readl_dbi(pci, reg);
    val &= ~PCIE_DBI_RO_WR_EN;
    dw_pcie_writel_dbi(pci, reg, val);
}

If the host triggers a Link Up event while the endpoint configuration is
taking place via configfs (which runs in process context), couldn't the
interrupt preempt the configuration thread? 

If the preempted thread had just set PCIE_DBI_RO_WR_EN to write registers,
the interrupt would unconditionally clear it. When the preempted thread
resumes, its writes to read-only hardware registers (like Vendor ID or
BAR configurations) would be silently ignored by the controller, which
could lead to enumeration failure on the host.

Should this register access be protected by a lock, or should the
enable/disable functions save and restore the previous register state?

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 05/16] PCI: dwc: meson: Use cached PCIe capability offset

[Hans Zhang]

dw_pcie_host_init() calls pp->ops->init (meson_pcie_host_init) before
caching the offset. Therefore, inside .init we must call
dw_pcie_get_pcie_cap() to obtain the offset (the helper will perform the
DBI read and cache the result). This is safe because the hardware is
already enabled by the driver's own initialization.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pci-meson, the call chain is:

  static const struct dw_pcie_host_ops meson_pcie_host_ops = {
    .init = meson_pcie_host_init,
  };
  meson_pcie_host_init()
    -> meson_set_max_payload()
      -> dw_pcie_find_capability()
    -> meson_set_max_rd_req_size()
      -> dw_pcie_find_capability()
---
 drivers/pci/controller/dwc/pci-meson.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
index 7a4da9fae1ea..c9d0388d262b 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -277,7 +277,7 @@ static void meson_set_max_payload(struct meson_pcie *mp, int size)
 {
 	struct dw_pcie *pci = &mp->pci;
 	u32 val;
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u8 offset = dw_pcie_get_pcie_cap(pci);
 	int max_payload_size = meson_size_to_payload(mp, size);
 
 	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
@@ -293,7 +293,7 @@ static void meson_set_max_rd_req_size(struct meson_pcie *mp, int size)
 {
 	struct dw_pcie *pci = &mp->pci;
 	u32 val;
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u8 offset = dw_pcie_get_pcie_cap(pci);
 	int max_rd_req_size = meson_size_to_payload(mp, size);
 
 	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 05/16] PCI: dwc: meson: Use cached PCIe capability offset

[sashiko-bot]

> dw_pcie_host_init() calls pp->ops->init (meson_pcie_host_init) before
> caching the offset. Therefore, inside .init we must call
> dw_pcie_get_pcie_cap() to obtain the offset (the helper will perform the
> DBI read and cache the result). This is safe because the hardware is
> already enabled by the driver's own initialization.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 06/16] PCI: dwc: rockchip: Use cached PCIe capability offset

[Hans Zhang]

dw_pcie_host_init() calls .init before caching the offset. So inside
.init we call dw_pcie_get_pcie_cap() to trigger caching. The helper will
perform the DBI read (hardware is already enabled) and cache the result.

Signed-off-by: Hans Zhang <18255117159@163.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
In pcie-dw-rockchip, the call chain is:

  static const struct dw_pcie_host_ops rockchip_pcie_host_ops = {
    .init = rockchip_pcie_host_init,
  };
  rockchip_pcie_host_init()
    -> rockchip_pcie_enable_l0s()
      -> dw_pcie_find_capability()
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 731d93663cca..be8b6187913d 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -369,7 +369,7 @@ static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
 	u32 cap, lnkcap;
 
 	/* Enable L0S capability for all SoCs */
-	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	cap = dw_pcie_get_pcie_cap(pci);
 	if (cap) {
 		lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
 		lnkcap |= PCI_EXP_LNKCAP_ASPM_L0S;
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 06/16] PCI: dwc: rockchip: Use cached PCIe capability offset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The DWC core code unconditionally uses `pci->pcie_cap` without checking if it is zero, leading to silent PCI configuration space corruption, particularly in endpoint (EP) mode where it remains uninitialized for many drivers.
- [Low] An unused variable and redundant DBI read were left behind in `dw_pcie_link_get_max_link_width()` during the `pci->pcie_cap` refactoring.

Pre-existing issues:
- [High] Missing cleanup of chained IRQ handler and IRQ domain on probe failure causes Use-After-Free.
--

--- Patch [6]: [PATCH v3 06/16] PCI: dwc: rockchip: Use cached PCIe capability offset ---
commit 66fc930727ddec47d08c4dc09e796cb0d73aa5a9
Author: Hans Zhang <18255117159@163.com>

    PCI: dwc: rockchip: Use cached PCIe capability offset
    
    dw_pcie_host_init() calls .init before caching the offset. So inside
    .init we call dw_pcie_get_pcie_cap() to trigger caching. The helper will
    perform the DBI read (hardware is already enabled) and cache the result.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 731d93663ccae..be8b6187913df 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -369,7 +369,7 @@ static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
>  	u32 cap, lnkcap;
>  
>  	/* Enable L0S capability for all SoCs */
> -	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	cap = dw_pcie_get_pcie_cap(pci);
>  	if (cap) {
>  		lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
>  		lnkcap |= PCI_EXP_LNKCAP_ASPM_L0S;

[Severity: High]
This is a pre-existing issue, but does this code leave a dangling pointer and
cause a use-after-free on probe failure?

drivers/pci/controller/dwc/pcie-dw-rockchip.c:
rockchip_pcie_host_init() {
    ...
	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
					 rockchip);
    ...
}

The chained handler is registered with the rockchip devres-managed structure.
If dw_pcie_host_init() fails later in the probe process (for example, while
waiting for the link to come up), the devres framework frees the rockchip
structure.

Since rockchip_pcie_host_ops lacks a .deinit callback to unregister the
chained handler and free the domain, will the handler dereference the freed
rockchip struct if the legacy IRQ fires afterward?

[Severity: High]
Can this corrupt the PCI configuration space if pci->pcie_cap is 0?

drivers/pci/controller/dwc/pcie-designware-ep.c:
dw_pcie_ep_init_non_sticky_registers() {
    ...
	dw_pcie_setup(pci);
    ...
}

drivers/pci/controller/dwc/pcie-designware.c:
dw_pcie_link_set_max_speed() {
    ...
	u8 offset = pci->pcie_cap;

	cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
    ...
}

If an EP driver does not call dw_pcie_get_pcie_cap() during its .init
callback, pci->pcie_cap remains 0. The dw_pcie_ep_init_non_sticky_registers()
function unconditionally calls dw_pcie_setup() without verifying if
pci->pcie_cap was initialized.

Does this blindly use offset 0 instead of the PCIe capability structure,
permanently corrupting critical PCI config registers (like Cache Line Size,
Latency Timer, Header Type, or BIST) when offset 0 is accessed?

This also seems contrary to the commit message's claim that a warning would
be printed and the operation skipped, as the DWC core code uses the offset
unconditionally.

[Severity: Low]
Is an unused variable left behind, causing a redundant DBI read?

drivers/pci/controller/dwc/pcie-designware.c:
dw_pcie_link_get_max_link_width() {
	u8 cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
	u32 lnkcap = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);
    ...
}

The cap variable is initialized via dw_pcie_find_capability() but is never
used, while the function proceeds to use pci->pcie_cap.

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 07/16] PCI: dwc: eswin: Use cached PCIe capability offset

[Hans Zhang]

This function is called after dw_pcie_host_init() has already cached the
offset (dw_pcie_link_up() is called after .init and after caching).
Therefore, we can directly use pci->pcie_cap.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pcie-eswin, the call chain is:

  static const struct dw_pcie_ops dw_pcie_ops = {
    .link_up = eswin_pcie_link_up,
  };
  eswin_pcie_link_up()
    -> dw_pcie_find_capability()
---
 drivers/pci/controller/dwc/pcie-eswin.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-eswin.c b/drivers/pci/controller/dwc/pcie-eswin.c
index ce8d64f8a395..6a37fd5a8384 100644
--- a/drivers/pci/controller/dwc/pcie-eswin.c
+++ b/drivers/pci/controller/dwc/pcie-eswin.c
@@ -84,8 +84,7 @@ static int eswin_pcie_start_link(struct dw_pcie *pci)
 
 static bool eswin_pcie_link_up(struct dw_pcie *pci)
 {
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	u16 val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
+	u16 val = dw_pcie_readw_dbi(pci, pci->pcie_cap + PCI_EXP_LNKSTA);
 
 	return val & PCI_EXP_LNKSTA_DLLLA;
 }
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 07/16] PCI: dwc: eswin: Use cached PCIe capability offset

[sashiko-bot]

> This function is called after dw_pcie_host_init() has already cached the
> offset (dw_pcie_link_up() is called after .init and after caching).
> Therefore, we can directly use pci->pcie_cap.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 08/16] PCI: dwc: fu740: Use cached PCIe capability offset

[Hans Zhang]

This callback is invoked after dw_pcie_host_init() has cached the offset,
so we can use pci->pcie_cap directly.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pcie-fu740, the call chain is:

  static const struct dw_pcie_ops dw_pcie_ops = {
    .start_link = fu740_pcie_start_link,
  };
  fu740_pcie_start_link()
    -> dw_pcie_find_capability()
---
 drivers/pci/controller/dwc/pcie-fu740.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
index d0a34f680397..72c0a6316fff 100644
--- a/drivers/pci/controller/dwc/pcie-fu740.c
+++ b/drivers/pci/controller/dwc/pcie-fu740.c
@@ -178,7 +178,7 @@ static int fu740_pcie_start_link(struct dw_pcie *pci)
 {
 	struct device *dev = pci->dev;
 	struct fu740_pcie *afp = dev_get_drvdata(dev);
-	u8 cap_exp = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u8 cap_exp = pci->pcie_cap;
 	int ret;
 	u32 orig, tmp;
 
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 08/16] PCI: dwc: fu740: Use cached PCIe capability offset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] DWC endpoint core uses uninitialized `pci->pcie_cap` during setup, leading to severe PCI configuration space corruption.
- [Low] Unused variable `cap` in `dw_pcie_link_get_max_link_width()`, leading to redundant MMIO reads.
--

--- Patch [8]: [PATCH v3 08/16] PCI: dwc: fu740: Use cached PCIe capability offset ---
commit 184aa9285c97c29f412656a3e7f63a515288813c
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: fu740: Use cached PCIe capability offset

This patch series updates the DesignWare PCIe core and several controller
drivers to cache the PCIe capability offset. By storing the offset in
pci->pcie_cap, it eliminates the need to repeatedly search for the
capability during link setup and initialization.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 7d2794945704e..14228be8bbce7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -1246,7 +1246,7 @@ static void dw_pcie_ep_init_non_sticky_registers(struct dw_pcie *pci)
>  	 * to all other functions as well.
>  	 */
>  	if (funcs > 1) {
> -		offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +		offset = pci->pcie_cap;

[Severity: Critical]
Since the generic endpoint initialization path never caches the capability
offset (unlike the Root Complex initialization path), doesn't this mean
pci->pcie_cap remains 0?

If it remains 0, this direct assignment will corrupt offset 0x0C when
reading and writing the capability.

Furthermore, earlier in this function:

drivers/pci/controller/dwc/pcie-designware-ep.c:dw_pcie_ep_init_non_sticky_registers() {
    ...
    dw_pcie_setup(pci);
    ...
}

Calling dw_pcie_setup() unconditionally invokes dw_pcie_link_set_max_speed()
and dw_pcie_link_set_max_link_width(). These functions now use pci->pcie_cap
which evaluates to 0, causing severe corruption of standard PCI header
registers (such as Header Type and Cache Line Size at offset 0x0C).

>  		func0_lnkcap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>  		func0_lnkcap = FIELD_GET(PCI_EXP_LNKCAP_MLW |
>  					 PCI_EXP_LNKCAP_SLS, func0_lnkcap);

[ ... ]

> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index ec4722ed93032..810729b918928 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c

[ ... ]

> @@ -843,7 +842,7 @@ EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
>  static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
>  {
>  	u32 cap, ctrl2, link_speed;
> -	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u8 offset = pci->pcie_cap;

[Severity: Critical]
When called from the endpoint initialization path via dw_pcie_setup(),
doesn't pci->pcie_cap evaluate to 0?

If so, this base offset resolves to standard PCI header bytes instead of
the PCIe capability registers, corrupting configuration space when
writing back the updated speed values.

>  
>  	cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>  

[ ... ]

> @@ -890,7 +889,7 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
>  int dw_pcie_link_get_max_link_width(struct dw_pcie *pci)
>  {
>  	u8 cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	u32 lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
> +	u32 lnkcap = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);

[Severity: Low]
Is the cap variable still needed here? 

It seems this performs a redundant MMIO search via dw_pcie_find_capability()
but the result is completely ignored since the next line directly accesses
pci->pcie_cap.

>  
>  	return FIELD_GET(PCI_EXP_LNKCAP_MLW, lnkcap);
>  }

[ ... ]

> @@ -935,10 +933,9 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
>  	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc);
>  	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc);
>  
> -	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
> +	lnkcap = dw_pcie_readl_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP);

[Severity: Critical]
Similar to dw_pcie_link_set_max_speed(), if this is called from the endpoint
setup path where pci->pcie_cap remains uninitialized, doesn't this result in
writes to standard PCI header registers instead of the PCIe capability?

>  	FIELD_MODIFY(PCI_EXP_LNKCAP_MLW, &lnkcap, num_lanes);
> -	dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap);
> +	dw_pcie_writel_dbi(pci, pci->pcie_cap + PCI_EXP_LNKCAP, lnkcap);
>  }

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 09/16] PCI: dwc: intel-gw: Use cached PCIe capability offset

[Hans Zhang]

dw_pcie_host_init() calls .init before caching the offset, so inside
.init we must call dw_pcie_get_pcie_cap() to obtain the offset (hardware
is already enabled). The helper will cache the result for later use.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pcie-intel-gw, the call chain is:

  static const struct dw_pcie_host_ops intel_pcie_dw_ops = {
    .init = intel_pcie_rc_init,
  };
  intel_pcie_rc_init()
    -> intel_pcie_host_setup()
      -> intel_pcie_link_setup()
        -> dw_pcie_find_capability()
---
 drivers/pci/controller/dwc/pcie-intel-gw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c
index 348e579e683f..a891c3f8eb01 100644
--- a/drivers/pci/controller/dwc/pcie-intel-gw.c
+++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
@@ -119,7 +119,7 @@ static void intel_pcie_ltssm_disable(struct intel_pcie *pcie)
 static void intel_pcie_link_setup(struct intel_pcie *pcie)
 {
 	u32 val;
-	u8 offset = dw_pcie_find_capability(&pcie->pci, PCI_CAP_ID_EXP);
+	u8 offset = dw_pcie_get_pcie_cap(&pcie->pci);
 
 	val = pcie_rc_cfg_rd(pcie, offset + PCI_EXP_LNKCTL);
 
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 09/16] PCI: dwc: intel-gw: Use cached PCIe capability offset

[sashiko-bot]

> dw_pcie_host_init() calls .init before caching the offset, so inside
> .init we must call dw_pcie_get_pcie_cap() to obtain the offset (hardware
> is already enabled). The helper will cache the result for later use.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 10/16] PCI: dwc: qcom-ep: Use cached PCIe capability offset

[Hans Zhang]

dw_pcie_ep_init() no longer caches the offset automatically. For qcom-ep,
hardware (clocks, PHY) is enabled only after PERST# deassert. Therefore,
we must call dw_pcie_get_pcie_cap() in qcom_pcie_perst_deassert() after
the resources are enabled, before any DBI access. This ensures the cached
offset is valid for subsequent uses in interrupt handlers and ICC updates.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pcie-qcom-ep, dw_pcie_find_capability() appears in:

  qcom_pcie_ep_probe()
    -> dw_pcie_ep_init()
    -> qcom_pcie_ep_enable_irq_resources()
      -> qcom_pcie_ep_global_irq_thread()
	-> qcom_pcie_ep_icc_update()
	  -> dw_pcie_find_capability()
	-> qcom_pcie_ep_perst_irq_thread()
	  -> qcom_pcie_perst_deassert()
	    -> offset = dw_pcie_find_capability()  (called twice)
---
 drivers/pci/controller/dwc/pcie-qcom-ep.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index 56184e6ca6e6..16b0891f9571 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -307,15 +307,13 @@ static void qcom_pcie_dw_write_dbi2(struct dw_pcie *pci, void __iomem *base,
 static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
 {
 	struct dw_pcie *pci = &pcie_ep->pci;
-	u32 offset, status;
-	int speed, width;
-	int ret;
+	int speed, width, ret;
+	u32 status;
 
 	if (!pcie_ep->icc_mem)
 		return;
 
-	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+	status = readw(pci->dbi_base + pci->pcie_cap + PCI_EXP_LNKSTA);
 
 	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
 	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
@@ -491,14 +489,15 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 
 	dw_pcie_dbi_ro_wr_en(pci);
 
+	dw_pcie_get_pcie_cap(pci);
+
 	/* Set the L0s Exit Latency to 2us-4us = 0x6 */
-	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	offset = pci->pcie_cap;
 	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
 	FIELD_MODIFY(PCI_EXP_LNKCAP_L0SEL, &val, 0x6);
 	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, val);
 
 	/* Set the L1 Exit Latency to be 32us-64 us = 0x6 */
-	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
 	FIELD_MODIFY(PCI_EXP_LNKCAP_L1EL, &val, 0x6);
 	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, val);
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 10/16] PCI: dwc: qcom-ep: Use cached PCIe capability offset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] `pci->pcie_cap` is used uninitialized (as 0) in `qcom_pcie_ep_icc_update()` if the global IRQ triggers before `perst_irq` completes.

Pre-existing issues:
- [High] Lost interrupt events due to mutually exclusive `if-else if` chain dropping concurrently pending status bits.
- [High] Concurrent execution of global IRQ and PERST IRQ leads to unclocked MMIO accesses and system crash.
--

--- Patch [10]: [PATCH v3 10/16] PCI: dwc: qcom-ep: Use cached PCIe capability offset ---
commit 66f17f8dee9121b185f046f6247725965823798a
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: qcom-ep: Use cached PCIe capability offset
    
dw_pcie_ep_init() no longer caches the offset automatically. For qcom-ep,
hardware (clocks, PHY) is enabled only after PERST# deassert. Therefore,
we must call dw_pcie_get_pcie_cap() in qcom_pcie_perst_deassert() after
the resources are enabled, before any DBI access. This ensures the cached
offset is valid for subsequent uses in interrupt handlers and ICC updates.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 56184e6ca6e69..16b0891f95713 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -307,15 +307,13 @@ static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
>  {
>  	struct dw_pcie *pci = &pcie_ep->pci;
> -	u32 offset, status;
> -	int speed, width;
> -	int ret;
> +	int speed, width, ret;
> +	u32 status;
>  
>  	if (!pcie_ep->icc_mem)
>  		return;
>  
> -	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> +	status = readw(pci->dbi_base + pci->pcie_cap + PCI_EXP_LNKSTA);

[Severity: High]
Does this read from an uninitialized offset if the global IRQ triggers
before the PERST IRQ completes?

qcom_pcie_ep_enable_irq_resources() requests the global IRQ without
IRQ_NOAUTOEN, making it active immediately. If the PCIe link is already
established (e.g., by the bootloader or firmware), the IRQ can trigger
before pci_epc_start() enables the PERST IRQ and qcom_pcie_perst_deassert()
caches the offset.

If this happens, pci->pcie_cap would be 0, causing the function to read
from DBI offset 0x12 instead of PCI_EXP_LNKSTA. This would compute arbitrary
speed and width values to pass to icc_set_bw(), which could misconfigure the
system interconnect bandwidth.

>  
>  	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
>  	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);

[ ... ]

> @@ -491,14 +489,15 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>  
>  	dw_pcie_dbi_ro_wr_en(pci);
>  
> +	dw_pcie_get_pcie_cap(pci);
> +
>  	/* Set the L0s Exit Latency to 2us-4us = 0x6 */
> -	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	offset = pci->pcie_cap;
>  	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);

[ ... ]


drivers/pci/controller/dwc/pcie-qcom-ep.c:qcom_pcie_ep_global_irq_thread() {
...
	writel_relaxed(status, pcie_ep->parf + PARF_INT_ALL_CLEAR);

	if (FIELD_GET(PARF_INT_ALL_LINK_DOWN, status)) {
...
	} else if (FIELD_GET(PARF_INT_ALL_BME, status)) {
...
}

[Severity: High]
This is a pre-existing issue, but does this if-else chain lose concurrent
interrupt events?

qcom_pcie_ep_global_irq_thread() clears all pending events simultaneously in
hardware. If the host sends multiple events in close succession (e.g., both
Bus Master Enable and Link Up are set in PARF_INT_ALL_STATUS), the else-if
logic will process the first matching event and discard the others.

Since the discarded events are already cleared from the register, they would
be permanently lost, potentially leaving the endpoint in an incorrect logical
state.


drivers/pci/controller/dwc/pcie-qcom-ep.c:qcom_pcie_ep_global_irq_thread() {
...
	u32 status = readl_relaxed(pcie_ep->parf + PARF_INT_ALL_STATUS);
...
}

drivers/pci/controller/dwc/pcie-qcom-ep.c:qcom_pcie_disable_resources() {
...
	clk_bulk_disable_unprepare(pcie_ep->num_clks, pcie_ep->clks);
...
}

[Severity: High]
This is a pre-existing issue, but can the global IRQ thread race with
PERST deassertion/assertion and cause an unclocked MMIO access?

Both the global and PERST IRQs are requested as threaded IRQs without mutual
exclusion. If the host toggles PERST, qcom_pcie_ep_perst_irq_thread() calls
qcom_pcie_perst_assert(), which unconditionally disables the PCIe clocks via
clk_bulk_disable_unprepare().

If a simultaneous link state change triggers qcom_pcie_ep_global_irq_thread()
and it reads from PARF_INT_ALL_STATUS after the clocks are disabled, this
would cause a synchronous external abort (SError) and crash the system.

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 11/16] PCI: dwc: qcom: Use cached PCIe capability offset

[Hans Zhang]

dw_pcie_host_init() caches the offset after .init, so .post_init callbacks
and later functions (.link_up, icc_opp_update) can use pci->pcie_cap
directly. For .init itself, we must call dw_pcie_get_pcie_cap() inside
qcom_pcie_host_init() to obtain the offset (hardware is already enabled).

Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pcie-qcom, dw_pcie_find_capability() appears in multiple call chains:

  static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
    .init = qcom_pcie_host_init,
  };
  qcom_pcie_host_init()
    -> qcom_pcie_clear_aspm_l0s()
      -> dw_pcie_find_capability()

  static const struct qcom_pcie_ops ops_* = {
    .post_init = qcom_pcie_post_init_*,
  };
  qcom_pcie_post_init_*()
    -> qcom_pcie_set_slot_nccs()  (for many versions)
      -> dw_pcie_find_capability()
    -> For 2_3_3 and 2_9_0: also calls dw_pcie_find_capability() directly

  static const struct dw_pcie_ops dw_pcie_ops = {
    .link_up = qcom_pcie_link_up,
  };
  qcom_pcie_link_up()
    -> dw_pcie_find_capability()

  qcom_pcie_probe()
    -> dw_pcie_host_init()
    -> qcom_pcie_icc_opp_update()
      -> dw_pcie_find_capability()
---
 drivers/pci/controller/dwc/pcie-qcom.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index d8eb52857f69..bd383d7cb02b 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -341,13 +341,13 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
 static void qcom_pcie_clear_aspm_l0s(struct dw_pcie *pci)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
-	u16 offset;
+	u8 offset;
 	u32 val;
 
 	if (!pcie->cfg->no_l0s)
 		return;
 
-	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	offset = dw_pcie_get_pcie_cap(pci);
 
 	dw_pcie_dbi_ro_wr_en(pci);
 
@@ -360,7 +360,6 @@ static void qcom_pcie_clear_aspm_l0s(struct dw_pcie *pci)
 
 static void qcom_pcie_set_slot_nccs(struct dw_pcie *pci)
 {
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 	u32 val;
 
 	dw_pcie_dbi_ro_wr_en(pci);
@@ -370,9 +369,9 @@ static void qcom_pcie_set_slot_nccs(struct dw_pcie *pci)
 	 * notifications for the Hot-Plug commands. So set the NCCS field to
 	 * avoid waiting for the completions.
 	 */
-	val = readl(pci->dbi_base + offset + PCI_EXP_SLTCAP);
+	val = readl(pci->dbi_base + pci->pcie_cap + PCI_EXP_SLTCAP);
 	val |= PCI_EXP_SLTCAP_NCCS;
-	writel(val, pci->dbi_base + offset + PCI_EXP_SLTCAP);
+	writel(val, pci->dbi_base + pci->pcie_cap + PCI_EXP_SLTCAP);
 
 	dw_pcie_dbi_ro_wr_dis(pci);
 }
@@ -935,7 +934,7 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
 static int qcom_pcie_post_init_2_3_3(struct qcom_pcie *pcie)
 {
 	struct dw_pcie *pci = pcie->pci;
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u8 offset = pci->pcie_cap;
 	u32 val;
 
 	/* Force PHY out of lowest power state */
@@ -1257,7 +1256,7 @@ static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
 static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
 {
 	struct dw_pcie *pci = pcie->pci;
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u8 offset = pci->pcie_cap;
 	u32 val;
 	int i;
 
@@ -1303,8 +1302,7 @@ static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
 
 static bool qcom_pcie_link_up(struct dw_pcie *pci)
 {
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	u16 val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+	u16 val = readw(pci->dbi_base + pci->pcie_cap + PCI_EXP_LNKSTA);
 
 	return val & PCI_EXP_LNKSTA_DLLLA;
 }
@@ -1664,15 +1662,14 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
 
 static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
 {
-	u32 offset, status, width, speed;
+	u32 status, width, speed;
 	struct dw_pcie *pci = pcie->pci;
 	struct dev_pm_opp_key key = {};
 	unsigned long freq_kbps;
 	struct dev_pm_opp *opp;
 	int ret, freq_mbps;
 
-	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+	status = readw(pci->dbi_base + pci->pcie_cap + PCI_EXP_LNKSTA);
 
 	/* Only update constraints if link is up. */
 	if (!(status & PCI_EXP_LNKSTA_DLLLA))
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 11/16] PCI: dwc: qcom: Use cached PCIe capability offset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `pci->pcie_cap` field remains uninitialized (0) when used in `.post_init` callbacks, leading to corruption of the standard PCI configuration header.
--

--- Patch [11]: [PATCH v3 11/16] PCI: dwc: qcom: Use cached PCIe capability offset ---
commit ddf8caa0039918520481c1cff7b9e80cbf8ee212
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: qcom: Use cached PCIe capability offset
    
This commit updates the Qualcomm PCIe driver to use the cached PCIe capability
offset (pci->pcie_cap) instead of repeatedly calling dw_pcie_find_capability().
It aims to optimize capability lookups during initialization and link updates.

> dw_pcie_host_init() caches the offset after .init, so .post_init callbacks
> and later functions (.link_up, icc_opp_update) can use pci->pcie_cap
> directly. For .init itself, we must call dw_pcie_get_pcie_cap() inside
> qcom_pcie_host_init() to obtain the offset (hardware is already enabled).

Is there a confusion here between dw_pcie_host_ops->post_init and
qcom_pcie_ops->post_init?

The commit message correctly notes that dw_pcie_host_init() caches the offset
after dw_pcie_host_ops->init returns. However, the .post_init callbacks
modified in this patch belong to struct qcom_pcie_ops.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index d8eb52857f69c..bd383d7cb02b1 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -360,7 +360,6 @@ static void qcom_pcie_clear_aspm_l0s(struct dw_pcie *pci)
>  
>  static void qcom_pcie_set_slot_nccs(struct dw_pcie *pci)
>  {
> -	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>  	u32 val;
>  
>  	dw_pcie_dbi_ro_wr_en(pci);
> @@ -370,9 +369,9 @@ static void qcom_pcie_set_slot_nccs(struct dw_pcie *pci)
>  	 * notifications for the Hot-Plug commands. So set the NCCS field to
>  	 * avoid waiting for the completions.
>  	 */
> -	val = readl(pci->dbi_base + offset + PCI_EXP_SLTCAP);
> +	val = readl(pci->dbi_base + pci->pcie_cap + PCI_EXP_SLTCAP);

[Severity: High]
Is pci->pcie_cap initialized when this is called?

Looking at qcom_pcie_host_init(), it calls pcie->cfg->ops->post_init(pcie)
before it calls qcom_pcie_clear_aspm_l0s() where dw_pcie_get_pcie_cap() is
finally executed:

qcom_pcie_host_init()
  pcie->cfg->ops->post_init(pcie) 
    qcom_pcie_set_slot_nccs(pcie->pci)
      val = readl(pci->dbi_base + pci->pcie_cap + PCI_EXP_SLTCAP);
  ...
  qcom_pcie_clear_aspm_l0s(pcie->pci)
    offset = dw_pcie_get_pcie_cap(pci);

Since pci->pcie_cap is 0 during qcom_pcie_ops->post_init, won't this access
standard PCI configuration space (like offset 0x14 for PCI_EXP_SLTCAP, which
is BAR1) instead of the PCIe capability registers?

>  	val |= PCI_EXP_SLTCAP_NCCS;
> -	writel(val, pci->dbi_base + offset + PCI_EXP_SLTCAP);
> +	writel(val, pci->dbi_base + pci->pcie_cap + PCI_EXP_SLTCAP);
>  
>  	dw_pcie_dbi_ro_wr_dis(pci);
>  }

[ ... ]

> @@ -935,7 +934,7 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
>  static int qcom_pcie_post_init_2_3_3(struct qcom_pcie *pcie)
>  {
>  	struct dw_pcie *pci = pcie->pci;
> -	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u8 offset = pci->pcie_cap;
>  	u32 val;

[Severity: High]
Does this also suffer from the same initialization ordering issue?

Since qcom_pcie_post_init_2_3_3() is called by qcom_pcie_host_init() before
pci->pcie_cap is populated, offset will be 0 here, causing the subsequent
writes to PCIE_CAP_SLOT_VAL and PCI_EXP_LNKCAP to overwrite standard PCI
header fields.

>  
>  	/* Force PHY out of lowest power state */

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 12/16] PCI: dwc: sophgo: Use cached PCIe capability offset

[Hans Zhang]

dw_pcie_host_init() calls .init before caching, so we must call
dw_pcie_get_pcie_cap() inside .init. The hardware is already enabled by
the driver's own initialization before this point. The helper will cache
the offset and avoid redundant searches.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pcie-sophgo, the call chain is:

  static const struct dw_pcie_host_ops sophgo_pcie_host_ops = {
    .init = sophgo_pcie_host_init,
  };
  sophgo_pcie_host_init()
    -> sophgo_pcie_disable_l0s_l1()
      -> dw_pcie_find_capability()
---
 drivers/pci/controller/dwc/pcie-sophgo.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-sophgo.c b/drivers/pci/controller/dwc/pcie-sophgo.c
index 044088898819..39703d2b7b5f 100644
--- a/drivers/pci/controller/dwc/pcie-sophgo.c
+++ b/drivers/pci/controller/dwc/pcie-sophgo.c
@@ -164,9 +164,10 @@ static void sophgo_pcie_msi_enable(struct dw_pcie_rp *pp)
 static void sophgo_pcie_disable_l0s_l1(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-	u32 offset, val;
+	u8 offset;
+	u32 val;
 
-	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	offset = dw_pcie_get_pcie_cap(pci);
 
 	dw_pcie_dbi_ro_wr_en(pci);
 
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 12/16] PCI: dwc: sophgo: Use cached PCIe capability offset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the INTx IRQ domain and chained handler are leaked on probe failure, which leaves a dangling pointer in the active IRQ handler and leads to a use-after-free if the interrupt fires.
--

--- Patch [12]: [PATCH v3 12/16] PCI: dwc: sophgo: Use cached PCIe capability offset ---
commit 2e86ab7dbd69d3bcfaa3d7b4f601df27c48317f3
Author: Hans Zhang <18255117159@163.com>
PCI: dwc: sophgo: Use cached PCIe capability offset

This commit modifies sophgo_pcie_disable_l0s_l1() to use the cached PCIe
capability offset rather than searching for it again. The offset is fetched
via dw_pcie_get_pcie_cap() inside the .init callback.

> diff --git a/drivers/pci/controller/dwc/pcie-sophgo.c b/drivers/pci/controller/dwc/pcie-sophgo.c
> index 044088898819e..39703d2b7b5f2 100644
> --- a/drivers/pci/controller/dwc/pcie-sophgo.c
> +++ b/drivers/pci/controller/dwc/pcie-sophgo.c
> @@ -164,9 +164,10 @@ static void sophgo_pcie_msi_enable(struct dw_pcie_rp *pp)
>  static void sophgo_pcie_disable_l0s_l1(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> -	u32 offset, val;
> +	u8 offset;
> +	u32 val;
>  
> -	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	offset = dw_pcie_get_pcie_cap(pci);

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the call
chain from sophgo_pcie_host_init() to sophgo_pcie_disable_l0s_l1(), I
noticed a potential issue with how the INTx IRQ domain is handled during
probe failures.

In sophgo_pcie_host_init(), the IRQ domain and chained handler are set up:

        irq = sophgo_pcie_init_irq_domain(pp);
        if (irq < 0)
                return irq;

        irq_set_chained_handler_and_data(irq, sophgo_pcie_intx_handler, pp);

If dw_pcie_host_init() subsequently fails during sophgo_pcie_probe(), devres
will free the struct sophgo_pcie structure, including the pp pointer.

However, there is no .deinit callback in sophgo_pcie_host_ops to call
irq_domain_remove() and irq_set_chained_handler_and_data(irq, NULL, NULL).

Could this leave a dangling pointer in the active IRQ handler, leading to
a use-after-free if the interrupt fires?

[ ... ]

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 13/16] PCI: dwc: spacemit-k1: Use cached PCIe capability offset

[Hans Zhang]

Because .init runs before core caching, we call dw_pcie_get_pcie_cap()
inside k1_pcie_disable_aspm_l1() to get the capability base, then add
PCI_EXP_LNKCAP. Hardware is already enabled at this point.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pcie-spacemit-k1, the call chain is:

  static const struct dw_pcie_host_ops k1_pcie_host_ops = {
    .init = k1_pcie_init,
  };
  k1_pcie_init()
    -> k1_pcie_disable_aspm_l1()
      -> dw_pcie_find_capability()
---
 drivers/pci/controller/dwc/pcie-spacemit-k1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-spacemit-k1.c b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
index 04241df8fd59..3e741324f44a 100644
--- a/drivers/pci/controller/dwc/pcie-spacemit-k1.c
+++ b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
@@ -116,7 +116,7 @@ static void k1_pcie_disable_aspm_l1(struct k1_pcie *k1)
 	u8 offset;
 	u32 val;
 
-	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	offset = dw_pcie_get_pcie_cap(pci);
 	offset += PCI_EXP_LNKCAP;
 
 	dw_pcie_dbi_ro_wr_en(pci);
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 13/16] PCI: dwc: spacemit-k1: Use cached PCIe capability offset

[sashiko-bot]

> Because .init runs before core caching, we call dw_pcie_get_pcie_cap()
> inside k1_pcie_disable_aspm_l1() to get the capability base, then add
> PCI_EXP_LNKCAP. Hardware is already enabled at this point.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 14/16] PCI: dwc: spear13xx: Use cached PCIe capability offset

[Hans Zhang]

Inside .init we must call dw_pcie_get_pcie_cap() to obtain the offset,
because the core has not yet cached it. The hardware is already enabled
by the driver's own initialization before this point.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pcie-spear13xx, the call chain is:

  static const struct dw_pcie_host_ops spear13xx_pcie_host_ops = {
    .init = spear13xx_pcie_host_init,
  };
  spear13xx_pcie_host_init()
    -> dw_pcie_find_capability()
---
 drivers/pci/controller/dwc/pcie-spear13xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-spear13xx.c b/drivers/pci/controller/dwc/pcie-spear13xx.c
index 01794a9d3ad2..6e4c11b497d4 100644
--- a/drivers/pci/controller/dwc/pcie-spear13xx.c
+++ b/drivers/pci/controller/dwc/pcie-spear13xx.c
@@ -122,7 +122,7 @@ static int spear13xx_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pci);
-	u32 exp_cap_off = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u8 exp_cap_off = dw_pcie_get_pcie_cap(pci);
 	u32 val;
 
 	spear13xx_pcie->app_base = pci->dbi_base + 0x2000;
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 14/16] PCI: dwc: spear13xx: Use cached PCIe capability offset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Mandatory API contract violation: ignored return value of `phy_init()`.
- [High] Unconditional `IRQ_HANDLED` in a shared IRQ handler.
- [High] Shared IRQ is requested before the hardware base address is initialized, leading to a potential NULL pointer dereference.
--

--- Patch [14]: [PATCH v3 14/16] PCI: dwc: spear13xx: Use cached PCIe capability offset ---
commit c41d49bf9e94ac238049768122148b7abb564665
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: spear13xx: Use cached PCIe capability offset

Inside .init we must call dw_pcie_get_pcie_cap() to obtain the offset,
because the core has not yet cached it. The hardware is already enabled
by the driver's own initialization before this point.

> diff --git a/drivers/pci/controller/dwc/pcie-spear13xx.c b/drivers/pci/controller/dwc/pcie-spear13xx.c
> index 01794a9d3ad29..6e4c11b497d45 100644
> --- a/drivers/pci/controller/dwc/pcie-spear13xx.c
> +++ b/drivers/pci/controller/dwc/pcie-spear13xx.c
> @@ -122,7 +122,7 @@ static int spear13xx_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pci);
> -	u32 exp_cap_off = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u8 exp_cap_off = dw_pcie_get_pcie_cap(pci);
>  	u32 val;
>  
>  	spear13xx_pcie->app_base = pci->dbi_base + 0x2000;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
NULL pointer dereference involving app_base here.

In spear13xx_add_pcie_port(), devm_request_irq() is called with IRQF_SHARED
before dw_pcie_host_init() sets spear13xx_pcie->app_base:

spear13xx_add_pcie_port() {
    ...
    ret = devm_request_irq(dev, pp->irq, spear13xx_pcie_irq_handler,
                           IRQF_SHARED | IRQF_NO_THREAD,
                           "spear1340-pcie", spear13xx_pcie);
    ...
    ret = dw_pcie_host_init(pp);
    ...
}

If another device on the shared IRQ line triggers an interrupt before
spear13xx_pcie_host_init() is called, spear13xx_pcie_irq_handler() will run
and dereference the still-NULL app_base:

spear13xx_pcie_irq_handler() {
    ...
    struct pcie_app_reg __iomem *app_reg = spear13xx_pcie->app_base;
    ...
    status = readl(&app_reg->int_sts);
    ...
}

Could the IRQ request be moved until after the base address is initialized?

[ ... ]

I also noticed a few other pre-existing issues in this file.

[Severity: High]
This is a pre-existing issue, but the integer return value of
phy_init() is completely ignored in spear13xx_pcie_probe():

spear13xx_pcie_probe() {
    ...
    phy_init(spear13xx_pcie->phy);

    spear13xx_pcie->clk = devm_clk_get(dev, NULL);
    ...
}

If phy_init() fails, the driver blindly proceeds to configure clocks and
initialize the PCIe host controller. Accessing an uninitialized or
unpowered PHY can lead to hardware lockups or synchronous external aborts.
Should the return value of phy_init() be checked here?

[Severity: High]
This is a pre-existing issue, but the shared IRQ handler unconditionally
returns IRQ_HANDLED:

spear13xx_pcie_irq_handler() {
    ...
    status = readl(&app_reg->int_sts);

    if (status & MSI_CTRL_INT) {
        BUG_ON(!IS_ENABLED(CONFIG_PCI_MSI));
        dw_handle_msi_irq(pp);
    }

    writel(status, &app_reg->int_clr);

    return IRQ_HANDLED;
}

Returning IRQ_HANDLED unconditionally in a shared IRQ handler breaks the
kernel's spurious interrupt detection and can swallow interrupts. If the
shared IRQ line screams, the kernel will not disable the line because it
thinks this driver is successfully handling the interrupts, which can lead
to CPU stalls. Should this return IRQ_NONE when status is 0?

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 15/16] PCI: dwc: tegra194: Use cached PCIe capability offset

[Hans Zhang]

In host mode: tegra_pcie_dw_host_init() runs inside .start_link, which is
called after dw_pcie_host_init() has cached the offset. However, the
driver's own initialization already enables hardware, so calling
dw_pcie_get_pcie_cap() is safe and will return the cached value.

In endpoint mode: pex_ep_event_pex_rst_deassert() runs after hardware is
enabled (PERST# deassert). Because the core no longer caches automatically,
we call dw_pcie_get_pcie_cap() here to cache the offset for subsequent
accesses. The helper performs the DBI read only when hardware is ready.

Thus, the private pcie_cap_base is always valid and can still be used for
register accesses.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pcie-tegra194, dw_pcie_find_capability() appears in:

  static const struct dw_pcie_ops tegra_dw_pcie_ops = {
    .start_link = tegra_pcie_dw_start_link,
  };
  tegra_pcie_dw_start_link()
    -> tegra_pcie_dw_host_init()
      -> dw_pcie_find_capability()

  tegra_pcie_dw_probe()
    case DW_PCIE_EP_TYPE:
      tegra_pcie_config_ep()
        -> pex_ep_event_pex_rst_deassert()
	  -> dw_pcie_find_capability()
---
 drivers/pci/controller/dwc/pcie-tegra194.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 795cef5a915d..bf482bc66a92 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -905,8 +905,7 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
 	pp->bridge->ops = &tegra_pci_ops;
 
 	if (!pcie->pcie_cap_base)
-		pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci,
-							      PCI_CAP_ID_EXP);
+		pcie->pcie_cap_base = dw_pcie_get_pcie_cap(pci);
 
 	val = dw_pcie_readl_dbi(pci, PCI_IO_BASE);
 	val &= ~(IO_BASE_IO_DECODE | IO_BASE_IO_DECODE_BIT8);
@@ -1889,8 +1888,7 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
 		dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
 	}
 
-	pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci,
-						      PCI_CAP_ID_EXP);
+	pcie->pcie_cap_base = dw_pcie_get_pcie_cap(pci);
 
 	/* Clear Slot Clock Configuration bit if SRNS configuration */
 	if (pcie->enable_srns) {
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 15/16] PCI: dwc: tegra194: Use cached PCIe capability offset

[sashiko-bot]

> In host mode: tegra_pcie_dw_host_init() runs inside .start_link, which is
> called after dw_pcie_host_init() has cached the offset. However, the
> driver's own initialization already enables hardware, so calling
> dw_pcie_get_pcie_cap() is safe and will return the cached value.
> 
> In endpoint mode: pex_ep_event_pex_rst_deassert() runs after hardware is
> enabled (PERST# deassert). Because the core no longer caches automatically,
> we call dw_pcie_get_pcie_cap() here to cache the offset for subsequent
> accesses. The helper performs the DBI read only when hardware is ready.
> 
> Thus, the private pcie_cap_base is always valid and can still be used for
> register accesses.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 16/16] PCI: dwc: ultrarisc: Use cached PCIe capability offset

[Hans Zhang]

Inside .init we call dw_pcie_get_pcie_cap() to obtain the offset, because
the core has not yet cached it. Hardware is already enabled by the driver
before this point.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
In pcie-ultrarisc, the call chain is:

  static const struct dw_pcie_host_ops ultrarisc_pcie_host_ops = {
    .init = ultrarisc_pcie_host_init,
  };
  ultrarisc_pcie_host_init()
    -> dw_pcie_find_capability()
---
 drivers/pci/controller/dwc/pcie-ultrarisc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-ultrarisc.c b/drivers/pci/controller/dwc/pcie-ultrarisc.c
index 6ee661ceff67..3f6e3ab0aa69 100644
--- a/drivers/pci/controller/dwc/pcie-ultrarisc.c
+++ b/drivers/pci/controller/dwc/pcie-ultrarisc.c
@@ -49,7 +49,7 @@ static int ultrarisc_pcie_host_init(struct dw_pcie_rp *pp)
 	FIELD_MODIFY(PORT_FLT_SF_MASK, &val, PORT_FLT_SF_VAL_64);
 	dw_pcie_writel_dbi(pci, PCIE_TIMER_CTRL_MAX_FUNC_NUM, val);
 
-	cap_exp = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	cap_exp = dw_pcie_get_pcie_cap(pci);
 	val = dw_pcie_readl_dbi(pci, cap_exp + PCI_EXP_LNKCTL2);
 	FIELD_MODIFY(PCI_EXP_LNKCTL2_TLS, &val, PCI_EXP_LNKCTL2_TLS_16_0GT);
 	dw_pcie_writel_dbi(pci, cap_exp + PCI_EXP_LNKCTL2, val);
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 16/16] PCI: dwc: ultrarisc: Use cached PCIe capability offset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] In Endpoint (EP) mode, `pci->pcie_cap` remains uninitialized (0), causing `dw_pcie_setup()` to silently corrupt standard PCI header registers.

Pre-existing issues:
- [High] 32-bit read-modify-write (RMW) of 16-bit PCIe Control registers accidentally clears Write-1-to-Clear (W1C) bits in the adjacent Status registers.
--

--- Patch [16]: [PATCH v3 16/16] PCI: dwc: ultrarisc: Use cached PCIe capability offset ---
commit 4d9321bed75fe3c10535cf01ea0a69ce527cb616
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: ultrarisc: Use cached PCIe capability offset

This commit updates ultrarisc_pcie_host_init to use dw_pcie_get_pcie_cap
to obtain the PCIe capability offset, as the core has not cached it yet.
Hardware is already enabled before this point.

> diff --git a/drivers/pci/controller/dwc/pcie-ultrarisc.c b/drivers/pci/controller/dwc/pcie-ultrarisc.c
> index 6ee661ceff67a..3f6e3ab0aa697 100644
> --- a/drivers/pci/controller/dwc/pcie-ultrarisc.c
> +++ b/drivers/pci/controller/dwc/pcie-ultrarisc.c
> @@ -49,7 +49,7 @@ static int ultrarisc_pcie_host_init(struct dw_pcie_rp *pp)
>  	FIELD_MODIFY(PORT_FLT_SF_MASK, &val, PORT_FLT_SF_VAL_64);
>  	dw_pcie_writel_dbi(pci, PCIE_TIMER_CTRL_MAX_FUNC_NUM, val);
>  
> -	cap_exp = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	cap_exp = dw_pcie_get_pcie_cap(pci);

[Severity: Critical]
Does caching the capability offset here expose an uninitialized value
in Endpoint mode?

If the broader patch series replaced dw_pcie_find_capability with direct
reads of pci->pcie_cap in the EP paths without initializing it,
pci->pcie_cap could remain 0.

For example, in drivers/pci/controller/dwc/pcie-designware-ep.c:

dw_pcie_ep_init_non_sticky_registers() {
    if (funcs > 1) {
        offset = pci->pcie_cap;
        func0_lnkcap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
        [ ... ]
}

If dw_pcie_get_pcie_cap() is not called before this in the EP
initialization sequence, offset evaluates to 0. This causes
dw_pcie_setup() to write into standard PCI header registers like Cache
Line Size or BIST instead of the PCIe capability space, silently
corrupting the header.

>  	val = dw_pcie_readl_dbi(pci, cap_exp + PCI_EXP_LNKCTL2);
>  	FIELD_MODIFY(PCI_EXP_LNKCTL2_TLS, &val, PCI_EXP_LNKCTL2_TLS_16_0GT);
>  	dw_pcie_writel_dbi(pci, cap_exp + PCI_EXP_LNKCTL2, val);

[Severity: High]
This is a pre-existing issue, but does this 32-bit read-modify-write
sequence unintentionally clear Write-1-to-Clear bits in the adjacent
Status register?

When ultrarisc_pcie_host_init() reads and writes 32 bits starting at
PCI_EXP_LNKCTL2:

drivers/pci/controller/dwc/pcie-ultrarisc.c:ultrarisc_pcie_host_init()
    val = dw_pcie_readl_dbi(pci, cap_exp + PCI_EXP_LNKCTL2);
    FIELD_MODIFY(PCI_EXP_LNKCTL2_TLS, &val, PCI_EXP_LNKCTL2_TLS_16_0GT);
    dw_pcie_writel_dbi(pci, cap_exp + PCI_EXP_LNKCTL2, val);

The read includes the 16-bit Link Control 2 register and the adjacent
16-bit Link Status 2 register. If any W1C bits are set in the status
register, writing the 32-bit value back will clear those events. This
can drop hardware events like Link Equalization Requests or Device Errors,
potentially breaking link training or Advanced Error Reporting.

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic