[PATCH v4 0/3] Migrate PCI Endpoint Subsystem tests to Kselftest

List overview All Threads
Download

newer

older

[PATCH 3/3] selftests/ftrace: Add...

[PATCH 1/2] selftests/mm: use...

Manivannan Sadhasivam

31 Dec 2024 31 Dec '24

1:13 p.m.

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.... [2] https://lore.kernel.org/linux-pci/b2a5db97-dc59-33ab-71cd-f591e0b1b34d@linux...

Changes in v4:

* Dropped the BAR fix patches and submitted them separately: https://lore.kernel.org/linux-pci/20241231130224.38206-1-manivannan.sadhasiv... * 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 (3): 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 | 155 ++++------ MAINTAINERS | 2 +- drivers/misc/pci_endpoint_test.c | 250 ++++++++--------- 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 | 194 +++++++++++++ 12 files changed, 386 insertions(+), 625 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

Show replies by date

Manivannan Sadhasivam

31 Dec 31 Dec

1:13 p.m.

New subject: [PATCH v4 1/3] misc: pci_endpoint_test: Fix the return value of IOCTL

IOCTLs are supposed to return 0 for success and negative error codes for failure. Currently, this driver is returning 0 for failure and 1 for success, that's not correct. Hence, fix it!

Reported-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Closes: https://lore.kernel.org/all/YvzNg5ROnxEApDgS@kroah.com Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device") Reviewed-by: Damien Le Moal dlemoal@kernel.org Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org --- drivers/misc/pci_endpoint_test.c | 250 +++++++++++++++---------------- tools/pci/pcitest.c | 51 +++---- 2 files changed, 148 insertions(+), 153 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 5c99da952b7a..7d3f78b6f854 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -169,43 +169,47 @@ static void pci_endpoint_test_free_irq_vectors(struct pci_endpoint_test *test) test->irq_type = IRQ_TYPE_UNDEFINED; }

-static bool pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, +static int pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, int type) { - int irq = -1; + int irq; struct pci_dev *pdev = test->pdev; struct device *dev = &pdev->dev; - bool res = true;

switch (type) { case IRQ_TYPE_INTX: irq = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX); - if (irq < 0) + if (irq < 0) { dev_err(dev, "Failed to get Legacy interrupt\n"); + return -ENOSPC; + } + break; case IRQ_TYPE_MSI: irq = pci_alloc_irq_vectors(pdev, 1, 32, PCI_IRQ_MSI); - if (irq < 0) + if (irq < 0) { dev_err(dev, "Failed to get MSI interrupts\n"); + return -ENOSPC; + } + break; case IRQ_TYPE_MSIX: irq = pci_alloc_irq_vectors(pdev, 1, 2048, PCI_IRQ_MSIX); - if (irq < 0) + if (irq < 0) { dev_err(dev, "Failed to get MSI-X interrupts\n"); + return -ENOSPC; + } + break; default: dev_err(dev, "Invalid IRQ type selected\n"); - } - - if (irq < 0) { - irq = 0; - res = false; + return -EINVAL; }

test->irq_type = type; test->num_irqs = irq;

- return res; + return 0; }

static void pci_endpoint_test_release_irq(struct pci_endpoint_test *test) @@ -220,22 +224,22 @@ static void pci_endpoint_test_release_irq(struct pci_endpoint_test *test) test->num_irqs = 0; }

-static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test) +static int pci_endpoint_test_request_irq(struct pci_endpoint_test *test) { int i; - int err; + int ret; struct pci_dev *pdev = test->pdev; struct device *dev = &pdev->dev;

for (i = 0; i < test->num_irqs; i++) { - err = devm_request_irq(dev, pci_irq_vector(pdev, i), + ret = devm_request_irq(dev, pci_irq_vector(pdev, i), pci_endpoint_test_irqhandler, IRQF_SHARED, test->name, test); - if (err) + if (ret) goto fail; }

- return true; + return 0;

fail: switch (irq_type) { @@ -255,7 +259,7 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test) break; }

- return false; + return ret; }

static const u32 bar_test_pattern[] = { @@ -280,7 +284,7 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test, return memcmp(write_buf, read_buf, size); }

-static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, +static int pci_endpoint_test_bar(struct pci_endpoint_test *test, enum pci_barno barno) { int j, bar_size, buf_size, iters, remain; @@ -289,7 +293,7 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, struct pci_dev *pdev = test->pdev;

if (!test->bar[barno]) - return false; + return -ENOMEM;

bar_size = pci_resource_len(pdev, barno);

@@ -304,25 +308,25 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,

write_buf = kmalloc(buf_size, GFP_KERNEL); if (!write_buf) - return false; + return -ENOMEM;

read_buf = kmalloc(buf_size, GFP_KERNEL); if (!read_buf) - return false; + return -ENOMEM;

iters = bar_size / buf_size; for (j = 0; j < iters; j++) if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j, write_buf, read_buf, buf_size)) - return false; + return -EIO;

remain = bar_size % buf_size; if (remain) if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * iters, write_buf, read_buf, remain)) - return false; + return -EIO;

- return true; + return 0; }

static u32 bar_test_pattern_with_offset(enum pci_barno barno, int offset) @@ -337,7 +341,7 @@ static u32 bar_test_pattern_with_offset(enum pci_barno barno, int offset) return val; }

-static bool pci_endpoint_test_bars_write_bar(struct pci_endpoint_test *test, +static void pci_endpoint_test_bars_write_bar(struct pci_endpoint_test *test, enum pci_barno barno) { struct pci_dev *pdev = test->pdev; @@ -351,11 +355,9 @@ static bool pci_endpoint_test_bars_write_bar(struct pci_endpoint_test *test, for (j = 0; j < size; j += 4) writel_relaxed(bar_test_pattern_with_offset(barno, j), test->bar[barno] + j); - - return true; }

-static bool pci_endpoint_test_bars_read_bar(struct pci_endpoint_test *test, +static int pci_endpoint_test_bars_read_bar(struct pci_endpoint_test *test, enum pci_barno barno) { struct pci_dev *pdev = test->pdev; @@ -376,14 +378,14 @@ static bool pci_endpoint_test_bars_read_bar(struct pci_endpoint_test *test, dev_err(dev, "BAR%d incorrect data at offset: %#x, got: %#x expected: %#x\n", barno, j, val, expected); - return false; + return -EIO; } }

- return true; + return 0; }

-static bool pci_endpoint_test_bars(struct pci_endpoint_test *test) +static int pci_endpoint_test_bars(struct pci_endpoint_test *test) { enum pci_barno bar; bool ret; @@ -407,10 +409,10 @@ static bool pci_endpoint_test_bars(struct pci_endpoint_test *test) } }

- return true; + return 0; }

-static bool pci_endpoint_test_intx_irq(struct pci_endpoint_test *test) +static int pci_endpoint_test_intx_irq(struct pci_endpoint_test *test) { u32 val;

@@ -422,12 +424,12 @@ static bool pci_endpoint_test_intx_irq(struct pci_endpoint_test *test) val = wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); if (!val) - return false; + return -ETIMEDOUT;

- return true; + return 0; }

-static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test, +static int pci_endpoint_test_msi_irq(struct pci_endpoint_test *test, u16 msi_num, bool msix) { u32 val; @@ -442,9 +444,12 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test, val = wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); if (!val) - return false; + return -ETIMEDOUT;

- return pci_irq_vector(pdev, msi_num - 1) == test->last_irq; + if (!(pci_irq_vector(pdev, msi_num - 1) == test->last_irq)) + return -EIO; + + return 0; }

static int pci_endpoint_test_validate_xfer_params(struct device *dev, @@ -463,11 +468,10 @@ static int pci_endpoint_test_validate_xfer_params(struct device *dev, return 0; }

-static bool pci_endpoint_test_copy(struct pci_endpoint_test *test, +static int pci_endpoint_test_copy(struct pci_endpoint_test *test, unsigned long arg) { struct pci_endpoint_test_xfer_param param; - bool ret = false; void *src_addr; void *dst_addr; u32 flags = 0; @@ -486,17 +490,17 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test, int irq_type = test->irq_type; u32 src_crc32; u32 dst_crc32; - int err; + int ret;

- err = copy_from_user(&param, (void __user *)arg, sizeof(param)); - if (err) { + ret = copy_from_user(&param, (void __user *)arg, sizeof(param)); + if (ret) { dev_err(dev, "Failed to get transfer param\n"); - return false; + return -EFAULT; }

- err = pci_endpoint_test_validate_xfer_params(dev, &param, alignment); - if (err) - return false; + ret = pci_endpoint_test_validate_xfer_params(dev, &param, alignment); + if (ret) + return ret;

size = param.size;

@@ -506,22 +510,21 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,

if (irq_type < IRQ_TYPE_INTX || irq_type > IRQ_TYPE_MSIX) { dev_err(dev, "Invalid IRQ type option\n"); - goto err; + return -EINVAL; }

orig_src_addr = kzalloc(size + alignment, GFP_KERNEL); if (!orig_src_addr) { dev_err(dev, "Failed to allocate source buffer\n"); - ret = false; - goto err; + return -ENOMEM; }

get_random_bytes(orig_src_addr, size + alignment); orig_src_phys_addr = dma_map_single(dev, orig_src_addr, size + alignment, DMA_TO_DEVICE); - if (dma_mapping_error(dev, orig_src_phys_addr)) { + ret = dma_mapping_error(dev, orig_src_phys_addr); + if (ret) { dev_err(dev, "failed to map source buffer address\n"); - ret = false; goto err_src_phys_addr; }

@@ -545,15 +548,15 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test, orig_dst_addr = kzalloc(size + alignment, GFP_KERNEL); if (!orig_dst_addr) { dev_err(dev, "Failed to allocate destination address\n"); - ret = false; + ret = -ENOMEM; goto err_dst_addr; }

orig_dst_phys_addr = dma_map_single(dev, orig_dst_addr, size + alignment, DMA_FROM_DEVICE); - if (dma_mapping_error(dev, orig_dst_phys_addr)) { + ret = dma_mapping_error(dev, orig_dst_phys_addr); + if (ret) { dev_err(dev, "failed to map destination buffer address\n"); - ret = false; goto err_dst_phys_addr; }

@@ -586,8 +589,8 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test, DMA_FROM_DEVICE);

dst_crc32 = crc32_le(~0, dst_addr, size); - if (dst_crc32 == src_crc32) - ret = true; + if (dst_crc32 != src_crc32) + ret = -EIO;

err_dst_phys_addr: kfree(orig_dst_addr); @@ -598,16 +601,13 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,

err_src_phys_addr: kfree(orig_src_addr); - -err: return ret; }

-static bool pci_endpoint_test_write(struct pci_endpoint_test *test, +static int pci_endpoint_test_write(struct pci_endpoint_test *test, unsigned long arg) { struct pci_endpoint_test_xfer_param param; - bool ret = false; u32 flags = 0; bool use_dma; u32 reg; @@ -622,17 +622,17 @@ static bool pci_endpoint_test_write(struct pci_endpoint_test *test, int irq_type = test->irq_type; size_t size; u32 crc32; - int err; + int ret;

- err = copy_from_user(&param, (void __user *)arg, sizeof(param)); - if (err != 0) { + ret = copy_from_user(&param, (void __user *)arg, sizeof(param)); + if (ret) { dev_err(dev, "Failed to get transfer param\n"); - return false; + return -EFAULT; }

- err = pci_endpoint_test_validate_xfer_params(dev, &param, alignment); - if (err) - return false; + ret = pci_endpoint_test_validate_xfer_params(dev, &param, alignment); + if (ret) + return ret;

size = param.size;

@@ -642,23 +642,22 @@ static bool pci_endpoint_test_write(struct pci_endpoint_test *test,

if (irq_type < IRQ_TYPE_INTX || irq_type > IRQ_TYPE_MSIX) { dev_err(dev, "Invalid IRQ type option\n"); - goto err; + return -EINVAL; }

orig_addr = kzalloc(size + alignment, GFP_KERNEL); if (!orig_addr) { dev_err(dev, "Failed to allocate address\n"); - ret = false; - goto err; + return -ENOMEM; }

get_random_bytes(orig_addr, size + alignment);

orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment, DMA_TO_DEVICE); - if (dma_mapping_error(dev, orig_phys_addr)) { + ret = dma_mapping_error(dev, orig_phys_addr); + if (ret) { dev_err(dev, "failed to map source buffer address\n"); - ret = false; goto err_phys_addr; }

@@ -691,24 +690,21 @@ static bool pci_endpoint_test_write(struct pci_endpoint_test *test, wait_for_completion(&test->irq_raised);

reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS); - if (reg & STATUS_READ_SUCCESS) - ret = true; + if (!(reg & STATUS_READ_SUCCESS)) + ret = -EIO;

dma_unmap_single(dev, orig_phys_addr, size + alignment, DMA_TO_DEVICE);

err_phys_addr: kfree(orig_addr); - -err: return ret; }

-static bool pci_endpoint_test_read(struct pci_endpoint_test *test, +static int pci_endpoint_test_read(struct pci_endpoint_test *test, unsigned long arg) { struct pci_endpoint_test_xfer_param param; - bool ret = false; u32 flags = 0; bool use_dma; size_t size; @@ -722,17 +718,17 @@ static bool pci_endpoint_test_read(struct pci_endpoint_test *test, size_t alignment = test->alignment; int irq_type = test->irq_type; u32 crc32; - int err; + int ret;

- err = pci_endpoint_test_validate_xfer_params(dev, &param, alignment); - if (err) - return false; + ret = pci_endpoint_test_validate_xfer_params(dev, &param, alignment); + if (ret) + return ret;

size = param.size;

@@ -742,21 +738,20 @@ static bool pci_endpoint_test_read(struct pci_endpoint_test *test,

if (irq_type < IRQ_TYPE_INTX || irq_type > IRQ_TYPE_MSIX) { dev_err(dev, "Invalid IRQ type option\n"); - goto err; + return -EINVAL; }

orig_addr = kzalloc(size + alignment, GFP_KERNEL); if (!orig_addr) { dev_err(dev, "Failed to allocate destination address\n"); - ret = false; - goto err; + return -ENOMEM; }

orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment, DMA_FROM_DEVICE); - if (dma_mapping_error(dev, orig_phys_addr)) { + ret = dma_mapping_error(dev, orig_phys_addr); + if (ret) { dev_err(dev, "failed to map source buffer address\n"); - ret = false; goto err_phys_addr; }

@@ -788,50 +783,51 @@ static bool pci_endpoint_test_read(struct pci_endpoint_test *test, DMA_FROM_DEVICE);

crc32 = crc32_le(~0, addr, size); - if (crc32 == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM)) - ret = true; + if (crc32 != pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM)) + ret = -EIO;

err_phys_addr: kfree(orig_addr); -err: return ret; }

-static bool pci_endpoint_test_clear_irq(struct pci_endpoint_test *test) +static int pci_endpoint_test_clear_irq(struct pci_endpoint_test *test) { pci_endpoint_test_release_irq(test); pci_endpoint_test_free_irq_vectors(test); - return true; + + return 0; }

-static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test, +static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test, int req_irq_type) { struct pci_dev *pdev = test->pdev; struct device *dev = &pdev->dev; + int ret;

if (req_irq_type < IRQ_TYPE_INTX || req_irq_type > IRQ_TYPE_MSIX) { dev_err(dev, "Invalid IRQ type option\n"); - return false; + return -EINVAL; }

if (test->irq_type == req_irq_type) - return true; + return 0;

pci_endpoint_test_release_irq(test); pci_endpoint_test_free_irq_vectors(test);

- if (!pci_endpoint_test_alloc_irq_vectors(test, req_irq_type)) - goto err; + ret = pci_endpoint_test_alloc_irq_vectors(test, req_irq_type); + if (ret) + return ret;

- if (!pci_endpoint_test_request_irq(test)) - goto err; - - return true; + ret = pci_endpoint_test_request_irq(test); + if (ret) { + pci_endpoint_test_free_irq_vectors(test); + return ret; + }

-err: - pci_endpoint_test_free_irq_vectors(test); - return false; + return 0; }

static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, @@ -913,7 +909,7 @@ static void pci_endpoint_test_get_capabilities(struct pci_endpoint_test *test) static int pci_endpoint_test_probe(struct pci_dev *pdev, const struct pci_device_id *ent) { - int err; + int ret; int id; char name[24]; enum pci_barno bar; @@ -952,24 +948,23 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,

dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48));

- err = pci_enable_device(pdev); - if (err) { + ret = pci_enable_device(pdev); + if (ret) { dev_err(dev, "Cannot enable PCI device\n"); - return err; + return ret; }

- err = pci_request_regions(pdev, DRV_MODULE_NAME); - if (err) { + ret = pci_request_regions(pdev, DRV_MODULE_NAME); + if (ret) { dev_err(dev, "Cannot obtain PCI resources\n"); goto err_disable_pdev; }

pci_set_master(pdev);

- if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) { - err = -EINVAL; + ret = pci_endpoint_test_alloc_irq_vectors(test, irq_type); + if (ret) goto err_disable_irq; - }

for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) { if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) { @@ -984,7 +979,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,

test->base = test->bar[test_reg_bar]; if (!test->base) { - err = -ENOMEM; + ret = -ENOMEM; dev_err(dev, "Cannot perform PCI test without BAR%d\n", test_reg_bar); goto err_iounmap; @@ -994,7 +989,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,

id = ida_alloc(&pci_endpoint_test_ida, GFP_KERNEL); if (id < 0) { - err = id; + ret = id; dev_err(dev, "Unable to get id\n"); goto err_iounmap; } @@ -1002,14 +997,13 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, snprintf(name, sizeof(name), DRV_MODULE_NAME ".%d", id); test->name = kstrdup(name, GFP_KERNEL); if (!test->name) { - err = -ENOMEM; + ret = -ENOMEM; goto err_ida_remove; }

- if (!pci_endpoint_test_request_irq(test)) { - err = -EINVAL; + ret = pci_endpoint_test_request_irq(test); + if (ret) goto err_kfree_test_name; - }

pci_endpoint_test_get_capabilities(test);

@@ -1017,14 +1011,14 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, misc_device->minor = MISC_DYNAMIC_MINOR; misc_device->name = kstrdup(name, GFP_KERNEL); if (!misc_device->name) { - err = -ENOMEM; + ret = -ENOMEM; goto err_release_irq; } misc_device->parent = &pdev->dev; misc_device->fops = &pci_endpoint_test_fops;

- err = misc_register(misc_device); - if (err) { + ret = misc_register(misc_device); + if (ret) { dev_err(dev, "Failed to register device\n"); goto err_kfree_name; } @@ -1056,7 +1050,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, err_disable_pdev: pci_disable_device(pdev);

- return err; + return ret; }

static void pci_endpoint_test_remove(struct pci_dev *pdev) diff --git a/tools/pci/pcitest.c b/tools/pci/pcitest.c index 08f355083754..b96cc118839b 100644 --- a/tools/pci/pcitest.c +++ b/tools/pci/pcitest.c @@ -16,7 +16,6 @@

#include <linux/pcitest.h>

-static char *result[] = { "NOT OKAY", "OKAY" }; static char *irq[] = { "LEGACY", "MSI", "MSI-X" };

struct pci_test { @@ -53,72 +52,74 @@ static int run_test(struct pci_test *test) ret = ioctl(fd, PCITEST_BAR, test->barnum); fprintf(stdout, "BAR%d:\t\t", test->barnum); if (ret < 0) - fprintf(stdout, "TEST FAILED\n"); + fprintf(stdout, "NOT OKAY\n"); else - fprintf(stdout, "%s\n", result[ret]); + fprintf(stdout, "OKAY\n"); }

if (test->consecutive_bar_test) { ret = ioctl(fd, PCITEST_BARS); fprintf(stdout, "Consecutive BAR test:\t\t"); if (ret < 0) - fprintf(stdout, "TEST FAILED\n"); + fprintf(stdout, "NOT OKAY\n"); else - fprintf(stdout, "%s\n", result[ret]); + fprintf(stdout, "OKAY\n"); }

if (test->set_irqtype) { ret = ioctl(fd, PCITEST_SET_IRQTYPE, test->irqtype); fprintf(stdout, "SET IRQ TYPE TO %s:\t\t", irq[test->irqtype]); if (ret < 0) - fprintf(stdout, "FAILED\n"); + fprintf(stdout, "NOT OKAY\n"); else - fprintf(stdout, "%s\n", result[ret]); + fprintf(stdout, "OKAY\n"); }

if (test->get_irqtype) { ret = ioctl(fd, PCITEST_GET_IRQTYPE); fprintf(stdout, "GET IRQ TYPE:\t\t"); - if (ret < 0) - fprintf(stdout, "FAILED\n"); - else + if (ret < 0) { + fprintf(stdout, "NOT OKAY\n"); + } else { fprintf(stdout, "%s\n", irq[ret]); + ret = 0; + } }

if (test->clear_irq) { ret = ioctl(fd, PCITEST_CLEAR_IRQ); fprintf(stdout, "CLEAR IRQ:\t\t"); if (ret < 0) - fprintf(stdout, "FAILED\n"); + fprintf(stdout, "NOT OKAY\n"); else - fprintf(stdout, "%s\n", result[ret]); + fprintf(stdout, "OKAY\n"); }

if (test->legacyirq) { ret = ioctl(fd, PCITEST_LEGACY_IRQ, 0); fprintf(stdout, "LEGACY IRQ:\t"); if (ret < 0) - fprintf(stdout, "TEST FAILED\n"); + fprintf(stdout, "NOT OKAY\n"); else - fprintf(stdout, "%s\n", result[ret]); + fprintf(stdout, "OKAY\n"); }

if (test->msinum > 0 && test->msinum <= 32) { ret = ioctl(fd, PCITEST_MSI, test->msinum); fprintf(stdout, "MSI%u:\t\t", test->msinum); if (ret < 0) - fprintf(stdout, "TEST FAILED\n"); + fprintf(stdout, "NOT OKAY\n"); else - fprintf(stdout, "%s\n", result[ret]); + fprintf(stdout, "OKAY\n"); }

if (test->msixnum > 0 && test->msixnum <= 2048) { ret = ioctl(fd, PCITEST_MSIX, test->msixnum); fprintf(stdout, "MSI-X%u:\t\t", test->msixnum); if (ret < 0) - fprintf(stdout, "TEST FAILED\n"); + fprintf(stdout, "NOT OKAY\n"); else - fprintf(stdout, "%s\n", result[ret]); + fprintf(stdout, "OKAY\n"); }

if (test->write) { @@ -128,9 +129,9 @@ static int run_test(struct pci_test *test) ret = ioctl(fd, PCITEST_WRITE, &param); fprintf(stdout, "WRITE (%7lu bytes):\t\t", test->size); if (ret < 0) - fprintf(stdout, "TEST FAILED\n"); + fprintf(stdout, "NOT OKAY\n"); else - fprintf(stdout, "%s\n", result[ret]); + fprintf(stdout, "OKAY\n"); }

if (test->read) { @@ -140,9 +141,9 @@ static int run_test(struct pci_test *test) ret = ioctl(fd, PCITEST_READ, &param); fprintf(stdout, "READ (%7lu bytes):\t\t", test->size); if (ret < 0) - fprintf(stdout, "TEST FAILED\n"); + fprintf(stdout, "NOT OKAY\n"); else - fprintf(stdout, "%s\n", result[ret]); + fprintf(stdout, "OKAY\n"); }

if (test->copy) { @@ -152,14 +153,14 @@ static int run_test(struct pci_test *test) ret = ioctl(fd, PCITEST_COPY, &param); fprintf(stdout, "COPY (%7lu bytes):\t\t", test->size); if (ret < 0) - fprintf(stdout, "TEST FAILED\n"); + fprintf(stdout, "NOT OKAY\n"); else - fprintf(stdout, "%s\n", result[ret]); + fprintf(stdout, "OKAY\n"); }

fflush(stdout); close(fd); - return (ret < 0) ? ret : 1 - ret; /* return 0 if test succeeded */ + return ret; }

int main(int argc, char **argv)

-- 2.25.1

Niklas Cassel

4:57 p.m.

New subject: [PATCH v4 1/3] misc: pci_endpoint_test: Fix the return value of IOCTL

On Tue, Dec 31, 2024 at 06:43:39PM +0530, Manivannan Sadhasivam wrote:

...

IOCTLs are supposed to return 0 for success and negative error codes for failure. Currently, this driver is returning 0 for failure and 1 for success, that's not correct. Hence, fix it!

Reported-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Closes: https://lore.kernel.org/all/YvzNg5ROnxEApDgS@kroah.com Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device") Reviewed-by: Damien Le Moal dlemoal@kernel.org Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org

drivers/misc/pci_endpoint_test.c | 250 +++++++++++++++---------------- tools/pci/pcitest.c | 51 +++---- 2 files changed, 148 insertions(+), 153 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 5c99da952b7a..7d3f78b6f854 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -169,43 +169,47 @@ static void pci_endpoint_test_free_irq_vectors(struct pci_endpoint_test *test) test->irq_type = IRQ_TYPE_UNDEFINED; } -static bool pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, +static int pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, int type) {

int irq = -1;

int irq; struct pci_dev *pdev = test->pdev; struct device *dev = &pdev->dev;

bool res = true;

switch (type) { case IRQ_TYPE_INTX: irq = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
if (irq < 0)
if (irq < 0) {
dev_err(dev, "Failed to get Legacy interrupt\n");
	return -ENOSPC;
}
break; case IRQ_TYPE_MSI: irq = pci_alloc_irq_vectors(pdev, 1, 32, PCI_IRQ_MSI);
if (irq < 0)
if (irq < 0) {
dev_err(dev, "Failed to get MSI interrupts\n");
	return -ENOSPC;
}
break; case IRQ_TYPE_MSIX: irq = pci_alloc_irq_vectors(pdev, 1, 2048, PCI_IRQ_MSIX);
if (irq < 0)
if (irq < 0) {
dev_err(dev, "Failed to get MSI-X interrupts\n");
	return -ENOSPC;

From the pci_alloc_irq_vectors() kdoc: * Return: number of allocated vectors (which might be smaller than * @max_vecs), -ENOSPC if less than @min_vecs interrupt vectors are * available, other errnos otherwise.

So pci_alloc_irq_vectors() can return errors different than ENOSPC, and in that case, you will overwrite that error.

...

@@ -442,9 +444,12 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test, val = wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); if (!val)
return false;
return -ETIMEDOUT;
return pci_irq_vector(pdev, msi_num - 1) == test->last_irq;

if (!(pci_irq_vector(pdev, msi_num - 1) == test->last_irq))

if (pci_irq_vector(pdev, msi_num - 1) != test->last_irq) ?

Or perhaps even:

ret = pci_irq_vector(); if (ret < 0) return ret;

if (ret != test->last_irq) return -EIO;

Otherwise, this looks good to me: Reviewed-by: Niklas Cassel cassel@kernel.org

Manivannan Sadhasivam

6:51 p.m.

New subject: [PATCH v4 1/3] misc: pci_endpoint_test: Fix the return value of IOCTL

On Tue, Dec 31, 2024 at 05:57:47PM +0100, Niklas Cassel wrote:

...

On Tue, Dec 31, 2024 at 06:43:39PM +0530, Manivannan Sadhasivam wrote:

...
IOCTLs are supposed to return 0 for success and negative error codes for failure. Currently, this driver is returning 0 for failure and 1 for success, that's not correct. Hence, fix it!

Reported-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Closes: https://lore.kernel.org/all/YvzNg5ROnxEApDgS@kroah.com Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device") Reviewed-by: Damien Le Moal dlemoal@kernel.org Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org

drivers/misc/pci_endpoint_test.c | 250 +++++++++++++++---------------- tools/pci/pcitest.c | 51 +++---- 2 files changed, 148 insertions(+), 153 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 5c99da952b7a..7d3f78b6f854 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -169,43 +169,47 @@ static void pci_endpoint_test_free_irq_vectors(struct pci_endpoint_test *test) test->irq_type = IRQ_TYPE_UNDEFINED; } -static bool pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, +static int pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, int type) {

int irq = -1;

int irq; struct pci_dev *pdev = test->pdev; struct device *dev = &pdev->dev;

bool res = true;

switch (type) { case IRQ_TYPE_INTX: irq = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
if (irq < 0)
if (irq < 0) {
dev_err(dev, "Failed to get Legacy interrupt\n");
	return -ENOSPC;
}
break; case IRQ_TYPE_MSI: irq = pci_alloc_irq_vectors(pdev, 1, 32, PCI_IRQ_MSI);
if (irq < 0)
if (irq < 0) {
dev_err(dev, "Failed to get MSI interrupts\n");
	return -ENOSPC;
}
break; case IRQ_TYPE_MSIX: irq = pci_alloc_irq_vectors(pdev, 1, 2048, PCI_IRQ_MSIX);
if (irq < 0)
if (irq < 0) {
dev_err(dev, "Failed to get MSI-X interrupts\n");
	return -ENOSPC;
From the pci_alloc_irq_vectors() kdoc:

Return: number of allocated vectors (which might be smaller than

@max_vecs), -ENOSPC if less than @min_vecs interrupt vectors are

available, other errnos otherwise.

So pci_alloc_irq_vectors() can return errors different than ENOSPC, and in that case, you will overwrite that error.

Ack.

...

...
@@ -442,9 +444,12 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test, val = wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); if (!val)
return false;
return -ETIMEDOUT;
return pci_irq_vector(pdev, msi_num - 1) == test->last_irq;

if (!(pci_irq_vector(pdev, msi_num - 1) == test->last_irq))
if (pci_irq_vector(pdev, msi_num - 1) != test->last_irq) ?

Or perhaps even:

ret = pci_irq_vector(); if (ret < 0) return ret;

if (ret != test->last_irq) return -EIO;

Ack.

...

Otherwise, this looks good to me: Reviewed-by: Niklas Cassel cassel@kernel.org

Thanks!

- Mani

-- மணிவண்ணன் சதாசிவம்

Manivannan Sadhasivam

1:13 p.m.

New subject: [PATCH v4 2/3] selftests: Move PCI Endpoint tests from tools/pci to Kselftests

This just moves the existing tests under tools/pci to tools/testing/selftests/pci_endpoint and adjusts the paths in Makefile accordingly. Migration to Kselftest framework will be done in subsequent commits.

Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org --- Documentation/PCI/endpoint/pci-test-howto.rst | 9 +++++---- MAINTAINERS | 2 +- tools/testing/selftests/pci_endpoint/.gitignore | 3 +++ tools/{pci => testing/selftests/pci_endpoint}/Build | 0 tools/{pci => testing/selftests/pci_endpoint}/Makefile | 10 +++++----- .../{pci => testing/selftests/pci_endpoint}/pcitest.c | 0 .../{pci => testing/selftests/pci_endpoint}/pcitest.sh | 0 7 files changed, 14 insertions(+), 10 deletions(-) create mode 100644 tools/testing/selftests/pci_endpoint/.gitignore rename tools/{pci => testing/selftests/pci_endpoint}/Build (100%) rename tools/{pci => testing/selftests/pci_endpoint}/Makefile (83%) rename tools/{pci => testing/selftests/pci_endpoint}/pcitest.c (100%) rename tools/{pci => testing/selftests/pci_endpoint}/pcitest.sh (100%)

diff --git a/Documentation/PCI/endpoint/pci-test-howto.rst b/Documentation/PCI/endpoint/pci-test-howto.rst index 909f770a07d6..c4ae7af50ede 100644 --- a/Documentation/PCI/endpoint/pci-test-howto.rst +++ b/Documentation/PCI/endpoint/pci-test-howto.rst @@ -123,16 +123,17 @@ above:: Using Endpoint Test function Device -----------------------------------

-pcitest.sh added in tools/pci/ can be used to run all the default PCI endpoint -tests. To compile this tool the following commands should be used:: +pcitest.sh added in tools/testing/selftests/pci_endpoint can be used to run all +the default PCI endpoint tests. To compile this tool the following commands +should be used::

# cd <kernel-dir> - # make -C tools/pci + # make -C tools/testing/selftests/pci_endpoint

or if you desire to compile and install in your system::

# cd <kernel-dir> - # make -C tools/pci install + # make -C tools/testing/selftests/pci_endpoint install

The tool and script will be located in <rootfs>/usr/bin/

diff --git a/MAINTAINERS b/MAINTAINERS index 1e930c7a58b1..0e611b845d50 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18003,7 +18003,7 @@ F: Documentation/PCI/endpoint/* F: Documentation/misc-devices/pci-endpoint-test.rst F: drivers/misc/pci_endpoint_test.c F: drivers/pci/endpoint/ -F: tools/pci/ +F: tools/testing/selftests/pci_endpoint/

PCI ENHANCED ERROR HANDLING (EEH) FOR POWERPC M: Mahesh J Salgaonkar mahesh@linux.ibm.com diff --git a/tools/testing/selftests/pci_endpoint/.gitignore b/tools/testing/selftests/pci_endpoint/.gitignore new file mode 100644 index 000000000000..29ab47c48484 --- /dev/null +++ b/tools/testing/selftests/pci_endpoint/.gitignore @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0-only +*.o +pcitest diff --git a/tools/pci/Build b/tools/testing/selftests/pci_endpoint/Build similarity index 100% rename from tools/pci/Build rename to tools/testing/selftests/pci_endpoint/Build diff --git a/tools/pci/Makefile b/tools/testing/selftests/pci_endpoint/Makefile similarity index 83% rename from tools/pci/Makefile rename to tools/testing/selftests/pci_endpoint/Makefile index 62d41f1a1e2c..3c6fe18e32cc 100644 --- a/tools/pci/Makefile +++ b/tools/testing/selftests/pci_endpoint/Makefile @@ -1,11 +1,10 @@ # SPDX-License-Identifier: GPL-2.0 -include ../scripts/Makefile.include +include ../../../scripts/Makefile.include

bindir ?= /usr/bin

ifeq ($(srctree),) -srctree := $(patsubst %/,%,$(dir $(CURDIR))) -srctree := $(patsubst %/,%,$(dir $(srctree))) +srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR))) endif

# Do not use make's built-in rules @@ -27,10 +26,11 @@ include $(srctree)/tools/build/Makefile.include # # We need the following to be outside of kernel tree # -$(OUTPUT)include/linux/: ../../include/uapi/linux/ +$(OUTPUT)include/linux/: ../../../../include/uapi/linux/ mkdir -p $(OUTPUT)include/linux/ 2>&1 || true - ln -sf $(CURDIR)/../../include/uapi/linux/pcitest.h $@ + ln -sf $(CURDIR)/../../../../include/uapi/linux/pcitest.h $@

+$(info ${CURDIR}) prepare: $(OUTPUT)include/linux/

PCITEST_IN := $(OUTPUT)pcitest-in.o diff --git a/tools/pci/pcitest.c b/tools/testing/selftests/pci_endpoint/pcitest.c similarity index 100% rename from tools/pci/pcitest.c rename to tools/testing/selftests/pci_endpoint/pcitest.c diff --git a/tools/pci/pcitest.sh b/tools/testing/selftests/pci_endpoint/pcitest.sh similarity index 100% rename from tools/pci/pcitest.sh rename to tools/testing/selftests/pci_endpoint/pcitest.sh

-- 2.25.1

Niklas Cassel

5:17 p.m.

New subject: [PATCH v4 2/3] selftests: Move PCI Endpoint tests from tools/pci to Kselftests

On Tue, Dec 31, 2024 at 06:43:40PM +0530, Manivannan Sadhasivam wrote:

...

This just moves the existing tests under tools/pci to tools/testing/selftests/pci_endpoint and adjusts the paths in Makefile accordingly. Migration to Kselftest framework will be done in subsequent commits.

Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org

Why not squash this patch with the subsequent patch that actually converts the tests to kselftests?

If you just apply this patch, then we have moved the code to testing/selftests/ but the tests are not actually kselftests, which doesn't really make sense IMO.

Kind regards, Niklas

...

Documentation/PCI/endpoint/pci-test-howto.rst | 9 +++++---- MAINTAINERS | 2 +- tools/testing/selftests/pci_endpoint/.gitignore | 3 +++ tools/{pci => testing/selftests/pci_endpoint}/Build | 0 tools/{pci => testing/selftests/pci_endpoint}/Makefile | 10 +++++----- .../{pci => testing/selftests/pci_endpoint}/pcitest.c | 0 .../{pci => testing/selftests/pci_endpoint}/pcitest.sh | 0 7 files changed, 14 insertions(+), 10 deletions(-) create mode 100644 tools/testing/selftests/pci_endpoint/.gitignore rename tools/{pci => testing/selftests/pci_endpoint}/Build (100%) rename tools/{pci => testing/selftests/pci_endpoint}/Makefile (83%) rename tools/{pci => testing/selftests/pci_endpoint}/pcitest.c (100%) rename tools/{pci => testing/selftests/pci_endpoint}/pcitest.sh (100%)

diff --git a/Documentation/PCI/endpoint/pci-test-howto.rst b/Documentation/PCI/endpoint/pci-test-howto.rst index 909f770a07d6..c4ae7af50ede 100644 --- a/Documentation/PCI/endpoint/pci-test-howto.rst +++ b/Documentation/PCI/endpoint/pci-test-howto.rst @@ -123,16 +123,17 @@ above:: Using Endpoint Test function Device

-pcitest.sh added in tools/pci/ can be used to run all the default PCI endpoint -tests. To compile this tool the following commands should be used:: +pcitest.sh added in tools/testing/selftests/pci_endpoint can be used to run all +the default PCI endpoint tests. To compile this tool the following commands +should be used:: # cd <kernel-dir>

# make -C tools/pci

# make -C tools/testing/selftests/pci_endpoint

or if you desire to compile and install in your system:: # cd <kernel-dir>

# make -C tools/pci install

# make -C tools/testing/selftests/pci_endpoint install

The tool and script will be located in <rootfs>/usr/bin/ diff --git a/MAINTAINERS b/MAINTAINERS index 1e930c7a58b1..0e611b845d50 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18003,7 +18003,7 @@ F: Documentation/PCI/endpoint/* F: Documentation/misc-devices/pci-endpoint-test.rst F: drivers/misc/pci_endpoint_test.c F: drivers/pci/endpoint/ -F: tools/pci/ +F: tools/testing/selftests/pci_endpoint/ PCI ENHANCED ERROR HANDLING (EEH) FOR POWERPC M: Mahesh J Salgaonkar mahesh@linux.ibm.com diff --git a/tools/testing/selftests/pci_endpoint/.gitignore b/tools/testing/selftests/pci_endpoint/.gitignore new file mode 100644 index 000000000000..29ab47c48484 --- /dev/null +++ b/tools/testing/selftests/pci_endpoint/.gitignore @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0-only +*.o +pcitest diff --git a/tools/pci/Build b/tools/testing/selftests/pci_endpoint/Build similarity index 100% rename from tools/pci/Build rename to tools/testing/selftests/pci_endpoint/Build diff --git a/tools/pci/Makefile b/tools/testing/selftests/pci_endpoint/Makefile similarity index 83% rename from tools/pci/Makefile rename to tools/testing/selftests/pci_endpoint/Makefile index 62d41f1a1e2c..3c6fe18e32cc 100644 --- a/tools/pci/Makefile +++ b/tools/testing/selftests/pci_endpoint/Makefile @@ -1,11 +1,10 @@ # SPDX-License-Identifier: GPL-2.0 -include ../scripts/Makefile.include +include ../../../scripts/Makefile.include bindir ?= /usr/bin ifeq ($(srctree),) -srctree := $(patsubst %/,%,$(dir $(CURDIR))) -srctree := $(patsubst %/,%,$(dir $(srctree))) +srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR))) endif # Do not use make's built-in rules @@ -27,10 +26,11 @@ include $(srctree)/tools/build/Makefile.include # # We need the following to be outside of kernel tree # -$(OUTPUT)include/linux/: ../../include/uapi/linux/ +$(OUTPUT)include/linux/: ../../../../include/uapi/linux/ mkdir -p $(OUTPUT)include/linux/ 2>&1 || true

ln -sf $(CURDIR)/../../include/uapi/linux/pcitest.h $@

ln -sf $(CURDIR)/../../../../include/uapi/linux/pcitest.h $@

+$(info ${CURDIR}) prepare: $(OUTPUT)include/linux/ PCITEST_IN := $(OUTPUT)pcitest-in.o diff --git a/tools/pci/pcitest.c b/tools/testing/selftests/pci_endpoint/pcitest.c similarity index 100% rename from tools/pci/pcitest.c rename to tools/testing/selftests/pci_endpoint/pcitest.c diff --git a/tools/pci/pcitest.sh b/tools/testing/selftests/pci_endpoint/pcitest.sh similarity index 100% rename from tools/pci/pcitest.sh rename to tools/testing/selftests/pci_endpoint/pcitest.sh -- 2.25.1

Manivannan Sadhasivam

6:53 p.m.

New subject: [PATCH v4 2/3] selftests: Move PCI Endpoint tests from tools/pci to Kselftests

On Tue, Dec 31, 2024 at 06:17:05PM +0100, Niklas Cassel wrote:

...

On Tue, Dec 31, 2024 at 06:43:40PM +0530, Manivannan Sadhasivam wrote:

...
This just moves the existing tests under tools/pci to tools/testing/selftests/pci_endpoint and adjusts the paths in Makefile accordingly. Migration to Kselftest framework will be done in subsequent commits.

Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org

Why not squash this patch with the subsequent patch that actually converts the tests to kselftests?

If you just apply this patch, then we have moved the code to testing/selftests/ but the tests are not actually kselftests, which doesn't really make sense IMO.

As I mentioned in the cover letter, this was suggested by both Shuah and Bjorn to preserve the history:

https://lore.kernel.org/linux-pci/b2a5db97-dc59-33ab-71cd-f591e0b1b34d@linux... https://lore.kernel.org/linux-pci/20230117195903.GA142672@bhelgaas/

- Mani

-- மணிவண்ணன் சதாசிவம்

Manivannan Sadhasivam

1:13 p.m.

New subject: [PATCH v4 3/3] selftests: pci_endpoint: Migrate to Kselftest framework

Migrate the PCI endpoint test to Kselftest framework. All the tests that were part of the previous pcitest.sh file were migrated.

Below is the exclusive list of tests:

1. BAR Tests (BAR0 to BAR5) 2. Consecutive BAR Tests 3. Legacy IRQ Tests 4. MSI Interrupt Tests (MSI1 to MSI32) 5. MSI-X Interrupt Tests (MSI-X1 to MSI-X2048) 6. Read Tests - MEMCPY (For 1, 1024, 1025, 1024000, 1024001 Bytes) 7. Write Tests - MEMCPY (For 1, 1024, 1025, 1024000, 1024001 Bytes) 8. Copy Tests - MEMCPY (For 1, 1024, 1025, 1024000, 1024001 Bytes) 9. Read Tests - DMA (For 1, 1024, 1025, 1024000, 1024001 Bytes) 10. Write Tests - DMA (For 1, 1024, 1025, 1024000, 1024001 Bytes) 11. Copy Tests - DMA (For 1, 1024, 1025, 1024000, 1024001 Bytes)

DMA and MEMCPY tests are added as fixture variants and can be executed separately as below:

$ pci_endpoint_test -V dma (excluding DMA tests) $ pci_endpoint_test -V memcpy (excluding MEMCPY tests)

Co-developed-by: Aman Gupta aman1.gupta@samsung.com Signed-off-by: Aman Gupta aman1.gupta@samsung.com Co-developed-by: Padmanabhan Rajanbabu p.rajanbabu@samsung.com Signed-off-by: Padmanabhan Rajanbabu p.rajanbabu@samsung.com [mani: reworked based on the IOCTL fix, cleanups, documentation, commit message] Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org --- Documentation/PCI/endpoint/pci-test-howto.rst | 154 ++++------ tools/testing/selftests/Makefile | 1 + .../testing/selftests/pci_endpoint/.gitignore | 3 +- tools/testing/selftests/pci_endpoint/Build | 1 - tools/testing/selftests/pci_endpoint/Makefile | 59 +--- tools/testing/selftests/pci_endpoint/config | 4 + .../pci_endpoint/pci_endpoint_test.c | 194 +++++++++++++ .../testing/selftests/pci_endpoint/pcitest.c | 265 ------------------ .../testing/selftests/pci_endpoint/pcitest.sh | 73 ----- 9 files changed, 258 insertions(+), 496 deletions(-) delete mode 100644 tools/testing/selftests/pci_endpoint/Build create mode 100644 tools/testing/selftests/pci_endpoint/config create mode 100644 tools/testing/selftests/pci_endpoint/pci_endpoint_test.c delete mode 100644 tools/testing/selftests/pci_endpoint/pcitest.c delete mode 100644 tools/testing/selftests/pci_endpoint/pcitest.sh

diff --git a/Documentation/PCI/endpoint/pci-test-howto.rst b/Documentation/PCI/endpoint/pci-test-howto.rst index c4ae7af50ede..3d664c279c80 100644 --- a/Documentation/PCI/endpoint/pci-test-howto.rst +++ b/Documentation/PCI/endpoint/pci-test-howto.rst @@ -123,9 +123,9 @@ above:: Using Endpoint Test function Device -----------------------------------

-pcitest.sh added in tools/testing/selftests/pci_endpoint can be used to run all -the default PCI endpoint tests. To compile this tool the following commands -should be used:: +Kselftest added in tools/testing/selftests/pci_endpoint can be used to run all +the default PCI endpoint tests. To build the Kselftest for PCI endpoint +subsystem, the following commands should be used::

# cd <kernel-dir> # make -C tools/testing/selftests/pci_endpoint @@ -133,104 +133,58 @@ should be used:: or if you desire to compile and install in your system::

# cd <kernel-dir> - # make -C tools/testing/selftests/pci_endpoint install + # make -C tools/testing/selftests/pci_endpoint INSTALL_PATH=/usr/bin install

-The tool and script will be located in <rootfs>/usr/bin/ +The test will be located in <rootfs>/usr/bin/

- -pcitest.sh Output -~~~~~~~~~~~~~~~~~ +Kselftest Output +~~~~~~~~~~~~~~~~ ::

- # pcitest.sh - BAR tests - - BAR0: OKAY - BAR1: OKAY - BAR2: OKAY - BAR3: OKAY - BAR4: NOT OKAY - BAR5: NOT OKAY - - Interrupt tests - - SET IRQ TYPE TO LEGACY: OKAY - LEGACY IRQ: NOT OKAY - SET IRQ TYPE TO MSI: OKAY - 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 - [...] - MSI-X2047: NOT OKAY - MSI-X2048: NOT OKAY - - Read Tests - - SET IRQ TYPE TO MSI: OKAY - READ ( 1 bytes): OKAY - READ ( 1024 bytes): OKAY - READ ( 1025 bytes): OKAY - READ (1024000 bytes): OKAY - READ (1024001 bytes): OKAY - - Write Tests - - WRITE ( 1 bytes): OKAY - WRITE ( 1024 bytes): OKAY - WRITE ( 1025 bytes): OKAY - WRITE (1024000 bytes): OKAY - WRITE (1024001 bytes): OKAY - - Copy Tests - - COPY ( 1 bytes): OKAY - COPY ( 1024 bytes): OKAY - COPY ( 1025 bytes): OKAY - COPY (1024000 bytes): OKAY - COPY (1024001 bytes): OKAY + # pci_endpoint_test + TAP version 13 + 1..11 + # Starting 11 tests from 3 test cases. + # RUN pci_ep_basic.BAR_TEST ... + # OK pci_ep_basic.BAR_TEST + ok 1 pci_ep_basic.BAR_TEST + # RUN pci_ep_basic.CONSECUTIVE_BAR_TEST ... + # OK pci_ep_basic.CONSECUTIVE_BAR_TEST + ok 2 pci_ep_basic.CONSECUTIVE_BAR_TEST + # RUN pci_ep_basic.LEGACY_IRQ_TEST ... + # OK pci_ep_basic.LEGACY_IRQ_TEST + ok 3 pci_ep_basic.LEGACY_IRQ_TEST + # RUN pci_ep_basic.MSI_TEST ... + # OK pci_ep_basic.MSI_TEST + ok 4 pci_ep_basic.MSI_TEST + # RUN pci_ep_basic.MSIX_TEST ... + # OK pci_ep_basic.MSIX_TEST + ok 5 pci_ep_basic.MSIX_TEST + # RUN pci_ep_data_transfer.memcpy.READ_TEST ... + # OK pci_ep_data_transfer.memcpy.READ_TEST + ok 6 pci_ep_data_transfer.memcpy.READ_TEST + # RUN pci_ep_data_transfer.memcpy.WRITE_TEST ... + # OK pci_ep_data_transfer.memcpy.WRITE_TEST + ok 7 pci_ep_data_transfer.memcpy.WRITE_TEST + # RUN pci_ep_data_transfer.memcpy.COPY_TEST ... + # OK pci_ep_data_transfer.memcpy.COPY_TEST + ok 8 pci_ep_data_transfer.memcpy.COPY_TEST + # RUN pci_ep_data_transfer.dma.READ_TEST ... + # OK pci_ep_data_transfer.dma.READ_TEST + ok 9 pci_ep_data_transfer.dma.READ_TEST + # RUN pci_ep_data_transfer.dma.WRITE_TEST ... + # OK pci_ep_data_transfer.dma.WRITE_TEST + ok 10 pci_ep_data_transfer.dma.WRITE_TEST + # RUN pci_ep_data_transfer.dma.COPY_TEST ... + # OK pci_ep_data_transfer.dma.COPY_TEST + ok 11 pci_ep_data_transfer.dma.COPY_TEST + # PASSED: 11 / 11 tests passed. + # Totals: pass:11 fail:0 xfail:0 xpass:0 skip:0 error:0 + + +Testcase 11 (pci_ep_data_transfer.dma.COPY_TEST) will fail for most of the DMA +capable endpoint controllers due to the absence of the MEMCPY over DMA. For such +controllers, it is advisable to skip the forementioned testcase using below +command:: + + # pci_endpoint_test -f pci_ep_basic -v memcpy -T COPY_TEST -v dma diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 2401e973c359..50931cd6aff2 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -72,6 +72,7 @@ TARGETS += net/packetdrill TARGETS += net/rds TARGETS += net/tcp_ao TARGETS += nsfs +TARGETS += pci_endpoint TARGETS += pcie_bwctrl TARGETS += perf_events TARGETS += pidfd diff --git a/tools/testing/selftests/pci_endpoint/.gitignore b/tools/testing/selftests/pci_endpoint/.gitignore index 29ab47c48484..6a4837a3e034 100644 --- a/tools/testing/selftests/pci_endpoint/.gitignore +++ b/tools/testing/selftests/pci_endpoint/.gitignore @@ -1,3 +1,2 @@ # SPDX-License-Identifier: GPL-2.0-only -*.o -pcitest +pci_endpoint_test diff --git a/tools/testing/selftests/pci_endpoint/Build b/tools/testing/selftests/pci_endpoint/Build deleted file mode 100644 index c375aea21790..000000000000 --- a/tools/testing/selftests/pci_endpoint/Build +++ /dev/null @@ -1 +0,0 @@ -pcitest-y += pcitest.o diff --git a/tools/testing/selftests/pci_endpoint/Makefile b/tools/testing/selftests/pci_endpoint/Makefile index 3c6fe18e32cc..bf21ebf20b4a 100644 --- a/tools/testing/selftests/pci_endpoint/Makefile +++ b/tools/testing/selftests/pci_endpoint/Makefile @@ -1,58 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 -include ../../../scripts/Makefile.include +CFLAGS += -O2 -Wl,-no-as-needed -Wall $(KHDR_INCLUDES) +LDFLAGS += -lrt -lpthread -lm

-bindir ?= /usr/bin +TEST_GEN_PROGS = pci_endpoint_test

-ifeq ($(srctree),) -srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR))) -endif - -# Do not use make's built-in rules -# (this improves performance and avoids hard-to-debug behaviour); -MAKEFLAGS += -r - -CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include - -ALL_TARGETS := pcitest -ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS)) - -SCRIPTS := pcitest.sh - -all: $(ALL_PROGRAMS) - -export srctree OUTPUT CC LD CFLAGS -include $(srctree)/tools/build/Makefile.include - -# -# We need the following to be outside of kernel tree -# -$(OUTPUT)include/linux/: ../../../../include/uapi/linux/ - mkdir -p $(OUTPUT)include/linux/ 2>&1 || true - ln -sf $(CURDIR)/../../../../include/uapi/linux/pcitest.h $@ - -$(info ${CURDIR}) -prepare: $(OUTPUT)include/linux/ - -PCITEST_IN := $(OUTPUT)pcitest-in.o -$(PCITEST_IN): prepare FORCE - $(Q)$(MAKE) $(build)=pcitest -$(OUTPUT)pcitest: $(PCITEST_IN) - $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@ - -clean: - rm -f $(ALL_PROGRAMS) - rm -rf $(OUTPUT)include/ - find $(or $(OUTPUT),.) -name '*.o' -delete -o -name '.*.cmd' -delete -o -name '.*.d' -delete - -install: $(ALL_PROGRAMS) - install -d -m 755 $(DESTDIR)$(bindir); \ - for program in $(ALL_PROGRAMS); do \ - install $$program $(DESTDIR)$(bindir); \ - done; \ - for script in $(SCRIPTS); do \ - install $$script $(DESTDIR)$(bindir); \ - done - -FORCE: - -.PHONY: all install clean FORCE prepare +include ../lib.mk diff --git a/tools/testing/selftests/pci_endpoint/config b/tools/testing/selftests/pci_endpoint/config new file mode 100644 index 000000000000..7cdcf117db8d --- /dev/null +++ b/tools/testing/selftests/pci_endpoint/config @@ -0,0 +1,4 @@ +CONFIG_PCI_ENDPOINT=y +CONFIG_PCI_ENDPOINT_CONFIGFS=y +CONFIG_PCI_EPF_TEST=m +CONFIG_PCI_ENDPOINT_TEST=m diff --git a/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c new file mode 100644 index 000000000000..964dfb80ff12 --- /dev/null +++ b/tools/testing/selftests/pci_endpoint/pci_endpoint_test.c @@ -0,0 +1,194 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Kselftest for PCI Endpoint Subsystem + * + * Copyright (c) 2022 Samsung Electronics Co., Ltd. + * https://www.samsung.com + * Author: Aman Gupta aman1.gupta@samsung.com + * + * Copyright (c) 2024, Linaro Ltd. + * Author: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org + */ + +#include <errno.h> +#include <fcntl.h> +#include <stdbool.h> +#include <stdio.h> +#include <stdlib.h> +#include <sys/ioctl.h> +#include <unistd.h> + +#include "../../../../include/uapi/linux/pcitest.h" + +#include "../kselftest_harness.h" + +#define pci_ep_ioctl(cmd, arg) \ +({ \ + ret = ioctl(self->fd, cmd, arg); \ + ret = ret < 0 ? -errno : 0; \ +}) + +static const char *test_device = "/dev/pci-endpoint-test.0"; +static const unsigned long test_size[5] = { 1, 1024, 1025, 1024000, 1024001 }; + +FIXTURE(pci_ep_basic) +{ + int fd; +}; + +FIXTURE_SETUP(pci_ep_basic) +{ + self->fd = open(test_device, O_RDWR); + + ASSERT_NE(-1, self->fd) TH_LOG("Can't open PCI Endpoint Test device"); +} + +FIXTURE_TEARDOWN(pci_ep_basic) +{ + close(self->fd); +} + +TEST_F(pci_ep_basic, BAR_TEST) +{ + int ret, i; + + for (i = 0; i <= 5; i++) { + pci_ep_ioctl(PCITEST_BAR, i); + EXPECT_FALSE(ret) TH_LOG("Test failed for BAR%d", i); + } +} + +TEST_F(pci_ep_basic, CONSECUTIVE_BAR_TEST) +{ + int ret; + + pci_ep_ioctl(PCITEST_BARS, 0); + EXPECT_FALSE(ret) TH_LOG("Consecutive BAR test failed"); +} + +TEST_F(pci_ep_basic, LEGACY_IRQ_TEST) +{ + int ret; + + pci_ep_ioctl(PCITEST_SET_IRQTYPE, 0); + ASSERT_EQ(0, ret) TH_LOG("Can't set Legacy IRQ type"); + + pci_ep_ioctl(PCITEST_LEGACY_IRQ, 0); + EXPECT_FALSE(ret) TH_LOG("Test failed for Legacy IRQ"); +} + +TEST_F(pci_ep_basic, MSI_TEST) +{ + int ret, i; + + pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1); + ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type"); + + for (i = 1; i <= 32; i++) { + pci_ep_ioctl(PCITEST_MSI, i); + EXPECT_FALSE(ret) TH_LOG("Test failed for MSI%d", i); + } +} + +TEST_F(pci_ep_basic, MSIX_TEST) +{ + int ret, i; + + pci_ep_ioctl(PCITEST_SET_IRQTYPE, 2); + ASSERT_EQ(0, ret) TH_LOG("Can't set MSI-X IRQ type"); + + for (i = 1; i <= 2048; i++) { + pci_ep_ioctl(PCITEST_MSIX, i); + EXPECT_FALSE(ret) TH_LOG("Test failed for MSI-X%d", i); + } +} + +FIXTURE(pci_ep_data_transfer) +{ + int fd; +}; + +FIXTURE_SETUP(pci_ep_data_transfer) +{ + self->fd = open(test_device, O_RDWR); + + ASSERT_NE(-1, self->fd) TH_LOG("Can't open PCI Endpoint Test device"); +} + +FIXTURE_TEARDOWN(pci_ep_data_transfer) +{ + close(self->fd); +} + +FIXTURE_VARIANT(pci_ep_data_transfer) +{ + bool use_dma; +}; + +FIXTURE_VARIANT_ADD(pci_ep_data_transfer, memcpy) +{ + .use_dma = false, +}; + +FIXTURE_VARIANT_ADD(pci_ep_data_transfer, dma) +{ + .use_dma = true, +}; + +TEST_F(pci_ep_data_transfer, READ_TEST) +{ + struct pci_endpoint_test_xfer_param param = {0}; + int ret, i; + + if (variant->use_dma) + param.flags = PCITEST_FLAGS_USE_DMA; + + pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1); + ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type"); + + for (i = 0; i < ARRAY_SIZE(test_size); i++) { + param.size = test_size[i]; + pci_ep_ioctl(PCITEST_READ, &param); + EXPECT_FALSE(ret) TH_LOG("Test failed for size (%ld)", + test_size[i]); + } +} + +TEST_F(pci_ep_data_transfer, WRITE_TEST) +{ + struct pci_endpoint_test_xfer_param param = {0}; + int ret, i; + + if (variant->use_dma) + param.flags = PCITEST_FLAGS_USE_DMA; + + pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1); + ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type"); + + for (i = 0; i < ARRAY_SIZE(test_size); i++) { + param.size = test_size[i]; + pci_ep_ioctl(PCITEST_WRITE, &param); + EXPECT_FALSE(ret) TH_LOG("Test failed for size (%ld)", + test_size[i]); + } +} + +TEST_F(pci_ep_data_transfer, COPY_TEST) +{ + struct pci_endpoint_test_xfer_param param = {0}; + int ret, i; + + if (variant->use_dma) + param.flags = PCITEST_FLAGS_USE_DMA; + + pci_ep_ioctl(PCITEST_SET_IRQTYPE, 1); + ASSERT_EQ(0, ret) TH_LOG("Can't set MSI IRQ type"); + + for (i = 0; i < ARRAY_SIZE(test_size); i++) { + param.size = test_size[i]; + pci_ep_ioctl(PCITEST_COPY, &param); + EXPECT_FALSE(ret) TH_LOG("Test failed for size (%ld)", + test_size[i]); + } +} +TEST_HARNESS_MAIN diff --git a/tools/testing/selftests/pci_endpoint/pcitest.c b/tools/testing/selftests/pci_endpoint/pcitest.c deleted file mode 100644 index b96cc118839b..000000000000 --- a/tools/testing/selftests/pci_endpoint/pcitest.c +++ /dev/null @@ -1,265 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/** - * Userspace PCI Endpoint Test Module - * - * Copyright (C) 2017 Texas Instruments - * Author: Kishon Vijay Abraham I kishon@ti.com - */ - -#include <errno.h> -#include <fcntl.h> -#include <stdbool.h> -#include <stdio.h> -#include <stdlib.h> -#include <sys/ioctl.h> -#include <unistd.h> - -#include <linux/pcitest.h> - -static char *irq[] = { "LEGACY", "MSI", "MSI-X" }; - -struct pci_test { - char *device; - char barnum; - bool consecutive_bar_test; - bool legacyirq; - unsigned int msinum; - unsigned int msixnum; - int irqtype; - bool set_irqtype; - bool get_irqtype; - bool clear_irq; - bool read; - bool write; - bool copy; - unsigned long size; - bool use_dma; -}; - -static int run_test(struct pci_test *test) -{ - struct pci_endpoint_test_xfer_param param = {}; - int ret = -EINVAL; - int fd; - - fd = open(test->device, O_RDWR); - if (fd < 0) { - perror("can't open PCI Endpoint Test device"); - return -ENODEV; - } - - if (test->barnum >= 0 && test->barnum <= 5) { - ret = ioctl(fd, PCITEST_BAR, test->barnum); - fprintf(stdout, "BAR%d:\t\t", test->barnum); - if (ret < 0) - fprintf(stdout, "NOT OKAY\n"); - else - fprintf(stdout, "OKAY\n"); - } - - if (test->consecutive_bar_test) { - ret = ioctl(fd, PCITEST_BARS); - fprintf(stdout, "Consecutive BAR test:\t\t"); - if (ret < 0) - fprintf(stdout, "NOT OKAY\n"); - else - fprintf(stdout, "OKAY\n"); - } - - if (test->set_irqtype) { - ret = ioctl(fd, PCITEST_SET_IRQTYPE, test->irqtype); - fprintf(stdout, "SET IRQ TYPE TO %s:\t\t", irq[test->irqtype]); - if (ret < 0) - fprintf(stdout, "NOT OKAY\n"); - else - fprintf(stdout, "OKAY\n"); - } - - if (test->get_irqtype) { - ret = ioctl(fd, PCITEST_GET_IRQTYPE); - fprintf(stdout, "GET IRQ TYPE:\t\t"); - if (ret < 0) { - fprintf(stdout, "NOT OKAY\n"); - } else { - fprintf(stdout, "%s\n", irq[ret]); - ret = 0; - } - } - - if (test->clear_irq) { - ret = ioctl(fd, PCITEST_CLEAR_IRQ); - fprintf(stdout, "CLEAR IRQ:\t\t"); - if (ret < 0) - fprintf(stdout, "NOT OKAY\n"); - else - fprintf(stdout, "OKAY\n"); - } - - if (test->legacyirq) { - ret = ioctl(fd, PCITEST_LEGACY_IRQ, 0); - fprintf(stdout, "LEGACY IRQ:\t"); - if (ret < 0) - fprintf(stdout, "NOT OKAY\n"); - else - fprintf(stdout, "OKAY\n"); - } - - if (test->msinum > 0 && test->msinum <= 32) { - ret = ioctl(fd, PCITEST_MSI, test->msinum); - fprintf(stdout, "MSI%u:\t\t", test->msinum); - if (ret < 0) - fprintf(stdout, "NOT OKAY\n"); - else - fprintf(stdout, "OKAY\n"); - } - - if (test->msixnum > 0 && test->msixnum <= 2048) { - ret = ioctl(fd, PCITEST_MSIX, test->msixnum); - fprintf(stdout, "MSI-X%u:\t\t", test->msixnum); - if (ret < 0) - fprintf(stdout, "NOT OKAY\n"); - else - fprintf(stdout, "OKAY\n"); - } - - if (test->write) { - param.size = test->size; - if (test->use_dma) - param.flags = PCITEST_FLAGS_USE_DMA; - ret = ioctl(fd, PCITEST_WRITE, &param); - fprintf(stdout, "WRITE (%7lu bytes):\t\t", test->size); - if (ret < 0) - fprintf(stdout, "NOT OKAY\n"); - else - fprintf(stdout, "OKAY\n"); - } - - if (test->read) { - param.size = test->size; - if (test->use_dma) - param.flags = PCITEST_FLAGS_USE_DMA; - ret = ioctl(fd, PCITEST_READ, &param); - fprintf(stdout, "READ (%7lu bytes):\t\t", test->size); - if (ret < 0) - fprintf(stdout, "NOT OKAY\n"); - else - fprintf(stdout, "OKAY\n"); - } - - if (test->copy) { - param.size = test->size; - if (test->use_dma) - param.flags = PCITEST_FLAGS_USE_DMA; - ret = ioctl(fd, PCITEST_COPY, &param); - fprintf(stdout, "COPY (%7lu bytes):\t\t", test->size); - if (ret < 0) - fprintf(stdout, "NOT OKAY\n"); - else - fprintf(stdout, "OKAY\n"); - } - - fflush(stdout); - close(fd); - return ret; -} - -int main(int argc, char **argv) -{ - int c; - struct pci_test *test; - - test = calloc(1, sizeof(*test)); - if (!test) { - perror("Fail to allocate memory for pci_test\n"); - return -ENOMEM; - } - - /* since '0' is a valid BAR number, initialize it to -1 */ - test->barnum = -1; - - /* set default size as 100KB */ - test->size = 0x19000; - - /* set default endpoint device */ - test->device = "/dev/pci-endpoint-test.0"; - - while ((c = getopt(argc, argv, "D:b:Cm:x:i:deIlhrwcs:")) != EOF) - switch (c) { - case 'D': - test->device = optarg; - continue; - case 'b': - test->barnum = atoi(optarg); - if (test->barnum < 0 || test->barnum > 5) - goto usage; - continue; - case 'C': - test->consecutive_bar_test = true; - continue; - case 'l': - test->legacyirq = true; - continue; - case 'm': - test->msinum = atoi(optarg); - if (test->msinum < 1 || test->msinum > 32) - goto usage; - continue; - case 'x': - test->msixnum = atoi(optarg); - if (test->msixnum < 1 || test->msixnum > 2048) - goto usage; - continue; - case 'i': - test->irqtype = atoi(optarg); - if (test->irqtype < 0 || test->irqtype > 2) - goto usage; - test->set_irqtype = true; - continue; - case 'I': - test->get_irqtype = true; - continue; - case 'r': - test->read = true; - continue; - case 'w': - test->write = true; - continue; - case 'c': - test->copy = true; - continue; - case 'e': - test->clear_irq = true; - continue; - case 's': - test->size = strtoul(optarg, NULL, 0); - continue; - case 'd': - test->use_dma = true; - continue; - case 'h': - default: -usage: - fprintf(stderr, - "usage: %s [options]\n" - "Options:\n" - "\t-D <dev> PCI endpoint test device {default: /dev/pci-endpoint-test.0}\n" - "\t-b <bar num> BAR test (bar number between 0..5)\n" - "\t-C Consecutive BAR test\n" - "\t-m <msi num> MSI test (msi number between 1..32)\n" - "\t-x <msix num> \tMSI-X test (msix number between 1..2048)\n" - "\t-i <irq type> \tSet IRQ type (0 - Legacy, 1 - MSI, 2 - MSI-X)\n" - "\t-e Clear IRQ\n" - "\t-I Get current IRQ type configured\n" - "\t-d Use DMA\n" - "\t-l Legacy IRQ test\n" - "\t-r Read buffer test\n" - "\t-w Write buffer test\n" - "\t-c Copy buffer test\n" - "\t-s <size> Size of buffer {default: 100KB}\n" - "\t-h Print this help message\n", - argv[0]); - return -EINVAL; - } - - return run_test(test); -} diff --git a/tools/testing/selftests/pci_endpoint/pcitest.sh b/tools/testing/selftests/pci_endpoint/pcitest.sh deleted file mode 100644 index 770f4d6df34b..000000000000 --- a/tools/testing/selftests/pci_endpoint/pcitest.sh +++ /dev/null @@ -1,73 +0,0 @@ -#!/bin/sh -# SPDX-License-Identifier: GPL-2.0 - -echo "BAR tests" -echo - -bar=0 - -while [ $bar -lt 6 ] -do - pcitest -b $bar - bar=`expr $bar + 1` -done -pcitest -C -echo - -echo "Interrupt tests" -echo - -pcitest -i 0 -pcitest -l - -pcitest -i 1 -msi=1 - -while [ $msi -lt 33 ] -do - pcitest -m $msi - msi=`expr $msi + 1` -done -echo - -pcitest -i 2 -msix=1 - -while [ $msix -lt 2049 ] -do - pcitest -x $msix - msix=`expr $msix + 1` -done -echo - -echo "Read Tests" -echo - -pcitest -i 1 - -pcitest -r -s 1 -pcitest -r -s 1024 -pcitest -r -s 1025 -pcitest -r -s 1024000 -pcitest -r -s 1024001 -echo - -echo "Write Tests" -echo - -pcitest -w -s 1 -pcitest -w -s 1024 -pcitest -w -s 1025 -pcitest -w -s 1024000 -pcitest -w -s 1024001 -echo - -echo "Copy Tests" -echo - -pcitest -c -s 1 -pcitest -c -s 1024 -pcitest -c -s 1025 -pcitest -c -s 1024000 -pcitest -c -s 1024001 -echo

-- 2.25.1

Niklas Cassel

5:42 p.m.

New subject: [PATCH v4 3/3] selftests: pci_endpoint: Migrate to Kselftest framework

On Tue, Dec 31, 2024 at 06:43:41PM +0530, Manivannan Sadhasivam wrote:

(...)

...

# RUN pci_ep_data_transfer.dma.COPY_TEST ...

# OK pci_ep_data_transfer.dma.COPY_TEST

ok 11 pci_ep_data_transfer.dma.COPY_TEST

# PASSED: 11 / 11 tests passed.

# Totals: pass:11 fail:0 xfail:0 xpass:0 skip:0 error:0

+Testcase 11 (pci_ep_data_transfer.dma.COPY_TEST) will fail for most of the DMA +capable endpoint controllers due to the absence of the MEMCPY over DMA. For such +controllers, it is advisable to skip the forementioned testcase using below +command::

Hm.. this is strictly not correct. If will currently fail because pci-epf-test.c does: if ((reg->flags & FLAG_USE_DMA) && epf_test->dma_private) return -EINVAL;

So even if a DMA driver has support for the DMA_MEMCPY cap, if the DMA driver also has the DMA_PRIVATE cap, this test will fail because of the code in pci-epf-test.c.

Not sure how to formulate this properly... Perhaps: Testcase 11 (pci_ep_data_transfer.dma.COPY_TEST) will fail for DMA drivers that have the DMA_PRIVATE cap set. For DMA drivers which have the DMA_PRIVATE cap set, it is advisable to skip the forementioned testcase using below command::

...

# pci_endpoint_test -f pci_ep_basic -v memcpy -T COPY_TEST -v dma

Is this really correct? I would guess that it should be pci_endpoint_test -f pci_ep_data_transfer -v memcpy -T COPY_TEST -v dma

(...)

...

+TEST_F(pci_ep_basic, BAR_TEST) +{
int ret, i;

for (i = 0; i <= 5; i++) {
pci_ep_ioctl(PCITEST_BAR, i);
EXPECT_FALSE(ret) TH_LOG("Test failed for BAR%d", i);
}
+}

From looking at this function, will we still be able to test a single BAR? Previous pcitest.c allowed us to do pcitest -b <barno> to only test a specific BAR. I think that is a useful feature that we shouldn't remove.

It would be nice if we could do something like: # pci_endpoint_test -f pci_ep_basic -T BAR_TEST -v <barno>

(...)

...

+TEST_F(pci_ep_data_transfer, COPY_TEST) +{

struct pci_endpoint_test_xfer_param param = {0};

This (also other places in this file) can be written as: struct pci_endpoint_test_xfer_param param = {};

Kind regards, Niklas

Manivannan Sadhasivam

7:18 p.m.

New subject: [PATCH v4 3/3] selftests: pci_endpoint: Migrate to Kselftest framework

On Tue, Dec 31, 2024 at 06:42:42PM +0100, Niklas Cassel wrote:

...

On Tue, Dec 31, 2024 at 06:43:41PM +0530, Manivannan Sadhasivam wrote:

(...)

...

# RUN pci_ep_data_transfer.dma.COPY_TEST ...

# OK pci_ep_data_transfer.dma.COPY_TEST

ok 11 pci_ep_data_transfer.dma.COPY_TEST

# PASSED: 11 / 11 tests passed.

# Totals: pass:11 fail:0 xfail:0 xpass:0 skip:0 error:0

+Testcase 11 (pci_ep_data_transfer.dma.COPY_TEST) will fail for most of the DMA +capable endpoint controllers due to the absence of the MEMCPY over DMA. For such +controllers, it is advisable to skip the forementioned testcase using below +command::

Hm.. this is strictly not correct. If will currently fail because pci-epf-test.c does: if ((reg->flags & FLAG_USE_DMA) && epf_test->dma_private) return -EINVAL;

So even if a DMA driver has support for the DMA_MEMCPY cap, if the DMA driver also has the DMA_PRIVATE cap, this test will fail because of the code in pci-epf-test.c.

Right. But I think the condition should be changed to test for the MEMCPY capability instead. Like,

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c index ef6677f34116..0b211d60a85b 100644 --- a/drivers/pci/endpoint/functions/pci-epf-test.c +++ b/drivers/pci/endpoint/functions/pci-epf-test.c @@ -328,7 +328,7 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test, void *copy_buf = NULL, *buf;

if (reg->flags & FLAG_USE_DMA) { - if (epf_test->dma_private) { + if (!dma_has_cap(DMA_MEMCPY, epf_test->dma_chan_tx->device->cap_mask)) { dev_err(dev, "Cannot transfer data using DMA\n"); ret = -EINVAL; goto set_status;

...

Not sure how to formulate this properly... Perhaps: Testcase 11 (pci_ep_data_transfer.dma.COPY_TEST) will fail for DMA drivers that have the DMA_PRIVATE cap set. For DMA drivers which have the DMA_PRIVATE cap set, it is advisable to skip the forementioned testcase using below command::

...

# pci_endpoint_test -f pci_ep_basic -v memcpy -T COPY_TEST -v dma

Is this really correct? I would guess that it should be pci_endpoint_test -f pci_ep_data_transfer -v memcpy -T COPY_TEST -v dma

ah, typo. Thanks for catching!

...

(...)

...
+TEST_F(pci_ep_basic, BAR_TEST) +{
int ret, i;

for (i = 0; i <= 5; i++) {
pci_ep_ioctl(PCITEST_BAR, i);
EXPECT_FALSE(ret) TH_LOG("Test failed for BAR%d", i);
}
+}
From looking at this function, will we still be able to test a single BAR? Previous pcitest.c allowed us to do pcitest -b <barno> to only test a specific BAR. I think that is a useful feature that we shouldn't remove.

It would be nice if we could do something like: # pci_endpoint_test -f pci_ep_basic -T BAR_TEST -v <barno>

I'll try to add it.

...

(...)

...

+TEST_F(pci_ep_data_transfer, COPY_TEST) +{

struct pci_endpoint_test_xfer_param param = {0};

This (also other places in this file) can be written as: struct pci_endpoint_test_xfer_param param = {};

Ack.

- Mani

-- மணிவண்ணன் சதாசிவம்

Niklas Cassel

7:33 p.m.

New subject: [PATCH v4 3/3] selftests: pci_endpoint: Migrate to Kselftest framework

On 31 December 2024 20:18:12 CET, Manivannan Sadhasivam manivannan.sadhasivam@linaro.org wrote:

...

On Tue, Dec 31, 2024 at 06:42:42PM +0100, Niklas Cassel wrote:

...
On Tue, Dec 31, 2024 at 06:43:41PM +0530, Manivannan Sadhasivam wrote:

(...)

...

# RUN pci_ep_data_transfer.dma.COPY_TEST ...

# OK pci_ep_data_transfer.dma.COPY_TEST

ok 11 pci_ep_data_transfer.dma.COPY_TEST

# PASSED: 11 / 11 tests passed.

# Totals: pass:11 fail:0 xfail:0 xpass:0 skip:0 error:0

+Testcase 11 (pci_ep_data_transfer.dma.COPY_TEST) will fail for most of the DMA +capable endpoint controllers due to the absence of the MEMCPY over DMA. For such +controllers, it is advisable to skip the forementioned testcase using below +command::

Hm.. this is strictly not correct. If will currently fail because pci-epf-test.c does: if ((reg->flags & FLAG_USE_DMA) && epf_test->dma_private) return -EINVAL;

So even if a DMA driver has support for the DMA_MEMCPY cap, if the DMA driver also has the DMA_PRIVATE cap, this test will fail because of the code in pci-epf-test.c.

Right. But I think the condition should be changed to test for the MEMCPY capability instead. Like,

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c index ef6677f34116..0b211d60a85b 100644 --- a/drivers/pci/endpoint/functions/pci-epf-test.c +++ b/drivers/pci/endpoint/functions/pci-epf-test.c @@ -328,7 +328,7 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test, void *copy_buf = NULL, *buf;
   if (reg->flags & FLAG_USE_DMA) {
          if (epf_test->dma_private) {
          if (!dma_has_cap(DMA_MEMCPY, epf_test->dma_chan_tx->device->cap_mask)) {
                 dev_err(dev, "Cannot transfer data using DMA\n");
                 ret = -EINVAL;
                 goto set_status;

That check does seem to make more sense than the code that is currently there. (Perhaps send this as a proper patch?) Note that I'm not an expert at dmaengine.

I have some patches that adds DMA_MEMCPY to dw-edma, but I'm not sure if the DWC eDMA hardware supports having both src and dst as PCI addresses, or if only one of them can be a PCI address (with the other one being a local address).

If only one of them can be a PCI address, then I'm not sure if your suggested patch is correct.

Kind regards, Niklas

Manivannan Sadhasivam

2 Jan 2 Jan

7:04 a.m.

New subject: [PATCH v4 3/3] selftests: pci_endpoint: Migrate to Kselftest framework

On Tue, Dec 31, 2024 at 08:33:57PM +0100, Niklas Cassel wrote:

...

On 31 December 2024 20:18:12 CET, Manivannan Sadhasivam manivannan.sadhasivam@linaro.org wrote:

...
On Tue, Dec 31, 2024 at 06:42:42PM +0100, Niklas Cassel wrote:

...
On Tue, Dec 31, 2024 at 06:43:41PM +0530, Manivannan Sadhasivam wrote:

(...)

...

# RUN pci_ep_data_transfer.dma.COPY_TEST ...

# OK pci_ep_data_transfer.dma.COPY_TEST

ok 11 pci_ep_data_transfer.dma.COPY_TEST

# PASSED: 11 / 11 tests passed.

# Totals: pass:11 fail:0 xfail:0 xpass:0 skip:0 error:0

+Testcase 11 (pci_ep_data_transfer.dma.COPY_TEST) will fail for most of the DMA +capable endpoint controllers due to the absence of the MEMCPY over DMA. For such +controllers, it is advisable to skip the forementioned testcase using below +command::

Hm.. this is strictly not correct. If will currently fail because pci-epf-test.c does: if ((reg->flags & FLAG_USE_DMA) && epf_test->dma_private) return -EINVAL;

So even if a DMA driver has support for the DMA_MEMCPY cap, if the DMA driver also has the DMA_PRIVATE cap, this test will fail because of the code in pci-epf-test.c.

Right. But I think the condition should be changed to test for the MEMCPY capability instead. Like,

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c index ef6677f34116..0b211d60a85b 100644 --- a/drivers/pci/endpoint/functions/pci-epf-test.c +++ b/drivers/pci/endpoint/functions/pci-epf-test.c @@ -328,7 +328,7 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test, void *copy_buf = NULL, *buf;
   if (reg->flags & FLAG_USE_DMA) {
          if (epf_test->dma_private) {
          if (!dma_has_cap(DMA_MEMCPY, epf_test->dma_chan_tx->device->cap_mask)) {
                 dev_err(dev, "Cannot transfer data using DMA\n");
                 ret = -EINVAL;
                 goto set_status;
That check does seem to make more sense than the code that is currently there. (Perhaps send this as a proper patch?)

Will do.

...

Note that I'm not an expert at dmaengine.

I have some patches that adds DMA_MEMCPY to dw-edma, but I'm not sure if the DWC eDMA hardware supports having both src and dst as PCI addresses, or if only one of them can be a PCI address (with the other one being a local address).

If only one of them can be a PCI address, then I'm not sure if your suggested patch is correct.

I don't see why that would be an issue. DMA_MEMCPY is independent of PCI/local addresses. If a dmaengine driver support doing MEMCPY, then the dma cap should be sufficient. As you said, if a controller supports both SLAVE and MEMCPY, the test currently errors out, which is wrong.

- Mani

-- மணிவண்ணன் சதாசிவம்

Niklas Cassel

2:23 p.m.

New subject: [PATCH v4 3/3] selftests: pci_endpoint: Migrate to Kselftest framework

Hello Mani, Vinod,

On Thu, Jan 02, 2025 at 12:34:04PM +0530, Manivannan Sadhasivam wrote:

...

On Tue, Dec 31, 2024 at 08:33:57PM +0100, Niklas Cassel wrote:

...
I have some patches that adds DMA_MEMCPY to dw-edma, but I'm not sure if the DWC eDMA hardware supports having both src and dst as PCI addresses, or if only one of them can be a PCI address (with the other one being a local address).

If only one of them can be a PCI address, then I'm not sure if your suggested patch is correct.

I don't see why that would be an issue. DMA_MEMCPY is independent of PCI/local addresses. If a dmaengine driver support doing MEMCPY, then the dma cap should be sufficient. As you said, if a controller supports both SLAVE and MEMCPY, the test currently errors out, which is wrong.

While I am okay with your suggested change to pci-epf-test.c:

...

```
          if (epf_test->dma_private) {
```

          if (!dma_has_cap(DMA_MEMCPY, epf_test->dma_chan_tx->device->cap_mask)) {

Since this will ensure that a DMA driver implementing DMA_MEMCPY, which cannot be shared (has DMA_PRIVATE set), will not error out.

What I'm trying to explain is that in: https://lore.kernel.org/linux-pci/Z2BW4CjdE1p50AhC@vaman/ https://lore.kernel.org/linux-pci/20241217090129.6dodrgi4tn7l3cod@thinkpad/

Vinod (any you) suggested that we should add support for prep_memcpy() (which implies also setting cap DMA_MEMCPY) in the dw-edma DMA driver.

However, from section "6.3 Using the DMA" in the DWC databook, the DWC eDMA hardware only supports: - Transfer (copy) of a block of data from local memory to remote memory. - Transfer (copy) of a block of data from remote memory to local memory.

Currently, we have: https://github.com/torvalds/linux/blob/v6.13-rc5/include/linux/dmaengine.h#L... https://github.com/torvalds/linux/blob/v6.13-rc5/drivers/dma/dw-edma/dw-edma...

Where we can expose per-channel capabilities, so we set MEM_TO_DEV/DEV_TO_MEM per channel, however, these are returned in a struct dma_slave_caps *caps, so this is AFAICT only for DMA_SLAVE, not for DMA_MEMCPY.

Looking at: https://github.com/torvalds/linux/blob/v6.13-rc5/include/linux/dmaengine.h#L... it seems that DMA_MEMCPY is always assumed to be MEM_TO_MEM.

To me, it seems that we would either need a new dma_transaction_type (e.g. DMA_COPY) where we can set dir: MEM_TO_DEV, DEV_TO_MEM, or DEV_TO_DEV. (dw-edma would not support DEV_TO_DEV.)

Or, if we should stick with DMA_MEMCPY, we would need another way of telling client drivers that only src or dst can be a remote address.

Until this is solved, I think I will stop my work on adding DMA_MEMCPY to the dw-edma driver.

Kind regards, Niklas

Manivannan Sadhasivam

16 Jan 16 Jan

4:47 a.m.

New subject: [PATCH v4 3/3] selftests: pci_endpoint: Migrate to Kselftest framework

On Thu, Jan 02, 2025 at 03:23:14PM +0100, Niklas Cassel wrote:

...

Hello Mani, Vinod,

On Thu, Jan 02, 2025 at 12:34:04PM +0530, Manivannan Sadhasivam wrote:

...
On Tue, Dec 31, 2024 at 08:33:57PM +0100, Niklas Cassel wrote:

...
I have some patches that adds DMA_MEMCPY to dw-edma, but I'm not sure if the DWC eDMA hardware supports having both src and dst as PCI addresses, or if only one of them can be a PCI address (with the other one being a local address).

If only one of them can be a PCI address, then I'm not sure if your suggested patch is correct.

I don't see why that would be an issue. DMA_MEMCPY is independent of PCI/local addresses. If a dmaengine driver support doing MEMCPY, then the dma cap should be sufficient. As you said, if a controller supports both SLAVE and MEMCPY, the test currently errors out, which is wrong.

While I am okay with your suggested change to pci-epf-test.c:

...
...
          if (epf_test->dma_private) {
          if (!dma_has_cap(DMA_MEMCPY, epf_test->dma_chan_tx->device->cap_mask)) {
Since this will ensure that a DMA driver implementing DMA_MEMCPY, which cannot be shared (has DMA_PRIVATE set), will not error out.

What I'm trying to explain is that in: https://lore.kernel.org/linux-pci/Z2BW4CjdE1p50AhC@vaman/ https://lore.kernel.org/linux-pci/20241217090129.6dodrgi4tn7l3cod@thinkpad/

Vinod (any you) suggested that we should add support for prep_memcpy() (which implies also setting cap DMA_MEMCPY) in the dw-edma DMA driver.

However, from section "6.3 Using the DMA" in the DWC databook, the DWC eDMA hardware only supports:

Transfer (copy) of a block of data from local memory to remote memory.

Transfer (copy) of a block of data from remote memory to local memory.

Currently, we have: https://github.com/torvalds/linux/blob/v6.13-rc5/include/linux/dmaengine.h#L... https://github.com/torvalds/linux/blob/v6.13-rc5/drivers/dma/dw-edma/dw-edma...

Where we can expose per-channel capabilities, so we set MEM_TO_DEV/DEV_TO_MEM per channel, however, these are returned in a struct dma_slave_caps *caps, so this is AFAICT only for DMA_SLAVE, not for DMA_MEMCPY.

Looking at: https://github.com/torvalds/linux/blob/v6.13-rc5/include/linux/dmaengine.h#L... it seems that DMA_MEMCPY is always assumed to be MEM_TO_MEM.

To me, it seems that we would either need a new dma_transaction_type (e.g. DMA_COPY) where we can set dir: MEM_TO_DEV, DEV_TO_MEM, or DEV_TO_DEV. (dw-edma would not support DEV_TO_DEV.)

Or, if we should stick with DMA_MEMCPY, we would need another way of telling client drivers that only src or dst can be a remote address.

Until this is solved, I think I will stop my work on adding DMA_MEMCPY to the dw-edma driver.

I think your concern is regarding setting the DMA transfer direction for MEMCPY, right? And you are saying that even if we use tx/rx channels, currently we cannot set DEV_TO_DEV like directions?

But I'm somewhat confused about what is blocking you from adding MEMCPY support to the dw-edma driver since that driver cannot support DEV_TO_DEV. In your WIP driver, you were setting the direction based on the channel. Isn't that sufficient enough?

- Mani

-- மணிவண்ணன் சதாசிவம்

Niklas Cassel

10:30 a.m.

New subject: [PATCH v4 3/3] selftests: pci_endpoint: Migrate to Kselftest framework

On Thu, Jan 16, 2025 at 10:17:25AM +0530, Manivannan Sadhasivam wrote:

...

On Thu, Jan 02, 2025 at 03:23:14PM +0100, Niklas Cassel wrote:

...
Hello Mani, Vinod,

On Thu, Jan 02, 2025 at 12:34:04PM +0530, Manivannan Sadhasivam wrote:

...
On Tue, Dec 31, 2024 at 08:33:57PM +0100, Niklas Cassel wrote:

...
I have some patches that adds DMA_MEMCPY to dw-edma, but I'm not sure if the DWC eDMA hardware supports having both src and dst as PCI addresses, or if only one of them can be a PCI address (with the other one being a local address).

If only one of them can be a PCI address, then I'm not sure if your suggested patch is correct.

I don't see why that would be an issue. DMA_MEMCPY is independent of PCI/local addresses. If a dmaengine driver support doing MEMCPY, then the dma cap should be sufficient. As you said, if a controller supports both SLAVE and MEMCPY, the test currently errors out, which is wrong.

While I am okay with your suggested change to pci-epf-test.c:

...
...
          if (epf_test->dma_private) {
          if (!dma_has_cap(DMA_MEMCPY, epf_test->dma_chan_tx->device->cap_mask)) {
Since this will ensure that a DMA driver implementing DMA_MEMCPY, which cannot be shared (has DMA_PRIVATE set), will not error out.

What I'm trying to explain is that in: https://lore.kernel.org/linux-pci/Z2BW4CjdE1p50AhC@vaman/ https://lore.kernel.org/linux-pci/20241217090129.6dodrgi4tn7l3cod@thinkpad/

Vinod (any you) suggested that we should add support for prep_memcpy() (which implies also setting cap DMA_MEMCPY) in the dw-edma DMA driver.

However, from section "6.3 Using the DMA" in the DWC databook, the DWC eDMA hardware only supports:

Transfer (copy) of a block of data from local memory to remote memory.

Transfer (copy) of a block of data from remote memory to local memory.

Currently, we have: https://github.com/torvalds/linux/blob/v6.13-rc5/include/linux/dmaengine.h#L... https://github.com/torvalds/linux/blob/v6.13-rc5/drivers/dma/dw-edma/dw-edma...

Where we can expose per-channel capabilities, so we set MEM_TO_DEV/DEV_TO_MEM per channel, however, these are returned in a struct dma_slave_caps *caps, so this is AFAICT only for DMA_SLAVE, not for DMA_MEMCPY.

Looking at: https://github.com/torvalds/linux/blob/v6.13-rc5/include/linux/dmaengine.h#L... it seems that DMA_MEMCPY is always assumed to be MEM_TO_MEM.

To me, it seems that we would either need a new dma_transaction_type (e.g. DMA_COPY) where we can set dir: MEM_TO_DEV, DEV_TO_MEM, or DEV_TO_DEV. (dw-edma would not support DEV_TO_DEV.)

Or, if we should stick with DMA_MEMCPY, we would need another way of telling client drivers that only src or dst can be a remote address.

Until this is solved, I think I will stop my work on adding DMA_MEMCPY to the dw-edma driver.
I think your concern is regarding setting the DMA transfer direction for MEMCPY, right? And you are saying that even if we use tx/rx channels, currently we cannot set DEV_TO_DEV like directions?

But I'm somewhat confused about what is blocking you from adding MEMCPY support to the dw-edma driver since that driver cannot support DEV_TO_DEV. In your WIP driver, you were setting the direction based on the channel. Isn't that sufficient enough?

What I did in the WIP driver patches was to set the direction to either DEV_TO_MEM, or MEM_TO_DEV.

But that is wrong, since the prep_memcpy() API doesn't take a direction.

In fact, it appears that memcpy is always assumed to be MEM_TO_MEM: https://github.com/torvalds/linux/blob/v6.13-rc7/include/linux/dmaengine.h#L...

E.g. the dw-edma driver cannot have both src address and dst address as a local address (MEM_TO_MEM), so using DMA_MEMCPY API feels totally wrong.

Either dst or src has to be a local address (MEM), and the one that isn't a local address has to be a PCI address (DEV). Sure, calling a PCI address DEV might not be 100% correct, but I cannot think of a better way...

We also cannot treat a PCI address as MEM, as dw-edma cannot do PCI to PCI transfers.

I think the best way forward would be to create a new _prep_slave_memcpy() or similar, that does take a direction, and thus does not require dmaengine_slave_config() to be called before every _prep_slave_memcpy() call, since that is basically what is not allowing us to have multiple transactions outstanding in parallel.

Kind regards, Niklas

days inactive

112

days old

linux-kselftest-mirror@lists.linaro.org

14 comments

participants

tags (0)

participants (2)

Manivannan Sadhasivam
Niklas Cassel