| Message ID | 20220819145018.35732-3-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pci_endpoint_test: Fix the return value of IOCTLs | expand |
On Fri, Aug 19, 2022 at 08:20:15PM +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! > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> This needs to come first in the series, along with a Fixes: tag, and a cc: stable tag so that it can be properly backported and fixed up everywhere. thanks, greg k-h
On Fri, Aug 19, 2022 at 08:20:15PM +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! > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/misc/pci_endpoint_test.c | 163 ++++++++++++++----------------- > 1 file changed, 76 insertions(+), 87 deletions(-) > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > index db0458039d7d..bbf903c5a5bd 100644 > --- a/drivers/misc/pci_endpoint_test.c > +++ b/drivers/misc/pci_endpoint_test.c > @@ -174,13 +174,12 @@ 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 = -EINVAL; > struct pci_dev *pdev = test->pdev; > struct device *dev = &pdev->dev; > - bool res = true; > > switch (type) { > case IRQ_TYPE_LEGACY: > @@ -202,15 +201,16 @@ static bool pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, > dev_err(dev, "Invalid IRQ type selected\n"); > } > > + test->irq_type = type; > + > if (irq < 0) { > - irq = 0; > - res = false; > + test->num_irqs = 0; > + return irq; Why are you setting the type if there is an error? > } > > - test->irq_type = type; > test->num_irqs = irq; > > - return res; > + return 0; > } > > static void pci_endpoint_test_release_irq(struct pci_endpoint_test *test) > @@ -225,7 +225,7 @@ 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; > @@ -240,7 +240,7 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test) > goto fail; > } > > - return true; > + return 0; > > fail: > switch (irq_type) { > @@ -260,10 +260,10 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test) > break; > } > > - return false; > + return err; > } > > -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; > @@ -272,7 +272,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; How is this no memory? Shouldn't this not even get here if the allocation failed? > > size = pci_resource_len(pdev, barno); > > @@ -285,13 +285,13 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, > for (j = 0; j < size; j += 4) { > val = pci_endpoint_test_bar_readl(test, barno, j); > if (val != 0xA0A0A0A0) > - return false; > + return -EINVAL; Is this really an invalid value sent to the ioctl? > } > > - return true; > + return 0; > } > > -static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test) > +static int pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test) > { > u32 val; > > @@ -303,12 +303,12 @@ static bool pci_endpoint_test_legacy_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; > @@ -324,19 +324,18 @@ 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; > > - if (pci_irq_vector(pdev, msi_num - 1) == test->last_irq) > - return true; > + if (pci_irq_vector(pdev, msi_num - 1) != test->last_irq) > + return -EINVAL; Again, is this an invalid value passed to the ioctl? Same for other places you are doing something and then returning this error value, are you sure that is correct? -EINVAL should be "the values you sent me was incorrect", not "something bad happened based on what you gave me". thanks, greg k-h
On Fri, Aug 19, 2022 at 05:25:01PM +0200, Greg KH wrote: > On Fri, Aug 19, 2022 at 08:20:15PM +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! > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/misc/pci_endpoint_test.c | 163 ++++++++++++++----------------- > > 1 file changed, 76 insertions(+), 87 deletions(-) > > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > > index db0458039d7d..bbf903c5a5bd 100644 > > --- a/drivers/misc/pci_endpoint_test.c > > +++ b/drivers/misc/pci_endpoint_test.c > > @@ -174,13 +174,12 @@ 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 = -EINVAL; > > struct pci_dev *pdev = test->pdev; > > struct device *dev = &pdev->dev; > > - bool res = true; > > > > switch (type) { > > case IRQ_TYPE_LEGACY: > > @@ -202,15 +201,16 @@ static bool pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, > > dev_err(dev, "Invalid IRQ type selected\n"); > > } > > > > + test->irq_type = type; > > + > > if (irq < 0) { > > - irq = 0; > > - res = false; > > + test->num_irqs = 0; > > + return irq; > > Why are you setting the type if there is an error? > This was the original behaviour, so I kept it as it is. If it needs to be changed, then it should be done in a separate patch I believe. > > > } > > > > - test->irq_type = type; > > test->num_irqs = irq; > > > > - return res; > > + return 0; > > } > > > > static void pci_endpoint_test_release_irq(struct pci_endpoint_test *test) > > @@ -225,7 +225,7 @@ 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; > > @@ -240,7 +240,7 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test) > > goto fail; > > } > > > > - return true; > > + return 0; > > > > fail: > > switch (irq_type) { > > @@ -260,10 +260,10 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test) > > break; > > } > > > > - return false; > > + return err; > > } > > > > -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; > > @@ -272,7 +272,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; > > How is this no memory? > No bar means a failure in pci_ioremap_bar() during probe. And that implies a failure while mapping the device's BAR in host memory. So -ENOMEM seems to be the apt error no. > Shouldn't this not even get here if the allocation failed? > No, the driver tries to map PCI_STD_NUM_BARS which is 6 and if some of them are not available except BAR_0 then it just logs an error and continues. So it is not fatal. > > > > size = pci_resource_len(pdev, barno); > > > > @@ -285,13 +285,13 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, > > for (j = 0; j < size; j += 4) { > > val = pci_endpoint_test_bar_readl(test, barno, j); > > if (val != 0xA0A0A0A0) > > - return false; > > + return -EINVAL; > > Is this really an invalid value sent to the ioctl? > > > > } > > > > - return true; > > + return 0; > > } > > > > -static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test) > > +static int pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test) > > { > > u32 val; > > > > @@ -303,12 +303,12 @@ static bool pci_endpoint_test_legacy_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; > > @@ -324,19 +324,18 @@ 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; > > > > - if (pci_irq_vector(pdev, msi_num - 1) == test->last_irq) > > - return true; > > + if (pci_irq_vector(pdev, msi_num - 1) != test->last_irq) > > + return -EINVAL; > > Again, is this an invalid value passed to the ioctl? > > Same for other places you are doing something and then returning this > error value, are you sure that is correct? > > -EINVAL should be "the values you sent me was incorrect", not "something > bad happened based on what you gave me". > Okay. Will revisit all of them. Thanks, Mani
On Fri, Aug 19, 2022 at 05:20:40PM +0200, Greg KH wrote: > On Fri, Aug 19, 2022 at 08:20:15PM +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! > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > This needs to come first in the series, Yeah, I thought about that but then I had one more patch touching this driver and if this comes first then that patch should be at 4/5. I was not sure if that's fine since two patches touching the same driver would be at different positions in a series. Anyway, since you said, I'll just move this to 1/5 and other one to 4/5. > along with a Fixes: tag, and a > cc: stable tag so that it can be properly backported and fixed up > everywhere. > Okay. Thanks, Mani > thanks, > > greg k-h
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index db0458039d7d..bbf903c5a5bd 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -174,13 +174,12 @@ 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 = -EINVAL; struct pci_dev *pdev = test->pdev; struct device *dev = &pdev->dev; - bool res = true; switch (type) { case IRQ_TYPE_LEGACY: @@ -202,15 +201,16 @@ static bool pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, dev_err(dev, "Invalid IRQ type selected\n"); } + test->irq_type = type; + if (irq < 0) { - irq = 0; - res = false; + test->num_irqs = 0; + return irq; } - test->irq_type = type; test->num_irqs = irq; - return res; + return 0; } static void pci_endpoint_test_release_irq(struct pci_endpoint_test *test) @@ -225,7 +225,7 @@ 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; @@ -240,7 +240,7 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test) goto fail; } - return true; + return 0; fail: switch (irq_type) { @@ -260,10 +260,10 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test) break; } - return false; + return err; } -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; @@ -272,7 +272,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; size = pci_resource_len(pdev, barno); @@ -285,13 +285,13 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, for (j = 0; j < size; j += 4) { val = pci_endpoint_test_bar_readl(test, barno, j); if (val != 0xA0A0A0A0) - return false; + return -EINVAL; } - return true; + return 0; } -static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test) +static int pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test) { u32 val; @@ -303,12 +303,12 @@ static bool pci_endpoint_test_legacy_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; @@ -324,19 +324,18 @@ 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; - if (pci_irq_vector(pdev, msi_num - 1) == test->last_irq) - return true; + if (pci_irq_vector(pdev, msi_num - 1) != test->last_irq) + return -EINVAL; - return false; + 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; @@ -360,12 +359,12 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test, err = copy_from_user(¶m, (void __user *)arg, sizeof(param)); if (err) { dev_err(dev, "Failed to get transfer param\n"); - return false; + return -EFAULT; } size = param.size; if (size > SIZE_MAX - alignment) - goto err; + return -EINVAL; use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA); if (use_dma) @@ -373,22 +372,21 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test, if (irq_type < IRQ_TYPE_LEGACY || 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)) { + err = dma_mapping_error(dev, orig_src_phys_addr); + if (err) { dev_err(dev, "failed to map source buffer address\n"); - ret = false; goto err_src_phys_addr; } @@ -412,15 +410,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; + err = -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)) { + err = dma_mapping_error(dev, orig_dst_phys_addr); + if (err) { dev_err(dev, "failed to map destination buffer address\n"); - ret = false; goto err_dst_phys_addr; } @@ -453,8 +451,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) + err = -EINVAL; err_dst_phys_addr: kfree(orig_dst_addr); @@ -465,16 +463,13 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test, err_src_phys_addr: kfree(orig_src_addr); - -err: - return ret; + return err; } -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; @@ -492,14 +487,14 @@ static bool pci_endpoint_test_write(struct pci_endpoint_test *test, int err; err = copy_from_user(¶m, (void __user *)arg, sizeof(param)); - if (err != 0) { + if (err) { dev_err(dev, "Failed to get transfer param\n"); - return false; + return -EFAULT; } size = param.size; if (size > SIZE_MAX - alignment) - goto err; + return -EINVAL; use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA); if (use_dma) @@ -507,23 +502,22 @@ static bool pci_endpoint_test_write(struct pci_endpoint_test *test, if (irq_type < IRQ_TYPE_LEGACY || 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)) { + err = dma_mapping_error(dev, orig_phys_addr); + if (err) { dev_err(dev, "failed to map source buffer address\n"); - ret = false; goto err_phys_addr; } @@ -556,24 +550,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)) + err = -EINVAL; dma_unmap_single(dev, orig_phys_addr, size + alignment, DMA_TO_DEVICE); err_phys_addr: kfree(orig_addr); - -err: - return ret; + return err; } -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; @@ -592,12 +583,12 @@ static bool pci_endpoint_test_read(struct pci_endpoint_test *test, err = copy_from_user(¶m, (void __user *)arg, sizeof(param)); if (err) { dev_err(dev, "Failed to get transfer param\n"); - return false; + return -EFAULT; } size = param.size; if (size > SIZE_MAX - alignment) - goto err; + return -EINVAL; use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA); if (use_dma) @@ -605,21 +596,20 @@ static bool pci_endpoint_test_read(struct pci_endpoint_test *test, if (irq_type < IRQ_TYPE_LEGACY || 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)) { + err = dma_mapping_error(dev, orig_phys_addr); + if (err) { dev_err(dev, "failed to map source buffer address\n"); - ret = false; goto err_phys_addr; } @@ -651,50 +641,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)) + err = -EINVAL; err_phys_addr: kfree(orig_addr); -err: - return ret; + return err; } -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 err; if (req_irq_type < IRQ_TYPE_LEGACY || 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; + err = pci_endpoint_test_alloc_irq_vectors(test, req_irq_type); + if (err) + return err; - return true; + err = pci_endpoint_test_request_irq(test); + if (err) { + pci_endpoint_test_free_irq_vectors(test); + return err; + } -err: - pci_endpoint_test_free_irq_vectors(test); - return false; + return 0; } static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, @@ -812,10 +803,9 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, pci_set_master(pdev); - if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) { - err = -EINVAL; + err = pci_endpoint_test_alloc_irq_vectors(test, irq_type); + if (err) goto err_disable_irq; - } for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) { if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) { @@ -850,10 +840,9 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, goto err_ida_remove; } - if (!pci_endpoint_test_request_irq(test)) { - err = -EINVAL; + err = pci_endpoint_test_request_irq(test); + if (err) goto err_kfree_test_name; - } misc_device = &test->miscdev; misc_device->minor = MISC_DYNAMIC_MINOR;
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! Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/misc/pci_endpoint_test.c | 163 ++++++++++++++----------------- 1 file changed, 76 insertions(+), 87 deletions(-)