Hi, Kishon, On 17/07/2018 03:43, Kishon Vijay Abraham I wrote: > Hi Gustavo, > > On Monday 16 July 2018 08:22 PM, Gustavo Pimentel wrote: >> Add MSI-X support and update driver documentation accordingly. >> >> Add 2 new IOCTL commands: >> - Allow to reconfigure driver IRQ type in runtime. >> - Allow to retrieve current driver IRQ type configured. >> >> Add IRQ type validation before executing the READ/WRITE/COPY tests. >> >> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx> >> --- >> Change v2->v3: >> - New patch file created base on the previous patch >> "misc: pci_endpoint_test: Add MSI-X support" patch file following >> Kishon's suggestion. >> Change v3->v4: >> - Rebased to Lorenzo's master branch v4.18-rc1. >> Change v4->v5: >> - Nothing changed, just to follow the patch set version. >> Change v5->v6: >> - Moved PCITEST_SET_IRQTYPE and PCITEST_GET_IRQTYPE ioctl entries >> from patch #10 to here. >> - Increased ioctl parameters range associated to >> drivers/misc/pci_endpoint_test.c driver. >> Change v6->v7: >> - irq_type variable update just before returning the function. >> Change v7->v8: >> - Re-sending the patch series. >> Change v8->v9: >> - Added a extra parameter to pci_endpoint_test_alloc_irq_vectors, >> that specifies which irq type should be allocated. >> Change v9->v10: >> - Fixed bug, report available: https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2018_7_16_11&d=DwIC-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=bkWxpLoW-f-E3EdiDCCa0_h0PicsViasSlvIpzZvPxs&m=kfPim9EoIsdNO2BRH1BV6fsKhqV4Qy_JAofUZ2bPmpg&s=sz6k8lKVuC2tpzwc3T-OwE9ShzNwtALcrS_fuJGfiK0&e= >> - Added IRQ type validation before executing the READ/WRITE/COPY >> tests. >> >> Documentation/ioctl/ioctl-number.txt | 2 +- >> Documentation/misc-devices/pci-endpoint-test.txt | 3 + >> drivers/misc/pci_endpoint_test.c | 192 +++++++++++++++++------ >> include/uapi/linux/pcitest.h | 2 + >> 4 files changed, 152 insertions(+), 47 deletions(-) >> >> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt >> index 65259d4..c15c4f3 100644 >> --- a/Documentation/ioctl/ioctl-number.txt >> +++ b/Documentation/ioctl/ioctl-number.txt >> @@ -166,7 +166,7 @@ Code Seq#(hex) Include File Comments >> 'P' all linux/soundcard.h conflict! >> 'P' 60-6F sound/sscape_ioctl.h conflict! >> 'P' 00-0F drivers/usb/class/usblp.c conflict! >> -'P' 01-07 drivers/misc/pci_endpoint_test.c conflict! >> +'P' 01-09 drivers/misc/pci_endpoint_test.c conflict! >> 'Q' all linux/soundcard.h >> 'R' 00-1F linux/random.h conflict! >> 'R' 01 linux/rfkill.h conflict! >> diff --git a/Documentation/misc-devices/pci-endpoint-test.txt b/Documentation/misc-devices/pci-endpoint-test.txt >> index fdfa0f6..58ccca4 100644 >> --- a/Documentation/misc-devices/pci-endpoint-test.txt >> +++ b/Documentation/misc-devices/pci-endpoint-test.txt >> @@ -28,6 +28,9 @@ ioctl >> to be tested should be passed as argument. >> PCITEST_MSIX: Tests message signalled interrupts. The MSI-X number >> to be tested should be passed as argument. >> + PCITEST_SET_IRQTYPE: Changes driver IRQ type configuration. The IRQ type >> + should be passed as argument (0: Legacy, 1:MSI, 2:MSI-X). >> + PCITEST_GET_IRQTYPE: Gets driver IRQ type configuration. >> PCITEST_WRITE: Perform write tests. The size of the buffer should be passed >> as argument. >> PCITEST_READ: Perform read tests. The size of the buffer should be passed >> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c >> index f4fef10..d909ca5 100644 >> --- a/drivers/misc/pci_endpoint_test.c >> +++ b/drivers/misc/pci_endpoint_test.c >> @@ -37,6 +37,7 @@ >> >> #define DRV_MODULE_NAME "pci-endpoint-test" >> >> +#define IRQ_TYPE_UNDEFINED -1 >> #define IRQ_TYPE_LEGACY 0 >> #define IRQ_TYPE_MSI 1 >> #define IRQ_TYPE_MSIX 2 >> @@ -157,6 +158,88 @@ static irqreturn_t pci_endpoint_test_irqhandler(int irq, void *dev_id) >> return IRQ_HANDLED; >> } >> >> +static void pci_endpoint_test_free_irq_vectors(struct pci_endpoint_test *test) >> +{ >> + int i; >> + struct pci_dev *pdev = test->pdev; >> + struct device *dev = &pdev->dev; >> + >> + for (i = 0; i < test->num_irqs; i++) >> + devm_free_irq(dev, pci_irq_vector(pdev, i), test); >> + >> + test->num_irqs = 0; >> +} >> + >> +static bool pci_endpoint_test_alloc_irq_vectors(struct pci_endpoint_test *test, >> + int type) >> +{ >> + int irq = -1; >> + struct pci_dev *pdev = test->pdev; >> + struct device *dev = &pdev->dev; >> + bool res = true; >> + >> + switch (type) { >> + case IRQ_TYPE_LEGACY: >> + irq = 0; >> + break; >> + case IRQ_TYPE_MSI: >> + irq = pci_alloc_irq_vectors(pdev, 1, 32, PCI_IRQ_MSI); >> + if (irq < 0) >> + dev_err(dev, "Failed to get MSI interrupts\n"); >> + break; >> + case IRQ_TYPE_MSIX: >> + irq = pci_alloc_irq_vectors(pdev, 1, 2048, PCI_IRQ_MSIX); >> + if (irq < 0) >> + dev_err(dev, "Failed to get MSI-X interrupts\n"); >> + break; >> + default: >> + dev_err(dev, "Invalid IRQ type selected\n"); >> + } >> + >> + if (irq < 0) { >> + irq = 0; >> + res = false; >> + } >> + test->num_irqs = irq; >> + >> + return res; >> +} >> + >> +static void pci_endpoint_test_release_irq(struct pci_endpoint_test *test) >> +{ >> + struct pci_dev *pdev = test->pdev; >> + >> + pci_disable_msi(pdev); >> + pci_disable_msix(pdev); >> +} >> + >> +static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test) >> +{ >> + int i; >> + int err; >> + struct pci_dev *pdev = test->pdev; >> + struct device *dev = &pdev->dev; >> + >> + err = devm_request_irq(dev, pdev->irq, pci_endpoint_test_irqhandler, >> + IRQF_SHARED, DRV_MODULE_NAME, test); >> + if (err) { >> + dev_err(dev, "Failed to request IRQ %d\n", pdev->irq); >> + return false; >> + } >> + >> + for (i = 1; i < test->num_irqs; i++) { >> + err = devm_request_irq(dev, pci_irq_vector(pdev, i), >> + pci_endpoint_test_irqhandler, >> + IRQF_SHARED, DRV_MODULE_NAME, test); >> + if (err) >> + dev_err(dev, "Failed to request IRQ %d for MSI%s %d\n", >> + pci_irq_vector(pdev, i), >> + irq_type == IRQ_TYPE_MSIX ? "-X" : "", i + 1); >> + } >> + >> + return true; >> +} >> + >> static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, >> enum pci_barno barno) >> { >> @@ -247,6 +330,11 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test, size_t size) >> if (size > SIZE_MAX - alignment) >> goto err; >> >> + if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) { >> + dev_err(dev, "Invalid IRQ type option\n"); >> + goto err; >> + } >> + >> orig_src_addr = dma_alloc_coherent(dev, size + alignment, >> &orig_src_phys_addr, GFP_KERNEL); >> if (!orig_src_addr) { >> @@ -337,6 +425,11 @@ static bool pci_endpoint_test_write(struct pci_endpoint_test *test, size_t size) >> if (size > SIZE_MAX - alignment) >> goto err; >> >> + if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) { >> + dev_err(dev, "Invalid IRQ type option\n"); >> + goto err; >> + } >> + >> orig_addr = dma_alloc_coherent(dev, size + alignment, &orig_phys_addr, >> GFP_KERNEL); >> if (!orig_addr) { >> @@ -400,6 +493,11 @@ static bool pci_endpoint_test_read(struct pci_endpoint_test *test, size_t size) >> if (size > SIZE_MAX - alignment) >> goto err; >> >> + if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) { >> + dev_err(dev, "Invalid IRQ type option\n"); >> + goto err; >> + } >> + >> orig_addr = dma_alloc_coherent(dev, size + alignment, &orig_phys_addr, >> GFP_KERNEL); >> if (!orig_addr) { >> @@ -440,6 +538,38 @@ static bool pci_endpoint_test_read(struct pci_endpoint_test *test, size_t size) >> return ret; >> } >> >> +static bool 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; >> + >> + if (req_irq_type < IRQ_TYPE_LEGACY || req_irq_type > IRQ_TYPE_MSIX) { >> + dev_err(dev, "Invalid IRQ type option\n"); >> + return false; >> + } >> + >> + if (irq_type == req_irq_type) >> + return true; >> + >> + pci_endpoint_test_free_irq_vectors(test); >> + pci_endpoint_test_release_irq(test); >> + >> + if (!pci_endpoint_test_alloc_irq_vectors(test, req_irq_type)) >> + goto err; >> + >> + if (!pci_endpoint_test_request_irq(test)) >> + goto err; >> + >> + irq_type = req_irq_type; >> + return true; >> + >> +err: >> + pci_endpoint_test_release_irq(test); > > sorry for not noticing this earlier.. > > pci_endpoint_test_alloc_irq_vectors enables MSI.. I'd have expected > pci_endpoint_test_free_irq_vectors to disable MSI. Similarly > pci_endpoint_test_request_irq requests irq. Ideally > pci_endpoint_test_release_irq should have freed irq. I feel the contents of > pci_endpoint_test_release_irq() and pci_endpoint_test_free_irq_vectors are swapped. > > For the patch itself, I have tested it now and the issue I reported earlier is > not seen in this series. I just sent the patch series V12 with that swapped. Please give the Acked for the missing patches #4, #7 and #10 (if you agree) to close this patch series. > > Thanks > Kishon > Regards, Gustavo
