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/[email protected]/
Changes in v2:
* Fixed the error numbers in pci_endpoint_test
* Added Fixes tag and CCed stable list for relevant patches. The patches
should get backported until 5.10 kernel only. Since for the LTS kernels
before that, the pci_endpoint_test driver was not supporting all commands.
Manivannan Sadhasivam (5):
misc: pci_endpoint_test: Fix the return value of IOCTL
tools: PCI: Fix parsing the return value of IOCTLs
Documentation: PCI: endpoint: Use the correct return value of
pcitest.sh
misc: pci_endpoint_test: Remove unnecessary WARN_ON
tools: PCI: Fix memory leak
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
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!
Cc: [email protected] #5.10
Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device")
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
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 8f786a225dcf..a7d8ae9730f6 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 = -ENOSPC;
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 -EIO;
}
- 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 -EIO;
- 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(¶m, (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 = -EIO;
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(¶m, (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 = -EIO;
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(¶m, (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 = -EIO;
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) {
@@ -852,10 +842,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
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 <[email protected]>
---
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 a7d8ae9730f6..5e4d4691a160 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -810,10 +810,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
"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.
Cc: [email protected] #5.10
Fixes: 3f2ed8134834 ("tools: PCI: Add a userspace tool to test PCI endpoint")
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
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, ¶m);
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, ¶m);
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, ¶m);
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
On Wed, Aug 24, 2022 at 06:00:07PM +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.
>
> Cc: [email protected] #5.10
> Fixes: 3f2ed8134834 ("tools: PCI: Add a userspace tool to test PCI endpoint")
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> 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");
Is this following the kernel TAP output rules? If not, why not? If so,
say that you are fixing that issue up in the changelog text.
thanks,
greg k-h
Memory allocated for "test" needs to be freed at the end of the main().
Fixes: 3f2ed8134834 ("tools: PCI: Add a userspace tool to test PCI endpoint")
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
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
The pci_endpoint_test driver has been fixed to return correct error no
from IOCTL. In that process, the pcitest tool now returns SUCCESS and
FAILED instead of OKAY and NOT_OKAY. So change that in documentation also.
Cc: [email protected] #5.10
Fixes: 16263d9e1ded ("Documentation: PCI: Add userguide for PCI endpoint test function")
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
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
On Wed, Aug 24, 2022 at 06:00:06PM +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!
>
> Cc: [email protected] #5.10
> Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device")
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
Reported-by: Greg Kroah-Hartman <[email protected]>
> ---
> 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 8f786a225dcf..a7d8ae9730f6 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 = -ENOSPC;
No need to set this if you:
> 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");
This should now return -EINVAL;
> }
>
> + test->irq_type = type;
Again, do not make a change to the kernel state if there is an error
above. That's wrong to do, and yes, the current code is incorrect,
don't keep that bug here as well when it's so easy to fix up
automatically.
I stopped reviewing here...
thanks,
greg k-h
On Wed, Aug 24, 2022 at 02:43:18PM +0200, Greg KH wrote:
> On Wed, Aug 24, 2022 at 06:00:06PM +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!
> >
> > Cc: [email protected] #5.10
> > Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device")
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
>
> Reported-by: Greg Kroah-Hartman <[email protected]>
>
> > ---
> > 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 8f786a225dcf..a7d8ae9730f6 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 = -ENOSPC;
>
> No need to set this if you:
>
> > 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");
>
> This should now return -EINVAL;
>
ack
>
> > }
> >
> > + test->irq_type = type;
>
> Again, do not make a change to the kernel state if there is an error
> above. That's wrong to do, and yes, the current code is incorrect,
> don't keep that bug here as well when it's so easy to fix up
> automatically.
>
Okay. Will fix it in this patch itself.
Thanks,
Mani
>
> I stopped reviewing here...
>
> thanks,
>
> greg k-h
--
மணிவண்ணன் சதாசிவம்
On Wed, Aug 24, 2022 at 02:44:06PM +0200, Greg KH wrote:
> On Wed, Aug 24, 2022 at 06:00:07PM +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.
> >
> > Cc: [email protected] #5.10
> > Fixes: 3f2ed8134834 ("tools: PCI: Add a userspace tool to test PCI endpoint")
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > ---
> > 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");
>
> Is this following the kernel TAP output rules? If not, why not? If so,
It is not following the TAP rules currently as I was not aware of it. Now that
you pointed out, I'll try to adapt to it.
Thanks,
Mani
> say that you are fixing that issue up in the changelog text.
>
> thanks,
>
> greg k-h
--
மணிவண்ணன் சதாசிவம்
On Wed, Aug 24, 2022 at 06:00:05PM +0530, Manivannan Sadhasivam wrote:
> 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/[email protected]/
>
> Changes in v2:
>
> * Fixed the error numbers in pci_endpoint_test
> * Added Fixes tag and CCed stable list for relevant patches. The patches
> should get backported until 5.10 kernel only. Since for the LTS kernels
> before that, the pci_endpoint_test driver was not supporting all commands.
>
> Manivannan Sadhasivam (5):
> misc: pci_endpoint_test: Fix the return value of IOCTL
> tools: PCI: Fix parsing the return value of IOCTLs
> Documentation: PCI: endpoint: Use the correct return value of
> pcitest.sh
> misc: pci_endpoint_test: Remove unnecessary WARN_ON
> tools: PCI: Fix memory leak
>
> 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(-)
May I ask where are we with this thread ? I have noticed some key
comments from Greg that need addressing so I'd expect a new version.
Thanks,
Lorenzo
On Fri, Sep 09, 2022 at 03:09:58PM +0200, Lorenzo Pieralisi wrote:
> On Wed, Aug 24, 2022 at 06:00:05PM +0530, Manivannan Sadhasivam wrote:
> > 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/[email protected]/
> >
> > Changes in v2:
> >
> > * Fixed the error numbers in pci_endpoint_test
> > * Added Fixes tag and CCed stable list for relevant patches. The patches
> > should get backported until 5.10 kernel only. Since for the LTS kernels
> > before that, the pci_endpoint_test driver was not supporting all commands.
> >
> > Manivannan Sadhasivam (5):
> > misc: pci_endpoint_test: Fix the return value of IOCTL
> > tools: PCI: Fix parsing the return value of IOCTLs
> > Documentation: PCI: endpoint: Use the correct return value of
> > pcitest.sh
> > misc: pci_endpoint_test: Remove unnecessary WARN_ON
> > tools: PCI: Fix memory leak
> >
> > 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(-)
>
> May I ask where are we with this thread ? I have noticed some key
> comments from Greg that need addressing so I'd expect a new version.
>
Sorry for the late response. Yes, there will be a new version.
Thanks,
Mani
> Thanks,
> Lorenzo
--
மணிவண்ணன் சதாசிவம்
On Wed, Aug 24, 2022 at 02:44:06PM +0200, Greg KH wrote:
> On Wed, Aug 24, 2022 at 06:00:07PM +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.
> >
> > Cc: [email protected] #5.10
> > Fixes: 3f2ed8134834 ("tools: PCI: Add a userspace tool to test PCI endpoint")
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > ---
> > 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");
>
> Is this following the kernel TAP output rules? If not, why not? If so,
> say that you are fixing that issue up in the changelog text.
>
Sorry to revive this two months old thread. Adapting to TAP output rules
requires this test to be moved to KUnit which is strictly not necessary and can
be done later.
Moreover, I do not have the hardware to run this testcase and I don't feel
comfortable moving this to KUnit without doing functional testing.
So for now, I will fix the return value of IOCTLs which is the real motive
behind this series.
Thanks,
Mani
> thanks,
>
> greg k-h
--
மணிவண்ணன் சதாசிவம்
On Tue, Nov 01, 2022 at 07:45:34PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Aug 24, 2022 at 02:44:06PM +0200, Greg KH wrote:
> > On Wed, Aug 24, 2022 at 06:00:07PM +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.
> > >
> > > Cc: [email protected] #5.10
> > > Fixes: 3f2ed8134834 ("tools: PCI: Add a userspace tool to test PCI endpoint")
> > > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > > ---
> > > 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");
> >
> > Is this following the kernel TAP output rules? If not, why not? If so,
> > say that you are fixing that issue up in the changelog text.
> >
>
> Sorry to revive this two months old thread. Adapting to TAP output rules
> requires this test to be moved to KUnit which is strictly not necessary and can
> be done later.
KUint has nothing to do with TAP output. TAP output is what the
framework in tools/testing/selftests/ wants to see. Why not move this
test into the proper location there and not in an odd location like
tools/pci/? It does not belong in tools/pci/ at all.
thanks,
greg k-h