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

[Hans Zhang <18255117159@163.com>]

On 9/11/26 21:34, sashiko-bot@kernel.org wrote:
> 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).

Hi,

Will add 0003 patch to rename function name.
s/meson_size_to_payload/meson_size_to_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.

The next version will remove:

#define PCIE_CAP_MAX_PAYLOAD_SIZE(x)	((x) << 5)
#define MAX_PAYLOAD_SIZE		256


Best regards,
Hans

> 
>> -	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;
> 


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