by Gustavo Pimentel

[permalink] [raw]

Subject: Re: [PATCH v8 09/11] pci_endpoint_test: Add 2 ioctl commands

Hi Alan,

On 09/07/2018 16:22, Alan Douglas wrote:
> Hi Gustavo,
>
> On 06 July 2018 16:52 Gustavo Pimentel <[email protected]> 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.
>>
>> Signed-off-by: Gustavo Pimentel <[email protected]>
>> ---
>> 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.
>>
>> Documentation/ioctl/ioctl-number.txt | 2 +-
>> Documentation/misc-devices/pci-endpoint-test.txt | 3 +
>> drivers/misc/pci_endpoint_test.c | 175 +++++++++++++++++------
>> include/uapi/linux/pcitest.h | 2 +
>> 4 files changed, 135 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..97082e3 100644
>> --- a/drivers/misc/pci_endpoint_test.c
>> +++ b/drivers/misc/pci_endpoint_test.c
>> @@ -157,6 +157,87 @@ 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 irq = -1;
>> + struct pci_dev *pdev = test->pdev;
>> + struct device *dev = &pdev->dev;
>> + bool res = true;
>> +
>> + switch (irq_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)
>> {
>> @@ -440,6 +521,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)) {
>> + pci_endpoint_test_release_irq(test);
>> + return false;
>> + }
> I am testing the latest patch set with the cadence driver, and find this step is not working correctly.
> pci_endpoint_test_alloc_irq_vectors relies on the value of static irq_type to request the appropriate IRQ.
> However, you don't set this value until a few lines later, so the first attempt to set the IRQ type fails. (A
> second attempt to set it will succeed however.) I suggest setting irq_type before the call, or even adding
> a parameter to pci_endpoint_test_alloc_irq_vectors() with the requested IRQ type to avoid having to set it
> back again after a failed call.

Thank you for testing the code. This bug appeared on the patch version 6 after
the Kishon's good suggestion of moving the irq type variable update immediately
before function returning. Unfortunately I forgot the
pci_endpoint_test_alloc_irq_vectors() dependency.
Once again, thanks!

Regards,
Gustavo

>> +
>> + if (!pci_endpoint_test_request_irq(test)) {
>> + pci_endpoint_test_release_irq(test);
>> + return false;
>> + }
>
>> +
>> + irq_type = req_irq_type;
>> +
>> + return true;
>> +}

2018-07-09 17:43:45

by Gustavo Pimentel

[permalink] [raw]

Subject: Re: [PATCH v8 08/11] pci-epf-test/pci_endpoint_test: Add MSI-X support

Hi,

On 09/07/2018 11:26, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Monday 09 July 2018 03:38 PM, Gustavo Pimentel wrote:
>> Hi,
>>
>> On 09/07/2018 05:48, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Friday 06 July 2018 09:21 PM, Gustavo Pimentel wrote:
>>>> Add MSI-X support and update driver documentation accordingly.
>>>>
>>>> Signed-off-by: Gustavo Pimentel <[email protected]>
>>>> ---
>>>> 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_MSIX ioctl entry from patch #10 to here.
>>>> - Documented ioctl parameter type associated to
>>>> drivers/misc/pci_endpoint_test.c driver.
>>>> Change v6->v7:
>>>> - Updated documentation.
>>>> - Added flag that enables or not the MSI-X on the EP features.
>>>> Change v7->v8:
>>>> - Re-sending the patch series.
>>>>
>>>> Documentation/PCI/endpoint/pci-test-function.txt | 2 +-
>>>> Documentation/ioctl/ioctl-number.txt | 1 +
>>>> Documentation/misc-devices/pci-endpoint-test.txt | 3 +++
>>>> drivers/misc/pci_endpoint_test.c | 29 +++++++++++++++++-------
>>>> drivers/pci/controller/dwc/pcie-designware-ep.c | 1 +
>>>> drivers/pci/endpoint/functions/pci-epf-test.c | 29 ++++++++++++++++++++++--
>>>> include/linux/pci-epc.h | 1 +
>>>> include/uapi/linux/pcitest.h | 1 +
>>>> 8 files changed, 56 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/Documentation/PCI/endpoint/pci-test-function.txt b/Documentation/PCI/endpoint/pci-test-function.txt
>>>> index 7ee2361..b1cbff3 100644
>>>> --- a/Documentation/PCI/endpoint/pci-test-function.txt
>>>> +++ b/Documentation/PCI/endpoint/pci-test-function.txt
>>>> @@ -34,7 +34,7 @@ that the endpoint device must perform.
>>>> Bitfield Description:
>>>> Bit 0 : raise legacy IRQ
>>>> Bit 1 : raise MSI IRQ
>>>> - Bit 2 : raise MSI-X IRQ (reserved for future implementation)
>>>> + Bit 2 : raise MSI-X IRQ
>>>> Bit 3 : read command (read data from RC buffer)
>>>> Bit 4 : write command (write data to RC buffer)
>>>> Bit 5 : copy command (copy data from one RC buffer to another
>>>> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
>>>> index 480c860..65259d4 100644
>>>> --- a/Documentation/ioctl/ioctl-number.txt
>>>> +++ b/Documentation/ioctl/ioctl-number.txt
>>>> @@ -166,6 +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!
>>>> '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 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 349794c..f4fef10 100644
>>>> --- a/drivers/misc/pci_endpoint_test.c
>>>> +++ b/drivers/misc/pci_endpoint_test.c
>>>> @@ -39,13 +39,14 @@
>>>>
>>>> #define IRQ_TYPE_LEGACY 0
>>>> #define IRQ_TYPE_MSI 1
>>>> +#define IRQ_TYPE_MSIX 2
>>>>
>>>> #define PCI_ENDPOINT_TEST_MAGIC 0x0
>>>>
>>>> #define PCI_ENDPOINT_TEST_COMMAND 0x4
>>>> #define COMMAND_RAISE_LEGACY_IRQ BIT(0)
>>>> #define COMMAND_RAISE_MSI_IRQ BIT(1)
>>>> -/* BIT(2) is reserved for raising MSI-X IRQ command */
>>>> +#define COMMAND_RAISE_MSIX_IRQ BIT(2)
>>>> #define COMMAND_READ BIT(3)
>>>> #define COMMAND_WRITE BIT(4)
>>>> #define COMMAND_COPY BIT(5)
>>>> @@ -84,7 +85,7 @@ MODULE_PARM_DESC(no_msi, "Disable MSI interrupt in pci_endpoint_test");
>>>>
>>>> static int irq_type = IRQ_TYPE_MSI;
>>>> module_param(irq_type, int, 0444);
>>>> -MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test (0 - Legacy, 1 - MSI)");
>>>> +MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test (0 - Legacy, 1 - MSI, 2 - MSI-X)");
>>>>
>>>> enum pci_barno {
>>>> BAR_0,
>>>> @@ -202,16 +203,18 @@ 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)
>>>> + u16 msi_num, bool msix)
>>>> {
>>>> u32 val;
>>>> struct pci_dev *pdev = test->pdev;
>>>>
>>>> pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE,
>>>> - IRQ_TYPE_MSI);
>>>> + msix == false ? IRQ_TYPE_MSI :
>>>> + IRQ_TYPE_MSIX);
>>>> pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, msi_num);
>>>> pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>>>> - COMMAND_RAISE_MSI_IRQ);
>>>> + msix == false ? COMMAND_RAISE_MSI_IRQ :
>>>> + COMMAND_RAISE_MSIX_IRQ);
>>>> val = wait_for_completion_timeout(&test->irq_raised,
>>>> msecs_to_jiffies(1000));
>>>> if (!val)
>>>> @@ -456,7 +459,8 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
>>>> ret = pci_endpoint_test_legacy_irq(test);
>>>> break;
>>>> case PCITEST_MSI:
>>>> - ret = pci_endpoint_test_msi_irq(test, arg);
>>>> + case PCITEST_MSIX:
>>>> + ret = pci_endpoint_test_msi_irq(test, arg, cmd == PCITEST_MSIX);
>>>> break;
>>>> case PCITEST_WRITE:
>>>> ret = pci_endpoint_test_write(test, arg);
>>>> @@ -542,6 +546,12 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>>>> dev_err(dev, "Failed to get MSI interrupts\n");
>>>> test->num_irqs = irq;
>>>> 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");
>>>> + test->num_irqs = irq;
>>>> + break;
>>>> default:
>>>> dev_err(dev, "Invalid IRQ type selected\n");
>>>> }
>>>> @@ -558,8 +568,9 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>>>> pci_endpoint_test_irqhandler,
>>>> IRQF_SHARED, DRV_MODULE_NAME, test);
>>>> if (err)
>>>> - dev_err(dev, "failed to request IRQ %d for MSI %d\n",
>>>> - pci_irq_vector(pdev, i), i + 1);
>>>> + 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);
>>>> }
>>>>
>>>> for (bar = BAR_0; bar <= BAR_5; bar++) {
>>>> @@ -625,6 +636,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>>>>
>>>> err_disable_msi:
>>>> pci_disable_msi(pdev);
>>>> + pci_disable_msix(pdev);
>>>> pci_release_regions(pdev);
>>>>
>>>> err_disable_pdev:
>>>> @@ -656,6 +668,7 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
>>>> for (i = 0; i < test->num_irqs; i++)
>>>> devm_free_irq(&pdev->dev, pci_irq_vector(pdev, i), test);
>>>> pci_disable_msi(pdev);
>>>> + pci_disable_msix(pdev);
>>>> pci_release_regions(pdev);
>>>> pci_disable_device(pdev);
>>>> }
>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>>> index 70d0688..36d83d1 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>>> @@ -585,6 +585,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>>> ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX);
>>>>
>>>> epc->features = EPC_FEATURE_NO_LINKUP_NOTIFIER;
>>>> + epc->features |= EPC_FEATURE_MSIX_AVAILABLE;
>>>
>>> This indicates all vendors of designware has MSIX enabled which is not true.
>>> We'll need more logic than that.
>>
>> You mentioned and excellent point. Any particularity related to this features
>> should be implemented each specific driver (in this case on
>> pcie-designware-plat.c file).
>>
>> And by looking at this code now that you mentioned, I think the line code
>> "epc->features = EPC_FEATURE_NO_LINKUP_NOTIFIER;" was added by mistake, if I
>> remember well by default the linkup notification is expected, right?
>
> right, since dra7x was the first platform to have EP support added and it has
> linkup notification mechanism.
>>
>> If I'm right, I may create an extra patch removing this 2 lines, do you agree?

I will send a patch as soon as possible to fix this nasty bug. Unfortunately it
will be more than just deleting 2 lines....

>
> Agree. I think we should use ->ep_init() present in dw_pcie_ep_ops for
> populating features. ->ep_init() right now is called in dw_pcie_ep_init() which
> is not needed. Stuffs that are right now done in ->ep_init callbacks can be
> done even before invoking dw_pcie_ep_init().

I don't think so, that we can invoke before, dw_pcie_ep_init() is responsible
for creating the epc object and set the object address to the epc pointer
present on the ep struct. This pointer is used later to access and set the
feature field.

>
> We might have to add a new API pci_epc_init() to be invoked from function
> driver, which should invoke ->ep_init() callback. The features of a particular
> platform should be populated here.

My solution for now is to repair this damage as soon as possible, for that I'll
move the the feature setting from the pcie-designware-ep.c to the
pcie-designware-plat.c and change some code order to work.

I propose we implement this change after this patch has entered, otherwise we'll
be adding yet another degree of complexity to this series, already quite complex.

Can you test the the patch series v9 on your side Kishon?

Regards,
Gustavo

>
> Thanks
> Kishon
>