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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `intel_pcie_resume_noirq` function resets the DWC PCIe core but fails to restore the Root Complex configuration (ATU, BARs, MSI).
- [High] Reading and writing back 32-bit values to 16-bit configuration registers (`PCI_EXP_LNKCTL` and `PCI_COMMAND`) unintentionally clears Write-1-to-Clear (W1C) bits in their adjacent registers (`PCI_EXP_LNKSTA` and `PCI_STATUS`).
--

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

PCI: dwc: intel-gw: Use cached PCIe capability offset

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.

> diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c
> index 2674cd376f495..568b2634d08da 100644
> --- a/drivers/pci/controller/dwc/pcie-intel-gw.c
> +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
> @@ -120,7 +120,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);

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

Modifying PCI_EXP_LNKCTL via a 32-bit read and write back will include
the upper 16 bits containing PCI_EXP_LNKSTA. Writing the read value back
to these W1C bits could clear any set error or status flags, such as the
Link Bandwidth Management Status or Detected Parity Error.

Would the same issue occur in pcie_update_bits() when it modifies
PCI_COMMAND, potentially clearing W1C bits in PCI_STATUS?

>  
>  	val &= ~(PCI_EXP_LNKCTL_LD | PCI_EXP_LNKCTL_ASPMC);
>  	pcie_rc_cfg_wr(pcie, offset + PCI_EXP_LNKCTL, val);
>  }

There is also another pre-existing issue in this file regarding the resume
flow. When intel_pcie_resume_noirq() runs, it calls intel_pcie_host_setup():

drivers/pci/controller/dwc/pcie-intel-gw.c:intel_pcie_host_setup() {
    ...
    intel_pcie_core_rst_assert(pcie);
    intel_pcie_device_rst_assert(pcie);
    intel_pcie_core_rst_deassert(pcie);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does the core reset in
intel_pcie_host_setup() wipe the internal DWC controller state, including
ATU mappings?

By overriding the generic resume flow and bypassing dw_pcie_setup_rc(), the
driver appears to leave the ATU windows unprogrammed. Will outbound memory
accesses from the CPU to PCIe devices and inbound DMA/MSI transactions fail
upon resume without the ATU being restored?

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