| Message ID | 1882177.s6zpHZ6crc@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
Hi! > > Wow, that function is fragile. It returns 0/1/-EINVAL, while being > > documented for 0/1... > > Oh, it generally should return 1 for !psd. > > > Patch does not look obviously wrong, but maybe > > > > @@ -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. > > What about this: Looks good to me. Reviewed-by: Pavel Machek <pavel@ucw.cz> Pavel
On Mon, 2013-05-06 at 14:09 +0200, Pavel Machek wrote: > Hi! > > > > Wow, that function is fragile. It returns 0/1/-EINVAL, while being > > > documented for 0/1... > > > > Oh, it generally should return 1 for !psd. > > > > > Patch does not look obviously wrong, but maybe > > > > > > @@ -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. > > > > What about this: > > Looks good to me. > > Reviewed-by: Pavel Machek <pavel@ucw.cz> 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
ZGV2X3BtX3B1dF9zdWJzeXNfZGF0YSgpIGNhbGxzIGtmcmVlKCkgd2hpbGUgaG9sZGluZyBkZXZp Y2UgcG93ZXIgbG9jaywgd2hlbg0KdGhlIHJlZmVyZW5jZSBjb3VudCBpcyAwLiBGaXggaXQgdG8g Y2FsbCBrZnJlZSgpIGFmdGVyIHJlbGVhc2luZyB0aGUgbG9jay4NCg0KU2lnbmVkLW9mZi1ieTog U2h1YWggS2hhbiA8c2h1YWgua2hAc2Ftc3VuZy5jb20+DQpSZXZpZXdlZC1ieTogUGF2ZWwgTWFj aGVrIDxwYXZlbEB1Y3cuY3o+DQpSZXZpZXdlZC1ieTogUmFmYWVsIFd5c29ja2kgPHJhZmFlbC5q Lnd5c29ja2lAaW50ZWwuY29tPg0KLS0tDQogZHJpdmVycy9iYXNlL3Bvd2VyL2NvbW1vbi5jIHwg ICAgOCArKysrKy0tLQ0KIDEgZmlsZSBjaGFuZ2VkLCA1IGluc2VydGlvbnMoKyksIDMgZGVsZXRp b25zKC0pDQoNCmRpZmYgLS1naXQgYS9kcml2ZXJzL2Jhc2UvcG93ZXIvY29tbW9uLmMgYi9kcml2 ZXJzL2Jhc2UvcG93ZXIvY29tbW9uLmMNCmluZGV4IDM5YzMyNTIuLmU1Yjk5ZjcgMTAwNjQ0DQot LS0gYS9kcml2ZXJzL2Jhc2UvcG93ZXIvY29tbW9uLmMNCisrKyBiL2RyaXZlcnMvYmFzZS9wb3dl ci9jb21tb24uYw0KQEAgLTYxLDI0ICs2MSwyNiBAQCBFWFBPUlRfU1lNQk9MX0dQTChkZXZfcG1f Z2V0X3N1YnN5c19kYXRhKTsNCiBpbnQgZGV2X3BtX3B1dF9zdWJzeXNfZGF0YShzdHJ1Y3QgZGV2 aWNlICpkZXYpDQogew0KIAlzdHJ1Y3QgcG1fc3Vic3lzX2RhdGEgKnBzZDsNCi0JaW50IHJldCA9 IDA7DQorCWludCByZXQgPSAxOw0KIA0KIAlzcGluX2xvY2tfaXJxKCZkZXYtPnBvd2VyLmxvY2sp Ow0KIA0KIAlwc2QgPSBkZXZfdG9fcHNkKGRldik7DQogCWlmICghcHNkKSB7DQotCQlyZXQgPSAt RUlOVkFMOw0KIAkJZ290byBvdXQ7DQogCX0NCiANCiAJaWYgKC0tcHNkLT5yZWZjb3VudCA9PSAw KSB7DQogCQlkZXYtPnBvd2VyLnN1YnN5c19kYXRhID0gTlVMTDsNCi0JCWtmcmVlKHBzZCk7DQog CQlyZXQgPSAxOw0KKwl9IGVsc2Ugew0KKwkJcHNkID0gTlVMTDsNCisJCXJldCA9IDA7DQogCX0N CiANCiAgb3V0Og0KIAlzcGluX3VubG9ja19pcnEoJmRldi0+cG93ZXIubG9jayk7DQorCWtmcmVl KHBzZCk7DQogDQogCXJldHVybiByZXQ7DQogfQ0KLS0gDQoxLjcuMTAuNA0K -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-05-06 at 08:00 -0600, Shuah Khan wrote: > On Mon, 2013-05-06 at 14:09 +0200, Pavel Machek wrote: > > Hi! > > > > > > Wow, that function is fragile. It returns 0/1/-EINVAL, while being > > > > documented for 0/1... > > > > > > Oh, it generally should return 1 for !psd. > > > > > > > Patch does not look obviously wrong, but maybe > > > > > > > > @@ -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. > > > > > > What about this: > > > > Looks good to me. > > > > Reviewed-by: Pavel Machek <pavel@ucw.cz> > > 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 I sent v2 patch that incorporates the comments from Rafael and Pavel. thanks, -- Shuah Shuah Khan Lead Kernel Developer - Open Source Group Samsung Research America (Silicon Valley) shuah.kh@samsung.com | (970) 672-0658
On Mon, May 06, 2013 at 07:04:53PM +0000, Shuah Khan wrote: > 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 <shuah.kh@samsung.com> > Reviewed-by: Pavel Machek <pavel@ucw.cz> > Reviewed-by: Rafael Wysocki <rafael.j.wysocki@intel.com> > --- > 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? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
T24gTW9uLCAyMDEzLTA1LTA2IGF0IDEyOjQxIC0wNzAwLCBncmVna2hAbGludXhmb3VuZGF0aW9u Lm9yZyB3cm90ZToNCj4gT24gTW9uLCBNYXkgMDYsIDIwMTMgYXQgMDc6MDQ6NTNQTSArMDAwMCwg U2h1YWggS2hhbiB3cm90ZToNCj4gPiBkZXZfcG1fcHV0X3N1YnN5c19kYXRhKCkgY2FsbHMga2Zy ZWUoKSB3aGlsZSBob2xkaW5nIGRldmljZSBwb3dlciBsb2NrLCB3aGVuDQo+ID4gdGhlIHJlZmVy ZW5jZSBjb3VudCBpcyAwLiBGaXggaXQgdG8gY2FsbCBrZnJlZSgpIGFmdGVyIHJlbGVhc2luZyB0 aGUgbG9jay4NCj4gPiANCj4gPiBTaWduZWQtb2ZmLWJ5OiBTaHVhaCBLaGFuIDxzaHVhaC5raEBz YW1zdW5nLmNvbT4NCj4gPiBSZXZpZXdlZC1ieTogUGF2ZWwgTWFjaGVrIDxwYXZlbEB1Y3cuY3o+ DQo+ID4gUmV2aWV3ZWQtYnk6IFJhZmFlbCBXeXNvY2tpIDxyYWZhZWwuai53eXNvY2tpQGludGVs LmNvbT4NCj4gPiAtLS0NCj4gPiAgZHJpdmVycy9iYXNlL3Bvd2VyL2NvbW1vbi5jIHwgICAgOCAr KysrKy0tLQ0KPiA+ICAxIGZpbGUgY2hhbmdlZCwgNSBpbnNlcnRpb25zKCspLCAzIGRlbGV0aW9u cygtKQ0KPiANCj4gSXMgdGhpcyBjYXVzaW5nIGEgcHJvYmxlbSBub3csIGFuZCBpdCBzaG91bGQg Z28gaW50byAzLjEwIGFuZCBlYXJsaWVyDQo+IGtlcm5lbHMgKGlmIHNvLCB3aGljaCBvbmVzPyks IG9yIGNhbiBpdCBqdXN0IHdhaXQgdW50aWwgMy4xMSBhcyBpdCdzDQo+IGp1c3QgYSBjbGVhbnVw IGZpeD8NCg0KSXQgY2FuIHdhaXQgdW50aWwgMy4xMS4gQXQgdGhpcyBwb2ludCBJIGRvbid0IGhh dmUgYW55IGV2aWRlbmNlIHRoYXQNCnRoaXMgaXMgYWN0dWFsbHkgY2F1c2luZyBwcm9ibGVtcy4g U28gcGxlYXNlIHRyZWF0IHRoaXMgYXMgYSBjbGVhbnVwIGZpeA0KZm91bmQgZHVyaW5nIGNvZGUg cmV2aWV3Lg0KDQp0aGFua3MsDQotLSBTaHVhaA0KDQotLSANClNodWFoIEtoYW4gTGVhZCBLZXJu ZWwgRGV2ZWxvcGVyIC0gT3BlbiBTb3VyY2UgR3JvdXAgDQpTYW1zdW5nIFJlc2VhcmNoIEFtZXJp Y2EgKFNpbGljb24gVmFsbGV5KQ0Kc2h1YWgua2hAc2Ftc3VuZy5jb20gfCAoOTcwKSA2NzItMDY1 OA0K -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, May 06, 2013 12:41:19 PM gregkh@linuxfoundation.org wrote: > On Mon, May 06, 2013 at 07:04:53PM +0000, Shuah Khan wrote: > > 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 <shuah.kh@samsung.com> > > Reviewed-by: Pavel Machek <pavel@ucw.cz> > > Reviewed-by: Rafael Wysocki <rafael.j.wysocki@intel.com> > > --- > > 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? 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). Thanks, Rafael
On Mon, May 06, 2013 at 09:56:56PM +0200, Rafael J. Wysocki wrote: > On Monday, May 06, 2013 12:41:19 PM gregkh@linuxfoundation.org wrote: > > On Mon, May 06, 2013 at 07:04:53PM +0000, Shuah Khan wrote: > > > 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 <shuah.kh@samsung.com> > > > Reviewed-by: Pavel Machek <pavel@ucw.cz> > > > Reviewed-by: Rafael Wysocki <rafael.j.wysocki@intel.com> > > > --- > > > 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? > > 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). Ok, feel free to add: Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-pm/drivers/base/power/common.c =================================================================== --- linux-pm.orig/drivers/base/power/common.c +++ linux-pm/drivers/base/power/common.c @@ -61,24 +61,24 @@ EXPORT_SYMBOL_GPL(dev_pm_get_subsys_data int dev_pm_put_subsys_data(struct device *dev) { struct pm_subsys_data *psd; - int ret = 0; + int ret = 1; spin_lock_irq(&dev->power.lock); psd = dev_to_psd(dev); - if (!psd) { - ret = -EINVAL; + if (!psd) goto out; - } if (--psd->refcount == 0) { dev->power.subsys_data = NULL; - kfree(psd); - ret = 1; + } else { + psd = NULL; + ret = 0; } out: spin_unlock_irq(&dev->power.lock); + kfree(psd); return ret; }