From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Markus Theil <markus.theil@tu-ilmenau.de>, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3] igb_uio: MSI IRQ mode, irq enable/disable refactored
Date: Wed, 30 Aug 2017 17:32:50 +0100 [thread overview]
Message-ID: <231558aa-0a93-bdf7-30a5-dc547212e4e9@intel.com> (raw)
In-Reply-To: <1503408514-20079-1-git-send-email-markus.theil@tu-ilmenau.de>
On 8/22/2017 2:28 PM, Markus Theil wrote:
> This patch adds MSI IRQ mode and in a way, that should
> also work on older kernel versions. The base for my patch
> was an attempt to do this in cf705bc36c which was later reverted in
> d8ee82745a. Compilation was tested on Linux 3.2, 4.10 and 4.12.
>
> MSI(X) setup was already using pci_alloc_irq_vectors before,
> but calls to pci_free_irq_vectors were missing and added.
Overall looks good to me, would you mind splitting this into multiple
patches:
1- Create igbuio_pci_enable_interrupts() and
igbuio_pci_disable_interrupts() functions without changing anything
2- Add pci_free_irq_vectors()
3- add MSI support
>
> Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
<...>
> +/*
> + * It masks the msi on/off of generating MSI messages.
> + */
> +static void
> +igbuio_msi_mask_irq(struct pci_dev *pdev, struct msi_desc *desc, int32_t state> +{
> + u32 mask_bits = desc->masked;
> + u32 offset = desc->irq - pdev->irq;
> + u32 mask = 1 << offset;
> + u32 flag = !!state << offset;
> +
> + if (!desc->msi_attrib.maskbit)
> + return;
> +
> + mask_bits &= ~mask;
> + mask_bits |= flag;
> +
> + if (mask_bits != desc->masked) {
> + pci_write_config_dword(pdev, desc->mask_pos, mask_bits);
> + desc->masked = mask_bits;
> + }
I am not familiar with details of this function, is there a sample usage
in kernel that can help understanding it?
> +}
> +
> /**
> * This is the irqcontrol callback to be registered to uio_info.
> * It can be used to disable/enable interrupt from user space processes.
> @@ -146,6 +170,16 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
> list_for_each_entry(desc, &pdev->dev.msi_list, list)
> igbuio_msix_mask_irq(desc, irq_state);
> #endif
> + } else if (udev->mode == RTE_INTR_MODE_MSI) {
> + struct msi_desc *desc;
Since two if block is using this, I think this can be moved to function
scope.
> +
> +#if (LINUX_VERSION_CODE < KERNEL_VERSION(4, 3, 0))
Can you please move version check to the compat.h? I am aware this
version check is there for msix, but since we are doubling it, it can be
good time to fix both.
> + list_for_each_entry(desc, &pdev->msi_list, list)> + igbuio_msi_mask_irq(pdev, desc, irq_state);
This walks on same list (msi_list) with above msix code. Is there a way
to say if this msi_desc for MSI or MSIx interrupts, how
igbuio_msi_mask_irq() will distinguish them?
> +#else
> + list_for_each_entry(desc, &pdev->dev.msi_list, list)
> + igbuio_msi_mask_irq(pdev, desc, irq_state);
> +#endif
> }
> pci_cfg_access_unlock(pdev);
>
> @@ -309,6 +343,91 @@ igbuio_pci_release_iomem(struct uio_info *info)
> }
>
> static int
> +igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
> +{
> + int err = 0;
> +#ifdef HAVE_PCI_ENABLE_MSIX
> + struct msix_entry msix_entry;
> +#endif
> +
> + switch (igbuio_intr_mode_preferred) {
> + case RTE_INTR_MODE_MSIX:
> + /* Only 1 msi-x vector needed */
> +#ifdef HAVE_PCI_ENABLE_MSIX
> + msix_entry.entry = 0;
> + if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
> + dev_dbg(&udev->pdev->dev, "using MSI-X");
> + udev->info.irq_flags = IRQF_NO_THREAD;
> + udev->info.irq = msix_entry.vector;
> + udev->mode = RTE_INTR_MODE_MSIX;
> + break;
> + }
> +#else
> + if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1) {
> + dev_dbg(&udev->pdev->dev, "using MSI-X");
> + udev->info.irq = pci_irq_vector(udev->pdev, 0);
> + udev->mode = RTE_INTR_MODE_MSIX;
> + break;
> + }
> +#endif
Please add fall back comment: /* fall back to MSI */
And related this fallback, previously flow was:
Try MSIX, if fails try legacy.
Now it is:
Try MSIX, if fails try MSI, if fails try legacy.
Do you think, does this new step included may break anyone's out that
would expect to fallback to legacy? (although I don't expect, I believe
good to sound it)
> + case RTE_INTR_MODE_MSI:
> +#ifdef HAVE_PCI_ENABLE_MSI
> + if (pci_enable_msi(udev->pdev) == 0) {
> + dev_dbg(&udev->pdev->dev, "using MSI");
> + udev->info.irq_flags = IRQF_NO_THREAD;
> + udev->info.irq = udev->pdev->irq;
> + udev->mode = RTE_INTR_MODE_MSI;
> + break;
> + }
> +#else
> + if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1) {
> + dev_dbg(&udev->pdev->dev, "using MSI");
> + udev->info.irq = pci_irq_vector(udev->pdev, 0);
> + udev->mode = RTE_INTR_MODE_MSI;
> + break;
> + }
> +#endif
> + /* fall back to INTX */
> + case RTE_INTR_MODE_LEGACY:
> + if (pci_intx_mask_supported(udev->pdev)) {
> + dev_dbg(&udev->pdev->dev, "using INTX");
> + udev->info.irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
> + udev->info.irq = udev->pdev->irq;
> + udev->mode = RTE_INTR_MODE_LEGACY;
> + break;
> + }
> + dev_notice(&udev->pdev->dev, "PCI INTX mask not supported\n");
> + /* fall back to no IRQ */
> + case RTE_INTR_MODE_NONE:
> + udev->mode = RTE_INTR_MODE_NONE;
> + udev->info.irq = 0;
> + break;
> +
> + default:
> + dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
> + igbuio_intr_mode_preferred);
> + err = -EINVAL;
> + }
> +
> + return err;
> +}
<...>
next prev parent reply other threads:[~2017-08-30 16:32 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-21 17:33 [dpdk-dev] [PATCH] " Markus Theil
2017-08-21 17:51 ` [dpdk-dev] [PATCH v2 1/2] " Markus Theil
2017-08-21 17:51 ` [dpdk-dev] [PATCH v2 2/2] igb_uio: conform to coding conventions Markus Theil
2017-08-22 13:28 ` [dpdk-dev] [PATCH v3] igb_uio: MSI IRQ mode, irq enable/disable refactored Markus Theil
2017-08-30 16:32 ` Ferruh Yigit [this message]
2017-08-31 22:05 ` Markus Theil
2017-08-31 10:22 ` [dpdk-dev] [PATCH v4 1/3] igb_uio: refactor irq enable/disable into own functions Markus Theil
2017-08-31 10:22 ` [dpdk-dev] [PATCH v4 2/3] igb_uio: fix irq disable on recent kernels Markus Theil
2017-08-31 10:22 ` [dpdk-dev] [PATCH v4 3/3] igb_uio: MSI IRQ mode Markus Theil
2017-08-31 15:32 ` Stephen Hemminger
2017-08-31 22:07 ` Markus Theil
2017-08-31 21:46 ` [dpdk-dev] [PATCH v5 1/5] igb_uio: refactor irq enable/disable into own functions Markus Theil
2017-08-31 21:46 ` [dpdk-dev] [PATCH v5 2/5] igb_uio: fix irq disable on recent kernels Markus Theil
2017-08-31 21:46 ` [dpdk-dev] [PATCH v5 3/5] igb_uio: MSI IRQ mode Markus Theil
2017-08-31 21:46 ` [dpdk-dev] [PATCH v5 4/5] igb_uio: use msi mask functions from kernel, little corrections Markus Theil
2017-09-01 15:40 ` Stephen Hemminger
2017-09-04 10:03 ` Burakov, Anatoly
2017-09-04 12:43 ` Ferruh Yigit
2017-09-04 15:16 ` Markus Theil
2017-09-04 16:55 ` Ferruh Yigit
2017-08-31 21:47 ` [dpdk-dev] [PATCH v5 5/5] igb_uio: release in exact reverse order Markus Theil
2017-08-22 16:55 ` [dpdk-dev] [PATCH] igb_uio: MSI IRQ mode, irq enable/disable refactored Stephen Hemminger
2017-08-22 18:19 ` Markus Theil
2017-08-23 8:47 ` Bruce Richardson
2017-08-23 8:51 ` Bruce Richardson
2017-09-04 18:17 ` [dpdk-dev] [PATCH v6 1/5] igb_uio: refactor irq enable/disable into own functions Markus Theil
2017-09-04 18:17 ` [dpdk-dev] [PATCH v6 2/5] igb_uio: fix irq disable on recent kernels Markus Theil
2017-09-04 21:32 ` Ferruh Yigit
2017-09-04 18:17 ` [dpdk-dev] [PATCH v6 3/5] igb_uio: fix MSI-X IRQ assignment with new IRQ function Markus Theil
2017-09-04 21:34 ` Ferruh Yigit
2017-09-04 18:17 ` [dpdk-dev] [PATCH v6 4/5] igb_uio: release in exact reverse order Markus Theil
2017-09-04 18:17 ` [dpdk-dev] [PATCH v6 5/5] igb_uio: MSI IRQ mode, use kernel functions for masking MSI and MSI-X Markus Theil
2017-09-04 21:38 ` Ferruh Yigit
2017-09-05 12:04 ` [dpdk-dev] [PATCH v7 1/6] igb_uio: refactor irq enable/disable into own functions Markus Theil
2017-09-05 12:04 ` [dpdk-dev] [PATCH v7 2/6] igb_uio: fix irq disable on recent kernels Markus Theil
2017-09-05 12:04 ` [dpdk-dev] [PATCH v7 3/6] igb_uio: fix MSI-X IRQ assignment with new IRQ function Markus Theil
2017-09-11 17:56 ` Ferruh Yigit
2017-09-11 22:04 ` Stephen Hemminger
2017-09-12 8:14 ` Bruce Richardson
2017-09-12 15:01 ` Stephen Hemminger
2017-09-12 8:16 ` Bruce Richardson
2017-09-12 16:31 ` Kevin Traynor
2017-09-05 12:04 ` [dpdk-dev] [PATCH v7 4/6] igb_uio: release in exact reverse order Markus Theil
2017-09-05 12:04 ` [dpdk-dev] [PATCH v7 5/6] igb_uio: use kernel functions for masking MSI-X Markus Theil
2017-10-09 21:56 ` Patrick MacArthur
2017-10-12 17:04 ` Ferruh Yigit
2017-10-13 2:49 ` Ferruh Yigit
2017-10-13 8:28 ` Kavanagh, Mark B
2017-10-12 10:50 ` Kavanagh, Mark B
2017-10-12 20:58 ` Ferruh Yigit
2017-10-12 21:15 ` Stephen Hemminger
2017-10-13 0:28 ` Ferruh Yigit
2017-09-05 12:04 ` [dpdk-dev] [PATCH v7 6/6] igb_uio: MSI IRQ mode Markus Theil
2017-09-11 17:48 ` [dpdk-dev] [PATCH v7 1/6] igb_uio: refactor irq enable/disable into own functions Ferruh Yigit
2017-09-14 12:04 ` Markus Theil
2017-10-06 22:05 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=231558aa-0a93-bdf7-30a5-dc547212e4e9@intel.com \
--to=ferruh.yigit@intel.com \
--cc=dev@dpdk.org \
--cc=markus.theil@tu-ilmenau.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).