PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

dev_pm_put_subsys_data() calls kfree() while holding device power lock, when the reference count is 0. Fix it to call kfree() after releasing the lock.

Signed-off-by Shuah Khan <[email protected]>

---
 drivers/base/power/common.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 39c3252..da05fe2 100644

drivers/base/power/common.c +0 -1

@@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
 
 	if (--psd->refcount == 0) {
 		dev->power.subsys_data = NULL;
-		kfree(psd);
 		ret = 1;
 	}
 

Hi!

Quoted 8 lines

dev_pm_put_subsys_data() calls kfree() while holding device power lock, when the reference count is 0. Fix it to call kfree() after releasing the lock.

Signed-off-by Shuah Khan <[email protected]>

---
 drivers/base/power/common.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

w> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c

Quoted 14 lines

index 39c3252..da05fe2 100644

drivers/base/power/common.c +0 -1

@@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
 
 	if (--psd->refcount == 0) {
 		dev->power.subsys_data = NULL;
-		kfree(psd);
 		ret = 1;
 	}
 
  out:
 	spin_unlock_irq(&dev->power.lock);
 

drivers/base/power/common.c int dev_pm_put_subsys_data(struct device *dev) · line 88

Quoted 7 lines

drivers/base/power/common.c +5 -0

+	if (ret == 1) {
+		/* kfree() verifies that its argument is nonzero. */
+		kfree(psd);
+	}
+
 	return ret;
 }

Wow, that function is fragile. It returns 0/1/-EINVAL, while being documented for 0/1...

Patch does not look obviously wrong, but maybe

(quoted diff) +2 -2

@@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
 
 	if (--psd->refcount == 0) {
 		dev->power.subsys_data = NULL;
+		spin_unlock_irq(&dev->power.lock);
-		kfree(psd);
- 		ret = 1;
+		return 1;
 	}
 

Would be cleaner.

Quoted 2 lines

Wow, that function is fragile. It returns 0/1/-EINVAL, while being documented for 0/1...

Oh, it generally should return 1 for !psd.

Quoted 12 lines

Patch does not look obviously wrong, but maybe

drivers/base/power/common.c +2 -2

@@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
 
 	if (--psd->refcount == 0) {
 		dev->power.subsys_data = NULL;
+		spin_unlock_irq(&dev->power.lock);
-		kfree(psd);
- 		ret = 1;
+		return 1;
 	}
 

Quoted 1 line

Would be cleaner.

What about this:

Hi!

Quoted 17 lines

Quoted 2 lines

Wow, that function is fragile. It returns 0/1/-EINVAL, while being documented for 0/1...

Oh, it generally should return 1 for !psd.

Quoted 12 lines

Patch does not look obviously wrong, but maybe

drivers/base/power/common.c +2 -2

@@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
 
 	if (--psd->refcount == 0) {
 		dev->power.subsys_data = NULL;
+		spin_unlock_irq(&dev->power.lock);
-		kfree(psd);
- 		ret = 1;
+		return 1;
 	}
 

Quoted 3 lines

Quoted 1 line

Would be cleaner.

What about this:

Looks good to me.

Reviewed-by Pavel Machek <[email protected]>

Quoted 19 lines · Pavel Machek

Hi!

Quoted 17 lines

Quoted 2 lines

Wow, that function is fragile. It returns 0/1/-EINVAL, while being documented for 0/1...

Oh, it generally should return 1 for !psd.

Quoted 12 lines

Patch does not look obviously wrong, but maybe

drivers/base/power/common.c +2 -2

@@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
 
 	if (--psd->refcount == 0) {
 		dev->power.subsys_data = NULL;
+		spin_unlock_irq(&dev->power.lock);
-		kfree(psd);
- 		ret = 1;
+		return 1;
 	}
 

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Would be cleaner.

What about this:

Looks good to me.

Reviewed-by Pavel Machek <[email protected]>

Rafael/Pavel,

I redid the patch based on Pavel's comments and just about to send it and then I saw your exchange. This version looks good to me. Do you want me to test the patch and resend?

thanks,
-- Shuah

dev_pm_put_subsys_data() calls kfree() while holding device power lock, when the reference count is 0. Fix it to call kfree() after releasing the lock.

Signed-off-by Shuah Khan <[email protected]>

Reviewed-by Pavel Machek <[email protected]>

Reviewed-by Rafael Wysocki <[email protected]>

---
 drivers/base/power/common.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Quoted 9 lines · Shuah Khan

dev_pm_put_subsys_data() calls kfree() while holding device power lock, when the reference count is 0. Fix it to call kfree() after releasing the lock.

Signed-off-by Shuah Khan <[email protected]>

Reviewed-by Pavel Machek <[email protected]>

Reviewed-by Rafael Wysocki <[email protected]>

---
 drivers/base/power/common.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Is this causing a problem now, and it should go into 3.10 and earlier kernels (if so, which ones?), or can it just wait until 3.11 as it's just a cleanup fix?

Quoted 9 lines · Shuah Khan

dev_pm_put_subsys_data() calls kfree() while holding device power lock, when the reference count is 0. Fix it to call kfree() after releasing the lock.

Signed-off-by Shuah Khan <[email protected]>

Reviewed-by Pavel Machek <[email protected]>

Reviewed-by Rafael Wysocki <[email protected]>

---
 drivers/base/power/common.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Is this causing a problem now, and it should go into 3.10 and earlier kernels (if so, which ones?), or can it just wait until 3.11 as it's just a cleanup fix?

Quoted 11 lines · On Monday, May 06, 2013 12:41:19 PM [email protected] wrote:

Quoted 9 lines · Shuah Khan

dev_pm_put_subsys_data() calls kfree() while holding device power lock, when the reference count is 0. Fix it to call kfree() after releasing the lock.

Signed-off-by Shuah Khan <[email protected]>

Reviewed-by Pavel Machek <[email protected]>

Reviewed-by Rafael Wysocki <[email protected]>

---
 drivers/base/power/common.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Quoted 3 lines

Is this causing a problem now, and it should go into 3.10 and earlier kernels (if so, which ones?), or can it just wait until 3.11 as it's just a cleanup fix?

I think it's OK to push the fix for 3.10, but I'll take care of this (I have a bunch of fixes for 3.10 anyway, but I'll send a pull request after -rc1, because people are sending me fixes pretty much continuously ATM).