| Message ID | 1eff59d9b51f8ade67bc1cf9f10683f9463ec9f6.1404996041.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
DQoNCj4g1NogMjAxNMTqN9TCMTDI1aOsMjA6NDGjrCJWaXJlc2ggS3VtYXIiIDx2aXJlc2gua3Vt YXJAbGluYXJvLm9yZz4g0LS1wKO6DQo+IA0KPiBUaGlzIGlzIG9ubHkgcmVsZXZhbnQgdG8gaW1w bGVtZW50YXRpb25zIHdpdGggbXVsdGlwbGUgY2x1c3RlcnMsIHdoZXJlIGNsdXN0ZXJzDQo+IGhh dmUgc2VwYXJhdGUgY2xvY2sgbGluZXMgYnV0IGFsbCBDUFVzIHdpdGhpbiBhIGNsdXN0ZXIgc2hh cmUgaXQuDQo+IA0KPiBDb25zaWRlciBhIGR1YWwgY2x1c3RlciBwbGF0Zm9ybSB3aXRoIDIgY29y ZXMgcGVyIGNsdXN0ZXIuIER1cmluZyBzdXNwZW5kIHdlDQo+IHN0YXJ0IG9mZmxpbmluZyBDUFVz IGZyb20gMSB0byAzLiBXaGVuIENQVTIgaXMgcmVtb3ZlLCBwb2xpY3ktPmtvYmogd291bGQgYmUN Cj4gbW92ZWQgdG8gQ1BVMyBhbmQgd2hlbiBDUFUzIGdvZXMgZG93biB3ZSB3b3VsZG4ndCBmcmVl IHBvbGljeSBvciBpdHMga29iai4NCj4gDQo+IE5vdyBvbiByZXN1bWUsIHdlIHdpbGwgZ2V0IENQ VTIgYmVmb3JlIENQVTMgYW5kIHdpbGwgY2FsbCBfX2NwdWZyZXFfYWRkX2RldigpLg0KPiBXZSB3 aWxsIHJlY292ZXIgdGhlIG9sZCBwb2xpY3kgYW5kIHVwZGF0ZSBwb2xpY3ktPmNwdSBmcm9tIDMg dG8gMiBmcm9tDQo+IHVwZGF0ZV9wb2xpY3lfY3B1KCkuDQo+IA0KPiBCdXQgdGhlIGtvYmogaXMg c3RpbGwgdGllZCB0byBDUFUzIGFuZCB3YXNuJ3QgbW92ZWQgdG8gQ1BVMi4gV2Ugd291bGRuJ3Qg Y3JlYXRlDQo+IGEgbGluayBmb3IgQ1BVMiwgYnV0IHdvdWxkIHRyeSB0aGF0IHdoaWxlIGJyaW5n aW5nIENQVTMgb25saW5lLiBXaGljaCB3aWxsDQo+IHJlcG9ydCBlcnJvcnMgYXMgQ1BVMyBhbHJl YWR5IGhhcyBrb2JqIGFzc2lnbmVkIHRvIGl0Lg0KPiANCj4gVGhpcyBidWcgZ290IGludHJvZHVj ZWQgd2l0aCBjb21taXQgNDJmOTIxYSwgd2hpY2ggb3Zlcmxvb2tlZCB0aGlzIHNjZW5hcmlvLg0K PiANCj4gVG8gZml4IHRoaXMsIGxldHMgbW92ZSBrb2JqIHRvIHRoZSBuZXcgcG9saWN5LT5jcHUg d2hpbGUgYnJpbmdpbmcgZmlyc3QgQ1BVIG9mIGENCj4gY2x1c3RlciBiYWNrLiBXZSBhbHJlYWR5 IGhhdmUgdXBkYXRlX3BvbGljeV9jcHUoKSByb3V0aW5lIHdoaWNoIGNhbiBiZSB1cGRhdGVkDQo+ IHdpdGggdGhpcyBjaGFuZ2UuIFRoYXQgd291bGQgZ2V0IHJpZCBvZiB0aGUgY3B1ZnJlcV9ub21p bmF0ZV9uZXdfcG9saWN5X2NwdSgpIGFzDQo+IHVwZGF0ZV9wb2xpY3lfY3B1KCkgaXMgYWxzbyBj YWxsZWQgb24gQ1BVIHJlbW92YWwuDQo+IA0KPiBUbyBhY2hpZXZlIHRoYXQgd2UgYWRkIGFub3Ro ZXIgcGFyYW1ldGVyIHRvIHVwZGF0ZV9wb2xpY3lfY3B1KCkgYXMgY3B1X2RldiBpcw0KPiBwcmVz ZW50IHdpdGggYm90aCB0aGUgY2FsbGVycy4NCj4gDQo+IEZpeGVzOiAoIjQyZjkyMWEgY3B1ZnJl cTogcmVtb3ZlIHN5c2ZzIGZpbGVzIGZvciBDUFVzIHdoaWNoIGZhaWxlZCB0byBjb21lIGJhY2sg YWZ0ZXIgcmVzdW1lIikNCj4gQ2M6IFN0YWJsZSA8c3RhYmxlQHZnZXIua2VybmVsLm9yZz4gIyAz LjEzKw0KPiBSZXBvcnRlZC1ieTogQnUgWWl0aWFuIDx5YnVAcXRpLnF1YWxjb21tLmNvbT4NCj4g UmVwb3J0ZWQtYnk6IFNhcmF2YW5hIEthbm5hbiA8c2thbm5hbkBjb2RlYXVyb3JhLm9yZz4NCj4g U2lnbmVkLW9mZi1ieTogVmlyZXNoIEt1bWFyIDx2aXJlc2gua3VtYXJAbGluYXJvLm9yZz4NCj4g LS0tDQo+IFYxLT5WMjogTW92ZSBrb2JqZWN0X21vdmUoKSBjYWxsIHRvIHVwZGF0ZV9wb2xpY3lf Y3B1KCksIHdoaWNoIG1ha2VzDQo+IGNwdWZyZXFfbm9taW5hdGVfbmV3X3BvbGljeV9jcHUoKSBh bG1vc3QgZW1wdHkuIFNvIHdlIHJlbW92ZSBpdCBjb21wbGV0ZWx5Lg0KPiANCj4gQFlpdGlhbjog U29ycnksIGJ1dCB5b3UgbmVlZCB0byB0ZXN0IHRoaXMgYWdhaW4gYXMgdGhlcmUgd2VyZSBlbm91 Z2gNCj4gbW9kaWZpY2F0aW9ucyBpbiBWMi4NCj4gDQoNClNvcnJ5LCBJIGRvbid0IGhhdmUgbGF0 ZXN0IGtlcm5lbCB0byB0ZXN0IHRoaXMgcGF0Y2guLi4NCldoYXQgSSBhbSB1c2luZyBpcyAzLjEw LCB0aGlzIHBhdGNoIHNlZW1zIHRvbyBiaWcgdG8gbWFudWFsbHkgYXBwbHkgdG8gMy4xMC4NCg0K DQo+IGRyaXZlcnMvY3B1ZnJlcS9jcHVmcmVxLmMgfCA3MyArKysrKysrKysrKysrKysrKysrKysr Ky0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPiAxIGZpbGUgY2hhbmdlZCwgMzYgaW5zZXJ0aW9u cygrKSwgMzcgZGVsZXRpb25zKC0pDQo+IA0KPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9jcHVmcmVx L2NwdWZyZXEuYyBiL2RyaXZlcnMvY3B1ZnJlcS9jcHVmcmVxLmMNCj4gaW5kZXggNjIyNTlkMi4u YzgxZDllYzYgMTAwNjQ0DQo+IC0tLSBhL2RyaXZlcnMvY3B1ZnJlcS9jcHVmcmVxLmMNCj4gKysr IGIvZHJpdmVycy9jcHVmcmVxL2NwdWZyZXEuYw0KPiBAQCAtMTA3NiwxMCArMTA3NiwyMCBAQCBz dGF0aWMgdm9pZCBjcHVmcmVxX3BvbGljeV9mcmVlKHN0cnVjdCBjcHVmcmVxX3BvbGljeSAqcG9s aWN5KQ0KPiAgICBrZnJlZShwb2xpY3kpOw0KPiB9DQo+IA0KPiAtc3RhdGljIHZvaWQgdXBkYXRl X3BvbGljeV9jcHUoc3RydWN0IGNwdWZyZXFfcG9saWN5ICpwb2xpY3ksIHVuc2lnbmVkIGludCBj cHUpDQo+ICtzdGF0aWMgaW50IHVwZGF0ZV9wb2xpY3lfY3B1KHN0cnVjdCBjcHVmcmVxX3BvbGlj eSAqcG9saWN5LCB1bnNpZ25lZCBpbnQgY3B1LA0KPiArICAgICAgICAgICAgICAgICBzdHJ1Y3Qg ZGV2aWNlICpjcHVfZGV2KQ0KPiB7DQo+ICsgICAgaW50IHJldDsNCj4gKw0KPiAgICBpZiAoV0FS Tl9PTihjcHUgPT0gcG9saWN5LT5jcHUpKQ0KPiAtICAgICAgICByZXR1cm47DQo+ICsgICAgICAg IHJldHVybiAwOw0KPiArDQo+ICsgICAgLyogTW92ZSBrb2JqZWN0IHRvIHRoZSBuZXcgcG9saWN5 LT5jcHUgKi8NCj4gKyAgICByZXQgPSBrb2JqZWN0X21vdmUoJnBvbGljeS0+a29iaiwgJmNwdV9k ZXYtPmtvYmopOw0KPiArICAgIGlmIChyZXQpIHsNCj4gKyAgICAgICAgcHJfZXJyKCIlczogRmFp bGVkIHRvIG1vdmUga29iajogJWRcbiIsIF9fZnVuY19fLCByZXQpOw0KPiArICAgICAgICByZXR1 cm4gcmV0Ow0KPiArICAgIH0NCj4gDQo+ICAgIGRvd25fd3JpdGUoJnBvbGljeS0+cndzZW0pOw0K PiANCj4gQEAgLTEwOTAsNiArMTEwMCw4IEBAIHN0YXRpYyB2b2lkIHVwZGF0ZV9wb2xpY3lfY3B1 KHN0cnVjdCBjcHVmcmVxX3BvbGljeSAqcG9saWN5LCB1bnNpZ25lZCBpbnQgY3B1KQ0KPiANCj4g ICAgYmxvY2tpbmdfbm90aWZpZXJfY2FsbF9jaGFpbigmY3B1ZnJlcV9wb2xpY3lfbm90aWZpZXJf bGlzdCwNCj4gICAgICAgICAgICBDUFVGUkVRX1VQREFURV9QT0xJQ1lfQ1BVLCBwb2xpY3kpOw0K PiArDQo+ICsgICAgcmV0dXJuIDA7DQo+IH0NCj4gDQo+IHN0YXRpYyBpbnQgX19jcHVmcmVxX2Fk ZF9kZXYoc3RydWN0IGRldmljZSAqZGV2LCBzdHJ1Y3Qgc3Vic3lzX2ludGVyZmFjZSAqc2lmKQ0K PiBAQCAtMTE1NCw3ICsxMTY2LDcgQEAgc3RhdGljIGludCBfX2NwdWZyZXFfYWRkX2RldihzdHJ1 Y3QgZGV2aWNlICpkZXYsIHN0cnVjdCBzdWJzeXNfaW50ZXJmYWNlICpzaWYpDQo+ICAgICAqIGJ5 IGludm9raW5nIHVwZGF0ZV9wb2xpY3lfY3B1KCkuDQo+ICAgICAqLw0KPiAgICBpZiAocmVjb3Zl cl9wb2xpY3kgJiYgY3B1ICE9IHBvbGljeS0+Y3B1KQ0KPiAtICAgICAgICB1cGRhdGVfcG9saWN5 X2NwdShwb2xpY3ksIGNwdSk7DQo+ICsgICAgICAgIFdBUk5fT04odXBkYXRlX3BvbGljeV9jcHUo cG9saWN5LCBjcHUsIGRldikpOw0KPiAgICBlbHNlDQo+ICAgICAgICBwb2xpY3ktPmNwdSA9IGNw dTsNCj4gDQo+IEBAIC0xMzA3LDM4ICsxMzE5LDExIEBAIHN0YXRpYyBpbnQgY3B1ZnJlcV9hZGRf ZGV2KHN0cnVjdCBkZXZpY2UgKmRldiwgc3RydWN0IHN1YnN5c19pbnRlcmZhY2UgKnNpZikNCj4g ICAgcmV0dXJuIF9fY3B1ZnJlcV9hZGRfZGV2KGRldiwgc2lmKTsNCj4gfQ0KPiANCj4gLXN0YXRp YyBpbnQgY3B1ZnJlcV9ub21pbmF0ZV9uZXdfcG9saWN5X2NwdShzdHJ1Y3QgY3B1ZnJlcV9wb2xp Y3kgKnBvbGljeSwNCj4gLSAgICAgICAgICAgICAgICAgICAgICAgdW5zaWduZWQgaW50IG9sZF9j cHUpDQo+IC17DQo+IC0gICAgc3RydWN0IGRldmljZSAqY3B1X2RldjsNCj4gLSAgICBpbnQgcmV0 Ow0KPiAtDQo+IC0gICAgLyogZmlyc3Qgc2libGluZyBub3cgb3ducyB0aGUgbmV3IHN5c2ZzIGRp ciAqLw0KPiAtICAgIGNwdV9kZXYgPSBnZXRfY3B1X2RldmljZShjcHVtYXNrX2FueV9idXQocG9s aWN5LT5jcHVzLCBvbGRfY3B1KSk7DQo+IC0NCj4gLSAgICBzeXNmc19yZW1vdmVfbGluaygmY3B1 X2Rldi0+a29iaiwgImNwdWZyZXEiKTsNCj4gLSAgICByZXQgPSBrb2JqZWN0X21vdmUoJnBvbGlj eS0+a29iaiwgJmNwdV9kZXYtPmtvYmopOw0KPiAtICAgIGlmIChyZXQpIHsNCj4gLSAgICAgICAg cHJfZXJyKCIlczogRmFpbGVkIHRvIG1vdmUga29iajogJWRcbiIsIF9fZnVuY19fLCByZXQpOw0K PiAtDQo+IC0gICAgICAgIGRvd25fd3JpdGUoJnBvbGljeS0+cndzZW0pOw0KPiAtICAgICAgICBj cHVtYXNrX3NldF9jcHUob2xkX2NwdSwgcG9saWN5LT5jcHVzKTsNCj4gLSAgICAgICAgdXBfd3Jp dGUoJnBvbGljeS0+cndzZW0pOw0KPiAtDQo+IC0gICAgICAgIHJldCA9IHN5c2ZzX2NyZWF0ZV9s aW5rKCZjcHVfZGV2LT5rb2JqLCAmcG9saWN5LT5rb2JqLA0KPiAtICAgICAgICAgICAgICAgICAg ICAiY3B1ZnJlcSIpOw0KPiAtDQo+IC0gICAgICAgIHJldHVybiAtRUlOVkFMOw0KPiAtICAgIH0N Cj4gLQ0KPiAtICAgIHJldHVybiBjcHVfZGV2LT5pZDsNCj4gLX0NCj4gLQ0KPiBzdGF0aWMgaW50 IF9fY3B1ZnJlcV9yZW1vdmVfZGV2X3ByZXBhcmUoc3RydWN0IGRldmljZSAqZGV2LA0KPiAgICAg ICAgICAgICAgICAgICAgc3RydWN0IHN1YnN5c19pbnRlcmZhY2UgKnNpZikNCj4gew0KPiAgICB1 bnNpZ25lZCBpbnQgY3B1ID0gZGV2LT5pZCwgY3B1czsNCj4gLSAgICBpbnQgbmV3X2NwdSwgcmV0 Ow0KPiArICAgIGludCByZXQ7DQo+ICAgIHVuc2lnbmVkIGxvbmcgZmxhZ3M7DQo+ICAgIHN0cnVj dCBjcHVmcmVxX3BvbGljeSAqcG9saWN5Ow0KPiANCj4gQEAgLTEzNzgsMTQgKzEzNjMsMjggQEAg c3RhdGljIGludCBfX2NwdWZyZXFfcmVtb3ZlX2Rldl9wcmVwYXJlKHN0cnVjdCBkZXZpY2UgKmRl diwNCj4gICAgaWYgKGNwdSAhPSBwb2xpY3ktPmNwdSkgew0KPiAgICAgICAgc3lzZnNfcmVtb3Zl X2xpbmsoJmRldi0+a29iaiwgImNwdWZyZXEiKTsNCj4gICAgfSBlbHNlIGlmIChjcHVzID4gMSkg ew0KPiAtICAgICAgICBuZXdfY3B1ID0gY3B1ZnJlcV9ub21pbmF0ZV9uZXdfcG9saWN5X2NwdShw b2xpY3ksIGNwdSk7DQo+IC0gICAgICAgIGlmIChuZXdfY3B1ID49IDApIHsNCj4gLSAgICAgICAg ICAgIHVwZGF0ZV9wb2xpY3lfY3B1KHBvbGljeSwgbmV3X2NwdSk7DQo+ICsgICAgICAgIC8qIE5v bWluYXRlIG5ldyBDUFUgKi8NCj4gKyAgICAgICAgaW50IG5ld19jcHUgPSBjcHVtYXNrX2FueV9i dXQocG9saWN5LT5jcHVzLCBjcHUpOw0KPiArICAgICAgICBzdHJ1Y3QgZGV2aWNlICpjcHVfZGV2 ID0gZ2V0X2NwdV9kZXZpY2UobmV3X2NwdSk7DQo+IA0KPiAtICAgICAgICAgICAgaWYgKCFjcHVm cmVxX3N1c3BlbmRlZCkNCj4gLSAgICAgICAgICAgICAgICBwcl9kZWJ1ZygiJXM6IHBvbGljeSBL b2JqZWN0IG1vdmVkIHRvIGNwdTogJWQgZnJvbTogJWRcbiIsDQo+IC0gICAgICAgICAgICAgICAg ICAgICBfX2Z1bmNfXywgbmV3X2NwdSwgY3B1KTsNCj4gKyAgICAgICAgc3lzZnNfcmVtb3ZlX2xp bmsoJmNwdV9kZXYtPmtvYmosICJjcHVmcmVxIik7DQo+ICsgICAgICAgIHJldCA9IHVwZGF0ZV9w b2xpY3lfY3B1KHBvbGljeSwgbmV3X2NwdSwgY3B1X2Rldik7DQo+ICsgICAgICAgIGlmIChyZXQp IHsNCj4gKyAgICAgICAgICAgIC8qDQo+ICsgICAgICAgICAgICAgKiBUbyBzdXByZXNzIGNvbXBp bGF0aW9uIHdhcm5pbmcgYWJvdXQgcmV0dXJuIHZhbHVlIG9mDQo+ICsgICAgICAgICAgICAgKiBz eXNmc19jcmVhdGVfbGluaygpLg0KPiArICAgICAgICAgICAgICovDQo+ICsgICAgICAgICAgICBp bnQgdGVtcDsNCj4gKw0KPiArICAgICAgICAgICAgLyogQ3JlYXRlIGxpbmsgYWdhaW4gaWYgd2Ug ZmFpbGVkLiAqLw0KPiArICAgICAgICAgICAgdGVtcCA9IHN5c2ZzX2NyZWF0ZV9saW5rKCZjcHVf ZGV2LT5rb2JqLCAmcG9saWN5LT5rb2JqLA0KPiArICAgICAgICAgICAgICAgICAgICAgICAgICJj cHVmcmVxIik7DQo+ICsgICAgICAgICAgICByZXR1cm4gcmV0Ow0KPiAgICAgICAgfQ0KPiArDQo+ ICsgICAgICAgIGlmICghY3B1ZnJlcV9zdXNwZW5kZWQpDQo+ICsgICAgICAgICAgICBwcl9kZWJ1 ZygiJXM6IHBvbGljeSBLb2JqZWN0IG1vdmVkIHRvIGNwdTogJWQgZnJvbTogJWRcbiIsDQo+ICsg ICAgICAgICAgICAgICAgIF9fZnVuY19fLCBuZXdfY3B1LCBjcHUpOw0KPiAgICB9IGVsc2UgaWYg KGNwdWZyZXFfZHJpdmVyLT5zdG9wX2NwdSAmJiBjcHVmcmVxX2RyaXZlci0+c2V0cG9saWN5KSB7 DQo+ICAgICAgICBjcHVmcmVxX2RyaXZlci0+c3RvcF9jcHUocG9saWN5KTsNCj4gICAgfQ0KPiAt LSANCj4gMi4wLjAucmMyDQo+IA0K -- 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 07/10/2014 08:12 AM, Bu, Yitian wrote: > > >> 在 2014年7月10日，20:41，"Viresh Kumar" <viresh.kumar@linaro.org> 写道： >> >> This is only relevant to implementations with multiple clusters, where clusters >> have separate clock lines but all CPUs within a cluster share it. >> >> Consider a dual cluster platform with 2 cores per cluster. During suspend we >> start offlining CPUs from 1 to 3. When CPU2 is remove, policy->kobj would be >> moved to CPU3 and when CPU3 goes down we wouldn't free policy or its kobj. >> >> Now on resume, we will get CPU2 before CPU3 and will call __cpufreq_add_dev(). >> We will recover the old policy and update policy->cpu from 3 to 2 from >> update_policy_cpu(). >> >> But the kobj is still tied to CPU3 and wasn't moved to CPU2. We wouldn't create >> a link for CPU2, but would try that while bringing CPU3 online. Which will >> report errors as CPU3 already has kobj assigned to it. >> >> This bug got introduced with commit 42f921a, which overlooked this scenario. >> >> To fix this, lets move kobj to the new policy->cpu while bringing first CPU of a >> cluster back. We already have update_policy_cpu() routine which can be updated >> with this change. That would get rid of the cpufreq_nominate_new_policy_cpu() as >> update_policy_cpu() is also called on CPU removal. >> >> To achieve that we add another parameter to update_policy_cpu() as cpu_dev is >> present with both the callers. >> >> Fixes: ("42f921a cpufreq: remove sysfs files for CPUs which failed to come back after resume") >> Cc: Stable <stable@vger.kernel.org> # 3.13+ >> Reported-by: Bu Yitian <ybu@qti.qualcomm.com> >> Reported-by: Saravana Kannan <skannan@codeaurora.org> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >> --- >> V1->V2: Move kobject_move() call to update_policy_cpu(), which makes >> cpufreq_nominate_new_policy_cpu() almost empty. So we remove it completely. >> >> @Yitian: Sorry, but you need to test this again as there were enough >> modifications in V2. >> > > Sorry, I don't have latest kernel to test this patch... > What I am using is 3.10, this patch seems too big to manually apply to 3.10. Even though our kernel is 3.10, I believe we have pulled in all cpufreq up to 3.14. So, if you have time, you could pull in rest of the cpufreq changes and test it out. For our tree, maybe the v1 patch would be sufficient? -Saravana > > >> drivers/cpufreq/cpufreq.c | 73 +++++++++++++++++++++++------------------------ >> 1 file changed, 36 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 62259d2..c81d9ec6 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1076,10 +1076,20 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy) >> kfree(policy); >> } >> >> -static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu) >> +static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu, >> + struct device *cpu_dev) >> { >> + int ret; >> + >> if (WARN_ON(cpu == policy->cpu)) >> - return; >> + return 0; >> + >> + /* Move kobject to the new policy->cpu */ >> + ret = kobject_move(&policy->kobj, &cpu_dev->kobj); >> + if (ret) { >> + pr_err("%s: Failed to move kobj: %d\n", __func__, ret); >> + return ret; >> + } >> >> down_write(&policy->rwsem); >> >> @@ -1090,6 +1100,8 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu) >> >> blocking_notifier_call_chain(&cpufreq_policy_notifier_list, >> CPUFREQ_UPDATE_POLICY_CPU, policy); >> + >> + return 0; >> } >> >> static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >> @@ -1154,7 +1166,7 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >> * by invoking update_policy_cpu(). >> */ >> if (recover_policy && cpu != policy->cpu) >> - update_policy_cpu(policy, cpu); >> + WARN_ON(update_policy_cpu(policy, cpu, dev)); >> else >> policy->cpu = cpu; >> >> @@ -1307,38 +1319,11 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >> return __cpufreq_add_dev(dev, sif); >> } >> >> -static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy, >> - unsigned int old_cpu) >> -{ >> - struct device *cpu_dev; >> - int ret; >> - >> - /* first sibling now owns the new sysfs dir */ >> - cpu_dev = get_cpu_device(cpumask_any_but(policy->cpus, old_cpu)); >> - >> - sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); >> - ret = kobject_move(&policy->kobj, &cpu_dev->kobj); >> - if (ret) { >> - pr_err("%s: Failed to move kobj: %d\n", __func__, ret); >> - >> - down_write(&policy->rwsem); >> - cpumask_set_cpu(old_cpu, policy->cpus); >> - up_write(&policy->rwsem); >> - >> - ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, >> - "cpufreq"); >> - >> - return -EINVAL; >> - } >> - >> - return cpu_dev->id; >> -} >> - >> static int __cpufreq_remove_dev_prepare(struct device *dev, >> struct subsys_interface *sif) >> { >> unsigned int cpu = dev->id, cpus; >> - int new_cpu, ret; >> + int ret; >> unsigned long flags; >> struct cpufreq_policy *policy; >> >> @@ -1378,14 +1363,28 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, >> if (cpu != policy->cpu) { >> sysfs_remove_link(&dev->kobj, "cpufreq"); >> } else if (cpus > 1) { >> - new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu); >> - if (new_cpu >= 0) { >> - update_policy_cpu(policy, new_cpu); >> + /* Nominate new CPU */ >> + int new_cpu = cpumask_any_but(policy->cpus, cpu); >> + struct device *cpu_dev = get_cpu_device(new_cpu); >> >> - if (!cpufreq_suspended) >> - pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n", >> - __func__, new_cpu, cpu); >> + sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); >> + ret = update_policy_cpu(policy, new_cpu, cpu_dev); >> + if (ret) { >> + /* >> + * To supress compilation warning about return value of >> + * sysfs_create_link(). >> + */ >> + int temp; >> + >> + /* Create link again if we failed. */ >> + temp = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, >> + "cpufreq"); >> + return ret; >> } >> + >> + if (!cpufreq_suspended) >> + pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n", >> + __func__, new_cpu, cpu); >> } else if (cpufreq_driver->stop_cpu && cpufreq_driver->setpolicy) { >> cpufreq_driver->stop_cpu(policy); >> } >> -- >> 2.0.0.rc2 >>
> 在 2014年7月11日，3:26，"Saravana Kannan" <skannan@codeaurora.org> 写道： > >> On 07/10/2014 08:12 AM, Bu, Yitian wrote: >> >> >>> 在 2014年7月10日，20:41，"Viresh Kumar" <viresh.kumar@linaro.org> 写道： >>> >>> This is only relevant to implementations with multiple clusters, where clusters >>> have separate clock lines but all CPUs within a cluster share it. >>> >>> Consider a dual cluster platform with 2 cores per cluster. During suspend we >>> start offlining CPUs from 1 to 3. When CPU2 is remove, policy->kobj would be >>> moved to CPU3 and when CPU3 goes down we wouldn't free policy or its kobj. >>> >>> Now on resume, we will get CPU2 before CPU3 and will call __cpufreq_add_dev(). >>> We will recover the old policy and update policy->cpu from 3 to 2 from >>> update_policy_cpu(). >>> >>> But the kobj is still tied to CPU3 and wasn't moved to CPU2. We wouldn't create >>> a link for CPU2, but would try that while bringing CPU3 online. Which will >>> report errors as CPU3 already has kobj assigned to it. >>> >>> This bug got introduced with commit 42f921a, which overlooked this scenario. >>> >>> To fix this, lets move kobj to the new policy->cpu while bringing first CPU of a >>> cluster back. We already have update_policy_cpu() routine which can be updated >>> with this change. That would get rid of the cpufreq_nominate_new_policy_cpu() as >>> update_policy_cpu() is also called on CPU removal. >>> >>> To achieve that we add another parameter to update_policy_cpu() as cpu_dev is >>> present with both the callers. >>> >>> Fixes: ("42f921a cpufreq: remove sysfs files for CPUs which failed to come back after resume") >>> Cc: Stable <stable@vger.kernel.org> # 3.13+ >>> Reported-by: Bu Yitian <ybu@qti.qualcomm.com> >>> Reported-by: Saravana Kannan <skannan@codeaurora.org> >>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >>> --- >>> V1->V2: Move kobject_move() call to update_policy_cpu(), which makes >>> cpufreq_nominate_new_policy_cpu() almost empty. So we remove it completely. >>> >>> @Yitian: Sorry, but you need to test this again as there were enough >>> modifications in V2. >> >> Sorry, I don't have latest kernel to test this patch... >> What I am using is 3.10, this patch seems too big to manually apply to 3.10. > > Even though our kernel is 3.10, I believe we have pulled in all cpufreq > up to 3.14. So, if you have time, you could pull in rest of the cpufreq > changes and test it out. For our tree, maybe the v1 patch would be > sufficient? > > -Saravana V1 patch is sufficient, which is the same as my original patch. I have already verified the V1 patch and it works. >> >> >>> drivers/cpufreq/cpufreq.c | 73 +++++++++++++++++++++++------------------------ >>> 1 file changed, 36 insertions(+), 37 deletions(-) >>> >>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >>> index 62259d2..c81d9ec6 100644 >>> --- a/drivers/cpufreq/cpufreq.c >>> +++ b/drivers/cpufreq/cpufreq.c >>> @@ -1076,10 +1076,20 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy) >>> kfree(policy); >>> } >>> >>> -static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu) >>> +static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu, >>> + struct device *cpu_dev) >>> { >>> + int ret; >>> + >>> if (WARN_ON(cpu == policy->cpu)) >>> - return; >>> + return 0; >>> + >>> + /* Move kobject to the new policy->cpu */ >>> + ret = kobject_move(&policy->kobj, &cpu_dev->kobj); >>> + if (ret) { >>> + pr_err("%s: Failed to move kobj: %d\n", __func__, ret); >>> + return ret; >>> + } >>> >>> down_write(&policy->rwsem); >>> >>> @@ -1090,6 +1100,8 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu) >>> >>> blocking_notifier_call_chain(&cpufreq_policy_notifier_list, >>> CPUFREQ_UPDATE_POLICY_CPU, policy); >>> + >>> + return 0; >>> } >>> >>> static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >>> @@ -1154,7 +1166,7 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >>> * by invoking update_policy_cpu(). >>> */ >>> if (recover_policy && cpu != policy->cpu) >>> - update_policy_cpu(policy, cpu); >>> + WARN_ON(update_policy_cpu(policy, cpu, dev)); >>> else >>> policy->cpu = cpu; >>> >>> @@ -1307,38 +1319,11 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >>> return __cpufreq_add_dev(dev, sif); >>> } >>> >>> -static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy, >>> - unsigned int old_cpu) >>> -{ >>> - struct device *cpu_dev; >>> - int ret; >>> - >>> - /* first sibling now owns the new sysfs dir */ >>> - cpu_dev = get_cpu_device(cpumask_any_but(policy->cpus, old_cpu)); >>> - >>> - sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); >>> - ret = kobject_move(&policy->kobj, &cpu_dev->kobj); >>> - if (ret) { >>> - pr_err("%s: Failed to move kobj: %d\n", __func__, ret); >>> - >>> - down_write(&policy->rwsem); >>> - cpumask_set_cpu(old_cpu, policy->cpus); >>> - up_write(&policy->rwsem); >>> - >>> - ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, >>> - "cpufreq"); >>> - >>> - return -EINVAL; >>> - } >>> - >>> - return cpu_dev->id; >>> -} >>> - >>> static int __cpufreq_remove_dev_prepare(struct device *dev, >>> struct subsys_interface *sif) >>> { >>> unsigned int cpu = dev->id, cpus; >>> - int new_cpu, ret; >>> + int ret; >>> unsigned long flags; >>> struct cpufreq_policy *policy; >>> >>> @@ -1378,14 +1363,28 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, >>> if (cpu != policy->cpu) { >>> sysfs_remove_link(&dev->kobj, "cpufreq"); >>> } else if (cpus > 1) { >>> - new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu); >>> - if (new_cpu >= 0) { >>> - update_policy_cpu(policy, new_cpu); >>> + /* Nominate new CPU */ >>> + int new_cpu = cpumask_any_but(policy->cpus, cpu); >>> + struct device *cpu_dev = get_cpu_device(new_cpu); >>> >>> - if (!cpufreq_suspended) >>> - pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n", >>> - __func__, new_cpu, cpu); >>> + sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); >>> + ret = update_policy_cpu(policy, new_cpu, cpu_dev); >>> + if (ret) { >>> + /* >>> + * To supress compilation warning about return value of >>> + * sysfs_create_link(). >>> + */ >>> + int temp; >>> + >>> + /* Create link again if we failed. */ >>> + temp = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, >>> + "cpufreq"); >>> + return ret; >>> } >>> + >>> + if (!cpufreq_suspended) >>> + pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n", >>> + __func__, new_cpu, cpu); >>> } else if (cpufreq_driver->stop_cpu && cpufreq_driver->setpolicy) { >>> cpufreq_driver->stop_cpu(policy); >>> } >>> -- >>> 2.0.0.rc2 > > > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > hosted by The Linux Foundation
On Thursday, July 10, 2014 06:11:44 PM Viresh Kumar wrote: > This is only relevant to implementations with multiple clusters, where clusters > have separate clock lines but all CPUs within a cluster share it. > > Consider a dual cluster platform with 2 cores per cluster. During suspend we > start offlining CPUs from 1 to 3. When CPU2 is remove, policy->kobj would be > moved to CPU3 and when CPU3 goes down we wouldn't free policy or its kobj. > > Now on resume, we will get CPU2 before CPU3 and will call __cpufreq_add_dev(). > We will recover the old policy and update policy->cpu from 3 to 2 from > update_policy_cpu(). > > But the kobj is still tied to CPU3 and wasn't moved to CPU2. We wouldn't create > a link for CPU2, but would try that while bringing CPU3 online. Which will > report errors as CPU3 already has kobj assigned to it. > > This bug got introduced with commit 42f921a, which overlooked this scenario. > > To fix this, lets move kobj to the new policy->cpu while bringing first CPU of a > cluster back. We already have update_policy_cpu() routine which can be updated > with this change. That would get rid of the cpufreq_nominate_new_policy_cpu() as > update_policy_cpu() is also called on CPU removal. > > To achieve that we add another parameter to update_policy_cpu() as cpu_dev is > present with both the callers. > > Fixes: ("42f921a cpufreq: remove sysfs files for CPUs which failed to come back after resume") > Cc: Stable <stable@vger.kernel.org> # 3.13+ > Reported-by: Bu Yitian <ybu@qti.qualcomm.com> > Reported-by: Saravana Kannan <skannan@codeaurora.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > V1->V2: Move kobject_move() call to update_policy_cpu(), which makes > cpufreq_nominate_new_policy_cpu() almost empty. So we remove it completely. > > @Yitian: Sorry, but you need to test this again as there were enough > modifications in V2. > > drivers/cpufreq/cpufreq.c | 73 +++++++++++++++++++++++------------------------ > 1 file changed, 36 insertions(+), 37 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 62259d2..c81d9ec6 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1076,10 +1076,20 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy) > kfree(policy); > } > > -static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu) > +static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu, > + struct device *cpu_dev) > { > + int ret; > + > if (WARN_ON(cpu == policy->cpu)) > - return; > + return 0; > + > + /* Move kobject to the new policy->cpu */ > + ret = kobject_move(&policy->kobj, &cpu_dev->kobj); > + if (ret) { > + pr_err("%s: Failed to move kobj: %d\n", __func__, ret); > + return ret; Previously, we returned -EINVAL in the kobject_move() failure case. Why are we changing that now? > + } > > down_write(&policy->rwsem); > > @@ -1090,6 +1100,8 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu) > > blocking_notifier_call_chain(&cpufreq_policy_notifier_list, > CPUFREQ_UPDATE_POLICY_CPU, policy); > + > + return 0; > } > > static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) > @@ -1154,7 +1166,7 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) > * by invoking update_policy_cpu(). > */ > if (recover_policy && cpu != policy->cpu) > - update_policy_cpu(policy, cpu); > + WARN_ON(update_policy_cpu(policy, cpu, dev)); This is an arbitrary difference in the handling of update_policy_cpu() return value. Why do we want the WARN_ON() here and not in the other place? Don't we want to recover from kobject_move() failures here as well? > else > policy->cpu = cpu; > > @@ -1307,38 +1319,11 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) > return __cpufreq_add_dev(dev, sif); > } > > -static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy, > - unsigned int old_cpu) > -{ > - struct device *cpu_dev; > - int ret; > - > - /* first sibling now owns the new sysfs dir */ > - cpu_dev = get_cpu_device(cpumask_any_but(policy->cpus, old_cpu)); > - > - sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); > - ret = kobject_move(&policy->kobj, &cpu_dev->kobj); > - if (ret) { > - pr_err("%s: Failed to move kobj: %d\n", __func__, ret); > - > - down_write(&policy->rwsem); > - cpumask_set_cpu(old_cpu, policy->cpus); > - up_write(&policy->rwsem); Why don't we need the above three lines in the new code? > - > - ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, > - "cpufreq"); > - > - return -EINVAL; > - } > - > - return cpu_dev->id; > -} > - > static int __cpufreq_remove_dev_prepare(struct device *dev, > struct subsys_interface *sif) > { > unsigned int cpu = dev->id, cpus; > - int new_cpu, ret; > + int ret; > unsigned long flags; > struct cpufreq_policy *policy; > > @@ -1378,14 +1363,28 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, > if (cpu != policy->cpu) { > sysfs_remove_link(&dev->kobj, "cpufreq"); > } else if (cpus > 1) { > - new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu); > - if (new_cpu >= 0) { > - update_policy_cpu(policy, new_cpu); > + /* Nominate new CPU */ > + int new_cpu = cpumask_any_but(policy->cpus, cpu); > + struct device *cpu_dev = get_cpu_device(new_cpu); > > - if (!cpufreq_suspended) > - pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n", > - __func__, new_cpu, cpu); > + sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); > + ret = update_policy_cpu(policy, new_cpu, cpu_dev); > + if (ret) { > + /* > + * To supress compilation warning about return value of > + * sysfs_create_link(). > + */ > + int temp; > + > + /* Create link again if we failed. */ > + temp = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, > + "cpufreq"); And this is *ugly*. > + return ret; > } > + > + if (!cpufreq_suspended) > + pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n", > + __func__, new_cpu, cpu); > } else if (cpufreq_driver->stop_cpu && cpufreq_driver->setpolicy) { > cpufreq_driver->stop_cpu(policy); > } >
On 17 July 2014 04:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> +static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu, >> + struct device *cpu_dev) >> { >> + int ret; >> + >> if (WARN_ON(cpu == policy->cpu)) >> - return; >> + return 0; >> + >> + /* Move kobject to the new policy->cpu */ >> + ret = kobject_move(&policy->kobj, &cpu_dev->kobj); >> + if (ret) { >> + pr_err("%s: Failed to move kobj: %d\n", __func__, ret); >> + return ret; > > Previously, we returned -EINVAL in the kobject_move() failure case. Why are > we changing that now? We should have preserved return value of kobject_move() earlier in cpufreq_nominate_new_policy_cpu() and sent that, but we returned -EINVAL. And I realized that its more appropriate to return the error returned by kobject_move(). >> static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >> @@ -1154,7 +1166,7 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >> * by invoking update_policy_cpu(). >> */ >> if (recover_policy && cpu != policy->cpu) >> - update_policy_cpu(policy, cpu); >> + WARN_ON(update_policy_cpu(policy, cpu, dev)); > > This is an arbitrary difference in the handling of update_policy_cpu() return > value. Why do we want the WARN_ON() here and not in the other place? We really can't recover in this case. We have reached here after a suspend/ resume, and probing first cpu of a non-boot cluster. And we *have* to make it policy-master. But in the other case, we are removing a CPU in PREPARE stage and so we can actually fail from there and let everybody know. Though I am not aware of anycase in which kobject_move() can fail there. > Don't we want to recover from kobject_move() failures here as well? In the other case, we have just removed the link from the new policy->cpu and so we try to recover for that in failures, but don't have something similar here. >> else >> policy->cpu = cpu; >> >> @@ -1307,38 +1319,11 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >> return __cpufreq_add_dev(dev, sif); >> } >> >> -static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy, >> - unsigned int old_cpu) >> -{ >> - struct device *cpu_dev; >> - int ret; >> - >> - /* first sibling now owns the new sysfs dir */ >> - cpu_dev = get_cpu_device(cpumask_any_but(policy->cpus, old_cpu)); >> - >> - sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); >> - ret = kobject_move(&policy->kobj, &cpu_dev->kobj); >> - if (ret) { >> - pr_err("%s: Failed to move kobj: %d\n", __func__, ret); >> - >> - down_write(&policy->rwsem); >> - cpumask_set_cpu(old_cpu, policy->cpus); >> - up_write(&policy->rwsem); > > Why don't we need the above three lines in the new code? It was probably meaningful when this was added initially, but later some commit moved the cpumask_clear_cpu() to __cpufreq_remove_dev_finish(). And so we don't really need to set the cpu to policy->cpus again, as it was never cleared by this stage.. >> - >> - ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, >> - "cpufreq"); >> - >> - return -EINVAL; >> - } >> - >> - return cpu_dev->id; >> -} >> - >> static int __cpufreq_remove_dev_prepare(struct device *dev, >> struct subsys_interface *sif) >> { >> unsigned int cpu = dev->id, cpus; >> - int new_cpu, ret; >> + int ret; >> unsigned long flags; >> struct cpufreq_policy *policy; >> >> @@ -1378,14 +1363,28 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, >> if (cpu != policy->cpu) { >> sysfs_remove_link(&dev->kobj, "cpufreq"); >> } else if (cpus > 1) { >> - new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu); >> - if (new_cpu >= 0) { >> - update_policy_cpu(policy, new_cpu); >> + /* Nominate new CPU */ >> + int new_cpu = cpumask_any_but(policy->cpus, cpu); >> + struct device *cpu_dev = get_cpu_device(new_cpu); >> >> - if (!cpufreq_suspended) >> - pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n", >> - __func__, new_cpu, cpu); >> + sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); >> + ret = update_policy_cpu(policy, new_cpu, cpu_dev); >> + if (ret) { >> + /* >> + * To supress compilation warning about return value of >> + * sysfs_create_link(). >> + */ >> + int temp; >> + >> + /* Create link again if we failed. */ >> + temp = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, >> + "cpufreq"); > > And this is *ugly*. Absolutely, Let me know what can we do to work around this. It was like this earlier as well, just that I added a descriptive comment this time. -- viresh -- 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
Quite frankly, to me this patch just does too much in one go without describing about a half of things it is doing. I would just add the kobject_move() call to __cpufreq_add_dev() to start with (because that's what you really want if I'm not mistaken) and do the cleanups separately, later. On Thu, Jul 17, 2014 at 2:21 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 17 July 2014 04:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >>> +static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu, >>> + struct device *cpu_dev) >>> { >>> + int ret; >>> + >>> if (WARN_ON(cpu == policy->cpu)) >>> - return; >>> + return 0; >>> + >>> + /* Move kobject to the new policy->cpu */ >>> + ret = kobject_move(&policy->kobj, &cpu_dev->kobj); >>> + if (ret) { >>> + pr_err("%s: Failed to move kobj: %d\n", __func__, ret); >>> + return ret; >> >> Previously, we returned -EINVAL in the kobject_move() failure case. Why are >> we changing that now? > > We should have preserved return value of kobject_move() earlier in > cpufreq_nominate_new_policy_cpu() and sent that, but we returned > -EINVAL. And I realized that its more appropriate to return the error > returned by kobject_move(). > >>> static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >>> @@ -1154,7 +1166,7 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >>> * by invoking update_policy_cpu(). >>> */ >>> if (recover_policy && cpu != policy->cpu) >>> - update_policy_cpu(policy, cpu); >>> + WARN_ON(update_policy_cpu(policy, cpu, dev)); >> >> This is an arbitrary difference in the handling of update_policy_cpu() return >> value. Why do we want the WARN_ON() here and not in the other place? > > We really can't recover in this case. We have reached here after a suspend/ > resume, and probing first cpu of a non-boot cluster. And we *have* to make > it policy-master. > > But in the other case, we are removing a CPU in PREPARE stage and so > we can actually fail from there and let everybody know. Though I am not > aware of anycase in which kobject_move() can fail there. > >> Don't we want to recover from kobject_move() failures here as well? > > In the other case, we have just removed the link from the new policy->cpu > and so we try to recover for that in failures, but don't have something > similar here. > >>> else >>> policy->cpu = cpu; >>> >>> @@ -1307,38 +1319,11 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >>> return __cpufreq_add_dev(dev, sif); >>> } >>> >>> -static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy, >>> - unsigned int old_cpu) >>> -{ >>> - struct device *cpu_dev; >>> - int ret; >>> - >>> - /* first sibling now owns the new sysfs dir */ >>> - cpu_dev = get_cpu_device(cpumask_any_but(policy->cpus, old_cpu)); >>> - >>> - sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); >>> - ret = kobject_move(&policy->kobj, &cpu_dev->kobj); >>> - if (ret) { >>> - pr_err("%s: Failed to move kobj: %d\n", __func__, ret); >>> - >>> - down_write(&policy->rwsem); >>> - cpumask_set_cpu(old_cpu, policy->cpus); >>> - up_write(&policy->rwsem); >> >> Why don't we need the above three lines in the new code? > > It was probably meaningful when this was added initially, but later > some commit moved the cpumask_clear_cpu() to > __cpufreq_remove_dev_finish(). And so we don't really need to > set the cpu to policy->cpus again, as it was never cleared by this > stage.. > >>> - >>> - ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, >>> - "cpufreq"); >>> - >>> - return -EINVAL; >>> - } >>> - >>> - return cpu_dev->id; >>> -} >>> - >>> static int __cpufreq_remove_dev_prepare(struct device *dev, >>> struct subsys_interface *sif) >>> { >>> unsigned int cpu = dev->id, cpus; >>> - int new_cpu, ret; >>> + int ret; >>> unsigned long flags; >>> struct cpufreq_policy *policy; >>> >>> @@ -1378,14 +1363,28 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, >>> if (cpu != policy->cpu) { >>> sysfs_remove_link(&dev->kobj, "cpufreq"); >>> } else if (cpus > 1) { >>> - new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu); >>> - if (new_cpu >= 0) { >>> - update_policy_cpu(policy, new_cpu); >>> + /* Nominate new CPU */ >>> + int new_cpu = cpumask_any_but(policy->cpus, cpu); >>> + struct device *cpu_dev = get_cpu_device(new_cpu); >>> >>> - if (!cpufreq_suspended) >>> - pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n", >>> - __func__, new_cpu, cpu); >>> + sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); >>> + ret = update_policy_cpu(policy, new_cpu, cpu_dev); >>> + if (ret) { >>> + /* >>> + * To supress compilation warning about return value of >>> + * sysfs_create_link(). >>> + */ >>> + int temp; >>> + >>> + /* Create link again if we failed. */ >>> + temp = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, >>> + "cpufreq"); >> >> And this is *ugly*. > > Absolutely, Let me know what can we do to work around this. > It was like this earlier as well, just that I added a descriptive > comment this time. > > -- > viresh > -- > 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 -- 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 17 July 2014 06:19, Rafael J. Wysocki <rafael@kernel.org> wrote: > Quite frankly, to me this patch just does too much in one go without > describing about a half of things it is doing. > > I would just add the kobject_move() call to __cpufreq_add_dev() to > start with (because that's what you really want if I'm not mistaken) > and do the cleanups separately, later. I understand that as I had similar feelings about it. See what I sent during V1: http://www.spinics.net/lists/stable/msg54338.html Only when Srivatsa asked me to do the cleanups with it, I went for V2. And still wasn't really sure about getting that as a bug fix. And so had a chat with him again and asked you to push V1, and we will do cleanups later. So, I don't see V1 in patchworks anymore and I must resend? I will make out a series out of V2, first patch would be a fix for 3.16 & stable, and others would be for 3.17. Hope that works. -- viresh -- 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
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 62259d2..c81d9ec6 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1076,10 +1076,20 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy) kfree(policy); } -static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu) +static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu, + struct device *cpu_dev) { + int ret; + if (WARN_ON(cpu == policy->cpu)) - return; + return 0; + + /* Move kobject to the new policy->cpu */ + ret = kobject_move(&policy->kobj, &cpu_dev->kobj); + if (ret) { + pr_err("%s: Failed to move kobj: %d\n", __func__, ret); + return ret; + } down_write(&policy->rwsem); @@ -1090,6 +1100,8 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu) blocking_notifier_call_chain(&cpufreq_policy_notifier_list, CPUFREQ_UPDATE_POLICY_CPU, policy); + + return 0; } static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) @@ -1154,7 +1166,7 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) * by invoking update_policy_cpu(). */ if (recover_policy && cpu != policy->cpu) - update_policy_cpu(policy, cpu); + WARN_ON(update_policy_cpu(policy, cpu, dev)); else policy->cpu = cpu; @@ -1307,38 +1319,11 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) return __cpufreq_add_dev(dev, sif); } -static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy, - unsigned int old_cpu) -{ - struct device *cpu_dev; - int ret; - - /* first sibling now owns the new sysfs dir */ - cpu_dev = get_cpu_device(cpumask_any_but(policy->cpus, old_cpu)); - - sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); - ret = kobject_move(&policy->kobj, &cpu_dev->kobj); - if (ret) { - pr_err("%s: Failed to move kobj: %d\n", __func__, ret); - - down_write(&policy->rwsem); - cpumask_set_cpu(old_cpu, policy->cpus); - up_write(&policy->rwsem); - - ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, - "cpufreq"); - - return -EINVAL; - } - - return cpu_dev->id; -} - static int __cpufreq_remove_dev_prepare(struct device *dev, struct subsys_interface *sif) { unsigned int cpu = dev->id, cpus; - int new_cpu, ret; + int ret; unsigned long flags; struct cpufreq_policy *policy; @@ -1378,14 +1363,28 @@ static int __cpufreq_remove_dev_prepare(struct device *dev, if (cpu != policy->cpu) { sysfs_remove_link(&dev->kobj, "cpufreq"); } else if (cpus > 1) { - new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu); - if (new_cpu >= 0) { - update_policy_cpu(policy, new_cpu); + /* Nominate new CPU */ + int new_cpu = cpumask_any_but(policy->cpus, cpu); + struct device *cpu_dev = get_cpu_device(new_cpu); - if (!cpufreq_suspended) - pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n", - __func__, new_cpu, cpu); + sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); + ret = update_policy_cpu(policy, new_cpu, cpu_dev); + if (ret) { + /* + * To supress compilation warning about return value of + * sysfs_create_link(). + */ + int temp; + + /* Create link again if we failed. */ + temp = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, + "cpufreq"); + return ret; } + + if (!cpufreq_suspended) + pr_debug("%s: policy Kobject moved to cpu: %d from: %d\n", + __func__, new_cpu, cpu); } else if (cpufreq_driver->stop_cpu && cpufreq_driver->setpolicy) { cpufreq_driver->stop_cpu(policy); }
This is only relevant to implementations with multiple clusters, where clusters have separate clock lines but all CPUs within a cluster share it. Consider a dual cluster platform with 2 cores per cluster. During suspend we start offlining CPUs from 1 to 3. When CPU2 is remove, policy->kobj would be moved to CPU3 and when CPU3 goes down we wouldn't free policy or its kobj. Now on resume, we will get CPU2 before CPU3 and will call __cpufreq_add_dev(). We will recover the old policy and update policy->cpu from 3 to 2 from update_policy_cpu(). But the kobj is still tied to CPU3 and wasn't moved to CPU2. We wouldn't create a link for CPU2, but would try that while bringing CPU3 online. Which will report errors as CPU3 already has kobj assigned to it. This bug got introduced with commit 42f921a, which overlooked this scenario. To fix this, lets move kobj to the new policy->cpu while bringing first CPU of a cluster back. We already have update_policy_cpu() routine which can be updated with this change. That would get rid of the cpufreq_nominate_new_policy_cpu() as update_policy_cpu() is also called on CPU removal. To achieve that we add another parameter to update_policy_cpu() as cpu_dev is present with both the callers. Fixes: ("42f921a cpufreq: remove sysfs files for CPUs which failed to come back after resume") Cc: Stable <stable@vger.kernel.org> # 3.13+ Reported-by: Bu Yitian <ybu@qti.qualcomm.com> Reported-by: Saravana Kannan <skannan@codeaurora.org> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- V1->V2: Move kobject_move() call to update_policy_cpu(), which makes cpufreq_nominate_new_policy_cpu() almost empty. So we remove it completely. @Yitian: Sorry, but you need to test this again as there were enough modifications in V2. drivers/cpufreq/cpufreq.c | 73 +++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 37 deletions(-)