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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
