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@xxxxxxxxxx>
> ---
> 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
