| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fe9da184-e085-05d2-2c12-4dc50633736f@ti.com>
Date: Tue, 17 Jul 2018 08:13:53 +0530
From: Kishon Vijay Abraham I <kishon@...com>
To: Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
<bhelgaas@...gle.com>, <lorenzo.pieralisi@....com>,
<joao.pinto@...opsys.com>, <jingoohan1@...il.com>,
<adouglas@...ence.com>, <jesper.nilsson@...s.com>,
<shawn.lin@...k-chips.com>
CC: <linux-pci@...r.kernel.org>, <linux-doc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v11 10/12] pci_endpoint_test: Add 2 ioctl commands
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@...opsys.com>
> ---
> 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://lkml.org/lkml/2018/7/16/11
> - 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.
Thanks
Kishon
Powered by blists - more mailing lists