| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1455925484.15454.2.camel@hpe.com>
Date: Fri, 19 Feb 2016 16:44:44 -0700
From: Toshi Kani <toshi.kani@....com>
To: Dan Williams <dan.j.williams@...el.com>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>,
Robert Moore <robert.moore@...el.com>,
"Elliott, Robert (Persistent Memory)" <elliott@....com>,
"linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>,
Linux ACPI <linux-acpi@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
devel@...ica.org, Lv Zheng <lv.zheng@...el.com>
Subject: Re: [PATCH 1/2] ACPI/NFIT: Update Control Region Structure per ACPI
6.1
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@....com> 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@....com>
> > Cc: Rafael J. Wysocki <rjw@...ysocki.net>
> > Cc: Dan Williams <dan.j.williams@...el.com>
> > Cc: Robert Moore <robert.moore@...el.com>
> > Cc: Robert Elliott <elliott@....com>
> > Cc: <devel@...ica.org>
> > ---
> > 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
Powered by blists - more mailing lists