| Message ID | e28c4b4deaf766910c366ab87b64325da59c8ad6.1443198783.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, 2015-09-25 at 09:41 -0700, Viresh Kumar wrote: > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > V3->V4: > - Create a local variable instead of changing type of global_lock > (Rafael) Err, surely that wasn't what Rafael meant, since it's clearly impossible to use a pointer to the stack, assign to it once, and the expect anything to wkr at all ... johannes -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25-09-15, 19:42, Johannes Berg wrote: > On Fri, 2015-09-25 at 09:41 -0700, Viresh Kumar wrote: > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > V3->V4: > > - Create a local variable instead of changing type of global_lock > > (Rafael) > > Err, surely that wasn't what Rafael meant, since it's clearly > impossible to use a pointer to the stack, assign to it once, and the > expect anything to wkr at all ... Sorry, I am not sure on what wouldn't work with this patch but this is what Rafael said, and I don't think I wrote it differently :) Rafael 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.
> Rafael 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. Ok, then, but that means Rafael is completely wrong ... debugfs_create_bool() takes a *pointer* and it needs to be long-lived, it can't be on the stack. You also don't get a call when it changes. If you cannot change the struct definition then you must implement a debugfs file with its own read/write handlers. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25-09-15, 20:49, Johannes Berg wrote: > Ok, then, but that means Rafael is completely wrong ... > debugfs_create_bool() takes a *pointer* and it needs to be long-lived, > it can't be on the stack. You also don't get a call when it changes. Ahh, ofcourse. My bad as well... I think we can change structure definition but will wait for Rafael's comment before that.
On Friday, September 25, 2015 09:41:37 AM 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. There are no such platforms yet, but the > code needs to be robust for such a case. > > Fix that by passing a local variable to debugfs_create_bool() and > assigning its value to global_lock later. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Greg, please take this one if the [2/2] looks good to you. > --- > V3->V4: > - Create a local variable instead of changing type of global_lock > (Rafael) > - Drop the stable tag > - BCC'd a lot of people (rather than cc'ing them) to make sure > - the series reaches them > - mailing lists do not block the patchset due to long cc list > - and we don't spam the BCC'd people for every reply > --- > drivers/acpi/ec_sys.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c > index b4c216bab22b..b44b91331a56 100644 > --- a/drivers/acpi/ec_sys.c > +++ b/drivers/acpi/ec_sys.c > @@ -110,6 +110,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count) > struct dentry *dev_dir; > char name[64]; > umode_t mode = 0400; > + u32 val; > > if (ec_device_count == 0) { > acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL); > @@ -127,10 +128,11 @@ 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)) > + if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val)) > goto error; > > + first_ec->global_lock = val; > + > if (write_support) > mode = 0600; > if (!debugfs_create_file("io", mode, dev_dir, ec, &acpi_ec_io_ops)) > Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, September 25, 2015 10:18:13 PM Rafael J. Wysocki wrote: > On Friday, September 25, 2015 09:41:37 AM 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. There are no such platforms yet, but the > > code needs to be robust for such a case. > > > > Fix that by passing a local variable to debugfs_create_bool() and > > assigning its value to global_lock later. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Greg, please take this one if the [2/2] looks good to you. Ouch, turns out it was a bad idea. Please scratch that. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 September 2015 at 13:33, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > You're going to change that into bool in the next patch, right? Yeah. > So what if bool is a byte and the field is not word-aligned Its between two 'unsigned long' variables today, and the struct isn't packed. So, it will be aligned, isn't it? > and changing > that byte requires a read-modify-write. How do we ensure that things remain > consistent in that case? I didn't understood why a read-modify-write is special here? That's what will happen to most of the non-word-sized fields anyway? Probably I didn't understood what you meant.. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, September 25, 2015 11:52:56 AM Viresh Kumar wrote: > On 25-09-15, 20:49, Johannes Berg wrote: > > Ok, then, but that means Rafael is completely wrong ... > > debugfs_create_bool() takes a *pointer* and it needs to be long-lived, > > it can't be on the stack. You also don't get a call when it changes. > > Ahh, ofcourse. My bad as well... Well, sorry about the wrong suggestion. > I think we can change structure definition but will wait for Rafael's > comment before that. OK, change the structure then. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, September 25, 2015 10:26:22 PM Rafael J. Wysocki wrote: > On Friday, September 25, 2015 11:52:56 AM Viresh Kumar wrote: > > On 25-09-15, 20:49, Johannes Berg wrote: > > > Ok, then, but that means Rafael is completely wrong ... > > > debugfs_create_bool() takes a *pointer* and it needs to be long-lived, > > > it can't be on the stack. You also don't get a call when it changes. > > > > Ahh, ofcourse. My bad as well... > > Well, sorry about the wrong suggestion. > > > I think we can change structure definition but will wait for Rafael's > > comment before that. > > OK, change the structure then. But here's a question. You're going to change that into bool in the next patch, right? So what if bool is a byte and the field is not word-aligned and changing that byte requires a read-modify-write. How do we ensure that things remain consistent in that case? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, September 25, 2015 01:25:49 PM Viresh Kumar wrote: > On 25 September 2015 at 13:33, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > You're going to change that into bool in the next patch, right? > > Yeah. > > > So what if bool is a byte and the field is not word-aligned > > Its between two 'unsigned long' variables today, and the struct isn't packed. > So, it will be aligned, isn't it? > > > and changing > > that byte requires a read-modify-write. How do we ensure that things remain > > consistent in that case? > > I didn't understood why a read-modify-write is special here? That's > what will happen > to most of the non-word-sized fields anyway? > > Probably I didn't understood what you meant.. Say you have three adjacent fields in a structure, x, y, z, each one byte long. Initially, all of them are equal to 0. CPU A writes 1 to x and CPU B writes 2 to y at the same time. What's the result? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25-09-15, 22:58, Rafael J. Wysocki wrote: > Say you have three adjacent fields in a structure, x, y, z, each one byte long. > Initially, all of them are equal to 0. > > CPU A writes 1 to x and CPU B writes 2 to y at the same time. > > What's the result? But then two CPUs can update the same variable as well, and we must have proper locking in place to fix that. Anyway, that problem isn't here for sure as its between two unsigned-longs. So, should I just move it to bool and resend ?
On Fri, Sep 25, 2015 at 11:44 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 25-09-15, 22:58, Rafael J. Wysocki wrote: >> Say you have three adjacent fields in a structure, x, y, z, each one byte long. >> Initially, all of them are equal to 0. >> >> CPU A writes 1 to x and CPU B writes 2 to y at the same time. >> >> What's the result? > > But then two CPUs can update the same variable as well, and we must > have proper locking in place to fix that. Right. So if you allow something like debugfs to update your structure, how do you make sure there is the proper locking? Is that not a problem in all of the places modified by the [2/2]? How can the future users of the API know what to do to avoid potential problems with it? > > Anyway, that problem isn't here for sure as its between two > unsigned-longs. So, should I just move it to bool and resend ? I guess it might be more convenient to fold this into the other patch, because we seem to be splitting hairs here. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 September 2015 at 15:19, Rafael J. Wysocki <rafael@kernel.org> wrote: > So if you allow something like debugfs to update your structure, how > do you make sure there is the proper locking? Not really sure at all.. Isn't there some debugfs locking that will jump in, to avoid updation of fields to the same device? > Is that not a problem in all of the places modified by the [2/2]? Right, but its not new AFAICT. We already have u32 fields in those structs and on 64 bit machines we have the same read-update-write problems for two consecutive u32's. Right? > How can the future users of the API know what to do to avoid potential > problems with it? No idea really :) >> Anyway, that problem isn't here for sure as its between two >> unsigned-longs. So, should I just move it to bool and resend ? > > I guess it might be more convenient to fold this into the other patch, > because we seem to be splitting hairs here. I can and that's what I did. But then Arnd asked me to separate it out. I can fold it back if that's what you want. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 26 September 2015 11:40:00 Viresh Kumar wrote: > On 25 September 2015 at 15:19, Rafael J. Wysocki <rafael@kernel.org> wrote: > > So if you allow something like debugfs to update your structure, how > > do you make sure there is the proper locking? > > Not really sure at all.. Isn't there some debugfs locking that will > jump in, to avoid updation of fields to the same device? No, if you need any locking to access variable, you cannot use the simple debugfs helpers but have to provide your own functions. > >> Anyway, that problem isn't here for sure as its between two > >> unsigned-longs. So, should I just move it to bool and resend ? > > > > I guess it might be more convenient to fold this into the other patch, > > because we seem to be splitting hairs here. > > I can and that's what I did. But then Arnd asked me to separate it > out. I can fold it back if that's what you want. It still makes sense to keep it separate I think, the patch is clearly different from the other parts. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2015-09-25 at 22:58 +0200, Rafael J. Wysocki wrote: > On Friday, September 25, 2015 01:25:49 PM Viresh Kumar wrote: > > On 25 September 2015 at 13:33, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > You're going to change that into bool in the next patch, right? > > > > Yeah. > > > > > So what if bool is a byte and the field is not word-aligned > > > > Its between two 'unsigned long' variables today, and the struct isn't packed. > > So, it will be aligned, isn't it? > > > > > and changing > > > that byte requires a read-modify-write. How do we ensure that things remain > > > consistent in that case? > > > > I didn't understood why a read-modify-write is special here? That's > > what will happen > > to most of the non-word-sized fields anyway? > > > > Probably I didn't understood what you meant.. > > Say you have three adjacent fields in a structure, x, y, z, each one byte long. > Initially, all of them are equal to 0. > > CPU A writes 1 to x and CPU B writes 2 to y at the same time. > > What's the result? I think every CPU's cache architecure guarantees adjacent store integrity, even in the face of SMP, so it's x==1 and y==2. If you're thinking of old alpha SMP system where the lowest store width is 32 bits and thus you have to do RMW to update a byte, this was usually fixed by padding (assuming the structure is not packed). However, it was such a problem that even the later alpha chips had byte extensions. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday, September 26, 2015 12:52:08 PM James Bottomley wrote: > On Fri, 2015-09-25 at 22:58 +0200, Rafael J. Wysocki wrote: > > On Friday, September 25, 2015 01:25:49 PM Viresh Kumar wrote: > > > On 25 September 2015 at 13:33, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > You're going to change that into bool in the next patch, right? > > > > > > Yeah. > > > > > > > So what if bool is a byte and the field is not word-aligned > > > > > > Its between two 'unsigned long' variables today, and the struct isn't packed. > > > So, it will be aligned, isn't it? > > > > > > > and changing > > > > that byte requires a read-modify-write. How do we ensure that things remain > > > > consistent in that case? > > > > > > I didn't understood why a read-modify-write is special here? That's > > > what will happen > > > to most of the non-word-sized fields anyway? > > > > > > Probably I didn't understood what you meant.. > > > > Say you have three adjacent fields in a structure, x, y, z, each one byte long. > > Initially, all of them are equal to 0. > > > > CPU A writes 1 to x and CPU B writes 2 to y at the same time. > > > > What's the result? > > I think every CPU's cache architecure guarantees adjacent store > integrity, even in the face of SMP, so it's x==1 and y==2. If you're > thinking of old alpha SMP system where the lowest store width is 32 bits > and thus you have to do RMW to update a byte, this was usually fixed by > padding (assuming the structure is not packed). However, it was such a > problem that even the later alpha chips had byte extensions. OK, thanks! Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday, September 26, 2015 09:33:56 PM Arnd Bergmann wrote: > On Saturday 26 September 2015 11:40:00 Viresh Kumar wrote: > > On 25 September 2015 at 15:19, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > So if you allow something like debugfs to update your structure, how > > > do you make sure there is the proper locking? > > > > Not really sure at all.. Isn't there some debugfs locking that will > > jump in, to avoid updation of fields to the same device? > > No, if you need any locking to access variable, you cannot use the > simple debugfs helpers but have to provide your own functions. > > > >> Anyway, that problem isn't here for sure as its between two > > >> unsigned-longs. So, should I just move it to bool and resend ? > > > > > > I guess it might be more convenient to fold this into the other patch, > > > because we seem to be splitting hairs here. > > > > I can and that's what I did. But then Arnd asked me to separate it > > out. I can fold it back if that's what you want. > > It still makes sense to keep it separate I think, the patch is clearly > different from the other parts. I just don't see much point in going from unsigned long to u32 and then from 32 to bool if we can go directly to bool in one go. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26 September 2015 at 22:31, Jiri Slaby <jirislaby@gmail.com> wrote: > But this has to crash whenever the file is read as val's storage is gone at > that moment already, right? Yeah, its fixed now in the new version. This was a *really* bad idea :( -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday 27 September 2015 16:10:48 Rafael J. Wysocki wrote: > On Saturday, September 26, 2015 09:33:56 PM Arnd Bergmann wrote: > > On Saturday 26 September 2015 11:40:00 Viresh Kumar wrote: > > > On 25 September 2015 at 15:19, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > So if you allow something like debugfs to update your structure, how > > > > do you make sure there is the proper locking? > > > > > > Not really sure at all.. Isn't there some debugfs locking that will > > > jump in, to avoid updation of fields to the same device? > > > > No, if you need any locking to access variable, you cannot use the > > simple debugfs helpers but have to provide your own functions. > > > > > >> Anyway, that problem isn't here for sure as its between two > > > >> unsigned-longs. So, should I just move it to bool and resend ? > > > > > > > > I guess it might be more convenient to fold this into the other patch, > > > > because we seem to be splitting hairs here. > > > > > > I can and that's what I did. But then Arnd asked me to separate it > > > out. I can fold it back if that's what you want. > > > > It still makes sense to keep it separate I think, the patch is clearly > > different from the other parts. > > I just don't see much point in going from unsigned long to u32 and then > from 32 to bool if we can go directly to bool in one go. It's only important to keep the 34-file multi-subsystem trivial cleanup that doesn't change any functionality separate from the bugfix. If you like to avoid patching one of the files twice, the alternative would be to first change the API for all other instances from u32 to bool and leave ACPI alone, and then do the second patch that changes ACPI from long to bool. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RnJvbTogUmFmYWVsIEouIFd5c29ja2kNCj4gU2VudDogMjcgU2VwdGVtYmVyIDIwMTUgMTU6MDkN Ci4uLg0KPiA+ID4gU2F5IHlvdSBoYXZlIHRocmVlIGFkamFjZW50IGZpZWxkcyBpbiBhIHN0cnVj dHVyZSwgeCwgeSwgeiwgZWFjaCBvbmUgYnl0ZSBsb25nLg0KPiA+ID4gSW5pdGlhbGx5LCBhbGwg b2YgdGhlbSBhcmUgZXF1YWwgdG8gMC4NCj4gPiA+DQo+ID4gPiBDUFUgQSB3cml0ZXMgMSB0byB4 IGFuZCBDUFUgQiB3cml0ZXMgMiB0byB5IGF0IHRoZSBzYW1lIHRpbWUuDQo+ID4gPg0KPiA+ID4g V2hhdCdzIHRoZSByZXN1bHQ/DQo+ID4NCj4gPiBJIHRoaW5rIGV2ZXJ5IENQVSdzICBjYWNoZSBh cmNoaXRlY3VyZSBndWFyYW50ZWVzIGFkamFjZW50IHN0b3JlDQo+ID4gaW50ZWdyaXR5LCBldmVu IGluIHRoZSBmYWNlIG9mIFNNUCwgc28gaXQncyB4PT0xIGFuZCB5PT0yLiAgSWYgeW91J3JlDQo+ ID4gdGhpbmtpbmcgb2Ygb2xkIGFscGhhIFNNUCBzeXN0ZW0gd2hlcmUgdGhlIGxvd2VzdCBzdG9y ZSB3aWR0aCBpcyAzMiBiaXRzDQo+ID4gYW5kIHRodXMgeW91IGhhdmUgdG8gZG8gUk1XIHRvIHVw ZGF0ZSBhIGJ5dGUsIHRoaXMgd2FzIHVzdWFsbHkgZml4ZWQgYnkNCj4gPiBwYWRkaW5nIChhc3N1 bWluZyB0aGUgc3RydWN0dXJlIGlzIG5vdCBwYWNrZWQpLiAgSG93ZXZlciwgaXQgd2FzIHN1Y2gg YQ0KPiA+IHByb2JsZW0gdGhhdCBldmVuIHRoZSBsYXRlciBhbHBoYSBjaGlwcyBoYWQgYnl0ZSBl eHRlbnNpb25zLg0KDQpEb2VzIGxpbnV4IHN0aWxsIHN1cHBvcnQgdGhvc2Ugb2xkIEFscGhhcz8N Cg0KVGhlIHg4NiBjcHVzIHdpbGwgYWxzbyBkbyAzMmJpdCB3aWRlIHJtdyBjeWNsZXMgZm9yIHRo ZSAnYml0JyBvcGVyYXRpb25zLg0KDQo+IE9LLCB0aGFua3MhDQoNCllvdSBzdGlsbCBoYXZlIHRv IGVuc3VyZSB0aGUgY29tcGlsZXIgZG9lc24ndCBkbyB3aWRlciBybXcgY3ljbGVzLg0KSSBiZWxp ZXZlIHRoZSByZWNlbnQgdmVyc2lvbnMgb2YgZ2NjIHdvbid0IGRvIHdpZGVyIGFjY2Vzc2VzIGZv ciB2b2xhdGlsZSBkYXRhLg0KDQoJRGF2aWQNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, September 28, 2015 10:24:58 AM Arnd Bergmann wrote: > On Sunday 27 September 2015 16:10:48 Rafael J. Wysocki wrote: > > On Saturday, September 26, 2015 09:33:56 PM Arnd Bergmann wrote: > > > On Saturday 26 September 2015 11:40:00 Viresh Kumar wrote: > > > > On 25 September 2015 at 15:19, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > So if you allow something like debugfs to update your structure, how > > > > > do you make sure there is the proper locking? > > > > > > > > Not really sure at all.. Isn't there some debugfs locking that will > > > > jump in, to avoid updation of fields to the same device? > > > > > > No, if you need any locking to access variable, you cannot use the > > > simple debugfs helpers but have to provide your own functions. > > > > > > > >> Anyway, that problem isn't here for sure as its between two > > > > >> unsigned-longs. So, should I just move it to bool and resend ? > > > > > > > > > > I guess it might be more convenient to fold this into the other patch, > > > > > because we seem to be splitting hairs here. > > > > > > > > I can and that's what I did. But then Arnd asked me to separate it > > > > out. I can fold it back if that's what you want. > > > > > > It still makes sense to keep it separate I think, the patch is clearly > > > different from the other parts. > > > > I just don't see much point in going from unsigned long to u32 and then > > from 32 to bool if we can go directly to bool in one go. > > It's only important to keep the 34-file multi-subsystem trivial cleanup > that doesn't change any functionality separate from the bugfix. Which isn't a bugfix really, because the EC code is not run on any big-endian systems to my knowledge. And it won't matter after the [2/2] anyway. And the changelog of it doesn't really makes sense, because it talks about future systems, but after the [2/2] no future systems will run that code in the first place. > If you like to avoid patching one of the files twice, the alternative would > be to first change the API for all other instances from u32 to bool > and leave ACPI alone, and then do the second patch that changes ACPI > from long to bool. My point is that this patch doesn't matter. It doesn't really fix anything and the result of it goes away after the second patch. The only marginal value of having it as a separate commit is in case if (a) we need to revert the [2/2] for some reason and (b) ACPI-based ARM systems (the big-endian ones) become full-hardware at one point. You know what the chances of that are, though. :-) That said I've ACKed the patch, because I don't care that much. I'm not exactly sure why you care either. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-09-28 at 08:58 +0000, David Laight wrote: > From: Rafael J. Wysocki > > Sent: 27 September 2015 15:09 > ... > > > > Say you have three adjacent fields in a structure, x, y, z, each one byte long. > > > > Initially, all of them are equal to 0. > > > > > > > > CPU A writes 1 to x and CPU B writes 2 to y at the same time. > > > > > > > > What's the result? > > > > > > I think every CPU's cache architecure guarantees adjacent store > > > integrity, even in the face of SMP, so it's x==1 and y==2. If you're > > > thinking of old alpha SMP system where the lowest store width is 32 bits > > > and thus you have to do RMW to update a byte, this was usually fixed by > > > padding (assuming the structure is not packed). However, it was such a > > > problem that even the later alpha chips had byte extensions. > > Does linux still support those old Alphas? > > The x86 cpus will also do 32bit wide rmw cycles for the 'bit' operations. That's different: it's an atomic RMW operation. The problem with the alpha was that the operation wasn't atomic (meaning that it can't be interrupted and no intermediate output states are visible). > > OK, thanks! > > You still have to ensure the compiler doesn't do wider rmw cycles. > I believe the recent versions of gcc won't do wider accesses for volatile data. I don't understand this comment. You seem to be implying gcc would do a 64 bit RMW for a 32 bit store ... that would be daft when a single instruction exists to perform the operation on all architectures. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] > Sent: 28 September 2015 15:27 > On Mon, 2015-09-28 at 08:58 +0000, David Laight wrote: > > From: Rafael J. Wysocki > > > Sent: 27 September 2015 15:09 > > ... > > > > > Say you have three adjacent fields in a structure, x, y, z, each one byte long. > > > > > Initially, all of them are equal to 0. > > > > > > > > > > CPU A writes 1 to x and CPU B writes 2 to y at the same time. > > > > > > > > > > What's the result? > > > > > > > > I think every CPU's cache architecure guarantees adjacent store > > > > integrity, even in the face of SMP, so it's x==1 and y==2. If you're > > > > thinking of old alpha SMP system where the lowest store width is 32 bits > > > > and thus you have to do RMW to update a byte, this was usually fixed by > > > > padding (assuming the structure is not packed). However, it was such a > > > > problem that even the later alpha chips had byte extensions. > > > > Does linux still support those old Alphas? > > > > The x86 cpus will also do 32bit wide rmw cycles for the 'bit' operations. > > That's different: it's an atomic RMW operation. The problem with the > alpha was that the operation wasn't atomic (meaning that it can't be > interrupted and no intermediate output states are visible). It is only atomic if prefixed by the 'lock' prefix. Normally the read and write are separate bus cycles. > > You still have to ensure the compiler doesn't do wider rmw cycles. > > I believe the recent versions of gcc won't do wider accesses for volatile data. > > I don't understand this comment. You seem to be implying gcc would do a > 64 bit RMW for a 32 bit store ... that would be daft when a single > instruction exists to perform the operation on all architectures. Read the object code and weep... It is most likely to happen for operations that are rmw (eg bit set). For instance the arm cpu has limited offsets for 16bit accesses, for normal structures the compiler is likely to use a 32bit rmw sequence for a 16bit field that has a large offset. The C language allows the compiler to do it for any access (IIRC including volatiles). David
On Mon, 2015-09-28 at 14:50 +0000, David Laight wrote: > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] > > Sent: 28 September 2015 15:27 > > On Mon, 2015-09-28 at 08:58 +0000, David Laight wrote: > > > From: Rafael J. Wysocki > > > > Sent: 27 September 2015 15:09 > > > ... > > > > > > Say you have three adjacent fields in a structure, x, y, z, each one byte long. > > > > > > Initially, all of them are equal to 0. > > > > > > > > > > > > CPU A writes 1 to x and CPU B writes 2 to y at the same time. > > > > > > > > > > > > What's the result? > > > > > > > > > > I think every CPU's cache architecure guarantees adjacent store > > > > > integrity, even in the face of SMP, so it's x==1 and y==2. If you're > > > > > thinking of old alpha SMP system where the lowest store width is 32 bits > > > > > and thus you have to do RMW to update a byte, this was usually fixed by > > > > > padding (assuming the structure is not packed). However, it was such a > > > > > problem that even the later alpha chips had byte extensions. > > > > > > Does linux still support those old Alphas? > > > > > > The x86 cpus will also do 32bit wide rmw cycles for the 'bit' operations. > > > > That's different: it's an atomic RMW operation. The problem with the > > alpha was that the operation wasn't atomic (meaning that it can't be > > interrupted and no intermediate output states are visible). > > It is only atomic if prefixed by the 'lock' prefix. > Normally the read and write are separate bus cycles. The essential point is that x86 has atomic bit ops and byte writes. Early alpha did not. > > > You still have to ensure the compiler doesn't do wider rmw cycles. > > > I believe the recent versions of gcc won't do wider accesses for volatile data. > > > > I don't understand this comment. You seem to be implying gcc would do a > > 64 bit RMW for a 32 bit store ... that would be daft when a single > > instruction exists to perform the operation on all architectures. > > Read the object code and weep... > It is most likely to happen for operations that are rmw (eg bit set). > For instance the arm cpu has limited offsets for 16bit accesses, for > normal structures the compiler is likely to use a 32bit rmw sequence > for a 16bit field that has a large offset. > The C language allows the compiler to do it for any access (IIRC including > volatiles). I think you might be confusing different things. Most RISC CPUs can't do 32 bit store immediates because there aren't enough bits in their arsenal, so they tend to split 32 bit loads into a left and right part (first the top then the offset). This (and other things) are mostly what you see in code. However, 32 bit register stores are still atomic, which is all we require. It's not really the compiler's fault, it's mostly an architectural limitation. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RnJvbTogSmFtZXMgQm90dG9tbGV5IA0KPiBTZW50OiAyOCBTZXB0ZW1iZXIgMjAxNSAxNjoxMg0K PiA+ID4gPiBUaGUgeDg2IGNwdXMgd2lsbCBhbHNvIGRvIDMyYml0IHdpZGUgcm13IGN5Y2xlcyBm b3IgdGhlICdiaXQnIG9wZXJhdGlvbnMuDQo+ID4gPg0KPiA+ID4gVGhhdCdzIGRpZmZlcmVudDog aXQncyBhbiBhdG9taWMgUk1XIG9wZXJhdGlvbi4gIFRoZSBwcm9ibGVtIHdpdGggdGhlDQo+ID4g PiBhbHBoYSB3YXMgdGhhdCB0aGUgb3BlcmF0aW9uIHdhc24ndCBhdG9taWMgKG1lYW5pbmcgdGhh dCBpdCBjYW4ndCBiZQ0KPiA+ID4gaW50ZXJydXB0ZWQgYW5kIG5vIGludGVybWVkaWF0ZSBvdXRw dXQgc3RhdGVzIGFyZSB2aXNpYmxlKS4NCj4gPg0KPiA+IEl0IGlzIG9ubHkgYXRvbWljIGlmIHBy ZWZpeGVkIGJ5IHRoZSAnbG9jaycgcHJlZml4Lg0KPiA+IE5vcm1hbGx5IHRoZSByZWFkIGFuZCB3 cml0ZSBhcmUgc2VwYXJhdGUgYnVzIGN5Y2xlcy4NCj4gDQo+IFRoZSBlc3NlbnRpYWwgcG9pbnQg aXMgdGhhdCB4ODYgaGFzIGF0b21pYyBiaXQgb3BzIGFuZCBieXRlIHdyaXRlcy4NCj4gRWFybHkg YWxwaGEgZGlkIG5vdC4NCg0KRWFybHkgYWxwaGEgZGlkbid0IGhhdmUgYW55IGJ5dGUgYWNjZXNz ZXMuDQoNCk9uIHg4NiBpZiB5b3UgaGF2ZSB0aGUgZm9sbG93aW5nOg0KCXN0cnVjdCB7DQoJCWNo YXIgIGE7DQoJCXZvbGF0aWxlIGNoYXIgYjsNCgl9ICpmb287DQoJZm9vLT5hIHw9IDQ7DQoNClRo ZSBjb21waWxlciBpcyBsaWtlbHkgdG8gZ2VuZXJhdGUgYSAnYmlzICM0LCAwKHJieCknIChvciBz aW1pbGFyKQ0KYW5kIHRoZSBjcHUgd2lsbCBkbyB0d28gMzJiaXQgbWVtb3J5IGN5Y2xlcyB0aGF0 IHJlYWQgYW5kIHdyaXRlDQp0aGUgJ3ZvbGF0aWxlJyBmaWVsZCAnYicuDQooZ2NjIGRlZmluaXRl bHkgdXNlZCB0byBkbyB0aGlzLi4uKQ0KDQpBIGxvdCBvZiBmaWVsZHMgd2VyZSBtYWRlIDMyYml0 IChhbmQgcHJvYmFibHkgbm90IGJpdGZpZWxkcykgaW4gdGhlIGxpbnV4DQprZXJuZWwgdHJlZSBh IHllYXIgb3IgdHdvIGFnbyB0byBhdm9pZCB0aGlzIHZlcnkgcHJvYmxlbS4NCg0KPiA+ID4gPiBZ b3Ugc3RpbGwgaGF2ZSB0byBlbnN1cmUgdGhlIGNvbXBpbGVyIGRvZXNuJ3QgZG8gd2lkZXIgcm13 IGN5Y2xlcy4NCj4gPiA+ID4gSSBiZWxpZXZlIHRoZSByZWNlbnQgdmVyc2lvbnMgb2YgZ2NjIHdv bid0IGRvIHdpZGVyIGFjY2Vzc2VzIGZvciB2b2xhdGlsZSBkYXRhLg0KPiA+ID4NCj4gPiA+IEkg ZG9uJ3QgdW5kZXJzdGFuZCB0aGlzIGNvbW1lbnQuICBZb3Ugc2VlbSB0byBiZSBpbXBseWluZyBn Y2Mgd291bGQgZG8gYQ0KPiA+ID4gNjQgYml0IFJNVyBmb3IgYSAzMiBiaXQgc3RvcmUgLi4uIHRo YXQgd291bGQgYmUgZGFmdCB3aGVuIGEgc2luZ2xlDQo+ID4gPiBpbnN0cnVjdGlvbiBleGlzdHMg dG8gcGVyZm9ybSB0aGUgb3BlcmF0aW9uIG9uIGFsbCBhcmNoaXRlY3R1cmVzLg0KPiA+DQo+ID4g UmVhZCB0aGUgb2JqZWN0IGNvZGUgYW5kIHdlZXAuLi4NCj4gPiBJdCBpcyBtb3N0IGxpa2VseSB0 byBoYXBwZW4gZm9yIG9wZXJhdGlvbnMgdGhhdCBhcmUgcm13IChlZyBiaXQgc2V0KS4NCj4gPiBG b3IgaW5zdGFuY2UgdGhlIGFybSBjcHUgaGFzIGxpbWl0ZWQgb2Zmc2V0cyBmb3IgMTZiaXQgYWNj ZXNzZXMsIGZvcg0KPiA+IG5vcm1hbCBzdHJ1Y3R1cmVzIHRoZSBjb21waWxlciBpcyBsaWtlbHkg dG8gdXNlIGEgMzJiaXQgcm13IHNlcXVlbmNlDQo+ID4gZm9yIGEgMTZiaXQgZmllbGQgdGhhdCBo YXMgYSBsYXJnZSBvZmZzZXQuDQo+ID4gVGhlIEMgbGFuZ3VhZ2UgYWxsb3dzIHRoZSBjb21waWxl ciB0byBkbyBpdCBmb3IgYW55IGFjY2VzcyAoSUlSQyBpbmNsdWRpbmcNCj4gPiB2b2xhdGlsZXMp Lg0KPiANCj4gSSB0aGluayB5b3UgbWlnaHQgYmUgY29uZnVzaW5nIGRpZmZlcmVudCB0aGluZ3Mu ICBNb3N0IFJJU0MgQ1BVcyBjYW4ndA0KPiBkbyAzMiBiaXQgc3RvcmUgaW1tZWRpYXRlcyBiZWNh dXNlIHRoZXJlIGFyZW4ndCBlbm91Z2ggYml0cyBpbiB0aGVpcg0KPiBhcnNlbmFsLCBzbyB0aGV5 IHRlbmQgdG8gc3BsaXQgMzIgYml0IGxvYWRzIGludG8gYSBsZWZ0IGFuZCByaWdodCBwYXJ0DQo+ IChmaXJzdCB0aGUgdG9wIHRoZW4gdGhlIG9mZnNldCkuICBUaGlzIChhbmQgb3RoZXIgdGhpbmdz KSBhcmUgbW9zdGx5DQo+IHdoYXQgeW91IHNlZSBpbiBjb2RlLiAgSG93ZXZlciwgMzIgYml0IHJl Z2lzdGVyIHN0b3JlcyBhcmUgc3RpbGwgYXRvbWljLA0KPiB3aGljaCBpcyBhbGwgd2UgcmVxdWly ZS4gIEl0J3Mgbm90IHJlYWxseSB0aGUgY29tcGlsZXIncyBmYXVsdCwgaXQncw0KPiBtb3N0bHkg YW4gYXJjaGl0ZWN0dXJhbCBsaW1pdGF0aW9uLg0KDQpObywgSSdtIG5vdCB0YWxraW5nIGFib3V0 IGhvdyAzMmJpdCBjb25zdGFudHMgYXJlIGdlbmVyYXRlZC4NCkknbSB0YWxraW5nIGFib3V0IHN0 cnVjdHVyZSBvZmZzZXRzLg0KDQoJRGF2aWQNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" 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..b44b91331a56 100644 --- a/drivers/acpi/ec_sys.c +++ b/drivers/acpi/ec_sys.c @@ -110,6 +110,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count) struct dentry *dev_dir; char name[64]; umode_t mode = 0400; + u32 val; if (ec_device_count == 0) { acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL); @@ -127,10 +128,11 @@ 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)) + if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val)) goto error; + first_ec->global_lock = val; + if (write_support) mode = 0600; if (!debugfs_create_file("io", mode, dev_dir, ec, &acpi_ec_io_ops))
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. There are no such platforms yet, but the code needs to be robust for such a case. Fix that by passing a local variable to debugfs_create_bool() and assigning its value to global_lock later. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- V3->V4: - Create a local variable instead of changing type of global_lock (Rafael) - Drop the stable tag - BCC'd a lot of people (rather than cc'ing them) to make sure - the series reaches them - mailing lists do not block the patchset due to long cc list - and we don't spam the BCC'd people for every reply --- drivers/acpi/ec_sys.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)