Message-ID: <de80df44-f797-4e8c-a411-09ed3c1286a3@samsung.com>
Date: Thu, 4 Dec 2025 14:13:45 +0100
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>, Bjorn
	Helgaas <bhelgaas@...gle.com>
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	iommu@...ts.linux.dev, Naresh Kamboju <naresh.kamboju@...aro.org>,
	Pavankumar Kondeti <quic_pkondeti@...cinc.com>, Xingang Wang
	<wangxingang5@...wei.com>, Robin Murphy <robin.murphy@....com>, Jason
	Gunthorpe <jgg@...pe.ca>, Manivannan Sadhasivam <mani@...nel.org>
Subject: Re: [PATCH v2 0/4] PCI: Fix ACS enablement for Root Ports in OF
 platforms

On 03.12.2025 13:04, Marek Szyprowski wrote:
> On 02.12.2025 15:22, Manivannan Sadhasivam wrote:
>> This series fixes the long standing issue with ACS in OF platforms. 
>> There are
>> two fixes in this series, both fixing independent issues on their 
>> own, but both
>> are needed to properly enable ACS on OF platforms.
>>
>> Issue(s) background
>> ===================
>>
>> Back in 2021, Xingang Wang first noted a failure in attaching the 
>> HiSilicon SEC
>> device to QEMU ARM64 pci-root-port device [1]. He then tracked down 
>> the issue to
>> ACS not being enabled for the QEMU Root Port device and he proposed a 
>> patch to
>> fix it [2].
>>
>> Once the patch got applied, people reported PCIe issues with 
>> linux-next on the
>> ARM Juno Development boards, where they saw failure in enumerating 
>> the endpoint
>> devices [3][4]. So soon, the patch got dropped, but the actual issue 
>> with the
>> ARM Juno boards was left behind.
>>
>> Fast forward to 2024, Pavan resubmitted the same fix [5] for his own 
>> usecase,
>> hoping that someone in the community would fix the issue with ARM 
>> Juno boards.
>> But the patch was rightly rejected, as a patch that was known to 
>> cause issues
>> should not be merged to the kernel. But again, no one investigated 
>> the Juno
>> issue and it was left behind again.
>>
>> Now it ended up in my plate and I managed to track down the issue 
>> with the help
>> of Naresh who got access to the Juno boards in LKFT. The Juno issue 
>> was with the
>> PCIe switch from Microsemi/IDT, which triggers ACS Source Validation 
>> error on
>> Completions received for the Configuration Read Request from a device 
>> connected
>> to the downstream port that has not yet captured the PCIe bus number. 
>> As per the
>> PCIe spec r6.0 sec 2.2.6.2, "Functions must capture the Bus and 
>> Device Numbers
>> supplied with all Type 0 Configuration Write Requests completed by 
>> the Function
>> and supply these numbers in the Bus and Device Number fields of the 
>> Requester ID
>> for all Requests". So during the first Configuration Read Request 
>> issued by the
>> switch downstream port during enumeration (for reading Vendor ID), 
>> Bus and
>> Device numbers will be unknown to the device. So it responds to the 
>> Read Request
>> with Completion having Bus and Device number as 0. The switch 
>> interprets the
>> Completion as an ACS Source Validation error and drops the 
>> completion, leading
>> to the failure in detecting the endpoint device. Though the PCIe spec 
>> r6.0, sec
>> 6.12.1.1, states that "Completions are never affected by ACS Source 
>> Validation".
>> This behavior is in violation of the spec.
>>
>> Solution
>> ========
>>
>> In September, I submitted a series [6] to fix both issues. For the 
>> IDT issue,
>> I reused the existing quirk in the PCI core which does a dummy config 
>> write
>> before issuing the first config read to the device. And for the ACS 
>> enablement
>> issue, I just resubmitted the original patch from Xingang which called
>> pci_request_acs() from devm_of_pci_bridge_init().
>>
>> But during the review of the series, several comments were received 
>> and they
>> required the series to be reworked completely. Hence, in this 
>> version, I've
>> incorported the comments as below:
>>
>> 1. For the ACS enablement issue, I've moved the pci_enable_acs() call 
>> from
>> pci_acs_init() to pci_dma_configure().
>>
>> 2. For the IDT issue, I've cached the ACS capabilities (RO) in 
>> 'pci_dev',
>> collected the broken capability for the IDT switches in the quirk and 
>> used it to
>> disable the capability in the cache. This also allowed me to get rid 
>> of the
>> earlier workaround for the switch.
>>
>> [1] 
>> https://lore.kernel.org/all/038397a6-57e2-b6fc-6e1c-7c03b7be9d96@huawei.com
>> [2] 
>> https://lore.kernel.org/all/1621566204-37456-1-git-send-email-wangxingang5@huawei.com
>> [3] 
>> https://lore.kernel.org/all/01314d70-41e6-70f9-e496-84091948701a@samsung.com
>> [4] 
>> https://lore.kernel.org/all/CADYN=9JWU3CMLzMEcD5MSQGnaLyDRSKc5SofBFHUax6YuTRaJA@mail.gmail.com
>> [5] 
>> https://lore.kernel.org/linux-pci/20241107-pci_acs_fix-v1-1-185a2462a571@quicinc.com
>> [6] 
>> https://lore.kernel.org/linux-pci/20250910-pci-acs-v1-0-fe9adb65ad7d@oss.qualcomm.com
>>
> Thanks for this patchset! I've tested it on my ARM Juno R1 and it 
> looks that it almost works fine. This patchset even fixed some issues 
> with PCI devices probe, as I again see SATA and GBit ethernet devices, 
> which were missing since Linux v6.14 (it looks that I've also missed 
> this in my tests).
>
> # lspci
> 00:00.0 PCI bridge: PLDA PCI Express Core Reference Design (rev 01)
> 01:00.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 
> 8090 (rev 02)
> 02:01.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 
> 8090 (rev 02)
> 02:02.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 
> 8090 (rev 02)
> 02:03.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 
> 8090 (rev 02)
> 02:0c.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 
> 8090 (rev 02)
> 02:10.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 
> 8090 (rev 02)
> 02:1f.0 PCI bridge: Integrated Device Technology, Inc. [IDT] Device 
> 8090 (rev 02)
> 03:00.0 Mass storage controller: Silicon Image, Inc. SiI 3132 Serial 
> ATA Raid II Controller (rev 01)
> 08:00.0 Ethernet controller: Marvell Technology Group Ltd. 88E8057 
> PCI-E Gigabit Ethernet Controller
>
> However there is also a regression. After applying this patchset 
> system suspend/resume stopped working. This is probably related to 
> this message:
>
> pcieport 0000:02:1f.0: Unable to change power state from D0 to D3hot, 
> device inaccessible
>
> which appears after calling 'rtcwake -s10 -mmem'. This might not be 
> related to this patchset, so I probably need to apply it on older 
> kernel releases and check.


Just one more information - I've applied this patchset on top of v6.16 
and it works perfectly on ARM Juno R1. SATA and GBit ethernet are 
visible again and system suspend/resume works too, so the issue with the 
latter on top of v6.18 seems not to be directly related to $subject 
patchset. I will try to bisect this issue when I have some spare time.

Feel free to add:

Tested-by: Marek Szyprowski <m.szyprowski@...sung.com>


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Powered by blists - more mailing lists