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: <CCADFA64-D3BD-4972-994A-48E2606CCC66@kernel.org>
Date: Thu, 16 Jan 2025 17:39:30 +0100
From: Niklas Cassel <cassel@...nel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
CC: kw@...ux.com, gregkh@...uxfoundation.org, arnd@...db.de,
 lpieralisi@...nel.org, shuah@...nel.org, kishon@...nel.org,
 linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, bhelgaas@...gle.com,
 linux-arm-msm@...r.kernel.org, robh@...nel.org,
 linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v5 0/4] Migrate PCI Endpoint Subsystem tests to Kselftest



On 16 January 2025 17:12:27 CET, Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org> wrote:
>On Thu, Jan 16, 2025 at 04:36:07PM +0100, Niklas Cassel wrote:
>> On Thu, Jan 16, 2025 at 07:21:02PM +0530, Manivannan Sadhasivam wrote:
>> > Hi,
>> > 
>> > This series carries forward the effort to add Kselftest for PCI Endpoint
>> > Subsystem started by Aman Gupta [1] a while ago. I reworked the initial version
>> > based on another patch that fixes the return values of IOCTLs in
>> > pci_endpoint_test driver and did many cleanups. Since the resulting work
>> > modified the initial version substantially, I took over the authorship.
>> > 
>> > This series also incorporates the review comment by Shuah Khan [2] to move the
>> > existing tests from 'tools/pci' to 'tools/testing/kselftest/pci_endpoint' before
>> > migrating to Kselftest framework. I made sure that the tests are executable in
>> > each commit and updated documentation accordingly.
>> > 
>> > - Mani
>> > 
>> > [1] https://lore.kernel.org/linux-pci/20221007053934.5188-1-aman1.gupta@samsung.com
>> > [2] https://lore.kernel.org/linux-pci/b2a5db97-dc59-33ab-71cd-f591e0b1b34d@linuxfoundation.org
>> > 
>> > Changes in v5:
>> > 
>> > * Incorporated comments from Niklas
>> > * Added a patch to fix the DMA MEMCPY check in pci-epf-test driver
>> > * Collected tags
>> > * Rebased on top of pci/next 0333f56dbbf7ef6bb46d2906766c3e1b2a04a94d
>> > 
>> > Changes in v4:
>> > 
>> > * Dropped the BAR fix patches and submitted them separately:
>> >   https://lore.kernel.org/linux-pci/20241231130224.38206-1-manivannan.sadhasivam@linaro.org/
>> > * Rebased on top of pci/next 9e1b45d7a5bc0ad20f6b5267992da422884b916e
>> > 
>> > Changes in v3:
>> > 
>> > * Collected tags.
>> > * Added a note about failing testcase 10 and command to skip it in
>> >   documentation.
>> > * Removed Aman Gupta and Padmanabhan Rajanbabu from CC as their addresses are
>> >   bouncing.
>> > 
>> > Changes in v2:
>> > 
>> > * Added a patch that fixes return values of IOCTL in pci_endpoint_test driver
>> > * Moved the existing tests to new location before migrating
>> > * Added a fix for BARs on Qcom devices
>> > * Updated documentation and also added fixture variants for memcpy & DMA modes
>> > 
>> > 
>> > Manivannan Sadhasivam (4):
>> >   PCI: endpoint: pci-epf-test: Fix the check for DMA MEMCPY test
>> >   misc: pci_endpoint_test: Fix the return value of IOCTL
>> >   selftests: Move PCI Endpoint tests from tools/pci to Kselftests
>> >   selftests: pci_endpoint: Migrate to Kselftest framework
>> > 
>> >  Documentation/PCI/endpoint/pci-test-howto.rst | 170 +++++------
>> >  MAINTAINERS                                   |   2 +-
>> >  drivers/misc/pci_endpoint_test.c              | 255 +++++++++--------
>> >  drivers/pci/endpoint/functions/pci-epf-test.c |   4 +-
>> >  tools/pci/Build                               |   1 -
>> >  tools/pci/Makefile                            |  58 ----
>> >  tools/pci/pcitest.c                           | 264 ------------------
>> >  tools/pci/pcitest.sh                          |  73 -----
>> >  tools/testing/selftests/Makefile              |   1 +
>> >  .../testing/selftests/pci_endpoint/.gitignore |   2 +
>> >  tools/testing/selftests/pci_endpoint/Makefile |   7 +
>> >  tools/testing/selftests/pci_endpoint/config   |   4 +
>> >  .../pci_endpoint/pci_endpoint_test.c          | 221 +++++++++++++++
>> >  13 files changed, 435 insertions(+), 627 deletions(-)
>> >  delete mode 100644 tools/pci/Build
>> >  delete mode 100644 tools/pci/Makefile
>> >  delete mode 100644 tools/pci/pcitest.c
>> >  delete mode 100644 tools/pci/pcitest.sh
>> >  create mode 100644 tools/testing/selftests/pci_endpoint/.gitignore
>> >  create mode 100644 tools/testing/selftests/pci_endpoint/Makefile
>> >  create mode 100644 tools/testing/selftests/pci_endpoint/config
>> >  create mode 100644 tools/testing/selftests/pci_endpoint/pci_endpoint_test.c
>> > 
>> > -- 
>> > 2.25.1
>> > 
>> 
>> I ran the test using simply:
>> 
>> $ ./pci_endpoint_test
>> 
>> and got:
>> 
>> #  RUN           pci_ep_basic.MSIX_TEST ...
>> # pci_endpoint_test.c:129:MSIX_TEST:Expected 0 (0) == ret (-110)
>> # pci_endpoint_test.c:129:MSIX_TEST:Test failed for MSI-X33
>> # pci_endpoint_test.c:129:MSIX_TEST:Expected 0 (0) == ret (-110)
>> # pci_endpoint_test.c:129:MSIX_TEST:Test failed for MSI-X34
>> # pci_endpoint_test.c:129:MSIX_TEST:Expected 0 (0) == ret (-110)
>> # pci_endpoint_test.c:129:MSIX_TEST:Test failed for MSI-X35
>> # pci_endpoint_test.c:129:MSIX_TEST:Expected 0 (0) == ret (-110)
>> # pci_endpoint_test.c:129:MSIX_TEST:Test failed for MSI-X36
>> # pci_endpoint_test.c:129:MSIX_TEST:Expected 0 (0) == ret (-110)
>> # pci_endpoint_test.c:129:MSIX_TEST:Test failed for MSI-X37
>> # pci_endpoint_test.c:129:MSIX_TEST:Expected 0 (0) == ret (-110)
>> # pci_endpoint_test.c:129:MSIX_TEST:Test failed for MSI-X38
>> # pci_endpoint_test.c:129:MSIX_TEST:Expected 0 (0) == ret (-110)
>> # pci_endpoint_test.c:129:MSIX_TEST:Test failed for MSI-X39
>> 
>> 
>> I think that you should also do:
>> 
>> diff --git a/Documentation/PCI/endpoint/pci-test-howto.rst b/Documentation/PCI/endpoint/pci-test-howto.rst
>> index 7d0dbad61456..7d5049c884dd 100644
>> --- a/Documentation/PCI/endpoint/pci-test-howto.rst
>> +++ b/Documentation/PCI/endpoint/pci-test-howto.rst
>> @@ -81,8 +81,8 @@ device, the following commands can be used::
>>  
>>         # echo 0x104c > functions/pci_epf_test/func1/vendorid
>>         # echo 0xb500 > functions/pci_epf_test/func1/deviceid
>> -       # echo 16 > functions/pci_epf_test/func1/msi_interrupts
>> -       # echo 8 > functions/pci_epf_test/func1/msix_interrupts
>> +       # echo 32 > functions/pci_epf_test/func1/msi_interrupts
>> +       # echo 2048 > functions/pci_epf_test/func1/msix_interrupts
>>  
>> 
>> Such that the documentation suggests values that will actually make the
>> pci_endpoint_test pass without any special parameters set.
>> 
>
>Agree and this is what I am doing locally, but that change is not really related
>to this series. So I'll submit it separately.

I don't not agree that it is not related,
before this series the example output was:


MSI1:           OKAY
-	MSI2:           OKAY
-	MSI3:           OKAY
-	MSI4:           OKAY
-	MSI5:           OKAY
-	MSI6:           OKAY
-	MSI7:           OKAY
-	MSI8:           OKAY
-	MSI9:           OKAY
-	MSI10:          OKAY
-	MSI11:          OKAY
-	MSI12:          OKAY
-	MSI13:          OKAY
-	MSI14:          OKAY
-	MSI15:          OKAY
-	MSI16:          OKAY
-	MSI17:          NOT OKAY
-	MSI18:          NOT OKAY
-	MSI19:          NOT OKAY
-	MSI20:          NOT OKAY
-	MSI21:          NOT OKAY
-	MSI22:          NOT OKAY
-	MSI23:          NOT OKAY
-	MSI24:          NOT OKAY
-	MSI25:          NOT OKAY
-	MSI26:          NOT OKAY
-	MSI27:          NOT OKAY
-	MSI28:          NOT OKAY
-	MSI29:          NOT OKAY
-	MSI30:          NOT OKAY
-	MSI31:          NOT OKAY
-	MSI32:          NOT OKAY
-	SET IRQ TYPE TO MSI-X:          OKAY
-	MSI-X1:         OKAY
-	MSI-X2:         OKAY
-	MSI-X3:         OKAY
-	MSI-X4:         OKAY
-	MSI-X5:         OKAY
-	MSI-X6:         OKAY
-	MSI-X7:         OKAY
-	MSI-X8:         OKAY
-	MSI-X9:         NOT OKAY
-	MSI-X10:        NOT OKAY
-	MSI-X11:        NOT OKAY
-	MSI-X12:        NOT OKAY
-	MSI-X13:        NOT OKAY
-	MSI-X14:        NOT OKAY
-	MSI-X15:        NOT OKAY
-	MSI-X16:        NOT OKAY


So the output matched the suggested values in the documentation.

After this series, the documentation still suggests msi == 16 and msi-x == 8,
but the example output now shows that
there was no failures.


So I think it is most consistent to just update the example in the same commit (in this series) that updates the output to show everything as good.



Kind regards,
Niklas



>
>> 
>> Other than that small nit, for the series:
>> Tested-by: Niklas Cassel <cassel@...nel.org>
>
>Thanks a lot!
>
>- Mani
>
>-- 
>மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ