Re: [dpdk-dev] [PATCH v3] igb_uio: MSI IRQ mode, irq enable/disable refactored

[Ferruh Yigit <ferruh.yigit@intel.com>]

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;
> +}

<...>