Re: [PATCH 2/5] misc: pci_endpoint_test: Fix the return value of IOCTL

[Greg KH]

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