[V4,1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

Message ID e28c4b4deaf766910c366ab87b64325da59c8ad6.1443198783.git.viresh.kumar@linaro.org
State New
Headers show

Commit Message

Viresh Kumar Sept. 25, 2015, 4:41 p.m.
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(-)

Comments

Johannes Berg Sept. 25, 2015, 5:42 p.m. | #1
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-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg Sept. 25, 2015, 6:49 p.m. | #2
> 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-bluetooth" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Sept. 25, 2015, 6:52 p.m. | #3
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.
Rafael J. Wysocki Sept. 25, 2015, 8:18 p.m. | #4
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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael J. Wysocki Sept. 25, 2015, 8:22 p.m. | #5
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Sept. 25, 2015, 8:25 p.m. | #6
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Sept. 25, 2015, 8:26 p.m. | #7
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Sept. 25, 2015, 8:33 p.m. | #8
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Sept. 25, 2015, 8:58 p.m. | #9
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-bluetooth" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Sept. 25, 2015, 9:44 p.m. | #10
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 ?
Rafael J. Wysocki Sept. 25, 2015, 10:19 p.m. | #11
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Sept. 26, 2015, 7:33 p.m. | #12
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley Sept. 26, 2015, 7:52 p.m. | #13
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-bluetooth" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Sept. 27, 2015, 2:09 p.m. | #14
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Sept. 27, 2015, 2:10 p.m. | #15
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 netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight Sept. 28, 2015, 8:58 a.m. | #16
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.

> 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.

	David

N‹§˛ćěr¸›yúčšŘb˛XŹśÇ§vŘ^–)Ţş{.nÇ+‰ˇĽŠ{ą‘ęçzX§ś›ĄÜ¨}Šž˛Ć zÚ&j:+v‰¨žŤ‘ęçzZ+€Ę+zfŁ˘ˇhšˆ§~†­†Űi˙űŕzšŽwĽ˘¸?™¨č­Ú&˘)ߢf”ů^jÇŤy§m…á@AŤaśÚ˙0śěhŽĺ’i
James Bottomley Sept. 28, 2015, 2:26 p.m. | #17
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
David Laight Sept. 28, 2015, 2:50 p.m. | #18
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
James Bottomley Sept. 28, 2015, 3:11 p.m. | #19
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight Sept. 28, 2015, 3:31 p.m. | #20
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-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

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))