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 <[email protected]>
> Closes: https://lore.kernel.org/all/[email protected]
> Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function
> device")
> Reviewed-by: Damien Le Moal <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> 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 <[email protected]>
