[dpdk-dev] [PATCH V4 7/9] igb_uio: fix uio release issue when hot unplug

[Ferruh Yigit]

On 6/29/2018 11:30 AM, Jeff Guo wrote:
> When hot unplug device, the kernel will release the device resource in the
> kernel side, such as the fd sys file will disappear, and the irq will be
> released. At this time, if igb uio driver still try to release this
> resource, it will cause kernel crash. On the other hand, something like
> interrupt disabling do not automatically process in kernel side. If not
> handler it, this redundancy and dirty thing will affect the interrupt
> resource be used by other device. So the igb_uio driver have to check the
> hot plug status, and the corresponding process should be taken in igb uio
> driver.
> 
> This patch propose to add structure of rte_udev_state into rte_uio_pci_dev
> of igb_uio kernel driver, which will record the state of uio device, such
> as probed/opened/released/removed/unplug. When detect the unexpected
> removal which cause of hot unplug behavior, it will corresponding disable
> interrupt resource, while for the part of releasement which kernel have
> already handle, just skip it to avoid double free or null pointer kernel
> crash issue.
> 
> Signed-off-by: Jeff Guo <jia.guo at intel.com>
> ---
> v4->v3:
> no change
> ---
>  kernel/linux/igb_uio/igb_uio.c | 50 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
> index b3233f1..d301302 100644
> --- a/kernel/linux/igb_uio/igb_uio.c
> +++ b/kernel/linux/igb_uio/igb_uio.c
> @@ -19,6 +19,15 @@
>  
>  #include "compat.h"
>  
> +/* uio pci device state */
> +enum rte_udev_state {
> +	RTE_UDEV_PROBED,
> +	RTE_UDEV_OPENNED,
> +	RTE_UDEV_RELEASED,
> +	RTE_UDEV_REMOVED,
> +	RTE_UDEV_UNPLUG
> +};
> +
>  /**
>   * A structure describing the private information for a uio device.
>   */
> @@ -28,6 +37,7 @@ struct rte_uio_pci_dev {
>  	enum rte_intr_mode mode;
>  	struct mutex lock;
>  	int refcnt;
> +	enum rte_udev_state state;
>  };
>  
>  static char *intr_mode;
> @@ -194,12 +204,20 @@ igbuio_pci_irqhandler(int irq, void *dev_id)
>  {
>  	struct rte_uio_pci_dev *udev = (struct rte_uio_pci_dev *)dev_id;
>  	struct uio_info *info = &udev->info;
> +	struct pci_dev *pdev = udev->pdev;
>  
>  	/* Legacy mode need to mask in hardware */
>  	if (udev->mode == RTE_INTR_MODE_LEGACY &&
>  	    !pci_check_and_mask_intx(udev->pdev))
>  		return IRQ_NONE;
>  
> +	/* check the uevent of the kobj */
> +	if ((&pdev->dev.kobj)->state_remove_uevent_sent == 1) {
> +		dev_notice(&pdev->dev, "device:%s, sent remove uevent!\n",
> +			   (&pdev->dev.kobj)->name);
> +		udev->state = RTE_UDEV_UNPLUG;
> +	}

I guess commit log says kernel can remove device, if so do we need any locking
before accessing dev?

> +
>  	uio_event_notify(info);
>  
>  	/* Message signal mode, no share IRQ and automasked */
> @@ -308,7 +326,6 @@ igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev)
>  #endif
>  }
>  
> -
>  /**
>   * This gets called while opening uio device file.
>   */
> @@ -330,24 +347,33 @@ igbuio_pci_open(struct uio_info *info, struct inode *inode)
>  
>  	/* enable interrupts */
>  	err = igbuio_pci_enable_interrupts(udev);
> -	mutex_unlock(&udev->lock);
>  	if (err) {
>  		dev_err(&dev->dev, "Enable interrupt fails\n");
> +		pci_clear_master(dev);
>  		return err;
>  	}
> +	udev->state = RTE_UDEV_OPENNED;
> +	mutex_unlock(&udev->lock);
>  	return 0;
>  }
>  
> +/**
> + * This gets called while closing uio device file.
> + */
>  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;
>  
> +	if (udev->state == RTE_UDEV_REMOVED)
> +		return 0;
> +
>  	mutex_lock(&udev->lock);
>  	if (--udev->refcnt > 0) {
>  		mutex_unlock(&udev->lock);
> -		return 0;
> +		return -1;
>  	}
>  
>  	/* disable interrupts */
> @@ -355,7 +381,7 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode)
>  
>  	/* stop the device from further DMA */
>  	pci_clear_master(dev);
> -
> +	udev->state = RTE_UDEV_RELEASED;
>  	mutex_unlock(&udev->lock);
>  	return 0;
>  }
> @@ -557,6 +583,7 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  			 (unsigned long long)map_dma_addr, map_addr);
>  	}
>  
> +	udev->state = RTE_UDEV_PROBED;
>  	return 0;
>  
>  fail_remove_group:
> @@ -573,11 +600,24 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  static void
>  igbuio_pci_remove(struct pci_dev *dev)
>  {
> +
>  	struct rte_uio_pci_dev *udev = pci_get_drvdata(dev);
> +	int ret;
> +
> +	/* handler hot unplug */
> +	if (udev->state == RTE_UDEV_OPENNED ||
> +		udev->state == RTE_UDEV_UNPLUG) {
> +		dev_notice(&dev->dev, "Unexpected removal!\n");
> +		ret = igbuio_pci_release(&udev->info, NULL);
> +		if (ret)
> +			return;
> +		udev->state = RTE_UDEV_REMOVED;
> +		return;
> +	}
>  
>  	mutex_destroy(&udev->lock);
> -	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
>  	uio_unregister_device(&udev->info);
> +	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
>  	igbuio_pci_release_iomem(&udev->info);
>  	pci_disable_device(dev);
>  	pci_set_drvdata(dev, NULL);
>