| 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: <a89dac70-860b-be00-f5f0-88e69613fe00@synopsys.com>
Date: Tue, 17 Jul 2018 10:33:53 +0100
From: Gustavo Pimentel <Gustavo.Pimentel@...opsys.com>
To: Kishon Vijay Abraham I <kishon@...com>,
Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
"joao.pinto@...opsys.com" <joao.pinto@...opsys.com>,
"jingoohan1@...il.com" <jingoohan1@...il.com>,
"adouglas@...ence.com" <adouglas@...ence.com>,
"jesper.nilsson@...s.com" <jesper.nilsson@...s.com>,
"shawn.lin@...k-chips.com" <shawn.lin@...k-chips.com>
CC: "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v11 10/12] pci_endpoint_test: Add 2 ioctl commands
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@...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://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
Powered by blists - more mailing lists