[dpdk-dev,v3] igb_uio: MSI IRQ mode, irq enable/disable refactored
Checks
Commit Message
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.
Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
lib/librte_eal/linuxapp/igb_uio/compat.h | 9 +-
lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 175 ++++++++++++++++++++++--------
2 files changed, 135 insertions(+), 49 deletions(-)
Comments
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;
> +}
<...>
On 30.08.2017 18:32, Ferruh Yigit wrote:
> 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)
>
I also don't expect it, but can't say it with 100% certainty of course.
@@ -125,5 +125,12 @@ static bool pci_check_and_mask_intx(struct pci_dev *pdev)
#endif /* < 3.3.0 */
#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 8, 0)
-#define HAVE_PCI_ENABLE_MSIX
+
+#define HAVE_PCI_ENABLE_MSIX 1
+#define HAVE_PCI_ENABLE_MSI 1
+
+#else
+
+#define HAVE_ALLOC_IRQ_VECTORS 1
+
#endif
@@ -91,6 +91,7 @@ static struct attribute *dev_attrs[] = {
static const struct attribute_group dev_attr_grp = {
.attrs = dev_attrs,
};
+
/*
* It masks the msix on/off of generating MSI-X messages.
*/
@@ -113,6 +114,29 @@ igbuio_msix_mask_irq(struct msi_desc *desc, int32_t state)
}
}
+/*
+ * 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;
+ }
+}
+
/**
* 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;
+
+#if (LINUX_VERSION_CODE < KERNEL_VERSION(4, 3, 0))
+ list_for_each_entry(desc, &pdev->msi_list, list)
+ igbuio_msi_mask_irq(pdev, desc, irq_state);
+#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
+ 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;
+}
+
+static void
+igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev)
+{
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+ if (udev->mode == RTE_INTR_MODE_MSIX)
+ pci_disable_msix(udev->pdev);
+ if (udev->mode == RTE_INTR_MODE_MSI)
+ pci_disable_msi(udev->pdev);
+#else
+ if (udev->mode == RTE_INTR_MODE_MSIX ||
+ udev->mode == RTE_INTR_MODE_MSI)
+ pci_free_irq_vectors(udev->pdev);
+#endif
+}
+
+static int
igbuio_setup_bars(struct pci_dev *dev, struct uio_info *info)
{
int i, iom, iop, ret;
@@ -356,9 +475,6 @@ static int
igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
struct rte_uio_pci_dev *udev;
-#ifdef HAVE_PCI_ENABLE_MSIX
- struct msix_entry msix_entry;
-#endif
dma_addr_t map_dma_addr;
void *map_addr;
int err;
@@ -413,48 +529,9 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
udev->info.priv = udev;
udev->pdev = dev;
- 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(dev, &msix_entry, 1) == 0) {
- dev_dbg(&dev->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(dev, 1, 1, PCI_IRQ_MSIX) == 1) {
- dev_dbg(&dev->dev, "using MSI-X");
- udev->info.irq = pci_irq_vector(dev, 0);
- udev->mode = RTE_INTR_MODE_MSIX;
- break;
- }
-#endif
- /* fall back to INTX */
- case RTE_INTR_MODE_LEGACY:
- if (pci_intx_mask_supported(dev)) {
- dev_dbg(&dev->dev, "using INTX");
- udev->info.irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
- udev->info.irq = dev->irq;
- udev->mode = RTE_INTR_MODE_LEGACY;
- break;
- }
- dev_notice(&dev->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(&dev->dev, "invalid IRQ mode %u",
- igbuio_intr_mode_preferred);
- err = -EINVAL;
+ err = igbuio_pci_enable_interrupts(udev);
+ if (err != 0)
goto fail_release_iomem;
- }
err = sysfs_create_group(&dev->dev.kobj, &dev_attr_grp);
if (err != 0)
@@ -497,8 +574,7 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
fail_release_iomem:
igbuio_pci_release_iomem(&udev->info);
- if (udev->mode == RTE_INTR_MODE_MSIX)
- pci_disable_msix(udev->pdev);
+ igbuio_pci_disable_interrupts(udev);
pci_disable_device(dev);
fail_free:
kfree(udev);
@@ -514,8 +590,7 @@ igbuio_pci_remove(struct pci_dev *dev)
sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
uio_unregister_device(&udev->info);
igbuio_pci_release_iomem(&udev->info);
- if (udev->mode == RTE_INTR_MODE_MSIX)
- pci_disable_msix(dev);
+ igbuio_pci_disable_interrupts(udev);
pci_disable_device(dev);
pci_set_drvdata(dev, NULL);
kfree(udev);
@@ -532,6 +607,9 @@ igbuio_config_intr_mode(char *intr_str)
if (!strcmp(intr_str, RTE_INTR_MODE_MSIX_NAME)) {
igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
pr_info("Use MSIX interrupt\n");
+ } else if (!strcmp(intr_str, RTE_INTR_MODE_MSI_NAME)) {
+ igbuio_intr_mode_preferred = RTE_INTR_MODE_MSI;
+ pr_info("Use MSI interrupt\n");
} else if (!strcmp(intr_str, RTE_INTR_MODE_LEGACY_NAME)) {
igbuio_intr_mode_preferred = RTE_INTR_MODE_LEGACY;
pr_info("Use legacy interrupt\n");
@@ -575,6 +653,7 @@ module_param(intr_mode, charp, S_IRUGO);
MODULE_PARM_DESC(intr_mode,
"igb_uio interrupt mode (default=msix):\n"
" " RTE_INTR_MODE_MSIX_NAME " Use MSIX interrupt\n"
+" " RTE_INTR_MODE_MSI_NAME " Use MSI interrupt\n"
" " RTE_INTR_MODE_LEGACY_NAME " Use Legacy interrupt\n"
"\n");