[PATCH 0/5] pci_endpoint_test: Fix the return value of IOCTLs

[PATCH 0/5] pci_endpoint_test: Fix the return value of IOCTLs

[Manivannan Sadhasivam]

During the review of a patch for pci_endpoint_test driver [1], Greg spotted
the wrong usage of the return value of IOCTLs in the driver. This series
fixes that by returning 0 for success and negative error code for failure.
Relevant change is also made to the userspace tool and the Documentation.

Along with those, there are couple more patches fixing other small issues
I noted.

NOTE: I have just compile tested this series. So it'd be good if someone
can test it on the PCI endpoint setup.

Thanks,
Mani

[1] https://lore.kernel.org/all/20220816100617.90720-1-mie@igel.co.jp/

Manivannan Sadhasivam (5):
  misc: pci_endpoint_test: Remove unnecessary WARN_ON
  misc: pci_endpoint_test: Fix the return value of IOCTL
  tools: PCI: Fix parsing the return value of IOCTLs
  tools: PCI: Fix memory leak
  Documentation: PCI: endpoint: Use the correct return value of
    pcitest.sh

 Documentation/PCI/endpoint/pci-test-howto.rst | 152 ++++++++--------
 drivers/misc/pci_endpoint_test.c              | 167 ++++++++----------
 tools/pci/pcitest.c                           |  48 ++---
 3 files changed, 179 insertions(+), 188 deletions(-)

-- 
2.25.1

[PATCH 1/5] misc: pci_endpoint_test: Remove unnecessary WARN_ON

[Manivannan Sadhasivam]

If unable to map test_reg_bar, then probe will fail with a dedicated
error message. So there is no real need of WARN_ON here.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/misc/pci_endpoint_test.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 8f786a225dcf..db0458039d7d 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -820,10 +820,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
 		if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
 			base = pci_ioremap_bar(pdev, bar);
-			if (!base) {
+			if (!base)
 				dev_err(dev, "Failed to read BAR%d\n", bar);
-				WARN_ON(bar == test_reg_bar);
-			}
 			test->bar[bar] = base;
 		}
 	}
-- 
2.25.1

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

[Manivannan Sadhasivam]

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@linaro.org>
---
 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;
 	}
 
-	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;
 
 	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;
 	}
 
-	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;
 
-	return false;
+	return 0;
 }
 
-static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
+static int pci_endpoint_test_copy(struct pci_endpoint_test *test,
 				   unsigned long arg)
 {
 	struct pci_endpoint_test_xfer_param param;
-	bool ret = false;
 	void *src_addr;
 	void *dst_addr;
 	u32 flags = 0;
@@ -360,12 +359,12 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
 	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
 	if (err) {
 		dev_err(dev, "Failed to get transfer param\n");
-		return false;
+		return -EFAULT;
 	}
 
 	size = param.size;
 	if (size > SIZE_MAX - alignment)
-		goto err;
+		return -EINVAL;
 
 	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
 	if (use_dma)
@@ -373,22 +372,21 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
 
 	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
 		dev_err(dev, "Invalid IRQ type option\n");
-		goto err;
+		return -EINVAL;
 	}
 
 	orig_src_addr = kzalloc(size + alignment, GFP_KERNEL);
 	if (!orig_src_addr) {
 		dev_err(dev, "Failed to allocate source buffer\n");
-		ret = false;
-		goto err;
+		return -ENOMEM;
 	}
 
 	get_random_bytes(orig_src_addr, size + alignment);
 	orig_src_phys_addr = dma_map_single(dev, orig_src_addr,
 					    size + alignment, DMA_TO_DEVICE);
-	if (dma_mapping_error(dev, orig_src_phys_addr)) {
+	err = dma_mapping_error(dev, orig_src_phys_addr);
+	if (err) {
 		dev_err(dev, "failed to map source buffer address\n");
-		ret = false;
 		goto err_src_phys_addr;
 	}
 
@@ -412,15 +410,15 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
 	orig_dst_addr = kzalloc(size + alignment, GFP_KERNEL);
 	if (!orig_dst_addr) {
 		dev_err(dev, "Failed to allocate destination address\n");
-		ret = false;
+		err = -ENOMEM;
 		goto err_dst_addr;
 	}
 
 	orig_dst_phys_addr = dma_map_single(dev, orig_dst_addr,
 					    size + alignment, DMA_FROM_DEVICE);
-	if (dma_mapping_error(dev, orig_dst_phys_addr)) {
+	err = dma_mapping_error(dev, orig_dst_phys_addr);
+	if (err) {
 		dev_err(dev, "failed to map destination buffer address\n");
-		ret = false;
 		goto err_dst_phys_addr;
 	}
 
@@ -453,8 +451,8 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
 			 DMA_FROM_DEVICE);
 
 	dst_crc32 = crc32_le(~0, dst_addr, size);
-	if (dst_crc32 == src_crc32)
-		ret = true;
+	if (dst_crc32 != src_crc32)
+		err = -EINVAL;
 
 err_dst_phys_addr:
 	kfree(orig_dst_addr);
@@ -465,16 +463,13 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
 
 err_src_phys_addr:
 	kfree(orig_src_addr);
-
-err:
-	return ret;
+	return err;
 }
 
-static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
+static int pci_endpoint_test_write(struct pci_endpoint_test *test,
 				    unsigned long arg)
 {
 	struct pci_endpoint_test_xfer_param param;
-	bool ret = false;
 	u32 flags = 0;
 	bool use_dma;
 	u32 reg;
@@ -492,14 +487,14 @@ static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
 	int err;
 
 	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
-	if (err != 0) {
+	if (err) {
 		dev_err(dev, "Failed to get transfer param\n");
-		return false;
+		return -EFAULT;
 	}
 
 	size = param.size;
 	if (size > SIZE_MAX - alignment)
-		goto err;
+		return -EINVAL;
 
 	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
 	if (use_dma)
@@ -507,23 +502,22 @@ static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
 
 	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
 		dev_err(dev, "Invalid IRQ type option\n");
-		goto err;
+		return -EINVAL;
 	}
 
 	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
 	if (!orig_addr) {
 		dev_err(dev, "Failed to allocate address\n");
-		ret = false;
-		goto err;
+		return -ENOMEM;
 	}
 
 	get_random_bytes(orig_addr, size + alignment);
 
 	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
 					DMA_TO_DEVICE);
-	if (dma_mapping_error(dev, orig_phys_addr)) {
+	err = dma_mapping_error(dev, orig_phys_addr);
+	if (err) {
 		dev_err(dev, "failed to map source buffer address\n");
-		ret = false;
 		goto err_phys_addr;
 	}
 
@@ -556,24 +550,21 @@ static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
 	wait_for_completion(&test->irq_raised);
 
 	reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
-	if (reg & STATUS_READ_SUCCESS)
-		ret = true;
+	if (!(reg & STATUS_READ_SUCCESS))
+		err = -EINVAL;
 
 	dma_unmap_single(dev, orig_phys_addr, size + alignment,
 			 DMA_TO_DEVICE);
 
 err_phys_addr:
 	kfree(orig_addr);
-
-err:
-	return ret;
+	return err;
 }
 
-static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
+static int pci_endpoint_test_read(struct pci_endpoint_test *test,
 				   unsigned long arg)
 {
 	struct pci_endpoint_test_xfer_param param;
-	bool ret = false;
 	u32 flags = 0;
 	bool use_dma;
 	size_t size;
@@ -592,12 +583,12 @@ static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
 	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
 	if (err) {
 		dev_err(dev, "Failed to get transfer param\n");
-		return false;
+		return -EFAULT;
 	}
 
 	size = param.size;
 	if (size > SIZE_MAX - alignment)
-		goto err;
+		return -EINVAL;
 
 	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
 	if (use_dma)
@@ -605,21 +596,20 @@ static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
 
 	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
 		dev_err(dev, "Invalid IRQ type option\n");
-		goto err;
+		return -EINVAL;
 	}
 
 	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
 	if (!orig_addr) {
 		dev_err(dev, "Failed to allocate destination address\n");
-		ret = false;
-		goto err;
+		return -ENOMEM;
 	}
 
 	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
 					DMA_FROM_DEVICE);
-	if (dma_mapping_error(dev, orig_phys_addr)) {
+	err = dma_mapping_error(dev, orig_phys_addr);
+	if (err) {
 		dev_err(dev, "failed to map source buffer address\n");
-		ret = false;
 		goto err_phys_addr;
 	}
 
@@ -651,50 +641,51 @@ static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
 			 DMA_FROM_DEVICE);
 
 	crc32 = crc32_le(~0, addr, size);
-	if (crc32 == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM))
-		ret = true;
+	if (crc32 != pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM))
+		err = -EINVAL;
 
 err_phys_addr:
 	kfree(orig_addr);
-err:
-	return ret;
+	return err;
 }
 
-static bool pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
+static int pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
 {
 	pci_endpoint_test_release_irq(test);
 	pci_endpoint_test_free_irq_vectors(test);
-	return true;
+
+	return 0;
 }
 
-static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
+static int pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
 				      int req_irq_type)
 {
 	struct pci_dev *pdev = test->pdev;
 	struct device *dev = &pdev->dev;
+	int err;
 
 	if (req_irq_type < IRQ_TYPE_LEGACY || req_irq_type > IRQ_TYPE_MSIX) {
 		dev_err(dev, "Invalid IRQ type option\n");
-		return false;
+		return -EINVAL;
 	}
 
 	if (test->irq_type == req_irq_type)
-		return true;
+		return 0;
 
 	pci_endpoint_test_release_irq(test);
 	pci_endpoint_test_free_irq_vectors(test);
 
-	if (!pci_endpoint_test_alloc_irq_vectors(test, req_irq_type))
-		goto err;
-
-	if (!pci_endpoint_test_request_irq(test))
-		goto err;
+	err = pci_endpoint_test_alloc_irq_vectors(test, req_irq_type);
+	if (err)
+		return err;
 
-	return true;
+	err = pci_endpoint_test_request_irq(test);
+	if (err) {
+		pci_endpoint_test_free_irq_vectors(test);
+		return err;
+	}
 
-err:
-	pci_endpoint_test_free_irq_vectors(test);
-	return false;
+	return 0;
 }
 
 static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
@@ -812,10 +803,9 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 
 	pci_set_master(pdev);
 
-	if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) {
-		err = -EINVAL;
+	err = pci_endpoint_test_alloc_irq_vectors(test, irq_type);
+	if (err)
 		goto err_disable_irq;
-	}
 
 	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
 		if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
@@ -850,10 +840,9 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 		goto err_ida_remove;
 	}
 
-	if (!pci_endpoint_test_request_irq(test)) {
-		err = -EINVAL;
+	err = pci_endpoint_test_request_irq(test);
+	if (err)
 		goto err_kfree_test_name;
-	}
 
 	misc_device = &test->miscdev;
 	misc_device->minor = MISC_DYNAMIC_MINOR;
-- 
2.25.1

[PATCH 3/5] tools: PCI: Fix parsing the return value of IOCTLs

[Manivannan Sadhasivam]

"pci_endpoint_test" driver now returns 0 for success and negative error
code for failure. So adapt to the change by reporting FAILURE if the
return value is < 0, and SUCCESS otherwise.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 tools/pci/pcitest.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/tools/pci/pcitest.c b/tools/pci/pcitest.c
index 441b54234635..a4e5b17cc3b5 100644
--- a/tools/pci/pcitest.c
+++ b/tools/pci/pcitest.c
@@ -18,7 +18,6 @@
 
 #define BILLION 1E9
 
-static char *result[] = { "NOT OKAY", "OKAY" };
 static char *irq[] = { "LEGACY", "MSI", "MSI-X" };
 
 struct pci_test {
@@ -54,9 +53,9 @@ static int run_test(struct pci_test *test)
 		ret = ioctl(fd, PCITEST_BAR, test->barnum);
 		fprintf(stdout, "BAR%d:\t\t", test->barnum);
 		if (ret < 0)
-			fprintf(stdout, "TEST FAILED\n");
+			fprintf(stdout, "FAILED\n");
 		else
-			fprintf(stdout, "%s\n", result[ret]);
+			fprintf(stdout, "SUCCESS\n");
 	}
 
 	if (test->set_irqtype) {
@@ -65,16 +64,18 @@ static int run_test(struct pci_test *test)
 		if (ret < 0)
 			fprintf(stdout, "FAILED\n");
 		else
-			fprintf(stdout, "%s\n", result[ret]);
+			fprintf(stdout, "SUCCESS\n");
 	}
 
 	if (test->get_irqtype) {
 		ret = ioctl(fd, PCITEST_GET_IRQTYPE);
 		fprintf(stdout, "GET IRQ TYPE:\t\t");
-		if (ret < 0)
+		if (ret < 0) {
 			fprintf(stdout, "FAILED\n");
-		else
+		} else {
 			fprintf(stdout, "%s\n", irq[ret]);
+			ret = 0;
+		}
 	}
 
 	if (test->clear_irq) {
@@ -83,34 +84,34 @@ static int run_test(struct pci_test *test)
 		if (ret < 0)
 			fprintf(stdout, "FAILED\n");
 		else
-			fprintf(stdout, "%s\n", result[ret]);
+			fprintf(stdout, "SUCCESS\n");
 	}
 
 	if (test->legacyirq) {
 		ret = ioctl(fd, PCITEST_LEGACY_IRQ, 0);
 		fprintf(stdout, "LEGACY IRQ:\t");
 		if (ret < 0)
-			fprintf(stdout, "TEST FAILED\n");
+			fprintf(stdout, "FAILED\n");
 		else
-			fprintf(stdout, "%s\n", result[ret]);
+			fprintf(stdout, "SUCCESS\n");
 	}
 
 	if (test->msinum > 0 && test->msinum <= 32) {
 		ret = ioctl(fd, PCITEST_MSI, test->msinum);
 		fprintf(stdout, "MSI%d:\t\t", test->msinum);
 		if (ret < 0)
-			fprintf(stdout, "TEST FAILED\n");
+			fprintf(stdout, "FAILED\n");
 		else
-			fprintf(stdout, "%s\n", result[ret]);
+			fprintf(stdout, "SUCCESS\n");
 	}
 
 	if (test->msixnum > 0 && test->msixnum <= 2048) {
 		ret = ioctl(fd, PCITEST_MSIX, test->msixnum);
 		fprintf(stdout, "MSI-X%d:\t\t", test->msixnum);
 		if (ret < 0)
-			fprintf(stdout, "TEST FAILED\n");
+			fprintf(stdout, "FAILED\n");
 		else
-			fprintf(stdout, "%s\n", result[ret]);
+			fprintf(stdout, "SUCCESS\n");
 	}
 
 	if (test->write) {
@@ -120,9 +121,9 @@ static int run_test(struct pci_test *test)
 		ret = ioctl(fd, PCITEST_WRITE, &param);
 		fprintf(stdout, "WRITE (%7ld bytes):\t\t", test->size);
 		if (ret < 0)
-			fprintf(stdout, "TEST FAILED\n");
+			fprintf(stdout, "FAILED\n");
 		else
-			fprintf(stdout, "%s\n", result[ret]);
+			fprintf(stdout, "SUCCESS\n");
 	}
 
 	if (test->read) {
@@ -132,9 +133,9 @@ static int run_test(struct pci_test *test)
 		ret = ioctl(fd, PCITEST_READ, &param);
 		fprintf(stdout, "READ (%7ld bytes):\t\t", test->size);
 		if (ret < 0)
-			fprintf(stdout, "TEST FAILED\n");
+			fprintf(stdout, "FAILED\n");
 		else
-			fprintf(stdout, "%s\n", result[ret]);
+			fprintf(stdout, "SUCCESS\n");
 	}
 
 	if (test->copy) {
@@ -144,14 +145,14 @@ static int run_test(struct pci_test *test)
 		ret = ioctl(fd, PCITEST_COPY, &param);
 		fprintf(stdout, "COPY (%7ld bytes):\t\t", test->size);
 		if (ret < 0)
-			fprintf(stdout, "TEST FAILED\n");
+			fprintf(stdout, "FAILED\n");
 		else
-			fprintf(stdout, "%s\n", result[ret]);
+			fprintf(stdout, "SUCCESS\n");
 	}
 
 	fflush(stdout);
 	close(fd);
-	return (ret < 0) ? ret : 1 - ret; /* return 0 if test succeeded */
+	return ret;
 }
 
 int main(int argc, char **argv)
-- 
2.25.1

[PATCH 4/5] tools: PCI: Fix memory leak

[Manivannan Sadhasivam]

Memory allocated for "test" needs to be freed at the end of the main().

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 tools/pci/pcitest.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/pci/pcitest.c b/tools/pci/pcitest.c
index a4e5b17cc3b5..a416a66802f3 100644
--- a/tools/pci/pcitest.c
+++ b/tools/pci/pcitest.c
@@ -157,7 +157,7 @@ static int run_test(struct pci_test *test)
 
 int main(int argc, char **argv)
 {
-	int c;
+	int c, ret;
 	struct pci_test *test;
 
 	test = calloc(1, sizeof(*test));
@@ -249,5 +249,8 @@ int main(int argc, char **argv)
 		return -EINVAL;
 	}
 
-	return run_test(test);
+	ret = run_test(test);
+	free(test);
+
+	return ret;
 }
-- 
2.25.1

[PATCH 5/5] Documentation: PCI: endpoint: Use the correct return value of pcitest.sh

[Manivannan Sadhasivam]

pcitest.sh now reports SUCCESS and FAILED instead of OKAY and NOT_OKAY.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 Documentation/PCI/endpoint/pci-test-howto.rst | 152 +++++++++---------
 1 file changed, 76 insertions(+), 76 deletions(-)

diff --git a/Documentation/PCI/endpoint/pci-test-howto.rst b/Documentation/PCI/endpoint/pci-test-howto.rst
index 909f770a07d6..3bc43b9f9856 100644
--- a/Documentation/PCI/endpoint/pci-test-howto.rst
+++ b/Documentation/PCI/endpoint/pci-test-howto.rst
@@ -144,92 +144,92 @@ pcitest.sh Output
 	# pcitest.sh
 	BAR tests
 
-	BAR0:           OKAY
-	BAR1:           OKAY
-	BAR2:           OKAY
-	BAR3:           OKAY
-	BAR4:           NOT OKAY
-	BAR5:           NOT OKAY
+	BAR0:           SUCCESS
+	BAR1:           SUCCESS
+	BAR2:           SUCCESS
+	BAR3:           SUCCESS
+	BAR4:           FAILED
+	BAR5:           FAILED
 
 	Interrupt tests
 
-	SET IRQ TYPE TO LEGACY:         OKAY
-	LEGACY IRQ:     NOT OKAY
-	SET IRQ TYPE TO MSI:            OKAY
-	MSI1:           OKAY
-	MSI2:           OKAY
-	MSI3:           OKAY
-	MSI4:           OKAY
-	MSI5:           OKAY
-	MSI6:           OKAY
-	MSI7:           OKAY
-	MSI8:           OKAY
-	MSI9:           OKAY
-	MSI10:          OKAY
-	MSI11:          OKAY
-	MSI12:          OKAY
-	MSI13:          OKAY
-	MSI14:          OKAY
-	MSI15:          OKAY
-	MSI16:          OKAY
-	MSI17:          NOT OKAY
-	MSI18:          NOT OKAY
-	MSI19:          NOT OKAY
-	MSI20:          NOT OKAY
-	MSI21:          NOT OKAY
-	MSI22:          NOT OKAY
-	MSI23:          NOT OKAY
-	MSI24:          NOT OKAY
-	MSI25:          NOT OKAY
-	MSI26:          NOT OKAY
-	MSI27:          NOT OKAY
-	MSI28:          NOT OKAY
-	MSI29:          NOT OKAY
-	MSI30:          NOT OKAY
-	MSI31:          NOT OKAY
-	MSI32:          NOT OKAY
-	SET IRQ TYPE TO MSI-X:          OKAY
-	MSI-X1:         OKAY
-	MSI-X2:         OKAY
-	MSI-X3:         OKAY
-	MSI-X4:         OKAY
-	MSI-X5:         OKAY
-	MSI-X6:         OKAY
-	MSI-X7:         OKAY
-	MSI-X8:         OKAY
-	MSI-X9:         NOT OKAY
-	MSI-X10:        NOT OKAY
-	MSI-X11:        NOT OKAY
-	MSI-X12:        NOT OKAY
-	MSI-X13:        NOT OKAY
-	MSI-X14:        NOT OKAY
-	MSI-X15:        NOT OKAY
-	MSI-X16:        NOT OKAY
+	SET IRQ TYPE TO LEGACY:         SUCCESS
+	LEGACY IRQ:     FAILED
+	SET IRQ TYPE TO MSI:            SUCCESS
+	MSI1:           SUCCESS
+	MSI2:           SUCCESS
+	MSI3:           SUCCESS
+	MSI4:           SUCCESS
+	MSI5:           SUCCESS
+	MSI6:           SUCCESS
+	MSI7:           SUCCESS
+	MSI8:           SUCCESS
+	MSI9:           SUCCESS
+	MSI10:          SUCCESS
+	MSI11:          SUCCESS
+	MSI12:          SUCCESS
+	MSI13:          SUCCESS
+	MSI14:          SUCCESS
+	MSI15:          SUCCESS
+	MSI16:          SUCCESS
+	MSI17:          FAILED
+	MSI18:          FAILED
+	MSI19:          FAILED
+	MSI20:          FAILED
+	MSI21:          FAILED
+	MSI22:          FAILED
+	MSI23:          FAILED
+	MSI24:          FAILED
+	MSI25:          FAILED
+	MSI26:          FAILED
+	MSI27:          FAILED
+	MSI28:          FAILED
+	MSI29:          FAILED
+	MSI30:          FAILED
+	MSI31:          FAILED
+	MSI32:          FAILED
+	SET IRQ TYPE TO MSI-X:          SUCCESS
+	MSI-X1:         SUCCESS
+	MSI-X2:         SUCCESS
+	MSI-X3:         SUCCESS
+	MSI-X4:         SUCCESS
+	MSI-X5:         SUCCESS
+	MSI-X6:         SUCCESS
+	MSI-X7:         SUCCESS
+	MSI-X8:         SUCCESS
+	MSI-X9:         FAILED
+	MSI-X10:        FAILED
+	MSI-X11:        FAILED
+	MSI-X12:        FAILED
+	MSI-X13:        FAILED
+	MSI-X14:        FAILED
+	MSI-X15:        FAILED
+	MSI-X16:        FAILED
 	[...]
-	MSI-X2047:      NOT OKAY
-	MSI-X2048:      NOT OKAY
+	MSI-X2047:      FAILED
+	MSI-X2048:      FAILED
 
 	Read Tests
 
-	SET IRQ TYPE TO MSI:            OKAY
-	READ (      1 bytes):           OKAY
-	READ (   1024 bytes):           OKAY
-	READ (   1025 bytes):           OKAY
-	READ (1024000 bytes):           OKAY
-	READ (1024001 bytes):           OKAY
+	SET IRQ TYPE TO MSI:            SUCCESS
+	READ (      1 bytes):           SUCCESS
+	READ (   1024 bytes):           SUCCESS
+	READ (   1025 bytes):           SUCCESS
+	READ (1024000 bytes):           SUCCESS
+	READ (1024001 bytes):           SUCCESS
 
 	Write Tests
 
-	WRITE (      1 bytes):          OKAY
-	WRITE (   1024 bytes):          OKAY
-	WRITE (   1025 bytes):          OKAY
-	WRITE (1024000 bytes):          OKAY
-	WRITE (1024001 bytes):          OKAY
+	WRITE (      1 bytes):          SUCCESS
+	WRITE (   1024 bytes):          SUCCESS
+	WRITE (   1025 bytes):          SUCCESS
+	WRITE (1024000 bytes):          SUCCESS
+	WRITE (1024001 bytes):          SUCCESS
 
 	Copy Tests
 
-	COPY (      1 bytes):           OKAY
-	COPY (   1024 bytes):           OKAY
-	COPY (   1025 bytes):           OKAY
-	COPY (1024000 bytes):           OKAY
-	COPY (1024001 bytes):           OKAY
+	COPY (      1 bytes):           SUCCESS
+	COPY (   1024 bytes):           SUCCESS
+	COPY (   1025 bytes):           SUCCESS
+	COPY (1024000 bytes):           SUCCESS
+	COPY (1024001 bytes):           SUCCESS
-- 
2.25.1

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@linaro.org>

This needs to come first in the series, along with a Fixes: tag, and a
 cc: stable tag so that it can be properly backported and fixed up
everywhere.

thanks,

greg k-h

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@linaro.org>
> ---
>  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

Re: [PATCH 1/5] misc: pci_endpoint_test: Remove unnecessary WARN_ON

[Greg KH]

On Fri, Aug 19, 2022 at 08:20:14PM +0530, Manivannan Sadhasivam wrote:
> If unable to map test_reg_bar, then probe will fail with a dedicated
> error message. So there is no real need of WARN_ON here.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/misc/pci_endpoint_test.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Should this go to stable kernels?

thanks,

greg k-h

Re: [PATCH 3/5] tools: PCI: Fix parsing the return value of IOCTLs

[Greg KH]

On Fri, Aug 19, 2022 at 08:20:16PM +0530, Manivannan Sadhasivam wrote:
> "pci_endpoint_test" driver now returns 0 for success and negative error
> code for failure. So adapt to the change by reporting FAILURE if the
> return value is < 0, and SUCCESS otherwise.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Fixes: tag and cc: stable?

thanks,

greg k-h

Re: [PATCH 1/5] misc: pci_endpoint_test: Remove unnecessary WARN_ON

[Manivannan Sadhasivam]

On Fri, Aug 19, 2022 at 05:27:02PM +0200, Greg KH wrote:
> On Fri, Aug 19, 2022 at 08:20:14PM +0530, Manivannan Sadhasivam wrote:
> > If unable to map test_reg_bar, then probe will fail with a dedicated
> > error message. So there is no real need of WARN_ON here.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/misc/pci_endpoint_test.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> Should this go to stable kernels?
> 

This is not a bug fix but a removal of unnecessary stacktrace, so I'm not sure
if this needs to be backported.

Thanks,
Mani

> thanks,
> 
> greg k-h

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

Re: [PATCH 3/5] tools: PCI: Fix parsing the return value of IOCTLs

[Manivannan Sadhasivam]

On Fri, Aug 19, 2022 at 05:27:24PM +0200, Greg KH wrote:
> On Fri, Aug 19, 2022 at 08:20:16PM +0530, Manivannan Sadhasivam wrote:
> > "pci_endpoint_test" driver now returns 0 for success and negative error
> > code for failure. So adapt to the change by reporting FAILURE if the
> > return value is < 0, and SUCCESS otherwise.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Fixes: tag and cc: stable?
> 

Okay, then the same needs to be done for pci_endpoint_test and Documentation.

Thanks,
Mani

> thanks,
> 
> greg k-h

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

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

[Manivannan Sadhasivam]

On Fri, Aug 19, 2022 at 05:25:01PM +0200, Greg KH wrote:
> 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@linaro.org>
> > ---
> >  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?
> 

This was the original behaviour, so I kept it as it is. If it needs to be
changed, then it should be done in a separate patch I believe.

> 
> >  	}
> >  
> > -	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?
>

No bar means a failure in pci_ioremap_bar() during probe. And that implies a
failure while mapping the device's BAR in host memory. So -ENOMEM seems to be
the apt error no.
 
> Shouldn't this not even get here if the allocation failed?
> 

No, the driver tries to map PCI_STD_NUM_BARS which is 6 and if some of them are
not available except BAR_0 then it just logs an error and continues. So it is
not fatal.

> >  
> >  	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".
> 

Okay. Will revisit all of them.

Thanks,
Mani

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

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

[Manivannan Sadhasivam]

On Fri, Aug 19, 2022 at 05:20:40PM +0200, Greg KH wrote:
> 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@linaro.org>
> 
> This needs to come first in the series,

Yeah, I thought about that but then I had one more patch touching this driver
and if this comes first then that patch should be at 4/5. I was not sure if
that's fine since two patches touching the same driver would be at different
positions in a series.

Anyway, since you said, I'll just move this to 1/5 and other one to 4/5.

> along with a Fixes: tag, and a
>  cc: stable tag so that it can be properly backported and fixed up
> everywhere.
> 

Okay.

Thanks,
Mani

> thanks,
> 
> greg k-h

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

Re: [PATCH 3/5] tools: PCI: Fix parsing the return value of IOCTLs

[Shunsuke Mie]

2022年8月19日(金) 23:50 Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>:
>
> "pci_endpoint_test" driver now returns 0 for success and negative error
> code for failure. So adapt to the change by reporting FAILURE if the
> return value is < 0, and SUCCESS otherwise.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  tools/pci/pcitest.c | 41 +++++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/tools/pci/pcitest.c b/tools/pci/pcitest.c
> index 441b54234635..a4e5b17cc3b5 100644
> --- a/tools/pci/pcitest.c
> +++ b/tools/pci/pcitest.c
> @@ -18,7 +18,6 @@
>
>  #define BILLION 1E9
>
> -static char *result[] = { "NOT OKAY", "OKAY" };
>  static char *irq[] = { "LEGACY", "MSI", "MSI-X" };
>
>  struct pci_test {
> @@ -54,9 +53,9 @@ static int run_test(struct pci_test *test)
>                 ret = ioctl(fd, PCITEST_BAR, test->barnum);
>                 fprintf(stdout, "BAR%d:\t\t", test->barnum);
>                 if (ret < 0)
> -                       fprintf(stdout, "TEST FAILED\n");
> +                       fprintf(stdout, "FAILED\n");
>                 else
> -                       fprintf(stdout, "%s\n", result[ret]);
> +                       fprintf(stdout, "SUCCESS\n");
>         }
>
>         if (test->set_irqtype) {
> @@ -65,16 +64,18 @@ static int run_test(struct pci_test *test)
>                 if (ret < 0)
>                         fprintf(stdout, "FAILED\n");
>                 else
> -                       fprintf(stdout, "%s\n", result[ret]);
> +                       fprintf(stdout, "SUCCESS\n");
>         }
>
>         if (test->get_irqtype) {
>                 ret = ioctl(fd, PCITEST_GET_IRQTYPE);
>                 fprintf(stdout, "GET IRQ TYPE:\t\t");
> -               if (ret < 0)
> +               if (ret < 0) {
>                         fprintf(stdout, "FAILED\n");
> -               else
> +               } else {
>                         fprintf(stdout, "%s\n", irq[ret]);
> +                       ret = 0;
> +               }
>         }
>
>         if (test->clear_irq) {
> @@ -83,34 +84,34 @@ static int run_test(struct pci_test *test)
>                 if (ret < 0)
>                         fprintf(stdout, "FAILED\n");
>                 else
> -                       fprintf(stdout, "%s\n", result[ret]);
> +                       fprintf(stdout, "SUCCESS\n");
>         }
>
>         if (test->legacyirq) {
>                 ret = ioctl(fd, PCITEST_LEGACY_IRQ, 0);
>                 fprintf(stdout, "LEGACY IRQ:\t");
>                 if (ret < 0)
> -                       fprintf(stdout, "TEST FAILED\n");
> +                       fprintf(stdout, "FAILED\n");
>                 else
> -                       fprintf(stdout, "%s\n", result[ret]);
> +                       fprintf(stdout, "SUCCESS\n");
>         }
>
>         if (test->msinum > 0 && test->msinum <= 32) {
>                 ret = ioctl(fd, PCITEST_MSI, test->msinum);
>                 fprintf(stdout, "MSI%d:\t\t", test->msinum);
>                 if (ret < 0)
> -                       fprintf(stdout, "TEST FAILED\n");
> +                       fprintf(stdout, "FAILED\n");
>                 else
> -                       fprintf(stdout, "%s\n", result[ret]);
> +                       fprintf(stdout, "SUCCESS\n");
>         }
>
>         if (test->msixnum > 0 && test->msixnum <= 2048) {
>                 ret = ioctl(fd, PCITEST_MSIX, test->msixnum);
>                 fprintf(stdout, "MSI-X%d:\t\t", test->msixnum);
>                 if (ret < 0)
> -                       fprintf(stdout, "TEST FAILED\n");
> +                       fprintf(stdout, "FAILED\n");
>                 else
> -                       fprintf(stdout, "%s\n", result[ret]);
> +                       fprintf(stdout, "SUCCESS\n");
>         }
>
>         if (test->write) {
> @@ -120,9 +121,9 @@ static int run_test(struct pci_test *test)
>                 ret = ioctl(fd, PCITEST_WRITE, &param);
>                 fprintf(stdout, "WRITE (%7ld bytes):\t\t", test->size);
>                 if (ret < 0)
> -                       fprintf(stdout, "TEST FAILED\n");
> +                       fprintf(stdout, "FAILED\n");
>                 else
> -                       fprintf(stdout, "%s\n", result[ret]);
> +                       fprintf(stdout, "SUCCESS\n");
>         }
>
>         if (test->read) {
> @@ -132,9 +133,9 @@ static int run_test(struct pci_test *test)
>                 ret = ioctl(fd, PCITEST_READ, &param);
>                 fprintf(stdout, "READ (%7ld bytes):\t\t", test->size);
>                 if (ret < 0)
> -                       fprintf(stdout, "TEST FAILED\n");
> +                       fprintf(stdout, "FAILED\n");
>                 else
> -                       fprintf(stdout, "%s\n", result[ret]);
> +                       fprintf(stdout, "SUCCESS\n");
>         }
>
>         if (test->copy) {
> @@ -144,14 +145,14 @@ static int run_test(struct pci_test *test)
>                 ret = ioctl(fd, PCITEST_COPY, &param);
>                 fprintf(stdout, "COPY (%7ld bytes):\t\t", test->size);
>                 if (ret < 0)
> -                       fprintf(stdout, "TEST FAILED\n");
> +                       fprintf(stdout, "FAILED\n");
>                 else
> -                       fprintf(stdout, "%s\n", result[ret]);
> +                       fprintf(stdout, "SUCCESS\n");
>         }
>
>         fflush(stdout);
>         close(fd);
> -       return (ret < 0) ? ret : 1 - ret; /* return 0 if test succeeded */
> +       return ret;
>  }
>
>  int main(int argc, char **argv)
> --
> 2.25.1
>

Don't you use the errno of ioctl?

Thanks,
Shunsuke