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

[Manivannan Sadhasivam]

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@xxxxxxxxxxxxxxxxxxx>
> > Closes: https://lore.kernel.org/all/YvzNg5ROnxEApDgS@xxxxxxxxx
> > Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device")
> > Reviewed-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxx>

Thanks!

- Mani

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