Re: [PATCH 1/2] ACPI/NFIT: Update Control Region Structure per ACPI 6.1

[Dan Williams]

On Fri, Feb 19, 2016 at 3:08 PM, Toshi Kani <toshi.kani@xxxxxxx> wrote:
> ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure
> as follows.
>  - Valid Fields, Manufacturing Location, and Manufacturing Date
>    are added from reserved range.  No change in the structure size.
>  - IDs defined as SPD values are arrays of bytes.  The spec
>    clarified that they need to be represented as arrays of bytes
>    as well.
>
> This patch makes the following changes to support this update.
>  - Change 'struct acpi_nfit_control_region' to reflect the update.
>    SPD IDs are defined as arrays of bytes, so that they can be
>    treated in the same way regardless of CPU endianness and are
>    not miss-treated as little-endian numeric values.
>  - Change the NFIT driver to show SPD ID values as array of bytes
>    in sysfs.
>  - Change the NFIT driver to show Manufacturing Location and
>    Manufacturing Date when they are valid.
>  - Change the NFIT driver to show an NVDIMM unique ID as defined
>    in section 5.2.25.9 of the spec.
>  - Change sprintf format to use "0x" instead of "#" since "%#02x"
>    does not prepend '0' in some reason.
>
> link: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf
> Signed-off-by: Toshi Kani <toshi.kani@xxxxxxx>
> Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Robert Moore <robert.moore@xxxxxxxxx>
> Cc: Robert Elliott <elliott@xxxxxxx>
> Cc: <devel@xxxxxxxxxx>
> ---
>  drivers/acpi/nfit.c   |   75 ++++++++++++++++++++++++++++++++++++++++++-------
>  include/acpi/actbl1.h |   24 ++++++++++------
>  2 files changed, 80 insertions(+), 19 deletions(-)

Hi Toshi,

I general looks good, but note that changes to include/acpi/actbl1.h
need to go through the ACPICA project.  Then their normal import
process will bring it into the kernel.  I added Lv to the cc.

>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index ad6d8c6..06677d9 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -722,7 +722,8 @@ static ssize_t vendor_show(struct device *dev,
>  {
>         struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
>
> -       return sprintf(buf, "%#x\n", dcr->vendor_id);
> +       return sprintf(buf, "0x%02x%02x\n",
> +                               dcr->vendor_id[0], dcr->vendor_id[1]);
>  }
>  static DEVICE_ATTR_RO(vendor);
>
> @@ -731,7 +732,8 @@ static ssize_t rev_id_show(struct device *dev,
>  {
>         struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
>
> -       return sprintf(buf, "%#x\n", dcr->revision_id);
> +       return sprintf(buf, "0x%02x%02x\n",
> +                               dcr->revision_id[0], dcr->revision_id[1]);
>  }
>  static DEVICE_ATTR_RO(rev_id);
>
> @@ -740,7 +742,8 @@ static ssize_t device_show(struct device *dev,
>  {
>         struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
>
> -       return sprintf(buf, "%#x\n", dcr->device_id);
> +       return sprintf(buf, "0x%02x%02x\n",
> +                               dcr->device_id[0], dcr->device_id[1]);
>  }
>  static DEVICE_ATTR_RO(device);
>
> @@ -749,7 +752,7 @@ static ssize_t format_show(struct device *dev,
>  {
>         struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
>
> -       return sprintf(buf, "%#x\n", dcr->code);
> +       return sprintf(buf, "0x%02x%02x\n", dcr->code[0], dcr->code[1]);
>  }
>  static DEVICE_ATTR_RO(format);
>
> @@ -758,7 +761,9 @@ static ssize_t serial_show(struct device *dev,
>  {
>         struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
>
> -       return sprintf(buf, "%#x\n", dcr->serial_number);
> +       return sprintf(buf, "0x%02x%02x%02x%02x\n",
> +                       dcr->serial_number[0], dcr->serial_number[1],
> +                       dcr->serial_number[2], dcr->serial_number[3]);
>  }
>  static DEVICE_ATTR_RO(serial);

These fixups look good.

Can we split the new sysfs attributes below into their own patch?  One
more comments below:

>
> @@ -776,6 +781,45 @@ static ssize_t flags_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(flags);
>
> +static ssize_t mfg_location_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
> +
> +       return sprintf(buf, "0x%02x\n", dcr->manufacturing_location);
> +}
> +static DEVICE_ATTR_RO(mfg_location);
> +
> +static ssize_t mfg_date_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
> +
> +       return sprintf(buf, "0x%02x%02x\n",
> +                       dcr->manufacturing_date[0], dcr->manufacturing_date[1]);
> +}
> +static DEVICE_ATTR_RO(mfg_date);
> +
> +static ssize_t unique_id_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
> +
> +       if (dcr->valid_fields & ACPI_NFIT_CONTROL_MFG_INFO_VALID)
> +               return sprintf(buf, "%02x%02x-%02x-%02x%02x-%02x%02x%02x%02x\n",
> +                       dcr->vendor_id[0], dcr->vendor_id[1],
> +                       dcr->manufacturing_location,
> +                       dcr->manufacturing_date[0], dcr->manufacturing_date[1],
> +                       dcr->serial_number[0], dcr->serial_number[1],
> +                       dcr->serial_number[2], dcr->serial_number[3]);
> +       else
> +               return sprintf(buf, "%02x%02x-%02x%02x%02x%02x\n",
> +                       dcr->vendor_id[0], dcr->vendor_id[1],
> +                       dcr->serial_number[0], dcr->serial_number[1],
> +                       dcr->serial_number[2], dcr->serial_number[3]);
> +}
> +static DEVICE_ATTR_RO(unique_id);

Let's just call it 'id' as not to confuse it with a 'uuid'.  Also if
the manufacturing info is incorporated into the id then I don't think
we need separate 'mfg_location' and 'mfg_date' attributes.  If
userspace really wants those separately it can just dump the NFIT
table.