[PATCH v3 2/4] 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 | 236 +++++++++++++++---------------- tools/pci/pcitest.c | 47 +++--- 2 files changed, 140 insertions(+), 143 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 3aaaf47fa4ee..752fa5fe52d7 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -166,43 +166,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) @@ -217,22 +221,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) { @@ -252,7 +256,7 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test) break; }

- return false; + return ret; }

static const u32 bar_test_pattern[] = { @@ -277,7 +281,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; @@ -286,7 +290,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);

@@ -301,28 +305,28 @@ 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 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;

@@ -334,12 +338,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; @@ -354,9 +358,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, @@ -375,11 +382,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; @@ -398,17 +404,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;

@@ -418,22 +424,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; }

@@ -457,15 +462,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; }

@@ -498,8 +503,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); @@ -510,16 +515,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; @@ -534,17 +536,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;

@@ -554,23 +556,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; }

@@ -603,24 +604,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; @@ -634,17 +632,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 = 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;

@@ -654,21 +652,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; }

@@ -700,50 +697,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; - - if (!pci_endpoint_test_request_irq(test)) - goto err; + ret = pci_endpoint_test_alloc_irq_vectors(test, req_irq_type); + if (ret) + return ret;

- 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, @@ -808,7 +806,7 @@ static const struct file_operations pci_endpoint_test_fops = { 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; @@ -847,24 +845,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) { @@ -879,7 +876,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; @@ -889,7 +886,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; } @@ -897,27 +894,26 @@ 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; - }

misc_device = &test->miscdev; 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; } @@ -949,7 +945,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 470258009ddc..545e04ad63a2 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 { @@ -52,63 +51,65 @@ 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->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%d:\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%d:\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) { @@ -118,9 +119,9 @@ static int run_test(struct pci_test *test) ret = ioctl(fd, PCITEST_WRITE, &param); fprintf(stdout, "WRITE (%7ld 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) { @@ -130,9 +131,9 @@ static int run_test(struct pci_test *test) ret = ioctl(fd, PCITEST_READ, &param); fprintf(stdout, "READ (%7ld 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) { @@ -142,14 +143,14 @@ static int run_test(struct pci_test *test) ret = ioctl(fd, PCITEST_COPY, &param); fprintf(stdout, "COPY (%7ld 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)