| 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
| ||
|
Date: Tue, 17 Apr 2018 18:38:58 +0100
From: Gustavo Pimentel <gustavo.pimentel@...opsys.com>
To: Kishon Vijay Abraham I <kishon@...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>,
"niklas.cassel@...s.com" <niklas.cassel@...s.com>,
"jesper.nilsson@...s.com" <jesper.nilsson@...s.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: [RFC 06/10] misc: pci_endpoint_test: Add MSI-X support
Hi Kishon,
On 17/04/2018 11:33, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 10 April 2018 10:44 PM, Gustavo Pimentel wrote:
>> Adds the MSI-X support and updates driver documentation accordingly.
>>
>> Changes the driver parameter in order to allow the interruption type
>> selection.
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@...opsys.com>
>> ---
>> Documentation/misc-devices/pci-endpoint-test.txt | 3 +
>> drivers/misc/pci_endpoint_test.c | 102 +++++++++++++++++------
>> 2 files changed, 79 insertions(+), 26 deletions(-)
>>
>> diff --git a/Documentation/misc-devices/pci-endpoint-test.txt b/Documentation/misc-devices/pci-endpoint-test.txt
>> index 4ebc359..fdfa0f6 100644
>> --- a/Documentation/misc-devices/pci-endpoint-test.txt
>> +++ b/Documentation/misc-devices/pci-endpoint-test.txt
>> @@ -10,6 +10,7 @@ The PCI driver for the test device performs the following tests
>> *) verifying addresses programmed in BAR
>> *) raise legacy IRQ
>> *) raise MSI IRQ
>> + *) raise MSI-X IRQ
>> *) read data
>> *) write data
>> *) copy data
>> @@ -25,6 +26,8 @@ ioctl
>> PCITEST_LEGACY_IRQ: Tests legacy IRQ
>> PCITEST_MSI: Tests message signalled interrupts. The MSI number
>> 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_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 37db0fc..a7d9354 100644
>> --- a/drivers/misc/pci_endpoint_test.c
>> +++ b/drivers/misc/pci_endpoint_test.c
>> @@ -42,11 +42,16 @@
>> #define PCI_ENDPOINT_TEST_COMMAND 0x4
>> #define COMMAND_RAISE_LEGACY_IRQ BIT(0)
>> #define COMMAND_RAISE_MSI_IRQ BIT(1)
>> -#define MSI_NUMBER_SHIFT 2
>> -/* 6 bits for MSI number */
>> -#define COMMAND_READ BIT(8)
>> -#define COMMAND_WRITE BIT(9)
>> -#define COMMAND_COPY BIT(10)
>> +#define COMMAND_RAISE_MSIX_IRQ BIT(2)
>> +#define IRQ_TYPE_SHIFT 3
>> +#define IRQ_TYPE_LEGACY 0
>> +#define IRQ_TYPE_MSI 1
>> +#define IRQ_TYPE_MSIX 2
>> +#define MSI_NUMBER_SHIFT 5
>
> Now that you are anyways fixing this, add a new register entry for MSI numbers.
> Let's not keep COMMAND and MSI's together.
What you suggest?
>> +/* 12 bits for MSI number */
>> +#define COMMAND_READ BIT(17)
>> +#define COMMAND_WRITE BIT(18)
>> +#define COMMAND_COPY BIT(19)
>
> This change should be done along with the pci-epf-test in a single patch.
To be clear, you're saying is this patch should be just be squashed into the
patch number 8 [1], because there is a lot of dependencies namely the defines,
that is used on the alter functions.
[1] -> https://patchwork.ozlabs.org/patch/896841/
>>
>> #define PCI_ENDPOINT_TEST_STATUS 0x8
>> #define STATUS_READ_SUCCESS BIT(0)
>> @@ -73,9 +78,9 @@ static DEFINE_IDA(pci_endpoint_test_ida);
>> #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
>> miscdev)
>>
>> -static bool no_msi;
>> -module_param(no_msi, bool, 0444);
>> -MODULE_PARM_DESC(no_msi, "Disable MSI interrupt in pci_endpoint_test");
>
> Let's not remove this just to make sure existing users doesn't get affected.
Hum, by making an internal conversion? Like this
no_msi = false <=> irq_type = 1
no_msi = true <=> irq_type = 0
>> +static int irq_type = IRQ_TYPE_MSIX;
>> +module_param(irq_type, int, 0444);
>> +MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test (0 - Legacy, 1 - MSI, 2 - MSI-X)");
>>
>> enum pci_barno {
>> BAR_0,
>> @@ -103,7 +108,7 @@ struct pci_endpoint_test {
>> struct pci_endpoint_test_data {
>> enum pci_barno test_reg_bar;
>> size_t alignment;
>> - bool no_msi;
>> + int irq_type;
>> };
>>
>> static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test,
>> @@ -177,10 +182,10 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>>
>> static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
>> {
>> - u32 val;
>> + u32 val = COMMAND_RAISE_LEGACY_IRQ;
>>
>> - pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>> - COMMAND_RAISE_LEGACY_IRQ);
>> + val |= (IRQ_TYPE_LEGACY << IRQ_TYPE_SHIFT);
>> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>> val = wait_for_completion_timeout(&test->irq_raised,
>> msecs_to_jiffies(1000));
>> if (!val)
>> @@ -192,12 +197,12 @@ static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
>> static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
>> u8 msi_num)
>> {
>> - u32 val;
>> + u32 val = COMMAND_RAISE_MSI_IRQ;
>> struct pci_dev *pdev = test->pdev;
>>
>> - pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>> - msi_num << MSI_NUMBER_SHIFT |
>> - COMMAND_RAISE_MSI_IRQ);
>> + val |= (msi_num << MSI_NUMBER_SHIFT);
>> + val |= (IRQ_TYPE_MSI << IRQ_TYPE_SHIFT);
>> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>> val = wait_for_completion_timeout(&test->irq_raised,
>> msecs_to_jiffies(1000));
>> if (!val)
>> @@ -209,6 +214,26 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
>> return false;
>> }
>>
>> +static bool pci_endpoint_test_msix_irq(struct pci_endpoint_test *test,
>> + u16 msix_num)
>> +{
>> + u32 val = COMMAND_RAISE_MSIX_IRQ;
>> + struct pci_dev *pdev = test->pdev;
>> +
>> + val |= (msix_num << MSI_NUMBER_SHIFT);
>> + val |= (IRQ_TYPE_MSIX << IRQ_TYPE_SHIFT);
>> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val);
>> + val = wait_for_completion_timeout(&test->irq_raised,
>> + msecs_to_jiffies(1000));
>> + if (!val)
>> + return false;
>> +
>> + if (test->last_irq - pdev->irq == msix_num - 1)
>> + return true;
>> +
>> + return false;
>
> I think you can have a single function for msix_irq and msi_irq.
Ok.
>
> Thanks
> Kishon
>
Powered by blists - more mailing lists