From f61b7634a3249d12b9daa36ffbdb9965b6f24c6c Mon Sep 17 00:00:00 2001 From: Damien Le Moal Date: Sat, 15 Apr 2023 11:35:39 +0900 Subject: misc: pci_endpoint_test: Free IRQs before removing the device In pci_endpoint_test_remove(), freeing the IRQs after removing the device creates a small race window for IRQs to be received with the test device memory already released, causing the IRQ handler to access invalid memory, resulting in an oops. Free the device IRQs before removing the device to avoid this issue. Link: https://lore.kernel.org/r/20230415023542.77601-15-dlemoal@kernel.org Fixes: e03327122e2c ("pci_endpoint_test: Add 2 ioctl commands") Signed-off-by: Damien Le Moal Signed-off-by: Lorenzo Pieralisi Signed-off-by: Bjorn Helgaas Reviewed-by: Manivannan Sadhasivam Cc: stable@vger.kernel.org --- drivers/misc/pci_endpoint_test.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers/misc/pci_endpoint_test.c') diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index a7244de081ec..01235236e9bc 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -938,6 +938,9 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev) if (id < 0) return; + pci_endpoint_test_release_irq(test); + pci_endpoint_test_free_irq_vectors(test); + misc_deregister(&test->miscdev); kfree(misc_device->name); kfree(test->name); @@ -947,9 +950,6 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev) pci_iounmap(pdev, test->bar[bar]); } - pci_endpoint_test_release_irq(test); - pci_endpoint_test_free_irq_vectors(test); - pci_release_regions(pdev); pci_disable_device(pdev); } -- cgit v1.2.3 From fb620ae73b70c2f57b9d3e911fc24c024ba2324f Mon Sep 17 00:00:00 2001 From: Damien Le Moal Date: Sat, 15 Apr 2023 11:35:40 +0900 Subject: misc: pci_endpoint_test: Re-init completion for every test The irq_raised completion used to detect the end of a test case is initialized when the test device is probed, but never reinitialized again before a test case. As a result, the irq_raised completion synchronization is effective only for the first ioctl test case executed. Any subsequent call to wait_for_completion() by another ioctl() call will immediately return, potentially too early, leading to false positive failures. Fix this by reinitializing the irq_raised completion before starting a new ioctl() test command. Link: https://lore.kernel.org/r/20230415023542.77601-16-dlemoal@kernel.org Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device") Signed-off-by: Damien Le Moal Signed-off-by: Lorenzo Pieralisi Signed-off-by: Bjorn Helgaas Reviewed-by: Manivannan Sadhasivam Cc: stable@vger.kernel.org --- drivers/misc/pci_endpoint_test.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'drivers/misc/pci_endpoint_test.c') diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 01235236e9bc..24efe3b88a1f 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -729,6 +729,10 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, struct pci_dev *pdev = test->pdev; mutex_lock(&test->mutex); + + reinit_completion(&test->irq_raised); + test->last_irq = -ENODATA; + switch (cmd) { case PCITEST_BAR: bar = arg; -- cgit v1.2.3 From 168e6f62e4298815125591ff9c85d374b2a93c6c Mon Sep 17 00:00:00 2001 From: Damien Le Moal Date: Sat, 15 Apr 2023 11:35:41 +0900 Subject: misc: pci_endpoint_test: Do not write status in IRQ handler pci_endpoint_test_irqhandler() always rewrites the status register when an IRQ is raised, either as-is if STATUS_IRQ_RAISED is not set, or with STATUS_IRQ_RAISED cleared if that flag is set. The first case creates a race window with the endpoint side, meaning that the host side test driver may end up reading what it just wrote, thus losing the real status as set by the endpoint side before raising the next interrupt. This can prevent detecting that the STATUS_IRQ_RAISED flag was set by the endpoint. Remove this race window by not clearing the STATUS_IRQ_RAISED status flag and not rewriting that register for every IRQ received. Link: https://lore.kernel.org/r/20230415023542.77601-17-dlemoal@kernel.org Signed-off-by: Damien Le Moal Signed-off-by: Lorenzo Pieralisi Signed-off-by: Bjorn Helgaas Reviewed-by: Manivannan Sadhasivam --- drivers/misc/pci_endpoint_test.c | 3 --- 1 file changed, 3 deletions(-) (limited to 'drivers/misc/pci_endpoint_test.c') diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 24efe3b88a1f..afd2577261f8 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -159,10 +159,7 @@ static irqreturn_t pci_endpoint_test_irqhandler(int irq, void *dev_id) if (reg & STATUS_IRQ_RAISED) { test->last_irq = irq; complete(&test->irq_raised); - reg &= ~STATUS_IRQ_RAISED; } - pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS, - reg); return IRQ_HANDLED; } -- cgit v1.2.3 From 4c50f933f50e018653a11bd77eb872d46d67c193 Mon Sep 17 00:00:00 2001 From: Damien Le Moal Date: Sat, 15 Apr 2023 11:35:42 +0900 Subject: misc: pci_endpoint_test: Simplify pci_endpoint_test_msi_irq() Simplify the code of pci_endpoint_test_msi_irq() by correctly using booleans: remove the msix comparison to false as that variable is already a boolean, and directly return the result of the comparison of the raised interrupt number. Link: https://lore.kernel.org/r/20230415023542.77601-18-dlemoal@kernel.org Signed-off-by: Damien Le Moal Signed-off-by: Lorenzo Pieralisi Signed-off-by: Bjorn Helgaas Reviewed-by: Manivannan Sadhasivam --- drivers/misc/pci_endpoint_test.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) (limited to 'drivers/misc/pci_endpoint_test.c') diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index afd2577261f8..ed4d0ef5e5c3 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -313,21 +313,17 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test, struct pci_dev *pdev = test->pdev; pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, - msix == false ? IRQ_TYPE_MSI : - IRQ_TYPE_MSIX); + msix ? IRQ_TYPE_MSIX : IRQ_TYPE_MSI); pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, msi_num); pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, - msix == false ? COMMAND_RAISE_MSI_IRQ : - COMMAND_RAISE_MSIX_IRQ); + msix ? COMMAND_RAISE_MSIX_IRQ : + COMMAND_RAISE_MSI_IRQ); val = wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); if (!val) return false; - if (pci_irq_vector(pdev, msi_num - 1) == test->last_irq) - return true; - - return false; + return pci_irq_vector(pdev, msi_num - 1) == test->last_irq; } static int pci_endpoint_test_validate_xfer_params(struct device *dev, -- cgit v1.2.3