From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH v3] igb_uio: MSI IRQ mode, irq enable/disable refactored Date: Wed, 30 Aug 2017 17:32:50 +0100 Message-ID: <231558aa-0a93-bdf7-30a5-dc547212e4e9@intel.com> References: <1503337872-14325-2-git-send-email-markus.theil@tu-ilmenau.de> <1503408514-20079-1-git-send-email-markus.theil@tu-ilmenau.de> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit To: Markus Theil , dev@dpdk.org Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id AE1E1DE0 for ; Wed, 30 Aug 2017 18:32:53 +0200 (CEST) In-Reply-To: <1503408514-20079-1-git-send-email-markus.theil@tu-ilmenau.de> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 <...> > +/* > + * 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; > +} <...>