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

[Niklas Cassel <cassel@kernel.org>]

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>