Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to comply ACPI 6.1

[Toshi Kani]

On Tue, 2016-03-01 at 10:14 -0800, Dan Williams wrote:
> On Tue, Mar 1, 2016 at 9:36 AM, Toshi Kani <toshi.kani@xxxxxxx> wrote:
> > On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote:
> > > We have a bunch of macros in include/acmacros.h -- like this:
> > > 
> > > ACPI_MOVE_16_TO_16(d, s)
> > 
> > There is a problem in using the ACPICA byte-swap macros.ÂÂACPI is
> > little-
> > endian arch, so the macros are set to perform byte-swappings when the
> > CPU
> > arch is big-endian.ÂÂThis case, however, is the other way around.ÂÂThe
> > fields in question are defined & stored as arrays of bytes.ÂÂIf you
> > treat
> > them as multi-bytes numeric values, then you need to byte-swap them
> > when
> > the CPU arch is little-endian because arrays of bytes have the same
> > addressing as big-endian.
> > 
> > Another issue is that it is not clear who needs to perform the byte-
> > swapping among ACPICA and drivers.ÂÂIf ACPICA, drivers must agree that
> > these fields are always treated as multi-bytes numeric values despite
> > of
> > the spec.ÂÂIf drivers, we need to make sure that only a single driver
> > performs this byte-swapping one time as ACPI tables are global
> > structures.
> > 
> > I think it is much clearer to define the structure according to the
> > ACPI
> > spec.
> 
> I think the "ACPI tables are little-endian" assumption is pervasive
> throughout the implementation.
> 
> Toshi, it seems all we need is conversions like:
> 
> - sprintf(buf, "%#x\n", dcr->vendor_id);
> + sprintf(buf, "%#x\n", le16_to_cpu(dcr->vendor_id));
> 
> ...for the values exported to userspace through sysfs, but otherwise
> leave the base table definitions as is.ÂÂWill this suffice?

Yes, I am OK to go with this option to get it done.

We still need to add new fields into the structure. ÂIs it OK to make this
change? ÂIf not, we will need to have a local structure to define the new
fields...

Thanks,
-Toshi