| Message ID | 9b705747a138c96c26faee5218f7b47403195b28.1442305897.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Tuesday, September 15, 2015 02:04:58 PM Viresh Kumar wrote: > global_lock is defined as an unsigned long and accessing only its lower > 32 bits from sysfs is incorrect, as we need to consider other 32 bits > for big endian 64 bit systems. > > Fix that by making global_lock an u32 instead. > > Cc: <stable@vger.kernel.org> # v4.1+ > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > Its marked just for # v4.1+, because arm64 has the first 64 big-endian > platform with ACPI. And ACPI support for that is mainlined recently > only (Arnd Bergmann). OK So are you aware of any ARM platform implementing the ACPI EC? I am not. Moreover, I'm not aware of any plans in that area. > Another thing worth noticing is that, global_lock is getting an unsigned > long long value assigned to it in ec_parse_device() and this is what > Arnd had to say about that: > > "I think that's fine, it does this because the _GLP variable in ACPI is > defined as an u64. And that's what happens on 32-bit architectures > anyway." > > This patch should go via GregKH, as the second patch has dependency on > it. > > V2->V3: > - Moved this out in a separate patch, so that it can be backported. Lv, can you have a look at this, please? > --- > drivers/acpi/ec_sys.c | 2 +- > drivers/acpi/internal.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c > index b4c216bab22b..bea8e425a8de 100644 > --- a/drivers/acpi/ec_sys.c > +++ b/drivers/acpi/ec_sys.c > @@ -128,7 +128,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count) > if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe)) > goto error; > if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, > - (u32 *)&first_ec->global_lock)) > + &first_ec->global_lock)) > goto error; > > if (write_support) > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > index 9e426210c2a8..9db196de003c 100644 > --- a/drivers/acpi/internal.h > +++ b/drivers/acpi/internal.h > @@ -138,7 +138,7 @@ struct acpi_ec { > unsigned long gpe; > unsigned long command_addr; > unsigned long data_addr; > - unsigned long global_lock; > + u32 global_lock; > unsigned long flags; > unsigned long reference_count; > struct mutex mutex; > Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16-09-15, 04:06, Rafael J. Wysocki wrote: > In any case, please just split the EC-related changes off from your second > patch and send them separately. That !! change isn't required anymore, will be dropping it completely.
On Wednesday, September 16, 2015 03:57:05 AM Rafael J. Wysocki wrote: > On Tuesday, September 15, 2015 02:04:58 PM Viresh Kumar wrote: > > global_lock is defined as an unsigned long and accessing only its lower > > 32 bits from sysfs is incorrect, as we need to consider other 32 bits > > for big endian 64 bit systems. > > > > Fix that by making global_lock an u32 instead. > > > > Cc: <stable@vger.kernel.org> # v4.1+ > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > --- > > Its marked just for # v4.1+, because arm64 has the first 64 big-endian > > platform with ACPI. And ACPI support for that is mainlined recently > > only (Arnd Bergmann). > > OK > > So are you aware of any ARM platform implementing the ACPI EC? > > I am not. Moreover, I'm not aware of any plans in that area. In any case, please just split the EC-related changes off from your second patch and send them separately. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
SGksDQoNCj4gRnJvbTogUmFmYWVsIEouIFd5c29ja2kgW21haWx0bzpyandAcmp3eXNvY2tpLm5l dF0NCj4gU2VudDogV2VkbmVzZGF5LCBTZXB0ZW1iZXIgMTYsIDIwMTUgOTo1NyBBTQ0KPiANCj4g T24gVHVlc2RheSwgU2VwdGVtYmVyIDE1LCAyMDE1IDAyOjA0OjU4IFBNIFZpcmVzaCBLdW1hciB3 cm90ZToNCj4gPiBnbG9iYWxfbG9jayBpcyBkZWZpbmVkIGFzIGFuIHVuc2lnbmVkIGxvbmcgYW5k IGFjY2Vzc2luZyBvbmx5IGl0cyBsb3dlcg0KPiA+IDMyIGJpdHMgZnJvbSBzeXNmcyBpcyBpbmNv cnJlY3QsIGFzIHdlIG5lZWQgdG8gY29uc2lkZXIgb3RoZXIgMzIgYml0cw0KPiA+IGZvciBiaWcg ZW5kaWFuIDY0IGJpdCBzeXN0ZW1zLg0KPiA+DQo+ID4gRml4IHRoYXQgYnkgbWFraW5nIGdsb2Jh bF9sb2NrIGFuIHUzMiBpbnN0ZWFkLg0KPiA+DQo+ID4gQ2M6IDxzdGFibGVAdmdlci5rZXJuZWwu b3JnPiAgIyB2NC4xKw0KPiA+IFNpZ25lZC1vZmYtYnk6IFZpcmVzaCBLdW1hciA8dmlyZXNoLmt1 bWFyQGxpbmFyby5vcmc+DQo+ID4NCj4gPiAtLS0NCj4gPiBJdHMgbWFya2VkIGp1c3QgZm9yICMg djQuMSssIGJlY2F1c2UgYXJtNjQgaGFzIHRoZSBmaXJzdCA2NCBiaWctZW5kaWFuDQo+ID4gcGxh dGZvcm0gd2l0aCBBQ1BJLiBBbmQgQUNQSSBzdXBwb3J0IGZvciB0aGF0IGlzIG1haW5saW5lZCBy ZWNlbnRseQ0KPiA+IG9ubHkgKEFybmQgQmVyZ21hbm4pLg0KPiANCj4gT0sNCj4gDQo+IFNvIGFy ZSB5b3UgYXdhcmUgb2YgYW55IEFSTSBwbGF0Zm9ybSBpbXBsZW1lbnRpbmcgdGhlIEFDUEkgRUM/ DQo+IA0KPiBJIGFtIG5vdC4gIE1vcmVvdmVyLCBJJ20gbm90IGF3YXJlIG9mIGFueSBwbGFucyBp biB0aGF0IGFyZWEuDQo+IA0KPiA+IEFub3RoZXIgdGhpbmcgd29ydGggbm90aWNpbmcgaXMgdGhh dCwgZ2xvYmFsX2xvY2sgaXMgZ2V0dGluZyBhbiB1bnNpZ25lZA0KPiA+IGxvbmcgbG9uZyB2YWx1 ZSBhc3NpZ25lZCB0byBpdCBpbiBlY19wYXJzZV9kZXZpY2UoKSBhbmQgdGhpcyBpcyB3aGF0DQo+ ID4gQXJuZCBoYWQgdG8gc2F5IGFib3V0IHRoYXQ6DQo+ID4NCj4gPiAiSSB0aGluayB0aGF0J3Mg ZmluZSwgaXQgZG9lcyB0aGlzIGJlY2F1c2UgdGhlIF9HTFAgdmFyaWFibGUgaW4gQUNQSSBpcw0K PiA+IGRlZmluZWQgYXMgYW4gdTY0LiBBbmQgdGhhdCdzIHdoYXQgaGFwcGVucyBvbiAzMi1iaXQg YXJjaGl0ZWN0dXJlcw0KPiA+IGFueXdheS4iDQo+ID4NCj4gPiBUaGlzIHBhdGNoIHNob3VsZCBn byB2aWEgR3JlZ0tILCBhcyB0aGUgc2Vjb25kIHBhdGNoIGhhcyBkZXBlbmRlbmN5IG9uDQo+ID4g aXQuDQo+ID4NCj4gPiBWMi0+VjM6DQo+ID4gLSBNb3ZlZCB0aGlzIG91dCBpbiBhIHNlcGFyYXRl IHBhdGNoLCBzbyB0aGF0IGl0IGNhbiBiZSBiYWNrcG9ydGVkLg0KPiANCj4gTHYsIGNhbiB5b3Ug aGF2ZSBhIGxvb2sgYXQgdGhpcywgcGxlYXNlPw0KPiANCj4gPiAtLS0NCj4gPiAgZHJpdmVycy9h Y3BpL2VjX3N5cy5jICAgfCAyICstDQo+ID4gIGRyaXZlcnMvYWNwaS9pbnRlcm5hbC5oIHwgMiAr LQ0KPiA+ICAyIGZpbGVzIGNoYW5nZWQsIDIgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkN Cj4gPg0KPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2FjcGkvZWNfc3lzLmMgYi9kcml2ZXJzL2Fj cGkvZWNfc3lzLmMNCj4gPiBpbmRleCBiNGMyMTZiYWIyMmIuLmJlYThlNDI1YThkZSAxMDA2NDQN Cj4gPiAtLS0gYS9kcml2ZXJzL2FjcGkvZWNfc3lzLmMNCj4gPiArKysgYi9kcml2ZXJzL2FjcGkv ZWNfc3lzLmMNCj4gPiBAQCAtMTI4LDcgKzEyOCw3IEBAIHN0YXRpYyBpbnQgYWNwaV9lY19hZGRf ZGVidWdmcyhzdHJ1Y3QgYWNwaV9lYyAqZWMsIHVuc2lnbmVkIGludCBlY19kZXZpY2VfY291bnQp DQo+ID4gIAlpZiAoIWRlYnVnZnNfY3JlYXRlX3gzMigiZ3BlIiwgMDQ0NCwgZGV2X2RpciwgKHUz MiAqKSZmaXJzdF9lYy0+Z3BlKSkNCj4gPiAgCQlnb3RvIGVycm9yOw0KPiA+ICAJaWYgKCFkZWJ1 Z2ZzX2NyZWF0ZV9ib29sKCJ1c2VfZ2xvYmFsX2xvY2siLCAwNDQ0LCBkZXZfZGlyLA0KPiA+IC0J CQkJICh1MzIgKikmZmlyc3RfZWMtPmdsb2JhbF9sb2NrKSkNCj4gPiArCQkJCSAmZmlyc3RfZWMt Pmdsb2JhbF9sb2NrKSkNCj4gPiAgCQlnb3RvIGVycm9yOw0KPiA+DQo+ID4gIAlpZiAod3JpdGVf c3VwcG9ydCkNCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9hY3BpL2ludGVybmFsLmggYi9kcml2 ZXJzL2FjcGkvaW50ZXJuYWwuaA0KPiA+IGluZGV4IDllNDI2MjEwYzJhOC4uOWRiMTk2ZGUwMDNj IDEwMDY0NA0KPiA+IC0tLSBhL2RyaXZlcnMvYWNwaS9pbnRlcm5hbC5oDQo+ID4gKysrIGIvZHJp dmVycy9hY3BpL2ludGVybmFsLmgNCj4gPiBAQCAtMTM4LDcgKzEzOCw3IEBAIHN0cnVjdCBhY3Bp X2VjIHsNCj4gPiAgCXVuc2lnbmVkIGxvbmcgZ3BlOw0KPiA+ICAJdW5zaWduZWQgbG9uZyBjb21t YW5kX2FkZHI7DQo+ID4gIAl1bnNpZ25lZCBsb25nIGRhdGFfYWRkcjsNCj4gPiAtCXVuc2lnbmVk IGxvbmcgZ2xvYmFsX2xvY2s7DQo+ID4gKwl1MzIgZ2xvYmFsX2xvY2s7DQo+ID4gIAl1bnNpZ25l ZCBsb25nIGZsYWdzOw0KPiA+ICAJdW5zaWduZWQgbG9uZyByZWZlcmVuY2VfY291bnQ7DQo+ID4g IAlzdHJ1Y3QgbXV0ZXggbXV0ZXg7DQo+ID4NCj4gDQoNCklmIHdlIHJlYWxseSB3YW50IHRvIGNo YW5nZSwgd2UgbWF5IGNoYW5nZSBldmVyeXRoaW5nIGhlcmUuDQpncGUvY29tbWFuZF9hZGRyL2Rh dGFfYWRkci9nbG9iYWxfbG9jay4NCkFuZCBJTU8sIGlmIHdlIHJlYWxseSB3YW50IHRvIGNoYW5n ZSBnbG9iYWxfbG9jaywgd2Ugc2hvdWxkIG1ha2UgaXQgYm9vbCBoZXJlLg0KDQpUaGFua3MgYW5k IGJlc3QgcmVnYXJkcw0KLUx2DQoNCj4gVGhhbmtzLA0KPiBSYWZhZWwNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15/09/15 09:34, Viresh Kumar wrote: > global_lock is defined as an unsigned long and accessing only its lower > 32 bits from sysfs is incorrect, as we need to consider other 32 bits > for big endian 64 bit systems. > > Fix that by making global_lock an u32 instead. > > Cc: <stable@vger.kernel.org> # v4.1+ > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > Its marked just for # v4.1+, because arm64 has the first 64 big-endian > platform with ACPI. And ACPI support for that is mainlined recently > only (Arnd Bergmann). > Just to clarify, we don't support big-endian with ACPI on ARM64. We mandate use of EFI for ACPI on ARM64 and EFI spec mandates only little endian. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23-09-15, 07:52, Zheng, Lv wrote:
> And IMO, if we really want to change global_lock, we should make it bool here.
Yeah, so the second patch in this series is doing just that. Just kept
this patch separate to make more sense. Will resend both and keep you
in cc.
On 25-09-15, 02:17, Rafael J. Wysocki wrote: > Actually, what about adding a local u32 variable, say val, here and doing > > > if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe)) > > goto error; > > if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, > > - (u32 *)&first_ec->global_lock)) > > + &first_ec->global_lock)) > > if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val)) > > > goto error; > > first_ec->global_lock = val; > > And then you can turn val into bool just fine without changing the structure > definition. sure. Looks good.
On Tuesday, September 15, 2015 02:04:58 PM Viresh Kumar wrote: > global_lock is defined as an unsigned long and accessing only its lower > 32 bits from sysfs is incorrect, as we need to consider other 32 bits > for big endian 64 bit systems. > > Fix that by making global_lock an u32 instead. > > Cc: <stable@vger.kernel.org> # v4.1+ > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > Its marked just for # v4.1+, because arm64 has the first 64 big-endian > platform with ACPI. And ACPI support for that is mainlined recently > only (Arnd Bergmann). > > Another thing worth noticing is that, global_lock is getting an unsigned > long long value assigned to it in ec_parse_device() and this is what > Arnd had to say about that: > > "I think that's fine, it does this because the _GLP variable in ACPI is > defined as an u64. And that's what happens on 32-bit architectures > anyway." > > This patch should go via GregKH, as the second patch has dependency on > it. > > V2->V3: > - Moved this out in a separate patch, so that it can be backported. > --- > drivers/acpi/ec_sys.c | 2 +- > drivers/acpi/internal.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c > index b4c216bab22b..bea8e425a8de 100644 > --- a/drivers/acpi/ec_sys.c > +++ b/drivers/acpi/ec_sys.c > @@ -128,7 +128,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count) Actually, what about adding a local u32 variable, say val, here and doing > if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe)) > goto error; > if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, > - (u32 *)&first_ec->global_lock)) > + &first_ec->global_lock)) if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val)) > goto error; first_ec->global_lock = val; And then you can turn val into bool just fine without changing the structure definition. > > if (write_support) > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > index 9e426210c2a8..9db196de003c 100644 > --- a/drivers/acpi/internal.h > +++ b/drivers/acpi/internal.h > @@ -138,7 +138,7 @@ struct acpi_ec { > unsigned long gpe; > unsigned long command_addr; > unsigned long data_addr; > - unsigned long global_lock; > + u32 global_lock; > unsigned long flags; > unsigned long reference_count; > struct mutex mutex; > Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" 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/acpi/ec_sys.c b/drivers/acpi/ec_sys.c index b4c216bab22b..bea8e425a8de 100644 --- a/drivers/acpi/ec_sys.c +++ b/drivers/acpi/ec_sys.c @@ -128,7 +128,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count) if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe)) goto error; if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, - (u32 *)&first_ec->global_lock)) + &first_ec->global_lock)) goto error; if (write_support) diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 9e426210c2a8..9db196de003c 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -138,7 +138,7 @@ struct acpi_ec { unsigned long gpe; unsigned long command_addr; unsigned long data_addr; - unsigned long global_lock; + u32 global_lock; unsigned long flags; unsigned long reference_count; struct mutex mutex;
global_lock is defined as an unsigned long and accessing only its lower 32 bits from sysfs is incorrect, as we need to consider other 32 bits for big endian 64 bit systems. Fix that by making global_lock an u32 instead. Cc: <stable@vger.kernel.org> # v4.1+ Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- Its marked just for # v4.1+, because arm64 has the first 64 big-endian platform with ACPI. And ACPI support for that is mainlined recently only (Arnd Bergmann). Another thing worth noticing is that, global_lock is getting an unsigned long long value assigned to it in ec_parse_device() and this is what Arnd had to say about that: "I think that's fine, it does this because the _GLP variable in ACPI is defined as an u64. And that's what happens on 32-bit architectures anyway." This patch should go via GregKH, as the second patch has dependency on it. V2->V3: - Moved this out in a separate patch, so that it can be backported. --- drivers/acpi/ec_sys.c | 2 +- drivers/acpi/internal.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)