linux-kernel - Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b364eed2-047d-4c74-9f30-45291305bc4b@163.com>
Date: Thu, 8 May 2025 00:47:12 +0800
From: Hans Zhang <18255117159@....com>
To: Pali Rohár <pali@...nel.org>,
 Niklas Cassel <cassel@...nel.org>
Cc: lpieralisi@...nel.org, kw@...ux.com, bhelgaas@...gle.com,
 heiko@...ech.de, manivannan.sadhasivam@...aro.org, yue.wang@...ogic.com,
 neil.armstrong@...aro.org, robh@...nel.org, jingoohan1@...il.com,
 khilman@...libre.com, jbrunet@...libre.com,
 martin.blumenstingl@...glemail.com, linux-pci@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-amlogic@...ts.infradead.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration



On 2025/5/8 00:36, Pali Rohár wrote:
> On Wednesday 07 May 2025 23:06:51 Hans Zhang wrote:
>> On 2025/5/7 23:03, Hans Zhang wrote:
>>> On 2025/5/7 01:41, Pali Rohár wrote:
>>>> On Wednesday 07 May 2025 01:34:39 Hans Zhang wrote:
>>>>> The Aardvark PCIe controller enforces a fixed 512B payload size via
>>>>> PCI_EXP_DEVCTL_PAYLOAD_512B, overriding hardware capabilities and PCIe
>>>>> core negotiations.
>>>>>
>>>>> Remove explicit MPS overrides (PCI_EXP_DEVCTL_PAYLOAD and
>>>>> PCI_EXP_DEVCTL_PAYLOAD_512B). MPS is now determined by the PCI core
>>>>> during device initialization, leveraging root port configurations and
>>>>> device-specific capabilities.
>>>>>
>>>>> Aligning Aardvark with the unified MPS framework ensures consistency,
>>>>> avoids artificial constraints, and allows the hardware to operate at
>>>>> its maximum supported payload size while adhering to PCIe
>>>>> specifications.
>>>>>
>>>>> Signed-off-by: Hans Zhang <18255117159@....com>
>>>>> ---
>>>>>    drivers/pci/controller/pci-aardvark.c | 2 --
>>>>>    1 file changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/controller/pci-aardvark.c
>>>>> b/drivers/pci/controller/pci-aardvark.c
>>>>> index a29796cce420..d8852892994a 100644
>>>>> --- a/drivers/pci/controller/pci-aardvark.c
>>>>> +++ b/drivers/pci/controller/pci-aardvark.c
>>>>> @@ -549,9 +549,7 @@ static void advk_pcie_setup_hw(struct
>>>>> advk_pcie *pcie)
>>>>>        reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
>>>>>        reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
>>>>>        reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
>>>>> -    reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
>>>>>        reg &= ~PCI_EXP_DEVCTL_READRQ;
>>>>> -    reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
>>>>>        reg |= PCI_EXP_DEVCTL_READRQ_512B;
>>>>>        advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
>>>>> -- 
>>>>> 2.25.1
>>>>>
>>>>
>>>> Please do not remove this code. It is required part of the
>>>> initialization of the aardvark PCI controller at the specific phase,
>>>> as defined in the Armada 3700 Functional Specification.
>>>>
>>>> There were reported more issues with those Armada PCIe controllers for
>>>> which were already sent patches to mailing list in last 5 years. But
>>>> unfortunately not all fixes were taken / applied yet.
>>>
>>> Hi Pali,
>>>
>>> I replied to you in version v2.
>>>
>>> Is the maximum MPS supported by Armada 3700 512 bytes?
> 
> IIRC yes, 512-byte mode is supported. And I think in past I was testing
> some PCIe endpoint card which required 512-byte long payload and did not
> worked in 256-byte long mode (not sure if the card was not able to split
> transaction or something other was broken, it is quite longer time).
> 
>>> What are the default values of DevCap.MPS and DevCtl.MPS?
> 
> Do you mean values in the PCI-to-PCI bridge device of PCIe Root Port
> type?
> 
> Aardvark controller does not have the real HW PCI-to-PCI bridge device.
> There is only in-kernel emulation drivers/pci/pci-bridge-emul.c which
> create fake kernel PCI device in the hierarchy to make kernel and
> userspace happy. Yes, this is deviation from the PCIe standard but well,
> buggy HW is also HW.
> 
> And same applies for the pci-mvebu.c driver for older Marvell PCIe HW.
> 
>>> Because the default value of DevCtl.MPS is not 512 bytes, it needs to be
>>> configured here, right?
>>>
>>> If it's my guess, RK3588 also has the same requirements as you, just
>>> like the first patch I submitted.
>>>
>>> Please take a look at the communication history:
>>> https://patchwork.kernel.org/project/linux-pci/patch/20250416151926.140202-1-18255117159@163.com/
>> And this:
>> https://patchwork.kernel.org/project/linux-pci/patch/20250425095708.32662-2-18255117159@163.com/
> 
> These changes are referring the to root ports PCI devices, which are not
> applicable for aardvark PCIe controller.
> 
>>>
>>> Please test it using patch 1/3 of this series. If there are any
>>> problems, please let me know.
>>>
>>>
>>> Best regards,
>>> Hans
>>
> 
> Sorry, but I stopped doing any testing of the aardvark driver with the
> mainline kernel after PCI maintainers stopped taking fixes for the
> driver and stopped responding.
> 
> I'm not going to debug same issues again, which I have analyzed,
> prepared fixes, sent patches and see no progress there.
> 
> Seems that there is a status quo, and I'm not going to change it.

Dear Niklas,

Do you have any opinion on Pali's reply? Should patch 3/3 be discarded?

Beat regards,
Hans