[PATCH 0/2] drivers/uio/uio_pdrv_genirq.c: FIx memory & concurrency issues

[PATCH 0/2] drivers/uio/uio_pdrv_genirq.c: FIx memory & concurrency issues

[Vitalii Demianets]

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.

[PATCH 1/2 v3] Fix memory freeing issues

[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.

Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
---
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(-)

diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 42202cd..cc5233b 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -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);
-- 
1.7.8.6

[PATCH 2/2] Fix concurrency issue

[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 <vitas@nppfactor.kiev.ua>
---
 drivers/uio/uio_pdrv_genirq.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index cc5233b..8075367 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -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);
 
-- 
1.7.8.6

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

[Hans J. Koch]

On Mon, Dec 10, 2012 at 11:44:45AM +0200, Vitalii Demianets wrote:
> 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 <vitas@nppfactor.kiev.ua>
> ---
> v2: Constants naming style
> v3: Comments: better wording in comment accompanying flags initialization &
>  removed obsoleted OF comment

That comment is obsolete as well.

> 
> ---
>  drivers/uio/uio_pdrv_genirq.c |   47 ++++++++++++++++++++++++----------------
>  1 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 42202cd..cc5233b 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -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.

> +
>  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.

>  		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);

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

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

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

> - 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);
> -- 
> 1.7.8.6
> 

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

[Vitalii Demianets]

On Thursday 13 December 2012 20:13:54 Hans J. Koch wrote:
> On Mon, Dec 10, 2012 at 11:44:45AM +0200, Vitalii Demianets wrote:
> > 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 <vitas@nppfactor.kiev.ua>
> > ---
> > 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.

> > ---
> >  drivers/uio/uio_pdrv_genirq.c |   47
> > ++++++++++++++++++++++++---------------- 1 files changed, 28
> > insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/uio/uio_pdrv_genirq.c
> > b/drivers/uio/uio_pdrv_genirq.c index 42202cd..cc5233b 100644
> > --- a/drivers/uio/uio_pdrv_genirq.c
> > +++ b/drivers/uio/uio_pdrv_genirq.c
> > @@ -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.

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.

>
> > +
> >  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.

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.


>
> >  		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);
>
> 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?

>
> >  	}
> >  	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);
>
> 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.

>
> > - 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);
> > --
> > 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?

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

[Hans J. Koch]

On Fri, Dec 14, 2012 at 11:33:50AM +0200, Vitalii Demianets wrote:
> 
> 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

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

[Vitalii Demianets]

On Saturday 15 December 2012 19:25:40 Hans J. Koch wrote:
> On Fri, Dec 14, 2012 at 11:33:50AM +0200, Vitalii Demianets wrote:
> > 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.

> 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.

> 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.

>
> 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?