On Tue, Mar 18, 2025 at 11:33:34AM +0100, Niklas Cassel wrote:
> The test cases for read/write/copy currently do:
> 1) ioctl(PCITEST_SET_IRQTYPE, MSI)
> 2) ioctl(PCITEST_{READ,WRITE,COPY})
>
> This design is quite bad for a few reasons:
> -It assumes that all PCI EPCs support MSI.
> -One ioctl should be sufficient for these test cases.
>
> Modify the PCITEST_{READ,WRITE,COPY} ioctls to set IRQ type automatically,
> overwriting the currently configured IRQ type. It there are no IRQ types
> supported in the CAPS register, fall back to MSI IRQs. This way the
> implementation is no worse than before this commit.
>
> Any test case that requires a specific IRQ type, e.g. MSIX_TEST, will do
> an explicit PCITEST_SET_IRQTYPE ioctl at the start of the test case, thus
> it is safe to always overwrite the configured IRQ type.
>
I don't quite understand this sentence. What if users wants to use a specific
IRQ type like MSI-X if the platform supports both MSI/MSI-X? That's why I wanted
to honor 'test->irq_type' if already set before READ,WRITE,COPY testcases.
Everything else looks good to me.
- Mani
> Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> ---
> drivers/misc/pci_endpoint_test.c | 126 +++++++++++++++++--------------
> 1 file changed, 68 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3c04121d24733..cfaeeea7642ac 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -464,6 +464,62 @@ static int pci_endpoint_test_validate_xfer_params(struct device *dev,
> return 0;
> }
>
> +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 0;
> +}
> +
> +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 ret;
> +
> + if (req_irq_type < PCITEST_IRQ_TYPE_INTX ||
> + req_irq_type > PCITEST_IRQ_TYPE_MSIX) {
> + dev_err(dev, "Invalid IRQ type option\n");
> + return -EINVAL;
> + }
> +
> + if (test->irq_type == req_irq_type)
> + return 0;
> +
> + pci_endpoint_test_release_irq(test);
> + pci_endpoint_test_free_irq_vectors(test);
> +
> + ret = pci_endpoint_test_alloc_irq_vectors(test, req_irq_type);
> + if (ret)
> + return ret;
> +
> + ret = pci_endpoint_test_request_irq(test);
> + if (ret) {
> + pci_endpoint_test_free_irq_vectors(test);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int pci_endpoint_test_set_auto_irq(struct pci_endpoint_test *test,
> + int *irq_type)
> +{
> + if (test->ep_caps & CAP_MSI)
> + *irq_type = PCITEST_IRQ_TYPE_MSI;
> + else if (test->ep_caps & CAP_MSIX)
> + *irq_type = PCITEST_IRQ_TYPE_MSIX;
> + else if (test->ep_caps & CAP_INTX)
> + *irq_type = PCITEST_IRQ_TYPE_INTX;
> + else
> + /* fallback to MSI if no caps defined */
> + *irq_type = PCITEST_IRQ_TYPE_MSI;
> +
> + return pci_endpoint_test_set_irq(test, *irq_type);
> +}
> +
> static int pci_endpoint_test_copy(struct pci_endpoint_test *test,
> unsigned long arg)
> {
> @@ -483,7 +539,7 @@ static int pci_endpoint_test_copy(struct pci_endpoint_test *test,
> dma_addr_t orig_dst_phys_addr;
> size_t offset;
> size_t alignment = test->alignment;
> - int irq_type = test->irq_type;
> + int irq_type;
> u32 src_crc32;
> u32 dst_crc32;
> int ret;
> @@ -504,11 +560,9 @@ static int pci_endpoint_test_copy(struct pci_endpoint_test *test,
> if (use_dma)
> flags |= FLAG_USE_DMA;
>
> - if (irq_type < PCITEST_IRQ_TYPE_INTX ||
> - irq_type > PCITEST_IRQ_TYPE_MSIX) {
> - dev_err(dev, "Invalid IRQ type option\n");
> - return -EINVAL;
> - }
> + ret = pci_endpoint_test_set_auto_irq(test, &irq_type);
> + if (ret)
> + return ret;
>
> orig_src_addr = kzalloc(size + alignment, GFP_KERNEL);
> if (!orig_src_addr) {
> @@ -616,7 +670,7 @@ static int pci_endpoint_test_write(struct pci_endpoint_test *test,
> dma_addr_t orig_phys_addr;
> size_t offset;
> size_t alignment = test->alignment;
> - int irq_type = test->irq_type;
> + int irq_type;
> size_t size;
> u32 crc32;
> int ret;
> @@ -637,11 +691,9 @@ static int pci_endpoint_test_write(struct pci_endpoint_test *test,
> if (use_dma)
> flags |= FLAG_USE_DMA;
>
> - if (irq_type < PCITEST_IRQ_TYPE_INTX ||
> - irq_type > PCITEST_IRQ_TYPE_MSIX) {
> - dev_err(dev, "Invalid IRQ type option\n");
> - return -EINVAL;
> - }
> + ret = pci_endpoint_test_set_auto_irq(test, &irq_type);
> + if (ret)
> + return ret;
>
> orig_addr = kzalloc(size + alignment, GFP_KERNEL);
> if (!orig_addr) {
> @@ -714,7 +766,7 @@ static int pci_endpoint_test_read(struct pci_endpoint_test *test,
> dma_addr_t orig_phys_addr;
> size_t offset;
> size_t alignment = test->alignment;
> - int irq_type = test->irq_type;
> + int irq_type;
> u32 crc32;
> int ret;
>
> @@ -734,11 +786,9 @@ static int pci_endpoint_test_read(struct pci_endpoint_test *test,
> if (use_dma)
> flags |= FLAG_USE_DMA;
>
> - if (irq_type < PCITEST_IRQ_TYPE_INTX ||
> - irq_type > PCITEST_IRQ_TYPE_MSIX) {
> - dev_err(dev, "Invalid IRQ type option\n");
> - return -EINVAL;
> - }
> + ret = pci_endpoint_test_set_auto_irq(test, &irq_type);
> + if (ret)
> + return ret;
>
> orig_addr = kzalloc(size + alignment, GFP_KERNEL);
> if (!orig_addr) {
> @@ -790,46 +840,6 @@ static int pci_endpoint_test_read(struct pci_endpoint_test *test,
> return ret;
> }
>
> -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 0;
> -}
> -
> -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 ret;
> -
> - if (req_irq_type < PCITEST_IRQ_TYPE_INTX ||
> - req_irq_type > PCITEST_IRQ_TYPE_MSIX) {
> - dev_err(dev, "Invalid IRQ type option\n");
> - return -EINVAL;
> - }
> -
> - if (test->irq_type == req_irq_type)
> - return 0;
> -
> - pci_endpoint_test_release_irq(test);
> - pci_endpoint_test_free_irq_vectors(test);
> -
> - ret = pci_endpoint_test_alloc_irq_vectors(test, req_irq_type);
> - if (ret)
> - return ret;
> -
> - ret = pci_endpoint_test_request_irq(test);
> - if (ret) {
> - pci_endpoint_test_free_irq_vectors(test);
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> --
> 2.48.1
>
--
மணிவண்ணன் சதாசிவம்
