Hi, On Tuesday 24 April 2018 04:27 PM, Gustavo Pimentel wrote: > Hi Kishon, > > On 24/04/2018 08:19, Kishon Vijay Abraham I wrote: >> Hi, >> >> On Tuesday 17 April 2018 11:08 PM, Gustavo Pimentel wrote: >>> 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@xxxxxxxxxxxx> >>>>> --- >>>>> 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? >> >> #define PCI_ENDPOINT_TEST_COMMAND 0x4 >> #define COMMAND_RAISE_LEGACY_IRQ BIT(0) >> #define COMMAND_RAISE_MSI_IRQ BIT(1) >> #define COMMAND_RAISE_MSIX_IRQ BIT(2) >> #define COMMAND_READ BIT(3) >> #define COMMAND_WRITE BIT(4) >> #define COMMAND_COPY BIT(5) >> >> #define PCI_ENDPOINT_TEST_STATUS 0x8 >> #define STATUS_READ_SUCCESS BIT(0) >> #define STATUS_READ_FAIL BIT(1) >> #define STATUS_WRITE_SUCCESS BIT(2) >> #define STATUS_WRITE_FAIL BIT(3) >> #define STATUS_COPY_SUCCESS BIT(4) >> #define STATUS_COPY_FAIL BIT(5) >> #define STATUS_IRQ_RAISED BIT(6) >> #define STATUS_SRC_ADDR_INVALID BIT(7) >> #define STATUS_DST_ADDR_INVALID BIT(8) >> >> #define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0xc >> #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR 0x10 >> >> #define PCI_ENDPOINT_TEST_LOWER_DST_ADDR 0x14 >> #define PCI_ENDPOINT_TEST_UPPER_DST_ADDR 0x18 >> >> #define PCI_ENDPOINT_TEST_SIZE 0x1c >> #define PCI_ENDPOINT_TEST_CHECKSUM 0x20 >> >> #define PCI_ENDPOINT_TEST_MSI_NUMBER 0x24 > > Ok. I will do it. > >> >> We should try not to modify either the existing register offsets or the bit >> fields within these registers in the future as EP and RC will be running on >> different systems and it is possible one of them might not have the updated >> kernel. > > I totally agree. > >>> >>>>> +/* 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://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_896841_&d=DwIC-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=bkWxpLoW-f-E3EdiDCCa0_h0PicsViasSlvIpzZvPxs&m=8urVwHCybXa1XMxsEbwHZAzzaEI_HJGXqmWgXpXb9TY&s=MRVr2YPY2Bk_WNFOxBfU4FGrFReTKdPhfzNDLiVxDbs&e= >> >> yeah. We have to make sure git bisect doesn't break functionality. > > Ok, it'll be squashed. > >>> >>>>> >>>>> #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 >> > Disregard previous comment, it doesn't make sense. I don't know where my head was. > > It will be like this on probe: > > if (no_msi) > irq_type = IRQ_TYPE_LEGACY; > > However since we are breaking the compatibility on terms of MSI/MSI-X > bits/registers (discussion on the top), it makes sense to keep the compatibility > on this parameter? This is userspace compatibility, so lets not break it. Btw can we have a sysfs entry per device for defining irq_type. Having a sysfs entry might be helpful instead of insmod/rmmod with different irq_type values? It will also help if a system has enumerated multiple PCI_ENDPOINT_TEST EP devices. Thanks Kishon
