[dpdk-dev] [PATCH] igb_uio: revert open and release operations

Wu, Jingjing jingjing.wu at intel.com
Wed Oct 18 02:14:47 CEST 2017


Hi, Ferruh

This one is generic, we can keep it.

Commit: 8cacf78469a7 ("net/i40e: fix VF initialization error")

Thanks
Jingjing

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, October 18, 2017 4:15 AM
> To: Thomas Monjalon <thomas at monjalon.net>; Yigit, Ferruh
> <ferruh.yigit at intel.com>
> Cc: dev at dpdk.org; Tan, Jianfeng <jianfeng.tan at intel.com>; Wu, Jingjing
> <jingjing.wu at intel.com>; Shijith Thotton
> <shijith.thotton at caviumnetworks.com>; Gregory Etelson <gregory at weka.io>;
> Harish Patil <harish.patil at cavium.com>; George Prekas
> <george.prekas at epfl.ch>; stable at dpdk.org
> Subject: [PATCH] igb_uio: revert open and release operations
> 
> This reverts commit 6b9ed026a8704b9e5ee5da7997617ef7cc82e114.
> This reverts commit 5f6ff30dc5075c49069d684bab229aef7ff0fdc3.
> This reverts commit b58eedfc7dd57eef6d12e2c654a52c834f36084a.
> 
> There were bug reports about terminated application may leave device in
> undesired state:
> http://dpdk.org/ml/archives/dev/2016-November/049745.html
> http://dpdk.org/ml/archives/dev/2016-November/050932.html
> 
> And a proposal to fix:
> http://dpdk.org/ml/archives/dev/2016-December/051844.html
> 
> Later another proposal triggered the discussion:
> http://dpdk.org/ml/archives/dev/2017-May/066317.html
> 
> Finally a fix patch pushed into v17.08:
> Commit: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device
> file")
> 
> Later a regression report sent related to the pushed patch:
> http://dpdk.org/ml/archives/dev/2017-September/075236.html
> 
> And a fix for regression integrated into v17.11-rc1:
> http://dpdk.org/ml/archives/dev/2017-October/079166.html
> Commit: 5f6ff30dc507 ("igb_uio: fix interrupt enablement after FLR in VM")
> Commit: 6b9ed026a870 ("igb_uio: fix build with kernel <= 3.17")
> 
> Even after the fix qede PMD reported to be broken:
> http://dpdk.org/ml/archives/dev/2017-October/079359.html
> 
> So this patch reverts original fix and related commits. The related igb_uio code
> part turns back to v17.05 base.
> 
> Cc: Jianfeng Tan <jianfeng.tan at intel.com>
> Cc: Jingjing Wu <jingjing.wu at intel.com>
> Cc: Shijith Thotton <shijith.thotton at caviumnetworks.com>
> Cc: Gregory Etelson <gregory at weka.io>
> Cc: Harish Patil <harish.patil at cavium.com>
> Cc: George Prekas <george.prekas at epfl.ch>
> 
> Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")
> Cc: stable at dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit at intel.com>
> ---
> It would be nice to solve this issue in LTS release, but being close to the release
> and the error report without details makes it hard to work more on this issue.
> 
> Thanks everyone who spent effort for this, hopefully we can continue to work
> on next release cycle.
> 
> Jingjing, there is a i40e commit, was part of igb_uio fix patchset, is it generic,
> or needs to be reverted with this patch?
> Commit: 8cacf78469a7 ("net/i40e: fix VF initialization error")
> ---
>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 201 +++++++++++-------------------
>  1 file changed, 76 insertions(+), 125 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> index f7ef82554..786df68a2 100644
> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> @@ -49,6 +49,7 @@ struct rte_uio_pci_dev {
> 
>  static char *intr_mode;
>  static enum rte_intr_mode igbuio_intr_mode_preferred =
> RTE_INTR_MODE_MSIX;
> +
>  /* sriov sysfs */
>  static ssize_t
>  show_max_vfs(struct device *dev, struct device_attribute *attr, @@ -207,22
> +208,80 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
>   * If yes, disable it here and will be enable later.
>   */
>  static irqreturn_t
> -igbuio_pci_irqhandler(int irq, void *dev_id)
> +igbuio_pci_irqhandler(int irq, struct uio_info *info)
>  {
> -	struct rte_uio_pci_dev *udev = (struct rte_uio_pci_dev *)dev_id;
> -	struct uio_info *info = &udev->info;
> +	struct rte_uio_pci_dev *udev = info->priv;
> 
>  	/* Legacy mode need to mask in hardware */
>  	if (udev->mode == RTE_INTR_MODE_LEGACY &&
>  	    !pci_check_and_mask_intx(udev->pdev))
>  		return IRQ_NONE;
> 
> -	uio_event_notify(info);
> -
>  	/* Message signal mode, no share IRQ and automasked */
>  	return IRQ_HANDLED;
>  }
> 
> +/* Remap pci resources described by bar #pci_bar in uio resource n. */
> +static int igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info
> +*info,
> +		       int n, int pci_bar, const char *name) {
> +	unsigned long addr, len;
> +	void *internal_addr;
> +
> +	if (n >= ARRAY_SIZE(info->mem))
> +		return -EINVAL;
> +
> +	addr = pci_resource_start(dev, pci_bar);
> +	len = pci_resource_len(dev, pci_bar);
> +	if (addr == 0 || len == 0)
> +		return -1;
> +	internal_addr = ioremap(addr, len);
> +	if (internal_addr == NULL)
> +		return -1;
> +	info->mem[n].name = name;
> +	info->mem[n].addr = addr;
> +	info->mem[n].internal_addr = internal_addr;
> +	info->mem[n].size = len;
> +	info->mem[n].memtype = UIO_MEM_PHYS;
> +	return 0;
> +}
> +
> +/* Get pci port io resources described by bar #pci_bar in uio resource
> +n. */ static int igbuio_pci_setup_ioport(struct pci_dev *dev, struct
> +uio_info *info,
> +		int n, int pci_bar, const char *name) {
> +	unsigned long addr, len;
> +
> +	if (n >= ARRAY_SIZE(info->port))
> +		return -EINVAL;
> +
> +	addr = pci_resource_start(dev, pci_bar);
> +	len = pci_resource_len(dev, pci_bar);
> +	if (addr == 0 || len == 0)
> +		return -EINVAL;
> +
> +	info->port[n].name = name;
> +	info->port[n].start = addr;
> +	info->port[n].size = len;
> +	info->port[n].porttype = UIO_PORT_X86;
> +
> +	return 0;
> +}
> +
> +/* Unmap previously ioremap'd resources */ static void
> +igbuio_pci_release_iomem(struct uio_info *info) {
> +	int i;
> +
> +	for (i = 0; i < MAX_UIO_MAPS; i++) {
> +		if (info->mem[i].internal_addr)
> +			iounmap(info->mem[i].internal_addr);
> +	}
> +}
> +
>  static int
>  igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)  { @@ -252,7
> +311,6 @@ igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
>  			break;
>  		}
>  #endif
> -
>  	/* fall back to MSI */
>  	case RTE_INTR_MODE_MSI:
>  #ifndef HAVE_ALLOC_IRQ_VECTORS
> @@ -291,28 +349,15 @@ igbuio_pci_enable_interrupts(struct rte_uio_pci_dev
> *udev)
>  	default:
>  		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
>  			igbuio_intr_mode_preferred);
> -		udev->info.irq = UIO_IRQ_NONE;
>  		err = -EINVAL;
>  	}
> 
> -	if (udev->info.irq != UIO_IRQ_NONE)
> -		err = request_irq(udev->info.irq, igbuio_pci_irqhandler,
> -				  udev->info.irq_flags, udev->info.name,
> -				  udev);
> -	dev_info(&udev->pdev->dev, "uio device registered with irq %lx\n",
> -		 udev->info.irq);
> -
>  	return err;
>  }
> 
>  static void
>  igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev)  {
> -	if (udev->info.irq) {
> -		free_irq(udev->info.irq, udev);
> -		udev->info.irq = 0;
> -	}
> -
>  #ifndef HAVE_ALLOC_IRQ_VECTORS
>  	if (udev->mode == RTE_INTR_MODE_MSIX)
>  		pci_disable_msix(udev->pdev);
> @@ -325,109 +370,6 @@ igbuio_pci_disable_interrupts(struct rte_uio_pci_dev
> *udev)  #endif  }
> 
> -
> -/**
> - * This gets called while opening uio device file.
> - */
> -static int
> -igbuio_pci_open(struct uio_info *info, struct inode *inode) -{
> -	struct rte_uio_pci_dev *udev = info->priv;
> -	struct pci_dev *dev = udev->pdev;
> -	int err;
> -
> -	pci_reset_function(dev);
> -
> -	/* set bus master, which was cleared by the reset function */
> -	pci_set_master(dev);
> -
> -	/* enable interrupts */
> -	err = igbuio_pci_enable_interrupts(udev);
> -	if (err) {
> -		dev_err(&dev->dev, "Enable interrupt fails\n");
> -		return err;
> -	}
> -	return 0;
> -}
> -
> -static int
> -igbuio_pci_release(struct uio_info *info, struct inode *inode) -{
> -	struct rte_uio_pci_dev *udev = info->priv;
> -	struct pci_dev *dev = udev->pdev;
> -
> -	/* disable interrupts */
> -	igbuio_pci_disable_interrupts(udev);
> -
> -	/* stop the device from further DMA */
> -	pci_clear_master(dev);
> -
> -	pci_reset_function(dev);
> -
> -	return 0;
> -}
> -
> -/* Remap pci resources described by bar #pci_bar in uio resource n. */ -static
> int -igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
> -		       int n, int pci_bar, const char *name)
> -{
> -	unsigned long addr, len;
> -	void *internal_addr;
> -
> -	if (n >= ARRAY_SIZE(info->mem))
> -		return -EINVAL;
> -
> -	addr = pci_resource_start(dev, pci_bar);
> -	len = pci_resource_len(dev, pci_bar);
> -	if (addr == 0 || len == 0)
> -		return -1;
> -	internal_addr = ioremap(addr, len);
> -	if (internal_addr == NULL)
> -		return -1;
> -	info->mem[n].name = name;
> -	info->mem[n].addr = addr;
> -	info->mem[n].internal_addr = internal_addr;
> -	info->mem[n].size = len;
> -	info->mem[n].memtype = UIO_MEM_PHYS;
> -	return 0;
> -}
> -
> -/* Get pci port io resources described by bar #pci_bar in uio resource n. */ -
> static int -igbuio_pci_setup_ioport(struct pci_dev *dev, struct uio_info *info,
> -		int n, int pci_bar, const char *name)
> -{
> -	unsigned long addr, len;
> -
> -	if (n >= ARRAY_SIZE(info->port))
> -		return -EINVAL;
> -
> -	addr = pci_resource_start(dev, pci_bar);
> -	len = pci_resource_len(dev, pci_bar);
> -	if (addr == 0 || len == 0)
> -		return -EINVAL;
> -
> -	info->port[n].name = name;
> -	info->port[n].start = addr;
> -	info->port[n].size = len;
> -	info->port[n].porttype = UIO_PORT_X86;
> -
> -	return 0;
> -}
> -
> -/* Unmap previously ioremap'd resources */ -static void -
> igbuio_pci_release_iomem(struct uio_info *info) -{
> -	int i;
> -
> -	for (i = 0; i < MAX_UIO_MAPS; i++) {
> -		if (info->mem[i].internal_addr)
> -			iounmap(info->mem[i].internal_addr);
> -	}
> -}
> -
>  static int
>  igbuio_setup_bars(struct pci_dev *dev, struct uio_info *info)  { @@ -518,16
> +460,19 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id
> *id)
>  	/* fill uio infos */
>  	udev->info.name = "igb_uio";
>  	udev->info.version = "0.1";
> +	udev->info.handler = igbuio_pci_irqhandler;
>  	udev->info.irqcontrol = igbuio_pci_irqcontrol;
> -	udev->info.open = igbuio_pci_open;
> -	udev->info.release = igbuio_pci_release;
>  	udev->info.priv = udev;
>  	udev->pdev = dev;
> 
> -	err = sysfs_create_group(&dev->dev.kobj, &dev_attr_grp);
> +	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)
> +		goto fail_disable_interrupts;
> +
>  	/* register uio driver */
>  	err = uio_register_device(&dev->dev, &udev->info);
>  	if (err != 0)
> @@ -535,6 +480,9 @@ igbuio_pci_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
> 
>  	pci_set_drvdata(dev, udev);
> 
> +	dev_info(&dev->dev, "uio device registered with irq %lx\n",
> +		 udev->info.irq);
> +
>  	/*
>  	 * Doing a harmless dma mapping for attaching the device to
>  	 * the iommu identity mapping if kernel boots with iommu=pt.
> @@ -560,6 +508,8 @@ igbuio_pci_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
> 
>  fail_remove_group:
>  	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
> +fail_disable_interrupts:
> +	igbuio_pci_disable_interrupts(udev);
>  fail_release_iomem:
>  	igbuio_pci_release_iomem(&udev->info);
>  	pci_disable_device(dev);
> @@ -576,6 +526,7 @@ igbuio_pci_remove(struct pci_dev *dev)
> 
>  	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
>  	uio_unregister_device(&udev->info);
> +	igbuio_pci_disable_interrupts(udev);
>  	igbuio_pci_release_iomem(&udev->info);
>  	pci_disable_device(dev);
>  	pci_set_drvdata(dev, NULL);
> --
> 2.13.6



More information about the dev mailing list