RE: [PATCH V3 1/2] ACPI / EC: Fix broken big-endian 64bit platforms using 'global_lock'

[Zheng, Lv]

Hi,

> From: Rafael J. Wysocki [mailto:rjw@xxxxxxxxxxxxx]
> Sent: Wednesday, September 16, 2015 9:57 AM
> 
> 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@xxxxxxxxxxxxxxx>  # v4.1+
> > Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> >
> > ---
> > 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;
> >
> 

If we really want to change, we may change everything here.
gpe/command_addr/data_addr/global_lock.
And IMO, if we really want to change global_lock, we should make it bool here.

Thanks and best regards
-Lv

> Thanks,
> Rafael

N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå