Re: [PATCH v8 2/2] PCI: dwc: Remove redundant MPS configuration

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Unused macros PCIE_CAP_MAX_PAYLOAD_SIZE and MAX_PAYLOAD_SIZE left behind after removing the MPS configuration function.
- [Low] Misleading function name and warning message for MRRS calculation after removing MPS configuration.

Pre-existing issues:
- [High] 32-bit read-modify-write of PCI_EXP_DEVCTL inadvertently clears W1C error bits in PCI_EXP_DEVSTA.
--

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

PCI: dwc: Remove redundant MPS configuration

The Meson PCIe controller driver manually configures maximum payload
size (MPS) through meson_set_max_payload, duplicating functionality now
centralized in the PCI core.  Deprecating redundant code simplifies the
driver and aligns it with the consolidated MPS management strategy,
improving long-term maintainability.

> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> index 8559d132dcde8..8c176f78953e9 100644
> --- a/drivers/pci/controller/dwc/pci-meson.c
> +++ b/drivers/pci/controller/dwc/pci-meson.c
> @@ -273,22 +273,6 @@ static int meson_size_to_payload(struct meson_pcie *mp, int size)
>  	return fls(size) - 8;
>  }

[Severity: Low]
With the removal of the payload configuration function, this helper is now
exclusively called by meson_set_max_rd_req_size() to calculate the Max Read
Request Size (MRRS).

Does this function name and its internal log message:

	dev_warn(dev, "payload size %d, set to default 256\n", size);

need to be updated to reflect that it is calculating MRRS? Leaving it as-is
might cause confusion for future maintainers.

>  
> -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);
> -	int max_payload_size = meson_size_to_payload(mp, size);
> -
> -	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
> -	val &= ~PCI_EXP_DEVCTL_PAYLOAD;
> -	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
> -
> -	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
> -	val |= PCIE_CAP_MAX_PAYLOAD_SIZE(max_payload_size);

[Severity: Low]
Should the definition of PCIE_CAP_MAX_PAYLOAD_SIZE be removed as well? Since
it is no longer used after removing this function, it appears to be dead
code now.

> -	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
> -}
> -
>  static void meson_set_max_rd_req_size(struct meson_pcie *mp, int size)
>  {
>  	struct dw_pcie *pci = &mp->pci;

[Severity: High]
This is a pre-existing issue, but does meson_set_max_rd_req_size()
inadvertently clear W1C error bits in PCI_EXP_DEVSTA?

This function performs a 32-bit read-modify-write on PCI_EXP_DEVCTL:

	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
	val &= ~PCI_EXP_DEVCTL_READRQ;
	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);

Because this 32-bit access covers both the 16-bit PCI_EXP_DEVCTL and the
adjacent 16-bit PCI_EXP_DEVSTA, it fetches both. PCI_EXP_DEVSTA contains
multiple Write-1-to-Clear (W1C) error flags. Writing the entire 32-bit value
back means any W1C bit in DEVSTA read as '1' is written as '1', inadvertently
clearing the error status and masking hardware faults.

Should this be updated to use dw_pcie_readw_dbi() and dw_pcie_writew_dbi()
instead?

[ ... ]

> @@ -363,7 +347,6 @@ static int meson_pcie_host_init(struct dw_pcie_rp *pp)
>  
>  	pp->bridge->ops = &meson_pci_ops;
>  
> -	meson_set_max_payload(mp, MAX_PAYLOAD_SIZE);

[Severity: Low]
Should the MAX_PAYLOAD_SIZE macro be removed as well? Since its usage is
removed here, leaving the macro behind leaves dead code.

>  	meson_set_max_rd_req_size(mp, MAX_READ_REQ_SIZE);
>  
>  	return 0;

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

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