diff mbox series

[v7,7/7] igb_uio: fix uio release issue when hot unplug

Message ID 1531137666-10351-8-git-send-email-jia.guo@intel.com (mailing list archive)
State Superseded, archived
Headers show
Series [v7,1/7] bus: add hotplug failure handler | expand

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Guo, Jia July 9, 2018, 12:01 p.m. UTC
When hotplug out 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
hotplug 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 hotplug out 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@intel.com>
---
 kernel/linux/igb_uio/igb_uio.c | 51 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 3 deletions(-)

Comments

Stephen Hemminger July 9, 2018, 10:44 p.m. UTC | #1
On Mon,  9 Jul 2018 20:01:06 +0800
Jeff Guo <jia.guo@intel.com> wrote:

> @@ -195,12 +205,22 @@ 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;
>  
> +	mutex_lock(&udev->lock);
> +	/* 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;
> +	}
> +	mutex_unlock(&udev->lock);

Did you run with lockdep?
I don't think you can safely acquire a mutex (would sleep) in IRQ context.
Guo, Jia July 10, 2018, 8:28 a.m. UTC | #2
hi, stephen

Thanks for your review.

On 7/10/2018 6:44 AM, Stephen Hemminger wrote:
> On Mon,  9 Jul 2018 20:01:06 +0800
> Jeff Guo <jia.guo@intel.com> wrote:
>
>> @@ -195,12 +205,22 @@ 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;
>>   
>> +	mutex_lock(&udev->lock);
>> +	/* 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;
>> +	}
>> +	mutex_unlock(&udev->lock);
> Did you run with lockdep?
> I don't think you can safely acquire a mutex (would sleep) in IRQ context.
I think lockdep could do me a favor about that,  but i think only the 
uio remove function will check the unplug status, so i think i could let 
this
check in the uio remove function, no need to let it in irq handler 
anymore, since like what you said acquire a mutex in IRQ context is no safe.
diff mbox series

Patch

diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
index 3398eac..adc8cea 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 int wc_activate;
@@ -195,12 +205,22 @@  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;
 
+	mutex_lock(&udev->lock);
+	/* 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;
+	}
+	mutex_unlock(&udev->lock);
+
 	uio_event_notify(info);
 
 	/* Message signal mode, no share IRQ and automasked */
@@ -309,7 +329,6 @@  igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev)
 #endif
 }
 
-
 /**
  * This gets called while opening uio device file.
  */
@@ -331,20 +350,29 @@  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);
+		mutex_unlock(&udev->lock);
 		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);
@@ -356,7 +384,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;
 }
@@ -562,6 +590,9 @@  igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 			 (unsigned long long)map_dma_addr, map_addr);
 	}
 
+	mutex_lock(&udev->lock);
+	udev->state = RTE_UDEV_PROBED;
+	mutex_unlock(&udev->lock);
 	return 0;
 
 fail_remove_group:
@@ -579,6 +610,20 @@  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;
+		mutex_lock(&udev->lock);
+		udev->state = RTE_UDEV_REMOVED;
+		mutex_unlock(&udev->lock);
+		return;
+	}
 
 	mutex_destroy(&udev->lock);
 	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);