Re: [PATCH 3/4] misc: pci_endpoint_test: Let PCITEST_{READ,WRITE,COPY} set IRQ type automatically

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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

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