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

Manivannan Sadhasivam
2024-12-31T18:51:29Z

On Tue, Dec 31, 2024 at 05:57:47PM +0100, Niklas Cassel wrote:
> 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.
>

Ack.

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

Ack.

>
> Otherwise, this looks good to me:
> Reviewed-by: Niklas Cassel <cassel@kernel.org>

Thanks!

- Mani

--
மணிவண்ணன் சதாசிவம்