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

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 <[email protected]>

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/[email protected]?part=4

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic