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

[Toshi Kani]

On Fri, 2016-02-19 at 14:34 -0800, Dan Williams wrote:
> 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.

Right. Thanks for adding Lv.

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

Will do.

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

Make sense. I will removeÂ'mfg_location' and 'mfg_date', and rename
'unique_id' to 'id'.

Thanks!
-Toshi