All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-05-26 15:55 ` Julia Lawall
  0 siblings, 0 replies; 26+ messages in thread
From: Julia Lawall @ 2010-05-26 15:55 UTC (permalink / raw)
  To: Joerg Roedel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

From: Julia Lawall <julia@diku.dk>

Add a spin_unlock missing on the error path.  The locks and unlocks are
balanced in other functions, so it seems that the same should be the case
here.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression E1;
@@

* spin_lock(E1,...);
  <+... when != E1
  if (...) {
    ... when != E1
*   return ...;
  }
  ...+>
* spin_unlock(E1,...);
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 arch/x86/kernel/amd_iommu.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index fa5a147..b98e1cd 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -1499,12 +1499,16 @@ static int __attach_device(struct device *dev,
 
 	/* Some sanity checks */
 	if (alias_data->domain != NULL &&
-	    alias_data->domain != domain)
+	    alias_data->domain != domain) {
+		spin_unlock(&domain->lock);
 		return -EBUSY;
+	}
 
 	if (dev_data->domain != NULL &&
-	    dev_data->domain != domain)
+	    dev_data->domain != domain) {
+		spin_unlock(&domain->lock);
 		return -EBUSY;
+	}
 
 	/* Do real assignment */
 	if (dev_data->alias != dev) {

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-05-26 15:55 ` Julia Lawall
  0 siblings, 0 replies; 26+ messages in thread
From: Julia Lawall @ 2010-05-26 15:55 UTC (permalink / raw)
  To: Joerg Roedel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

From: Julia Lawall <julia@diku.dk>

Add a spin_unlock missing on the error path.  The locks and unlocks are
balanced in other functions, so it seems that the same should be the case
here.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression E1;
@@

* spin_lock(E1,...);
  <+... when != E1
  if (...) {
    ... when != E1
*   return ...;
  }
  ...+>
* spin_unlock(E1,...);
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 arch/x86/kernel/amd_iommu.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index fa5a147..b98e1cd 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -1499,12 +1499,16 @@ static int __attach_device(struct device *dev,
 
 	/* Some sanity checks */
 	if (alias_data->domain != NULL &&
-	    alias_data->domain != domain)
+	    alias_data->domain != domain) {
+		spin_unlock(&domain->lock);
 		return -EBUSY;
+	}
 
 	if (dev_data->domain != NULL &&
-	    dev_data->domain != domain)
+	    dev_data->domain != domain) {
+		spin_unlock(&domain->lock);
 		return -EBUSY;
+	}
 
 	/* Do real assignment */
 	if (dev_data->alias != dev) {

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
  2010-05-26 15:55 ` Julia Lawall
@ 2010-05-27 11:06   ` Roedel, Joerg
  -1 siblings, 0 replies; 26+ messages in thread
From: Roedel, Joerg @ 2010-05-27 11:06 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, iommu,
	linux-kernel, kernel-janitors

On Wed, May 26, 2010 at 11:55:59AM -0400, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
> 
> Add a spin_unlock missing on the error path.  The locks and unlocks are
> balanced in other functions, so it seems that the same should be the case
> here.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)

Applied the attached version of the patch. Thanks for catching this.

	Joerg

>From 84fe6c19e4a598e8071e3bd1b2c923454eae1268 Mon Sep 17 00:00:00 2001
From: Julia Lawall <julia@diku.dk>
Date: Thu, 27 May 2010 12:31:51 +0200
Subject: [PATCH] arch/x86/kernel: Add missing spin_unlock

Add a spin_unlock missing on the error path.  The locks and unlocks are
balanced in other functions, so it seems that the same should be the case
here.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression E1;
@@

* spin_lock(E1,...);
  <+... when != E1
  if (...) {
    ... when != E1
*   return ...;
  }
  ...+>
* spin_unlock(E1,...);
// </smpl>

Cc: stable@kernel.org
Signed-off-by: Julia Lawall <julia@diku.dk>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index fa5a147..8a9aaa8 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -1487,6 +1487,7 @@ static int __attach_device(struct device *dev,
 			   struct protection_domain *domain)
 {
 	struct iommu_dev_data *dev_data, *alias_data;
+	int ret;
 
 	dev_data   = get_dev_data(dev);
 	alias_data = get_dev_data(dev_data->alias);
@@ -1498,13 +1499,14 @@ static int __attach_device(struct device *dev,
 	spin_lock(&domain->lock);
 
 	/* Some sanity checks */
+	ret = -EBUSY;
 	if (alias_data->domain != NULL &&
 	    alias_data->domain != domain)
-		return -EBUSY;
+		goto out_unlock;
 
 	if (dev_data->domain != NULL &&
 	    dev_data->domain != domain)
-		return -EBUSY;
+		goto out_unlock;
 
 	/* Do real assignment */
 	if (dev_data->alias != dev) {
@@ -1520,10 +1522,14 @@ static int __attach_device(struct device *dev,
 
 	atomic_inc(&dev_data->bind);
 
+	ret = 0;
+
+out_unlock:
+
 	/* ready */
 	spin_unlock(&domain->lock);
 
-	return 0;
+	return ret;
 }
 
 /*
-- 
1.7.1



^ permalink raw reply related	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-05-27 11:06   ` Roedel, Joerg
  0 siblings, 0 replies; 26+ messages in thread
From: Roedel, Joerg @ 2010-05-27 11:06 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, iommu,
	linux-kernel, kernel-janitors

On Wed, May 26, 2010 at 11:55:59AM -0400, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
> 
> Add a spin_unlock missing on the error path.  The locks and unlocks are
> balanced in other functions, so it seems that the same should be the case
> here.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)

Applied the attached version of the patch. Thanks for catching this.

	Joerg

From 84fe6c19e4a598e8071e3bd1b2c923454eae1268 Mon Sep 17 00:00:00 2001
From: Julia Lawall <julia@diku.dk>
Date: Thu, 27 May 2010 12:31:51 +0200
Subject: [PATCH] arch/x86/kernel: Add missing spin_unlock

Add a spin_unlock missing on the error path.  The locks and unlocks are
balanced in other functions, so it seems that the same should be the case
here.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression E1;
@@

* spin_lock(E1,...);
  <+... when != E1
  if (...) {
    ... when != E1
*   return ...;
  }
  ...+>
* spin_unlock(E1,...);
// </smpl>

Cc: stable@kernel.org
Signed-off-by: Julia Lawall <julia@diku.dk>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index fa5a147..8a9aaa8 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -1487,6 +1487,7 @@ static int __attach_device(struct device *dev,
 			   struct protection_domain *domain)
 {
 	struct iommu_dev_data *dev_data, *alias_data;
+	int ret;
 
 	dev_data   = get_dev_data(dev);
 	alias_data = get_dev_data(dev_data->alias);
@@ -1498,13 +1499,14 @@ static int __attach_device(struct device *dev,
 	spin_lock(&domain->lock);
 
 	/* Some sanity checks */
+	ret = -EBUSY;
 	if (alias_data->domain != NULL &&
 	    alias_data->domain != domain)
-		return -EBUSY;
+		goto out_unlock;
 
 	if (dev_data->domain != NULL &&
 	    dev_data->domain != domain)
-		return -EBUSY;
+		goto out_unlock;
 
 	/* Do real assignment */
 	if (dev_data->alias != dev) {
@@ -1520,10 +1522,14 @@ static int __attach_device(struct device *dev,
 
 	atomic_inc(&dev_data->bind);
 
+	ret = 0;
+
+out_unlock:
+
 	/* ready */
 	spin_unlock(&domain->lock);
 
-	return 0;
+	return ret;
 }
 
 /*
-- 
1.7.1



^ permalink raw reply related	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
  2010-05-26 15:55 ` Julia Lawall
@ 2010-05-27 11:11   ` Roedel, Joerg
  -1 siblings, 0 replies; 26+ messages in thread
From: Roedel, Joerg @ 2010-05-27 11:11 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, iommu,
	linux-kernel, kernel-janitors

On Wed, May 26, 2010 at 11:55:59AM -0400, Julia Lawall wrote:
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression E1;
> @@
> 
> * spin_lock(E1,...);
>   <+... when != E1
>   if (...) {
>     ... when != E1
> *   return ...;
>   }
>   ...+>
> * spin_unlock(E1,...);
> // </smpl>

Btw, I think it would be great to have a collection of these semantic
match scripts in the kernel repository together with a build target to
run these scripts over the kernel sources (like the cscope target).
Opinions?

	Joerg



^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-05-27 11:11   ` Roedel, Joerg
  0 siblings, 0 replies; 26+ messages in thread
From: Roedel, Joerg @ 2010-05-27 11:11 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, iommu,
	linux-kernel, kernel-janitors

On Wed, May 26, 2010 at 11:55:59AM -0400, Julia Lawall wrote:
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression E1;
> @@
> 
> * spin_lock(E1,...);
>   <+... when != E1
>   if (...) {
>     ... when != E1
> *   return ...;
>   }
>   ...+>
> * spin_unlock(E1,...);
> // </smpl>

Btw, I think it would be great to have a collection of these semantic
match scripts in the kernel repository together with a build target to
run these scripts over the kernel sources (like the cscope target).
Opinions?

	Joerg



^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
  2010-05-27 11:11   ` Roedel, Joerg
@ 2010-05-27 11:17     ` Julia Lawall
  -1 siblings, 0 replies; 26+ messages in thread
From: Julia Lawall @ 2010-05-27 11:17 UTC (permalink / raw)
  To: Roedel, Joerg
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, iommu,
	linux-kernel, kernel-janitors, Nicolas Palix

On Thu, 27 May 2010, Roedel, Joerg wrote:

> On Wed, May 26, 2010 at 11:55:59AM -0400, Julia Lawall wrote:
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@
> > expression E1;
> > @@
> > 
> > * spin_lock(E1,...);
> >   <+... when != E1
> >   if (...) {
> >     ... when != E1
> > *   return ...;
> >   }
> >   ...+>
> > * spin_unlock(E1,...);
> > // </smpl>
> 
> Btw, I think it would be great to have a collection of these semantic
> match scripts in the kernel repository together with a build target to
> run these scripts over the kernel sources (like the cscope target).
> Opinions?

We have submitted and received some feedback on an initial version of 
this, but I'm not completely sure of the current status.

julia

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-05-27 11:17     ` Julia Lawall
  0 siblings, 0 replies; 26+ messages in thread
From: Julia Lawall @ 2010-05-27 11:17 UTC (permalink / raw)
  To: Roedel, Joerg
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, iommu,
	linux-kernel, kernel-janitors, Nicolas Palix

On Thu, 27 May 2010, Roedel, Joerg wrote:

> On Wed, May 26, 2010 at 11:55:59AM -0400, Julia Lawall wrote:
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@
> > expression E1;
> > @@
> > 
> > * spin_lock(E1,...);
> >   <+... when != E1
> >   if (...) {
> >     ... when != E1
> > *   return ...;
> >   }
> >   ...+>
> > * spin_unlock(E1,...);
> > // </smpl>
> 
> Btw, I think it would be great to have a collection of these semantic
> match scripts in the kernel repository together with a build target to
> run these scripts over the kernel sources (like the cscope target).
> Opinions?

We have submitted and received some feedback on an initial version of 
this, but I'm not completely sure of the current status.

julia

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
  2010-05-27 11:17     ` Julia Lawall
@ 2010-05-27 11:42       ` Nicolas Palix
  -1 siblings, 0 replies; 26+ messages in thread
From: Nicolas Palix @ 2010-05-27 11:42 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Roedel, Joerg, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

On Thursday 27 May 2010 13:17:58 Julia Lawall wrote:
> On Thu, 27 May 2010, Roedel, Joerg wrote:
> 
> > On Wed, May 26, 2010 at 11:55:59AM -0400, Julia Lawall wrote:
> > > The semantic match that finds this problem is as follows:
> > > (http://coccinelle.lip6.fr/)
> > > 
> > > // <smpl>
> > > @@
> > > expression E1;
> > > @@
> > > 
> > > * spin_lock(E1,...);
> > >   <+... when != E1
> > >   if (...) {
> > >     ... when != E1
> > > *   return ...;
> > >   }
> > >   ...+>
> > > * spin_unlock(E1,...);
> > > // </smpl>
> > 
> > Btw, I think it would be great to have a collection of these semantic
> > match scripts in the kernel repository together with a build target to
> > run these scripts over the kernel sources (like the cscope target).
> > Opinions?
> 
> We have submitted and received some feedback on an initial version of 
> this, but I'm not completely sure of the current status.

You can see the latest feedback we get at
http://lkml.org/lkml/2010/5/10/257

The initial submission and its comments are at
http://lkml.org/lkml/2010/4/26/269

> 
> julia
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Nicolas Palix
Tel: (+33) 1 44 27 87 25
Tel: (+33) 6 81 07 91 72
Web: http://www.diku.dk/~npalix/

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-05-27 11:42       ` Nicolas Palix
  0 siblings, 0 replies; 26+ messages in thread
From: Nicolas Palix @ 2010-05-27 11:42 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Roedel, Joerg, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

On Thursday 27 May 2010 13:17:58 Julia Lawall wrote:
> On Thu, 27 May 2010, Roedel, Joerg wrote:
> 
> > On Wed, May 26, 2010 at 11:55:59AM -0400, Julia Lawall wrote:
> > > The semantic match that finds this problem is as follows:
> > > (http://coccinelle.lip6.fr/)
> > > 
> > > // <smpl>
> > > @@
> > > expression E1;
> > > @@
> > > 
> > > * spin_lock(E1,...);
> > >   <+... when != E1
> > >   if (...) {
> > >     ... when != E1
> > > *   return ...;
> > >   }
> > >   ...+>
> > > * spin_unlock(E1,...);
> > > // </smpl>
> > 
> > Btw, I think it would be great to have a collection of these semantic
> > match scripts in the kernel repository together with a build target to
> > run these scripts over the kernel sources (like the cscope target).
> > Opinions?
> 
> We have submitted and received some feedback on an initial version of 
> this, but I'm not completely sure of the current status.

You can see the latest feedback we get at
http://lkml.org/lkml/2010/5/10/257

The initial submission and its comments are at
http://lkml.org/lkml/2010/4/26/269

> 
> julia
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Nicolas Palix
Tel: (+33) 1 44 27 87 25
Tel: (+33) 6 81 07 91 72
Web: http://www.diku.dk/~npalix/

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
  2010-05-27 11:42       ` Nicolas Palix
@ 2010-05-28  7:11         ` Roedel, Joerg
  -1 siblings, 0 replies; 26+ messages in thread
From: Roedel, Joerg @ 2010-05-28  7:11 UTC (permalink / raw)
  To: Nicolas Palix
  Cc: Julia Lawall, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

On Thu, May 27, 2010 at 07:42:25AM -0400, Nicolas Palix wrote:
> > We have submitted and received some feedback on an initial version of 
> > this, but I'm not completely sure of the current status.
> 
> You can see the latest feedback we get at
> http://lkml.org/lkml/2010/5/10/257
> 
> The initial submission and its comments are at
> http://lkml.org/lkml/2010/4/26/269

I've also sent some feedback. Would be cool if you could work the
feedback in and do a repost asking Andrew to take it. Would be cool to
have this merged with 2.6.36.

	Joerg



^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-05-28  7:11         ` Roedel, Joerg
  0 siblings, 0 replies; 26+ messages in thread
From: Roedel, Joerg @ 2010-05-28  7:11 UTC (permalink / raw)
  To: Nicolas Palix
  Cc: Julia Lawall, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

On Thu, May 27, 2010 at 07:42:25AM -0400, Nicolas Palix wrote:
> > We have submitted and received some feedback on an initial version of 
> > this, but I'm not completely sure of the current status.
> 
> You can see the latest feedback we get at
> http://lkml.org/lkml/2010/5/10/257
> 
> The initial submission and its comments are at
> http://lkml.org/lkml/2010/4/26/269

I've also sent some feedback. Would be cool if you could work the
feedback in and do a repost asking Andrew to take it. Would be cool to
have this merged with 2.6.36.

	Joerg



^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
  2010-05-28  7:11         ` Roedel, Joerg
@ 2010-05-28 16:45           ` H. Peter Anvin
  -1 siblings, 0 replies; 26+ messages in thread
From: H. Peter Anvin @ 2010-05-28 16:45 UTC (permalink / raw)
  To: Roedel, Joerg
  Cc: Nicolas Palix, Julia Lawall, Thomas Gleixner, Ingo Molnar, x86,
	iommu, linux-kernel, kernel-janitors

On 05/28/2010 12:11 AM, Roedel, Joerg wrote:
> On Thu, May 27, 2010 at 07:42:25AM -0400, Nicolas Palix wrote:
>>> We have submitted and received some feedback on an initial version of 
>>> this, but I'm not completely sure of the current status.
>>
>> You can see the latest feedback we get at
>> http://lkml.org/lkml/2010/5/10/257
>>
>> The initial submission and its comments are at
>> http://lkml.org/lkml/2010/4/26/269
> 
> I've also sent some feedback. Would be cool if you could work the
> feedback in and do a repost asking Andrew to take it. Would be cool to
> have this merged with 2.6.36.
> 

I don't see why scripts that don't *in themselves* change the output
binaries need to wait for .36.  Instead, it would be better to get them
in sooner to make them available to developers in advance of the .36 cycle.

Of course, I'm not Linus, and I don't see him Cc:'d on this, but that
would be the normal rules.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-05-28 16:45           ` H. Peter Anvin
  0 siblings, 0 replies; 26+ messages in thread
From: H. Peter Anvin @ 2010-05-28 16:45 UTC (permalink / raw)
  To: Roedel, Joerg
  Cc: Nicolas Palix, Julia Lawall, Thomas Gleixner, Ingo Molnar, x86,
	iommu, linux-kernel, kernel-janitors

On 05/28/2010 12:11 AM, Roedel, Joerg wrote:
> On Thu, May 27, 2010 at 07:42:25AM -0400, Nicolas Palix wrote:
>>> We have submitted and received some feedback on an initial version of 
>>> this, but I'm not completely sure of the current status.
>>
>> You can see the latest feedback we get at
>> http://lkml.org/lkml/2010/5/10/257
>>
>> The initial submission and its comments are at
>> http://lkml.org/lkml/2010/4/26/269
> 
> I've also sent some feedback. Would be cool if you could work the
> feedback in and do a repost asking Andrew to take it. Would be cool to
> have this merged with 2.6.36.
> 

I don't see why scripts that don't *in themselves* change the output
binaries need to wait for .36.  Instead, it would be better to get them
in sooner to make them available to developers in advance of the .36 cycle.

Of course, I'm not Linus, and I don't see him Cc:'d on this, but that
would be the normal rules.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
  2010-05-28 16:45           ` H. Peter Anvin
@ 2010-06-01  9:58             ` Joerg Roedel
  -1 siblings, 0 replies; 26+ messages in thread
From: Joerg Roedel @ 2010-06-01  9:58 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Roedel, Joerg, Nicolas Palix, x86, kernel-janitors, linux-kernel,
	iommu, Ingo Molnar, Julia Lawall, Thomas Gleixner,
	Linus Torvalds

On Fri, May 28, 2010 at 09:45:16AM -0700, H. Peter Anvin wrote:
> On 05/28/2010 12:11 AM, Roedel, Joerg wrote:
> > On Thu, May 27, 2010 at 07:42:25AM -0400, Nicolas Palix wrote:
> >>> We have submitted and received some feedback on an initial version of 
> >>> this, but I'm not completely sure of the current status.
> >>
> >> You can see the latest feedback we get at
> >> http://lkml.org/lkml/2010/5/10/257
> >>
> >> The initial submission and its comments are at
> >> http://lkml.org/lkml/2010/4/26/269
> > 
> > I've also sent some feedback. Would be cool if you could work the
> > feedback in and do a repost asking Andrew to take it. Would be cool to
> > have this merged with 2.6.36.
> > 
> 
> I don't see why scripts that don't *in themselves* change the output
> binaries need to wait for .36.  Instead, it would be better to get them
> in sooner to make them available to developers in advance of the .36 cycle.
> 
> Of course, I'm not Linus, and I don't see him Cc:'d on this, but that
> would be the normal rules.

Yeah, merging it now would be even better. Since these scripts don't
change the kernel itself there is little risk to break anything. So how
about working in the feedback and sending an updated version to Linus
soon?

	Joerg


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-06-01  9:58             ` Joerg Roedel
  0 siblings, 0 replies; 26+ messages in thread
From: Joerg Roedel @ 2010-06-01  9:58 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Roedel, Joerg, Nicolas Palix, x86, kernel-janitors, linux-kernel,
	iommu, Ingo Molnar, Julia Lawall, Thomas Gleixner,
	Linus Torvalds

On Fri, May 28, 2010 at 09:45:16AM -0700, H. Peter Anvin wrote:
> On 05/28/2010 12:11 AM, Roedel, Joerg wrote:
> > On Thu, May 27, 2010 at 07:42:25AM -0400, Nicolas Palix wrote:
> >>> We have submitted and received some feedback on an initial version of 
> >>> this, but I'm not completely sure of the current status.
> >>
> >> You can see the latest feedback we get at
> >> http://lkml.org/lkml/2010/5/10/257
> >>
> >> The initial submission and its comments are at
> >> http://lkml.org/lkml/2010/4/26/269
> > 
> > I've also sent some feedback. Would be cool if you could work the
> > feedback in and do a repost asking Andrew to take it. Would be cool to
> > have this merged with 2.6.36.
> > 
> 
> I don't see why scripts that don't *in themselves* change the output
> binaries need to wait for .36.  Instead, it would be better to get them
> in sooner to make them available to developers in advance of the .36 cycle.
> 
> Of course, I'm not Linus, and I don't see him Cc:'d on this, but that
> would be the normal rules.

Yeah, merging it now would be even better. Since these scripts don't
change the kernel itself there is little risk to break anything. So how
about working in the feedback and sending an updated version to Linus
soon?

	Joerg


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
  2010-05-26 15:55 ` Julia Lawall
@ 2010-06-01 21:15   ` Andrew Morton
  -1 siblings, 0 replies; 26+ messages in thread
From: Andrew Morton @ 2010-06-01 21:15 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Joerg Roedel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

On Wed, 26 May 2010 17:55:59 +0200 (CEST)
Julia Lawall <julia@diku.dk> wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> Add a spin_unlock missing on the error path.  The locks and unlocks are
> balanced in other functions, so it seems that the same should be the case
> here.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression E1;
> @@
> 
> * spin_lock(E1,...);
>   <+... when != E1
>   if (...) {
>     ... when != E1
> *   return ...;
>   }
>   ...+>
> * spin_unlock(E1,...);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  arch/x86/kernel/amd_iommu.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> index fa5a147..b98e1cd 100644
> --- a/arch/x86/kernel/amd_iommu.c
> +++ b/arch/x86/kernel/amd_iommu.c
> @@ -1499,12 +1499,16 @@ static int __attach_device(struct device *dev,
>  
>  	/* Some sanity checks */
>  	if (alias_data->domain != NULL &&
> -	    alias_data->domain != domain)
> +	    alias_data->domain != domain) {
> +		spin_unlock(&domain->lock);
>  		return -EBUSY;
> +	}
>  
>  	if (dev_data->domain != NULL &&
> -	    dev_data->domain != domain)
> +	    dev_data->domain != domain) {
> +		spin_unlock(&domain->lock);
>  		return -EBUSY;
> +	}
>  
>  	/* Do real assignment */
>  	if (dev_data->alias != dev) {

The reason why these bugs occur is that we sprinkle multiple `return'
statements inside the middle of non-trivial functions.  People miss
some or fail to modify some when later changing locking rules and we
gain bugs (or, similarly, resource leaks).

So I'd suggest that when fixing such bugs, we also fix their cause.

ie:

--- a/arch/x86/kernel/amd_iommu.c~arch-x86-kernel-add-missing-spin_unlock
+++ a/arch/x86/kernel/amd_iommu.c
@@ -1487,6 +1487,7 @@ static int __attach_device(struct device
 			   struct protection_domain *domain)
 {
 	struct iommu_dev_data *dev_data, *alias_data;
+	int ret;
 
 	dev_data   = get_dev_data(dev);
 	alias_data = get_dev_data(dev_data->alias);
@@ -1497,14 +1498,17 @@ static int __attach_device(struct device
 	/* lock domain */
 	spin_lock(&domain->lock);
 
+	ret = -EBUSY;
 	/* Some sanity checks */
 	if (alias_data->domain != NULL &&
 	    alias_data->domain != domain)
-		return -EBUSY;
+		goto out;
 
 	if (dev_data->domain != NULL &&
 	    dev_data->domain != domain)
-		return -EBUSY;
+		goto out;
+
+	ret = 0;
 
 	/* Do real assignment */
 	if (dev_data->alias != dev) {
@@ -1522,8 +1526,8 @@ static int __attach_device(struct device
 
 	/* ready */
 	spin_unlock(&domain->lock);
-
-	return 0;
+out:
+	return ret;
 }
 
 /*
_


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-06-01 21:15   ` Andrew Morton
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Morton @ 2010-06-01 21:15 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Joerg Roedel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

On Wed, 26 May 2010 17:55:59 +0200 (CEST)
Julia Lawall <julia@diku.dk> wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> Add a spin_unlock missing on the error path.  The locks and unlocks are
> balanced in other functions, so it seems that the same should be the case
> here.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression E1;
> @@
> 
> * spin_lock(E1,...);
>   <+... when != E1
>   if (...) {
>     ... when != E1
> *   return ...;
>   }
>   ...+>
> * spin_unlock(E1,...);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  arch/x86/kernel/amd_iommu.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> index fa5a147..b98e1cd 100644
> --- a/arch/x86/kernel/amd_iommu.c
> +++ b/arch/x86/kernel/amd_iommu.c
> @@ -1499,12 +1499,16 @@ static int __attach_device(struct device *dev,
>  
>  	/* Some sanity checks */
>  	if (alias_data->domain != NULL &&
> -	    alias_data->domain != domain)
> +	    alias_data->domain != domain) {
> +		spin_unlock(&domain->lock);
>  		return -EBUSY;
> +	}
>  
>  	if (dev_data->domain != NULL &&
> -	    dev_data->domain != domain)
> +	    dev_data->domain != domain) {
> +		spin_unlock(&domain->lock);
>  		return -EBUSY;
> +	}
>  
>  	/* Do real assignment */
>  	if (dev_data->alias != dev) {

The reason why these bugs occur is that we sprinkle multiple `return'
statements inside the middle of non-trivial functions.  People miss
some or fail to modify some when later changing locking rules and we
gain bugs (or, similarly, resource leaks).

So I'd suggest that when fixing such bugs, we also fix their cause.

ie:

--- a/arch/x86/kernel/amd_iommu.c~arch-x86-kernel-add-missing-spin_unlock
+++ a/arch/x86/kernel/amd_iommu.c
@@ -1487,6 +1487,7 @@ static int __attach_device(struct device
 			   struct protection_domain *domain)
 {
 	struct iommu_dev_data *dev_data, *alias_data;
+	int ret;
 
 	dev_data   = get_dev_data(dev);
 	alias_data = get_dev_data(dev_data->alias);
@@ -1497,14 +1498,17 @@ static int __attach_device(struct device
 	/* lock domain */
 	spin_lock(&domain->lock);
 
+	ret = -EBUSY;
 	/* Some sanity checks */
 	if (alias_data->domain != NULL &&
 	    alias_data->domain != domain)
-		return -EBUSY;
+		goto out;
 
 	if (dev_data->domain != NULL &&
 	    dev_data->domain != domain)
-		return -EBUSY;
+		goto out;
+
+	ret = 0;
 
 	/* Do real assignment */
 	if (dev_data->alias != dev) {
@@ -1522,8 +1526,8 @@ static int __attach_device(struct device
 
 	/* ready */
 	spin_unlock(&domain->lock);
-
-	return 0;
+out:
+	return ret;
 }
 
 /*
_


^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
  2010-06-01 21:15   ` Andrew Morton
@ 2010-06-01 21:47     ` Thomas Gleixner
  -1 siblings, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2010-06-01 21:47 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Julia Lawall, Joerg Roedel, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

On Tue, 1 Jun 2010, Andrew Morton wrote:

> On Wed, 26 May 2010 17:55:59 +0200 (CEST)
> Julia Lawall <julia@diku.dk> wrote:
> 
> > From: Julia Lawall <julia@diku.dk>
> > 
> > Add a spin_unlock missing on the error path.  The locks and unlocks are
> > balanced in other functions, so it seems that the same should be the case
> > here.
> > 
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@
> > expression E1;
> > @@
> > 
> > * spin_lock(E1,...);
> >   <+... when != E1
> >   if (...) {
> >     ... when != E1
> > *   return ...;
> >   }
> >   ...+>
> > * spin_unlock(E1,...);
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  arch/x86/kernel/amd_iommu.c |    8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> > index fa5a147..b98e1cd 100644
> > --- a/arch/x86/kernel/amd_iommu.c
> > +++ b/arch/x86/kernel/amd_iommu.c
> > @@ -1499,12 +1499,16 @@ static int __attach_device(struct device *dev,
> >  
> >  	/* Some sanity checks */
> >  	if (alias_data->domain != NULL &&
> > -	    alias_data->domain != domain)
> > +	    alias_data->domain != domain) {
> > +		spin_unlock(&domain->lock);
> >  		return -EBUSY;
> > +	}
> >  
> >  	if (dev_data->domain != NULL &&
> > -	    dev_data->domain != domain)
> > +	    dev_data->domain != domain) {
> > +		spin_unlock(&domain->lock);
> >  		return -EBUSY;
> > +	}
> >  
> >  	/* Do real assignment */
> >  	if (dev_data->alias != dev) {
> 
> The reason why these bugs occur is that we sprinkle multiple `return'
> statements inside the middle of non-trivial functions.  People miss
> some or fail to modify some when later changing locking rules and we
> gain bugs (or, similarly, resource leaks).
> 
> So I'd suggest that when fixing such bugs, we also fix their cause.
> 
> ie:
> 
> --- a/arch/x86/kernel/amd_iommu.c~arch-x86-kernel-add-missing-spin_unlock
> +++ a/arch/x86/kernel/amd_iommu.c
> @@ -1487,6 +1487,7 @@ static int __attach_device(struct device
>  			   struct protection_domain *domain)
>  {
>  	struct iommu_dev_data *dev_data, *alias_data;
> +	int ret;
>  
>  	dev_data   = get_dev_data(dev);
>  	alias_data = get_dev_data(dev_data->alias);
> @@ -1497,14 +1498,17 @@ static int __attach_device(struct device
>  	/* lock domain */
>  	spin_lock(&domain->lock);
>  
> +	ret = -EBUSY;
>  	/* Some sanity checks */
>  	if (alias_data->domain != NULL &&
>  	    alias_data->domain != domain)
> -		return -EBUSY;
> +		goto out;
>  
>  	if (dev_data->domain != NULL &&
>  	    dev_data->domain != domain)
> -		return -EBUSY;
> +		goto out;
> +
> +	ret = 0;
>  
>  	/* Do real assignment */
>  	if (dev_data->alias != dev) {
> @@ -1522,8 +1526,8 @@ static int __attach_device(struct device
>  
>  	/* ready */
>  	spin_unlock(&domain->lock);
> -
> -	return 0;
> +out:

Moving the label _before_ spin_unlock() might fix it really. :)

> +	return ret;
>  }

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-06-01 21:47     ` Thomas Gleixner
  0 siblings, 0 replies; 26+ messages in thread
From: Thomas Gleixner @ 2010-06-01 21:47 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Julia Lawall, Joerg Roedel, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

On Tue, 1 Jun 2010, Andrew Morton wrote:

> On Wed, 26 May 2010 17:55:59 +0200 (CEST)
> Julia Lawall <julia@diku.dk> wrote:
> 
> > From: Julia Lawall <julia@diku.dk>
> > 
> > Add a spin_unlock missing on the error path.  The locks and unlocks are
> > balanced in other functions, so it seems that the same should be the case
> > here.
> > 
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@
> > expression E1;
> > @@
> > 
> > * spin_lock(E1,...);
> >   <+... when != E1
> >   if (...) {
> >     ... when != E1
> > *   return ...;
> >   }
> >   ...+>
> > * spin_unlock(E1,...);
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  arch/x86/kernel/amd_iommu.c |    8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> > index fa5a147..b98e1cd 100644
> > --- a/arch/x86/kernel/amd_iommu.c
> > +++ b/arch/x86/kernel/amd_iommu.c
> > @@ -1499,12 +1499,16 @@ static int __attach_device(struct device *dev,
> >  
> >  	/* Some sanity checks */
> >  	if (alias_data->domain != NULL &&
> > -	    alias_data->domain != domain)
> > +	    alias_data->domain != domain) {
> > +		spin_unlock(&domain->lock);
> >  		return -EBUSY;
> > +	}
> >  
> >  	if (dev_data->domain != NULL &&
> > -	    dev_data->domain != domain)
> > +	    dev_data->domain != domain) {
> > +		spin_unlock(&domain->lock);
> >  		return -EBUSY;
> > +	}
> >  
> >  	/* Do real assignment */
> >  	if (dev_data->alias != dev) {
> 
> The reason why these bugs occur is that we sprinkle multiple `return'
> statements inside the middle of non-trivial functions.  People miss
> some or fail to modify some when later changing locking rules and we
> gain bugs (or, similarly, resource leaks).
> 
> So I'd suggest that when fixing such bugs, we also fix their cause.
> 
> ie:
> 
> --- a/arch/x86/kernel/amd_iommu.c~arch-x86-kernel-add-missing-spin_unlock
> +++ a/arch/x86/kernel/amd_iommu.c
> @@ -1487,6 +1487,7 @@ static int __attach_device(struct device
>  			   struct protection_domain *domain)
>  {
>  	struct iommu_dev_data *dev_data, *alias_data;
> +	int ret;
>  
>  	dev_data   = get_dev_data(dev);
>  	alias_data = get_dev_data(dev_data->alias);
> @@ -1497,14 +1498,17 @@ static int __attach_device(struct device
>  	/* lock domain */
>  	spin_lock(&domain->lock);
>  
> +	ret = -EBUSY;
>  	/* Some sanity checks */
>  	if (alias_data->domain != NULL &&
>  	    alias_data->domain != domain)
> -		return -EBUSY;
> +		goto out;
>  
>  	if (dev_data->domain != NULL &&
>  	    dev_data->domain != domain)
> -		return -EBUSY;
> +		goto out;
> +
> +	ret = 0;
>  
>  	/* Do real assignment */
>  	if (dev_data->alias != dev) {
> @@ -1522,8 +1526,8 @@ static int __attach_device(struct device
>  
>  	/* ready */
>  	spin_unlock(&domain->lock);
> -
> -	return 0;
> +out:

Moving the label _before_ spin_unlock() might fix it really. :)

> +	return ret;
>  }

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
  2010-06-01 21:15   ` Andrew Morton
@ 2010-06-02  5:29     ` Julia Lawall
  -1 siblings, 0 replies; 26+ messages in thread
From: Julia Lawall @ 2010-06-02  5:29 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Joerg Roedel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

On Tue, 1 Jun 2010, Andrew Morton wrote:

> On Wed, 26 May 2010 17:55:59 +0200 (CEST)
> Julia Lawall <julia@diku.dk> wrote:
> 
> > From: Julia Lawall <julia@diku.dk>
> > 
> > Add a spin_unlock missing on the error path.  The locks and unlocks are
> > balanced in other functions, so it seems that the same should be the case
> > here.
> > 
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@
> > expression E1;
> > @@
> > 
> > * spin_lock(E1,...);
> >   <+... when != E1
> >   if (...) {
> >     ... when != E1
> > *   return ...;
> >   }
> >   ...+>
> > * spin_unlock(E1,...);
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  arch/x86/kernel/amd_iommu.c |    8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> > index fa5a147..b98e1cd 100644
> > --- a/arch/x86/kernel/amd_iommu.c
> > +++ b/arch/x86/kernel/amd_iommu.c
> > @@ -1499,12 +1499,16 @@ static int __attach_device(struct device *dev,
> >  
> >  	/* Some sanity checks */
> >  	if (alias_data->domain != NULL &&
> > -	    alias_data->domain != domain)
> > +	    alias_data->domain != domain) {
> > +		spin_unlock(&domain->lock);
> >  		return -EBUSY;
> > +	}
> >  
> >  	if (dev_data->domain != NULL &&
> > -	    dev_data->domain != domain)
> > +	    dev_data->domain != domain) {
> > +		spin_unlock(&domain->lock);
> >  		return -EBUSY;
> > +	}
> >  
> >  	/* Do real assignment */
> >  	if (dev_data->alias != dev) {
> 
> The reason why these bugs occur is that we sprinkle multiple `return'
> statements inside the middle of non-trivial functions.  People miss
> some or fail to modify some when later changing locking rules and we
> gain bugs (or, similarly, resource leaks).
> 
> So I'd suggest that when fixing such bugs, we also fix their cause.
> 
> ie:
> 
> --- a/arch/x86/kernel/amd_iommu.c~arch-x86-kernel-add-missing-spin_unlock
> +++ a/arch/x86/kernel/amd_iommu.c
> @@ -1487,6 +1487,7 @@ static int __attach_device(struct device
>  			   struct protection_domain *domain)
>  {
>  	struct iommu_dev_data *dev_data, *alias_data;
> +	int ret;
>  
>  	dev_data   = get_dev_data(dev);
>  	alias_data = get_dev_data(dev_data->alias);
> @@ -1497,14 +1498,17 @@ static int __attach_device(struct device
>  	/* lock domain */
>  	spin_lock(&domain->lock);
>  
> +	ret = -EBUSY;
>  	/* Some sanity checks */
>  	if (alias_data->domain != NULL &&
>  	    alias_data->domain != domain)
> -		return -EBUSY;
> +		goto out;
>  
>  	if (dev_data->domain != NULL &&
>  	    dev_data->domain != domain)
> -		return -EBUSY;
> +		goto out;
> +
> +	ret = 0;
>  
>  	/* Do real assignment */
>  	if (dev_data->alias != dev) {
> @@ -1522,8 +1526,8 @@ static int __attach_device(struct device
>  
>  	/* ready */
>  	spin_unlock(&domain->lock);
> -
> -	return 0;
> +out:
> +	return ret;
>  }

I don't have the impression that this actually fixes the problem, only the 
code structure.  Out should be above the spin_lock, and there should just 
be one return, of ret.  I will send another patch shortly.

julia

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-06-02  5:29     ` Julia Lawall
  0 siblings, 0 replies; 26+ messages in thread
From: Julia Lawall @ 2010-06-02  5:29 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Joerg Roedel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

On Tue, 1 Jun 2010, Andrew Morton wrote:

> On Wed, 26 May 2010 17:55:59 +0200 (CEST)
> Julia Lawall <julia@diku.dk> wrote:
> 
> > From: Julia Lawall <julia@diku.dk>
> > 
> > Add a spin_unlock missing on the error path.  The locks and unlocks are
> > balanced in other functions, so it seems that the same should be the case
> > here.
> > 
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@
> > expression E1;
> > @@
> > 
> > * spin_lock(E1,...);
> >   <+... when != E1
> >   if (...) {
> >     ... when != E1
> > *   return ...;
> >   }
> >   ...+>
> > * spin_unlock(E1,...);
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  arch/x86/kernel/amd_iommu.c |    8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> > index fa5a147..b98e1cd 100644
> > --- a/arch/x86/kernel/amd_iommu.c
> > +++ b/arch/x86/kernel/amd_iommu.c
> > @@ -1499,12 +1499,16 @@ static int __attach_device(struct device *dev,
> >  
> >  	/* Some sanity checks */
> >  	if (alias_data->domain != NULL &&
> > -	    alias_data->domain != domain)
> > +	    alias_data->domain != domain) {
> > +		spin_unlock(&domain->lock);
> >  		return -EBUSY;
> > +	}
> >  
> >  	if (dev_data->domain != NULL &&
> > -	    dev_data->domain != domain)
> > +	    dev_data->domain != domain) {
> > +		spin_unlock(&domain->lock);
> >  		return -EBUSY;
> > +	}
> >  
> >  	/* Do real assignment */
> >  	if (dev_data->alias != dev) {
> 
> The reason why these bugs occur is that we sprinkle multiple `return'
> statements inside the middle of non-trivial functions.  People miss
> some or fail to modify some when later changing locking rules and we
> gain bugs (or, similarly, resource leaks).
> 
> So I'd suggest that when fixing such bugs, we also fix their cause.
> 
> ie:
> 
> --- a/arch/x86/kernel/amd_iommu.c~arch-x86-kernel-add-missing-spin_unlock
> +++ a/arch/x86/kernel/amd_iommu.c
> @@ -1487,6 +1487,7 @@ static int __attach_device(struct device
>  			   struct protection_domain *domain)
>  {
>  	struct iommu_dev_data *dev_data, *alias_data;
> +	int ret;
>  
>  	dev_data   = get_dev_data(dev);
>  	alias_data = get_dev_data(dev_data->alias);
> @@ -1497,14 +1498,17 @@ static int __attach_device(struct device
>  	/* lock domain */
>  	spin_lock(&domain->lock);
>  
> +	ret = -EBUSY;
>  	/* Some sanity checks */
>  	if (alias_data->domain != NULL &&
>  	    alias_data->domain != domain)
> -		return -EBUSY;
> +		goto out;
>  
>  	if (dev_data->domain != NULL &&
>  	    dev_data->domain != domain)
> -		return -EBUSY;
> +		goto out;
> +
> +	ret = 0;
>  
>  	/* Do real assignment */
>  	if (dev_data->alias != dev) {
> @@ -1522,8 +1526,8 @@ static int __attach_device(struct device
>  
>  	/* ready */
>  	spin_unlock(&domain->lock);
> -
> -	return 0;
> +out:
> +	return ret;
>  }

I don't have the impression that this actually fixes the problem, only the 
code structure.  Out should be above the spin_lock, and there should just 
be one return, of ret.  I will send another patch shortly.

julia

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
  2010-06-01 21:15   ` Andrew Morton
@ 2010-06-02  8:38     ` Roedel, Joerg
  -1 siblings, 0 replies; 26+ messages in thread
From: Roedel, Joerg @ 2010-06-02  8:38 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Julia Lawall, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

Hi Andrew,

On Tue, Jun 01, 2010 at 05:15:29PM -0400, Andrew Morton wrote:
> The reason why these bugs occur is that we sprinkle multiple `return'
> statements inside the middle of non-trivial functions.  People miss
> some or fail to modify some when later changing locking rules and we
> gain bugs (or, similarly, resource leaks).

Right. I changed that in Julia's patch too before merging it into my
tree. It is already in -tip. See
http://git.kernel.org/tip/84fe6c19e4a598e8071e3bd1b2c923454eae1268

	Joerg



^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-06-02  8:38     ` Roedel, Joerg
  0 siblings, 0 replies; 26+ messages in thread
From: Roedel, Joerg @ 2010-06-02  8:38 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Julia Lawall, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

Hi Andrew,

On Tue, Jun 01, 2010 at 05:15:29PM -0400, Andrew Morton wrote:
> The reason why these bugs occur is that we sprinkle multiple `return'
> statements inside the middle of non-trivial functions.  People miss
> some or fail to modify some when later changing locking rules and we
> gain bugs (or, similarly, resource leaks).

Right. I changed that in Julia's patch too before merging it into my
tree. It is already in -tip. See
http://git.kernel.org/tip/84fe6c19e4a598e8071e3bd1b2c923454eae1268

	Joerg



^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
  2010-06-02  8:38     ` Roedel, Joerg
@ 2010-06-02  8:42       ` Julia Lawall
  -1 siblings, 0 replies; 26+ messages in thread
From: Julia Lawall @ 2010-06-02  8:42 UTC (permalink / raw)
  To: Roedel, Joerg
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

On Wed, 2 Jun 2010, Roedel, Joerg wrote:

> Hi Andrew,
> 
> On Tue, Jun 01, 2010 at 05:15:29PM -0400, Andrew Morton wrote:
> > The reason why these bugs occur is that we sprinkle multiple `return'
> > statements inside the middle of non-trivial functions.  People miss
> > some or fail to modify some when later changing locking rules and we
> > gain bugs (or, similarly, resource leaks).
> 
> Right. I changed that in Julia's patch too before merging it into my
> tree. It is already in -tip. See
> http://git.kernel.org/tip/84fe6c19e4a598e8071e3bd1b2c923454eae1268

Thanks.  That looks fine.

julia

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
@ 2010-06-02  8:42       ` Julia Lawall
  0 siblings, 0 replies; 26+ messages in thread
From: Julia Lawall @ 2010-06-02  8:42 UTC (permalink / raw)
  To: Roedel, Joerg
  Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	iommu, linux-kernel, kernel-janitors

On Wed, 2 Jun 2010, Roedel, Joerg wrote:

> Hi Andrew,
> 
> On Tue, Jun 01, 2010 at 05:15:29PM -0400, Andrew Morton wrote:
> > The reason why these bugs occur is that we sprinkle multiple `return'
> > statements inside the middle of non-trivial functions.  People miss
> > some or fail to modify some when later changing locking rules and we
> > gain bugs (or, similarly, resource leaks).
> 
> Right. I changed that in Julia's patch too before merging it into my
> tree. It is already in -tip. See
> http://git.kernel.org/tip/84fe6c19e4a598e8071e3bd1b2c923454eae1268

Thanks.  That looks fine.

julia

^ permalink raw reply	[flat|nested] 26+ messages in thread
end of thread, other threads:[~2010-06-02  8:42 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-05-26 15:55 [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock Julia Lawall
2010-05-26 15:55 ` Julia Lawall
2010-05-27 11:06 ` Roedel, Joerg
2010-05-27 11:06   ` Roedel, Joerg
2010-05-27 11:11 ` Roedel, Joerg
2010-05-27 11:11   ` Roedel, Joerg
2010-05-27 11:17   ` Julia Lawall
2010-05-27 11:17     ` Julia Lawall
2010-05-27 11:42     ` Nicolas Palix
2010-05-27 11:42       ` Nicolas Palix
2010-05-28  7:11       ` Roedel, Joerg
2010-05-28  7:11         ` Roedel, Joerg
2010-05-28 16:45         ` H. Peter Anvin
2010-05-28 16:45           ` H. Peter Anvin
2010-06-01  9:58           ` Joerg Roedel
2010-06-01  9:58             ` Joerg Roedel
2010-06-01 21:15 ` Andrew Morton
2010-06-01 21:15   ` Andrew Morton
2010-06-01 21:47   ` Thomas Gleixner
2010-06-01 21:47     ` Thomas Gleixner
2010-06-02  5:29   ` Julia Lawall
2010-06-02  5:29     ` Julia Lawall
2010-06-02  8:38   ` Roedel, Joerg
2010-06-02  8:38     ` Roedel, Joerg
2010-06-02  8:42     ` Julia Lawall
2010-06-02  8:42       ` Julia Lawall

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.