* [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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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.org, kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, 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.org, kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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@kernel.org, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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.