drivers/uio/uio_pdrv_genirq.c: FIx memory & concurrency issues

PATCH New Vitalii Demianets 2 participants 7 messages Dec 10, 2012 – Dec 17, 2012 Userspace I/O (UIO)

Share & tools

Vitalii Demianets

author

2012-12-10 09:18 UTC #0 lore raw

cover drivers/uio/uio_pdrv_genirq.c: FIx memory & concurrency issues

This patch series consists of two patches fixing different issues but in the same file, so the second patch depends on the first:
1. Fix memory freeing issues
2. Fix concurrency issue on SMP systems.

Vitalii Demianets

author

2012-12-10 09:44 UTC #1 lore raw

PATCH v3 1/2 Fix memory freeing issues

1. uioinfo was kfreed based on the presence of pdev->dev.of_node, which was obviously wrong and unrelated to the fact if uioinfo was allocated statically or dynamically. This patch introduces new flag which clearly shows if uioinfo was allocated dynamically and kfrees uioinfo based on that flag;
2. Fix: priv data was not freed in case platform_get_irq() failed. As it was caused mainly by improper exit labels naming, labels were renamed too;
3. The case of uioinfo AND pdev->dev.of_node both NULL (not initialized in platform data) was not treated properly.

Signed-off-by Vitalii Demianets <[email protected]>

---
v2: Constants naming style
v3: Comments: better wording in comment accompanying flags initialization &
removed obsoleted OF comment

---
 drivers/uio/uio_pdrv_genirq.c |   47 ++++++++++++++++++++++++----------------
 1 files changed, 28 insertions(+), 19 deletions(-)

drivers/uio/uio_pdrv_genirq.c +28 ~~-19~~

index 42202cd..cc5233b 100644
@@ -30,6 +30,11 @@
 
 #define DRIVER_NAME "uio_pdrv_genirq"
 
+enum {
+	UIO_IRQ_DISABLED = 0,
+	UIO_INFO_ALLOCED = 1,
+};
+
 struct uio_pdrv_genirq_platdata {
 	struct uio_info *uioinfo;
 	spinlock_t lock;

@@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info)
 	 * remember the state so we can allow user space to enable it later.
 	 */
 
-	if (!test_and_set_bit(0, &priv->flags))
+	if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
 		disable_irq_nosync(irq);
 
 	return IRQ_HANDLED;

@@ -83,10 +88,10 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
 
 	spin_lock_irqsave(&priv->lock, flags);
 	if (irq_on) {
-		if (test_and_clear_bit(0, &priv->flags))
+		if (test_and_clear_bit(UIO_IRQ_DISABLED, &priv->flags))
 			enable_irq(dev_info->irq);
 	} else {
-		if (!test_and_set_bit(0, &priv->flags))
+		if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
 			disable_irq(dev_info->irq);
 	}
 	spin_unlock_irqrestore(&priv->lock, flags);

@@ -101,8 +106,9 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 	struct uio_mem *uiomem;
 	int ret = -EINVAL;
 	int i;
+	bool uioinfo_alloced = false;
 
-	if (!uioinfo) {
+	if (!uioinfo && pdev->dev.of_node) {
 		int irq;
 
 		/* alloc uioinfo for one device */

@@ -110,10 +116,11 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 		if (!uioinfo) {
 			ret = -ENOMEM;
 			dev_err(&pdev->dev, "unable to kmalloc\n");
-			goto bad2;
+			goto out;
 		}
 		uioinfo->name = pdev->dev.of_node->name;
 		uioinfo->version = "devicetree";
+		uioinfo_alloced = true;
 
 		/* Multiple IRQs are not supported */
 		irq = platform_get_irq(pdev, 0);

@@ -125,32 +132,35 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 
 	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
 		dev_err(&pdev->dev, "missing platform_data\n");
-		goto bad0;
+		goto out_uioinfo;
 	}
 
 	if (uioinfo->handler || uioinfo->irqcontrol ||
 	    uioinfo->irq_flags & IRQF_SHARED) {
 		dev_err(&pdev->dev, "interrupt configuration error\n");
-		goto bad0;
+		goto out_uioinfo;
 	}
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
 		ret = -ENOMEM;
 		dev_err(&pdev->dev, "unable to kmalloc\n");
-		goto bad0;
+		goto out_uioinfo;
 	}
 
 	priv->uioinfo = uioinfo;
 	spin_lock_init(&priv->lock);
-	priv->flags = 0; /* interrupt is enabled to begin with */
+	/* Interrupt is enabled to begin with,
+	 * that's why UIO_IRQ_DISABLED flag is not initially set.
+	 */
+	priv->flags = uioinfo_alloced ? BIT_MASK(UIO_INFO_ALLOCED) : 0;
 	priv->pdev = pdev;
 
 	if (!uioinfo->irq) {
 		ret = platform_get_irq(pdev, 0);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "failed to get IRQ\n");
-			goto bad0;
+			goto out_priv;
 		}
 		uioinfo->irq = ret;
 	}

@@ -205,19 +215,19 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 	ret = uio_register_device(&pdev->dev, priv->uioinfo);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to register uio device\n");
-		goto bad1;
+		goto out_pm;
 	}
 
 	platform_set_drvdata(pdev, priv);
 	return 0;
- bad1:
-	kfree(priv);
+out_pm:
 	pm_runtime_disable(&pdev->dev);
- bad0:
-	/* kfree uioinfo for OF */
-	if (pdev->dev.of_node)
+out_priv:
+	kfree(priv);
+out_uioinfo:
+	if (uioinfo_alloced)
 		kfree(uioinfo);
- bad2:
+out:
 	return ret;
 }

@@ -231,8 +241,7 @@ static int uio_pdrv_genirq_remove(struct platform_device *pdev)
 	priv->uioinfo->handler = NULL;
 	priv->uioinfo->irqcontrol = NULL;
 
-	/* kfree uioinfo for OF */
-	if (pdev->dev.of_node)
+	if (test_bit(UIO_INFO_ALLOCED, &priv->flags))
 		kfree(priv->uioinfo);
 
 	kfree(priv);

Signature · 1 line

1.7.8.6

Hans J. Koch

2012-12-13 18:14 UTC #2 lore raw

Re: [PATCH 1/2 v3] Fix memory freeing issues

Quoted 9 lines · Vitalii Demianets

1. uioinfo was kfreed based on the presence of pdev->dev.of_node, which was obviously wrong and unrelated to the fact if uioinfo was allocated statically or dynamically. This patch introduces new flag which clearly shows if uioinfo was allocated dynamically and kfrees uioinfo based on that flag;
2. Fix: priv data was not freed in case platform_get_irq() failed. As it was caused mainly by improper exit labels naming, labels were renamed too;
3. The case of uioinfo AND pdev->dev.of_node both NULL (not initialized in platform data) was not treated properly.

Quoted 5 lines

Signed-off-by Vitalii Demianets <[email protected]>
---
v2: Constants naming style
v3: Comments: better wording in comment accompanying flags initialization &
removed obsoleted OF comment

That comment is obsolete as well.

Quoted 10 lines

---
 drivers/uio/uio_pdrv_genirq.c |   47 ++++++++++++++++++++++++----------------
 1 files changed, 28 insertions(+), 19 deletions(-)

drivers/uio/uio_pdrv_genirq.c +0 -0

index 42202cd..cc5233b 100644

drivers/uio/uio_pdrv_genirq.c line 36

Quoted 7 lines

drivers/uio/uio_pdrv_genirq.c +4 -0

@@ -30,6 +30,11 @@
 
 #define DRIVER_NAME "uio_pdrv_genirq"
 
+enum {
+	UIO_IRQ_DISABLED = 0,
+	UIO_INFO_ALLOCED = 1,
+};

We don't need these flags.

drivers/uio/uio_pdrv_genirq.c static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info) · line 71

Quoted 10 lines

drivers/uio/uio_pdrv_genirq.c +2 -1

+
 struct uio_pdrv_genirq_platdata {
 	struct uio_info *uioinfo;
 	spinlock_t lock;

@@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info)
 	 * remember the state so we can allow user space to enable it later.
 	 */
 
-	if (!test_and_set_bit(0, &priv->flags))
+	if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))

Remove the "if" completely, we need to disable the irq unconditionally.

Quoted 4 lines

drivers/uio/uio_pdrv_genirq.c +0 -0
 		disable_irq_nosync(irq);
 
 	return IRQ_HANDLED;

drivers/uio/uio_pdrv_genirq.c static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on) · line 95

Quoted 10 lines

drivers/uio/uio_pdrv_genirq.c +2 -2

@@ -83,10 +88,10 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
 
 	spin_lock_irqsave(&priv->lock, flags);
 	if (irq_on) {
-		if (test_and_clear_bit(0, &priv->flags))
+		if (test_and_clear_bit(UIO_IRQ_DISABLED, &priv->flags))
 			enable_irq(dev_info->irq);
 	} else {
-		if (!test_and_set_bit(0, &priv->flags))
+		if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
 			disable_irq(dev_info->irq);

Same here: irqcontrol has to enable/disable the irq unconditionally.

Quoted 79 lines

drivers/uio/uio_pdrv_genirq.c +13 ~~-10~~

 	}
 	spin_unlock_irqrestore(&priv->lock, flags);

@@ -101,8 +106,9 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 	struct uio_mem *uiomem;
 	int ret = -EINVAL;
 	int i;
+	bool uioinfo_alloced = false;
 
-	if (!uioinfo) {
+	if (!uioinfo && pdev->dev.of_node) {
 		int irq;
 
 		/* alloc uioinfo for one device */

@@ -110,10 +116,11 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 		if (!uioinfo) {
 			ret = -ENOMEM;
 			dev_err(&pdev->dev, "unable to kmalloc\n");
-			goto bad2;
+			goto out;
 		}
 		uioinfo->name = pdev->dev.of_node->name;
 		uioinfo->version = "devicetree";
+		uioinfo_alloced = true;
 
 		/* Multiple IRQs are not supported */
 		irq = platform_get_irq(pdev, 0);

@@ -125,32 +132,35 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 
 	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
 		dev_err(&pdev->dev, "missing platform_data\n");
-		goto bad0;
+		goto out_uioinfo;
 	}
 
 	if (uioinfo->handler || uioinfo->irqcontrol ||
 	    uioinfo->irq_flags & IRQF_SHARED) {
 		dev_err(&pdev->dev, "interrupt configuration error\n");
-		goto bad0;
+		goto out_uioinfo;
 	}
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
 		ret = -ENOMEM;
 		dev_err(&pdev->dev, "unable to kmalloc\n");
-		goto bad0;
+		goto out_uioinfo;
 	}
 
 	priv->uioinfo = uioinfo;
 	spin_lock_init(&priv->lock);
-	priv->flags = 0; /* interrupt is enabled to begin with */
+	/* Interrupt is enabled to begin with,
+	 * that's why UIO_IRQ_DISABLED flag is not initially set.
+	 */
+	priv->flags = uioinfo_alloced ? BIT_MASK(UIO_INFO_ALLOCED) : 0;
 	priv->pdev = pdev;
 
 	if (!uioinfo->irq) {
 		ret = platform_get_irq(pdev, 0);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "failed to get IRQ\n");
-			goto bad0;
+			goto out_priv;
 		}
 		uioinfo->irq = ret;
 	}

@@ -205,19 +215,19 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 	ret = uio_register_device(&pdev->dev, priv->uioinfo);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to register uio device\n");
-		goto bad1;
+		goto out_pm;
 	}
 
 	platform_set_drvdata(pdev, priv);
 	return 0;
- bad1:
-	kfree(priv);

drivers/uio/uio_pdrv_genirq.c static int uio_pdrv_genirq_probe(struct platform_device *pdev) · line 229

Quoted 10 lines

drivers/uio/uio_pdrv_genirq.c +5 -3

+out_pm:
 	pm_runtime_disable(&pdev->dev);
- bad0:
-	/* kfree uioinfo for OF */
-	if (pdev->dev.of_node)
+out_priv:
+	kfree(priv);
+out_uioinfo:
+	if (uioinfo_alloced)
 		kfree(uioinfo);

why check that variable? kfree can handle NULL pointers very well.

Quoted 18 lines

drivers/uio/uio_pdrv_genirq.c +2 -3

- bad2:
+out:
 	return ret;
 }

@@ -231,8 +241,7 @@ static int uio_pdrv_genirq_remove(struct platform_device *pdev)
 	priv->uioinfo->handler = NULL;
 	priv->uioinfo->irqcontrol = NULL;
 
-	/* kfree uioinfo for OF */
-	if (pdev->dev.of_node)
+	if (test_bit(UIO_INFO_ALLOCED, &priv->flags))
 		kfree(priv->uioinfo);
 
 	kfree(priv);

Signature · 1 line

1.7.8.6

Vitalii Demianets

author

2012-12-14 09:33 UTC #3 lore raw

Re: [PATCH 1/2 v3] Fix memory freeing issues

Quoted 10 lines · On Thursday 13 December 2012 20:13:54 Hans J. Koch wrote:

Quoted 9 lines · Vitalii Demianets
1. uioinfo was kfreed based on the presence of pdev->dev.of_node, which was obviously wrong and unrelated to the fact if uioinfo was allocated statically or dynamically. This patch introduces new flag which clearly shows if uioinfo was allocated dynamically and kfrees uioinfo based on that flag; 2. Fix: priv data was not freed in case platform_get_irq() failed. As it was caused mainly by improper exit labels naming, labels were renamed too; 3. The case of uioinfo AND pdev->dev.of_node both NULL (not initialized in platform data) was not treated properly.

Quoted 8 lines

Quoted 5 lines
Signed-off-by Vitalii Demianets <[email protected]>
---
v2: Constants naming style
v3: Comments: better wording in comment accompanying flags initialization & removed obsoleted OF comment
That comment is obsolete as well.

Hans, this patch deals only and exclusively with memory freeing issues. This patch does not change in any way any other functionality. Particularly, it does not change irq-related bit flag.
That comment is about irq-related bit flag. As long as this patch does not remove that flag, the comment is not obsolete.

Quoted 11 lines

Quoted 11 lines
---
 drivers/uio/uio_pdrv_genirq.c |   47
++++++++++++++++++++++++---------------- 1 files changed, 28 insertions(+), 19 deletions(-)
(quoted diff) +0 -0
b/drivers/uio/uio_pdrv_genirq.c index 42202cd..cc5233b 100644
drivers/uio/uio_pdrv_genirq.c +0 -0
@@ -30,6 +30,11 @@
 

Quoted 8 lines

drivers/uio/uio_pdrv_genirq.c line 36
Quoted 6 lines
drivers/uio/uio_pdrv_genirq.c +4 -0
 #define DRIVER_NAME "uio_pdrv_genirq"
 
+enum {
+	UIO_IRQ_DISABLED = 0,
+	UIO_INFO_ALLOCED = 1,
+};
We don't need these flags.

1) This patch does need UIO_INFO_ALLOCED flag, because we need to know in the uio_pdrv_genirq_remove() routine if uioinfo was allocated dynamically or statically. If flag UIO_INFO_ALLOCED is set, then uioinfo was allocated dynamically and should be freed.
2) As for UIO_IRQ_DISABLED - I'm not introducing this flag. It is not new; it was there in the priv->flags already, although unnamed. What this patch does - it gives the name to the previously unnamed flag. It does not change its semantics, it does nod add or remove it. The patch is keeping all not memory-related things in status quo.

I understand you want this flag removed. But it is completely another story, nothing in common with memory freeing issues. And this patch is dealing with memory freeing issues only. Not touching any other functionality.

Quoted 9 lines

Quoted 9 lines
drivers/uio/uio_pdrv_genirq.c +1 -0
+
 struct uio_pdrv_genirq_platdata {
 	struct uio_info *uioinfo;
 	spinlock_t lock;
@@ -63,7 +68,7 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq,
struct uio_info *dev_info) * remember the state so we can allow user space to enable it later. */

Quoted 4 lines

drivers/uio/uio_pdrv_genirq.c
Quoted 2 lines
drivers/uio/uio_pdrv_genirq.c +1 -1
-	if (!test_and_set_bit(0, &priv->flags))
+	if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
Remove the "if" completely, we need to disable the irq unconditionally.

Hans, why do you want to put in this patch, which is dealing with memory-freeing issues only, completely unrelated functional changes? I understand you want that flag removed. So, you can do it in another patch. This patch does not change anything except memory freeing bugs.

Quoted 7 lines

drivers/uio/uio_pdrv_genirq.c +0 -0

 		disable_irq_nosync(irq);
 
 	return IRQ_HANDLED;

@@ -83,10 +88,10 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info
 *dev_info, s32 irq_on)

Quoted 11 lines

drivers/uio/uio_pdrv_genirq.c static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on) · line 95
Quoted 9 lines
drivers/uio/uio_pdrv_genirq.c +2 -2
 	spin_lock_irqsave(&priv->lock, flags);
 	if (irq_on) {
-		if (test_and_clear_bit(0, &priv->flags))
+		if (test_and_clear_bit(UIO_IRQ_DISABLED, &priv->flags))
 			enable_irq(dev_info->irq);
 	} else {
-		if (!test_and_set_bit(0, &priv->flags))
+		if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
 			disable_irq(dev_info->irq);
Same here: irqcontrol has to enable/disable the irq unconditionally.

Same here. This patch is dealing with memory freeing issues only. Why do you want to put in this patch completely unrelated functional changes?

Quoted 78 lines

drivers/uio/uio_pdrv_genirq.c +0 -0

drivers/uio/uio_pdrv_genirq.c +12 -7

 	spin_unlock_irqrestore(&priv->lock, flags);

@@ -101,8 +106,9 @@ static int uio_pdrv_genirq_probe(struct
 platform_device *pdev) struct uio_mem *uiomem;
 	int ret = -EINVAL;
 	int i;
+	bool uioinfo_alloced = false;
 
-	if (!uioinfo) {
+	if (!uioinfo && pdev->dev.of_node) {
 		int irq;
 
 		/* alloc uioinfo for one device */

@@ -110,10 +116,11 @@ static int uio_pdrv_genirq_probe(struct
 platform_device *pdev) if (!uioinfo) {
 			ret = -ENOMEM;
 			dev_err(&pdev->dev, "unable to kmalloc\n");
-			goto bad2;
+			goto out;
 		}
 		uioinfo->name = pdev->dev.of_node->name;
 		uioinfo->version = "devicetree";
+		uioinfo_alloced = true;
 
 		/* Multiple IRQs are not supported */
 		irq = platform_get_irq(pdev, 0);

@@ -125,32 +132,35 @@ static int uio_pdrv_genirq_probe(struct
 platform_device *pdev)
 
 	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
 		dev_err(&pdev->dev, "missing platform_data\n");
-		goto bad0;
+		goto out_uioinfo;
 	}
 
 	if (uioinfo->handler || uioinfo->irqcontrol ||
 	    uioinfo->irq_flags & IRQF_SHARED) {
 		dev_err(&pdev->dev, "interrupt configuration error\n");
-		goto bad0;
+		goto out_uioinfo;
 	}
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
 		ret = -ENOMEM;
 		dev_err(&pdev->dev, "unable to kmalloc\n");
-		goto bad0;
+		goto out_uioinfo;
 	}
 
 	priv->uioinfo = uioinfo;
 	spin_lock_init(&priv->lock);
-	priv->flags = 0; /* interrupt is enabled to begin with */
+	/* Interrupt is enabled to begin with,
+	 * that's why UIO_IRQ_DISABLED flag is not initially set.
+	 */
+	priv->flags = uioinfo_alloced ? BIT_MASK(UIO_INFO_ALLOCED) : 0;
 	priv->pdev = pdev;
 
 	if (!uioinfo->irq) {
 		ret = platform_get_irq(pdev, 0);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "failed to get IRQ\n");
-			goto bad0;
+			goto out_priv;
 		}
 		uioinfo->irq = ret;

drivers/uio/uio_pdrv_genirq.c +1 -1

@@ -205,19 +215,19 @@ static int uio_pdrv_genirq_probe(struct
 platform_device *pdev) ret = uio_register_device(&pdev->dev,
 priv->uioinfo);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to register uio device\n");
-		goto bad1;
+		goto out_pm;
 	}

Quoted 16 lines

Quoted 4 lines
drivers/uio/uio_pdrv_genirq.c +0 -2
 	platform_set_drvdata(pdev, priv);
 	return 0;
- bad1:
-	kfree(priv);
drivers/uio/uio_pdrv_genirq.c static int uio_pdrv_genirq_probe(struct platform_device *pdev) · line 229
Quoted 10 lines
drivers/uio/uio_pdrv_genirq.c +5 -3
+out_pm:
 	pm_runtime_disable(&pdev->dev);
- bad0:
-	/* kfree uioinfo for OF */
-	if (pdev->dev.of_node)
+out_priv:
+	kfree(priv);
+out_uioinfo:
+	if (uioinfo_alloced)
 		kfree(uioinfo);
why check that variable? kfree can handle NULL pointers very well.

This variable shows if uioinfo was allocated dynamically or statically. We want kfree(uioinfo) only for the dynamically allocated memory.

Quoted 10 lines

drivers/uio/uio_pdrv_genirq.c +1 -1

- bad2:
+out:
 	return ret;
 }

@@ -231,8 +241,7 @@ static int uio_pdrv_genirq_remove(struct
 platform_device *pdev) priv->uioinfo->handler = NULL;
 	priv->uioinfo->irqcontrol = NULL;

Quoted 8 lines

drivers/uio/uio_pdrv_genirq.c +1 -3

-	/* kfree uioinfo for OF */
-	if (pdev->dev.of_node)
+	if (test_bit(UIO_INFO_ALLOCED, &priv->flags))
 		kfree(priv->uioinfo);
 
 	kfree(priv);
--

1.7.8.6

Please, note, that this patch does not change irq-related functionality at all. I understand that you want to change it, but it is not what this patch is about. It deals with memory freeing issues only.

Do you have technical objections to the memory issues fixes, which this patch is about?

Hans J. Koch

2012-12-15 17:25 UTC #4 lore raw

Re: [PATCH 1/2 v3] Fix memory freeing issues

Quoted 3 lines · Vitalii Demianets

Hans, why do you want to put in this patch, which is dealing with memory-freeing issues only, completely unrelated functional changes?

Because during review of your patch we happened to find another issue a few lines up and down. Why not fix it on the way?

What I'd like is simply

[PATCH] Fix uio_pdrv_genirq issues

If you like, make it two patches, one with your memory-freeing issue and one "Remove irq tracking" or something like that. That's just three or four lines difference, I'd even accept it if it were only one patch.

I don't want to fix one thing now and leave the other one unresolved. That would just be a waste of time.

To be clear, I have no objections regarding your memory freeing ideas.

Thanks,
Hans

Vitalii Demianets

author

2012-12-17 08:58 UTC #5 lore raw

Re: [PATCH 1/2 v3] Fix memory freeing issues

Quoted 7 lines · On Saturday 15 December 2012 19:25:40 Hans J. Koch wrote:

Quoted 2 lines · Vitalii Demianets
Hans, why do you want to put in this patch, which is dealing with memory-freeing issues only, completely unrelated functional changes?
Because during review of your patch we happened to find another issue a few lines up and down. Why not fix it on the way?

Because my heart is not with your solution of irq-related problem. I can't do it.

Quoted 2 lines

If you like, make it two patches, one with your memory-freeing issue and one "Remove irq tracking" or something like that.

I've done exactly that. The series of two patches. First [patch 1/2] - deals exclusively with memory freeing issues and you have no objections to it. Second [patch 2/2] which we disagreed upon - deals with irq-related issues.

Quoted 6 lines

That's just
three or four lines difference, I'd even accept it if it were only one patch.
I don't want to fix one thing now and leave the other one unresolved. That would just be a waste of time.

Me too. But we have different vision of the solution to the irq-related issue. That's why I won't write the irq-related part.
Also, I don't understand, why do you want two unrelated fixes in one patch? When they are separated, they are easier to discuss, study and revert if needed.

Quoted 2 lines

To be clear, I have no objections regarding your memory freeing ideas.

So, why not to stop here and accept this one patch? I won't write the irq-related part anyway, as my heart is with the solution in [patch 2/2] and you disagree with it.
So, the irq-related part should be done by someone else. Maybe by you, why not?

Vitalii Demianets

author

2012-12-10 09:46 UTC #6 lore raw

PATCH 2/2 Fix concurrency issue

In reply to Vitalii Demianets

In a SMP case there was a race condition issue between uio_pdrv_genirq_irqcontrol() running on one CPU and irq handler on another CPU. Fix it by spin_locking shared resources access inside irq handler. Also:
- Change disable_irq to disable_irq_nosync to avoid deadlock, because
disable_irq waits for the completion of the irq handler
- Change atomic bit-manipulation routines to their non-atomic counterparts as
we already are guarding the code by spinlock.

Signed-off-by Vitalii Demianets <[email protected]>

---
 drivers/uio/uio_pdrv_genirq.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

drivers/uio/uio_pdrv_genirq.c +8 -5

index cc5233b..8075367 100644
@@ -68,8 +68,10 @@ static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info)
 	 * remember the state so we can allow user space to enable it later.
 	 */
 
-	if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
+	spin_lock(&priv->lock);
+	if (!__test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
 		disable_irq_nosync(irq);
+	spin_unlock(&priv->lock);
 
 	return IRQ_HANDLED;
 }

@@ -83,16 +85,17 @@ static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
 	 * in the interrupt controller, but keep track of the
 	 * state to prevent per-irq depth damage.
 	 *
-	 * Serialize this operation to support multiple tasks.
+	 * Serialize this operation to support multiple tasks and concurrency
+	 * with irq handler on SMP systems.
 	 */
 
 	spin_lock_irqsave(&priv->lock, flags);
 	if (irq_on) {
-		if (test_and_clear_bit(UIO_IRQ_DISABLED, &priv->flags))
+		if (__test_and_clear_bit(UIO_IRQ_DISABLED, &priv->flags))
 			enable_irq(dev_info->irq);
 	} else {
-		if (!test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
-			disable_irq(dev_info->irq);
+		if (!__test_and_set_bit(UIO_IRQ_DISABLED, &priv->flags))
+			disable_irq_nosync(dev_info->irq);
 	}
 	spin_unlock_irqrestore(&priv->lock, flags);

Signature · 1 line

1.7.8.6