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

[Moore, Robert]

More importantly, the changes to actbl1.h need to be backported to the native ACPICA format.


> -----Original Message-----
> From: Dan Williams [mailto:dan.j.williams@xxxxxxxxx]
> Sent: Friday, February 19, 2016 2:35 PM
> To: Toshi Kani
> Cc: Rafael J. Wysocki; Moore, Robert; Elliott, Robert (Persistent Memory);
> linux-nvdimm@xxxxxxxxxxxx; Linux ACPI; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxx; Zheng, Lv
> Subject: Re: [PATCH 1/2] ACPI/NFIT: Update Control Region Structure per
> ACPI 6.1
> 
> 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.