ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
NVDIMM status. User can check the NVDIMM status from sysfs and
dmesg output, but the output can be confusing at this point.
Patch 01 clarifies the output strings from the sysfs and dmesg.
Patch 02 clarifies the define name of not-arm bit in the flags.
This patchset is based on libnvdimm-pending of the nvdimm tree.
---
Toshi Kani (2):
1/2 nfit: Clarify memory device state flags strings
2/2 acpica/nfit: Rename not-armed bit definition
---
drivers/acpi/nfit.c | 25 +++++++++++++------------
drivers/acpi/nfit.h | 2 +-
include/acpi/actbl1.h | 2 +-
tools/testing/nvdimm/test/nfit.c | 2 +-
4 files changed, 16 insertions(+), 15 deletions(-)
ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
NVDIMM status as follows. These bits indicate multiple info,
such as failures, pending event, and capability.
Bit [0] set to 1 to indicate that the previous SAVE to the
Memory Device failed.
Bit [1] set to 1 to indicate that the last RESTORE from the
Memory Device failed.
Bit [2] set to 1 to indicate that platform flush of data to
Memory Device failed. As a result, the restored data content
may be inconsistent even if SAVE and RESTORE do not indicate
failure.
Bit [3] set to 1 to indicate that the Memory Device is observed
to be not armed prior to OSPM hand off. A Memory Device is
considered armed if it is able to accept persistent writes.
Bit [4] set to 1 to indicate that the Memory Device observed
SMART and health events prior to OSPM handoff.
Bit [5] is set to 1 to indicate that platform firmware is
enabled to notify OSPM on SMART and health events related to
the memory device using Notify codes as specified in Section
5.6.6.
/sys/bus/nd/devices/nmemX/nfit/flags shows this flags info.
The output strings associated with the bits are "save", "restore",
"smart", etc., which can be confusing as they may be interpreted
as positive status, i.e. save succeeded.
Change the strings to be more descriptive per the ACPI spec.
Also add a string to bit 5 for completeness.
Change also the dev_info() message in acpi_nfit_register_dimms()
to be consistent with the sysfs flags strings.
Reported-by: Robert Elliott <[email protected]>
Signed-off-by: Toshi Kani <[email protected]>
Cc: Dan Williams <[email protected]>
---
drivers/acpi/nfit.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index c3fe206..6993ff2 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -701,12 +701,13 @@ static ssize_t flags_show(struct device *dev,
{
u16 flags = to_nfit_memdev(dev)->flags;
- return sprintf(buf, "%s%s%s%s%s\n",
- flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save " : "",
- flags & ACPI_NFIT_MEM_RESTORE_FAILED ? "restore " : "",
- flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush " : "",
- flags & ACPI_NFIT_MEM_ARMED ? "arm " : "",
- flags & ACPI_NFIT_MEM_HEALTH_OBSERVED ? "smart " : "");
+ return sprintf(buf, "%s%s%s%s%s%s\n",
+ flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save_fail " : "",
+ flags & ACPI_NFIT_MEM_RESTORE_FAILED ? "restore_fail " : "",
+ flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush_fail " : "",
+ flags & ACPI_NFIT_MEM_ARMED ? "not_arm " : "",
+ flags & ACPI_NFIT_MEM_HEALTH_OBSERVED ? "smart_event " : "",
+ flags & ACPI_NFIT_MEM_HEALTH_ENABLED ? "notify_enabled " : "");
}
static DEVICE_ATTR_RO(flags);
@@ -834,11 +835,11 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
continue;
dev_info(acpi_desc->dev, "%s: failed: %s%s%s%s\n",
- nvdimm_name(nvdimm),
- mem_flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save " : "",
- mem_flags & ACPI_NFIT_MEM_RESTORE_FAILED ? "restore " : "",
- mem_flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush " : "",
- mem_flags & ACPI_NFIT_MEM_ARMED ? "arm " : "");
+ nvdimm_name(nvdimm),
+ mem_flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save_fail " : "",
+ mem_flags & ACPI_NFIT_MEM_RESTORE_FAILED ? "restore_fail ":"",
+ mem_flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush_fail " : "",
+ mem_flags & ACPI_NFIT_MEM_ARMED ? "not_arm " : "");
}
ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
bit 3 as follows.
Bit [3] set to 1 to indicate that the Memory Device is observed
to be not armed prior to OSPM hand off. A Memory Device is
considered armed if it is able to accept persistent writes.
This bit is currently defined as ACPI_NFIT_MEM_ARMED, which can be
confusing as if the Memory Device is armed when this bit is set.
Change the name to ACPI_NFIT_MEM_NOT_ARMED per the spec.
Signed-off-by: Toshi Kani <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Bob Moore <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/nfit.c | 6 +++---
drivers/acpi/nfit.h | 2 +-
include/acpi/actbl1.h | 2 +-
tools/testing/nvdimm/test/nfit.c | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index 6993ff2..1eb3654 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -705,7 +705,7 @@ static ssize_t flags_show(struct device *dev,
flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save_fail " : "",
flags & ACPI_NFIT_MEM_RESTORE_FAILED ? "restore_fail " : "",
flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush_fail " : "",
- flags & ACPI_NFIT_MEM_ARMED ? "not_arm " : "",
+ flags & ACPI_NFIT_MEM_NOT_ARMED ? "not_arm " : "",
flags & ACPI_NFIT_MEM_HEALTH_OBSERVED ? "smart_event " : "",
flags & ACPI_NFIT_MEM_HEALTH_ENABLED ? "notify_enabled " : "");
}
@@ -815,7 +815,7 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
flags |= NDD_ALIASING;
mem_flags = __to_nfit_memdev(nfit_mem)->flags;
- if (mem_flags & ACPI_NFIT_MEM_ARMED)
+ if (mem_flags & ACPI_NFIT_MEM_NOT_ARMED)
flags |= NDD_UNARMED;
rc = acpi_nfit_add_dimm(acpi_desc, nfit_mem, device_handle);
@@ -839,7 +839,7 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
mem_flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save_fail " : "",
mem_flags & ACPI_NFIT_MEM_RESTORE_FAILED ? "restore_fail ":"",
mem_flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush_fail " : "",
- mem_flags & ACPI_NFIT_MEM_ARMED ? "not_arm " : "");
+ mem_flags & ACPI_NFIT_MEM_NOT_ARMED ? "not_arm " : "");
}
diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
index f2c2bb7..de4a00d 100644
--- a/drivers/acpi/nfit.h
+++ b/drivers/acpi/nfit.h
@@ -24,7 +24,7 @@
#define UUID_NFIT_DIMM "4309ac30-0d11-11e4-9191-0800200c9a66"
#define ACPI_NFIT_MEM_FAILED_MASK (ACPI_NFIT_MEM_SAVE_FAILED \
| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
- | ACPI_NFIT_MEM_ARMED)
+ | ACPI_NFIT_MEM_NOT_ARMED)
enum nfit_uuids {
NFIT_SPA_VOLATILE,
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index fcd5709..238754e 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -1012,7 +1012,7 @@ struct acpi_nfit_memory_map {
#define ACPI_NFIT_MEM_SAVE_FAILED (1) /* 00: Last SAVE to Memory Device failed */
#define ACPI_NFIT_MEM_RESTORE_FAILED (1<<1) /* 01: Last RESTORE from Memory Device failed */
#define ACPI_NFIT_MEM_FLUSH_FAILED (1<<2) /* 02: Platform flush failed */
-#define ACPI_NFIT_MEM_ARMED (1<<3) /* 03: Memory Device observed to be not armed */
+#define ACPI_NFIT_MEM_NOT_ARMED (1<<3) /* 03: Memory Device observed to be not armed */
#define ACPI_NFIT_MEM_HEALTH_OBSERVED (1<<4) /* 04: Memory Device observed SMART/health events */
#define ACPI_NFIT_MEM_HEALTH_ENABLED (1<<5) /* 05: SMART/health events enabled */
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 28dba91..9495249 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -998,7 +998,7 @@ static void nfit_test1_setup(struct nfit_test *t)
memdev->interleave_ways = 1;
memdev->flags = ACPI_NFIT_MEM_SAVE_FAILED | ACPI_NFIT_MEM_RESTORE_FAILED
| ACPI_NFIT_MEM_FLUSH_FAILED | ACPI_NFIT_MEM_HEALTH_OBSERVED
- | ACPI_NFIT_MEM_ARMED;
+ | ACPI_NFIT_MEM_NOT_ARMED;
offset += sizeof(*memdev);
/* dcr-descriptor0 */
On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]> wrote:
> ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
> bit 3 as follows.
>
> Bit [3] set to 1 to indicate that the Memory Device is observed
> to be not armed prior to OSPM hand off. A Memory Device is
> considered armed if it is able to accept persistent writes.
>
> This bit is currently defined as ACPI_NFIT_MEM_ARMED, which can be
> confusing as if the Memory Device is armed when this bit is set.
>
> Change the name to ACPI_NFIT_MEM_NOT_ARMED per the spec.
>
> Signed-off-by: Toshi Kani <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Bob Moore <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/nfit.c | 6 +++---
> drivers/acpi/nfit.h | 2 +-
> include/acpi/actbl1.h | 2 +-
This file "include/acpi/actbl1.h" is owned by the ACPICA project so
any changes need to come through them. But that said, I'm not sure we
need friendly names at this level.
What I usually say about sysfs name changes to be more human friendly
is "sysfs is not a UI", i.e. it's not necessarily meant to be user
friendly. As long as the names for the flags are distinct then
wrapping descriptive / accurate names around them is the role of
libndctl and userspace management software.
Similar feedback for patch1 in the sense that I don't think we need to
update the sysfs naming. For example the API to retrieve the state of
the "arm" flag in libndctl is ndctl_dimm_failed_arm().
On 8/26/2015 1:16 PM, Dan Williams wrote:
> On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]> wrote:
>> ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
>> bit 3 as follows.
>>
>> Bit [3] set to 1 to indicate that the Memory Device is observed
>> to be not armed prior to OSPM hand off. A Memory Device is
>> considered armed if it is able to accept persistent writes.
>>
>> This bit is currently defined as ACPI_NFIT_MEM_ARMED, which can be
>> confusing as if the Memory Device is armed when this bit is set.
>>
>> Change the name to ACPI_NFIT_MEM_NOT_ARMED per the spec.
>>
>> Signed-off-by: Toshi Kani <[email protected]>
>> Cc: Dan Williams <[email protected]>
>> Cc: Bob Moore <[email protected]>
>> Cc: Rafael J. Wysocki <[email protected]>
>> ---
>> drivers/acpi/nfit.c | 6 +++---
>> drivers/acpi/nfit.h | 2 +-
>> include/acpi/actbl1.h | 2 +-
>
> This file "include/acpi/actbl1.h" is owned by the ACPICA project so
> any changes need to come through them. But that said, I'm not sure we
> need friendly names at this level.
>
> What I usually say about sysfs name changes to be more human friendly
> is "sysfs is not a UI", i.e. it's not necessarily meant to be user
> friendly. As long as the names for the flags are distinct then
> wrapping descriptive / accurate names around them is the role of
> libndctl and userspace management software.
I think there's a difference between unfriendly and misleading or confusing.
If names didn't matter at all we could just call them bit0, bit1, bit2,...
> Similar feedback for patch1 in the sense that I don't think we need to
> update the sysfs naming. For example the API to retrieve the state of
> the "arm" flag in libndctl is ndctl_dimm_failed_arm().
It would be so nice for scripts and humans if the sysfs names made as
much sense.
-- ljk
> _______________________________________________
> Linux-nvdimm mailing list
> [email protected]
> https://lists.01.org/mailman/listinfo/linux-nvdimm
>
On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote:
> On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]> wrote:
> > ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
> > bit 3 as follows.
> >
> > Bit [3] set to 1 to indicate that the Memory Device is observed
> > to be not armed prior to OSPM hand off. A Memory Device is
> > considered armed if it is able to accept persistent writes.
> >
> > This bit is currently defined as ACPI_NFIT_MEM_ARMED, which can be
> > confusing as if the Memory Device is armed when this bit is set.
> >
> > Change the name to ACPI_NFIT_MEM_NOT_ARMED per the spec.
> >
> > Signed-off-by: Toshi Kani <[email protected]>
> > Cc: Dan Williams <[email protected]>
> > Cc: Bob Moore <[email protected]>
> > Cc: Rafael J. Wysocki <[email protected]>
> > ---
> > drivers/acpi/nfit.c | 6 +++---
> > drivers/acpi/nfit.h | 2 +-
> > include/acpi/actbl1.h | 2 +-
>
> This file "include/acpi/actbl1.h" is owned by the ACPICA project so
> any changes need to come through them. But that said, I'm not sure we
> need friendly names at this level.
I think the name is misleading, but I agree with the process and this patch2
can be dropped. It'd be nice if the ACPICA project can pick it up later
when they have a chance, though.
> What I usually say about sysfs name changes to be more human friendly
> is "sysfs is not a UI", i.e. it's not necessarily meant to be user
> friendly. As long as the names for the flags are distinct then
> wrapping descriptive / accurate names around them is the role of
> libndctl and userspace management software.
>
> Similar feedback for patch1 in the sense that I don't think we need to
> update the sysfs naming. For example the API to retrieve the state of
> the "arm" flag in libndctl is ndctl_dimm_failed_arm().
I agree that we do not want to change sysfs API for friendliness, and I
understand that libndctl already consumes the strings... But I think they
can be confusing for the long run, i.e. the flags is likely extended for
additional info, and more people may be looking at sysfs for the state.
It'd be a lot harder to change them later.
Thanks,
-Toshi
On Wed, Aug 26, 2015 at 2:12 PM, Toshi Kani <[email protected]> wrote:
> On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote:
>> On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]> wrote:
>> > ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
>> > bit 3 as follows.
>> >
>> > Bit [3] set to 1 to indicate that the Memory Device is observed
>> > to be not armed prior to OSPM hand off. A Memory Device is
>> > considered armed if it is able to accept persistent writes.
>> >
>> > This bit is currently defined as ACPI_NFIT_MEM_ARMED, which can be
>> > confusing as if the Memory Device is armed when this bit is set.
>> >
>> > Change the name to ACPI_NFIT_MEM_NOT_ARMED per the spec.
>> >
>> > Signed-off-by: Toshi Kani <[email protected]>
>> > Cc: Dan Williams <[email protected]>
>> > Cc: Bob Moore <[email protected]>
>> > Cc: Rafael J. Wysocki <[email protected]>
>> > ---
>> > drivers/acpi/nfit.c | 6 +++---
>> > drivers/acpi/nfit.h | 2 +-
>> > include/acpi/actbl1.h | 2 +-
>>
>> This file "include/acpi/actbl1.h" is owned by the ACPICA project so
>> any changes need to come through them. But that said, I'm not sure we
>> need friendly names at this level.
>
> I think the name is misleading, but I agree with the process and this patch2
> can be dropped. It'd be nice if the ACPICA project can pick it up later
> when they have a chance, though.
>
>> What I usually say about sysfs name changes to be more human friendly
>> is "sysfs is not a UI", i.e. it's not necessarily meant to be user
>> friendly. As long as the names for the flags are distinct then
>> wrapping descriptive / accurate names around them is the role of
>> libndctl and userspace management software.
>>
>> Similar feedback for patch1 in the sense that I don't think we need to
>> update the sysfs naming. For example the API to retrieve the state of
>> the "arm" flag in libndctl is ndctl_dimm_failed_arm().
>
> I agree that we do not want to change sysfs API for friendliness, and I
> understand that libndctl already consumes the strings... But I think they
> can be confusing for the long run, i.e. the flags is likely extended for
> additional info, and more people may be looking at sysfs for the state.
> It'd be a lot harder to change them later.
The starting premise though is that this will be nicer for scripts
that want to avoid the library. Properly handling the async device
registration semantics of the libnvdimm-sysfs interface is hard to get
right in a script. I'm trying my best to discourage raw use of sysfs
for this reason. Small fixes to the names of flags seems to miss this
wider point.
On Wed, 2015-08-26 at 14:30 -0700, Dan Williams wrote:
> On Wed, Aug 26, 2015 at 2:12 PM, Toshi Kani <[email protected]> wrote:
> > On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote:
> > > On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]> wrote:
> > > > ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
> > > > bit 3 as follows.
> > > >
> > > > Bit [3] set to 1 to indicate that the Memory Device is observed
> > > > to be not armed prior to OSPM hand off. A Memory Device is
> > > > considered armed if it is able to accept persistent writes.
> > > >
> > > > This bit is currently defined as ACPI_NFIT_MEM_ARMED, which can be
> > > > confusing as if the Memory Device is armed when this bit is set.
> > > >
> > > > Change the name to ACPI_NFIT_MEM_NOT_ARMED per the spec.
> > > >
> > > > Signed-off-by: Toshi Kani <[email protected]>
> > > > Cc: Dan Williams <[email protected]>
> > > > Cc: Bob Moore <[email protected]>
> > > > Cc: Rafael J. Wysocki <[email protected]>
> > > > ---
> > > > drivers/acpi/nfit.c | 6 +++---
> > > > drivers/acpi/nfit.h | 2 +-
> > > > include/acpi/actbl1.h | 2 +-
> > >
> > > This file "include/acpi/actbl1.h" is owned by the ACPICA project so
> > > any changes need to come through them. But that said, I'm not sure we
> > > need friendly names at this level.
> >
> > I think the name is misleading, but I agree with the process and this
> > patch2
> > can be dropped. It'd be nice if the ACPICA project can pick it up later
> > when they have a chance, though.
> >
> > > What I usually say about sysfs name changes to be more human friendly
> > > is "sysfs is not a UI", i.e. it's not necessarily meant to be user
> > > friendly. As long as the names for the flags are distinct then
> > > wrapping descriptive / accurate names around them is the role of
> > > libndctl and userspace management software.
> > >
> > > Similar feedback for patch1 in the sense that I don't think we need to
> > > update the sysfs naming. For example the API to retrieve the state of
> > > the "arm" flag in libndctl is ndctl_dimm_failed_arm().
> >
> > I agree that we do not want to change sysfs API for friendliness, and I
> > understand that libndctl already consumes the strings... But I think
> > they
> > can be confusing for the long run, i.e. the flags is likely extended for
> > additional info, and more people may be looking at sysfs for the state.
> > It'd be a lot harder to change them later.
>
> The starting premise though is that this will be nicer for scripts
> that want to avoid the library. Properly handling the async device
> registration semantics of the libnvdimm-sysfs interface is hard to get
> right in a script. I'm trying my best to discourage raw use of sysfs
> for this reason. Small fixes to the names of flags seems to miss this
> wider point.
Okay, I guess I will have to jump on the bandwagon and discourage people to
look at sysfs... ;-P
Thanks,
-Toshi
On Wed, Aug 26, 2015 at 2:44 PM, Toshi Kani <[email protected]> wrote:
> On Wed, 2015-08-26 at 14:30 -0700, Dan Williams wrote:
>> On Wed, Aug 26, 2015 at 2:12 PM, Toshi Kani <[email protected]> wrote:
>> > On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote:
>> > > On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]> wrote:
>> > > > ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
>> > > > bit 3 as follows.
>> > > >
>> > > > Bit [3] set to 1 to indicate that the Memory Device is observed
>> > > > to be not armed prior to OSPM hand off. A Memory Device is
>> > > > considered armed if it is able to accept persistent writes.
>> > > >
>> > > > This bit is currently defined as ACPI_NFIT_MEM_ARMED, which can be
>> > > > confusing as if the Memory Device is armed when this bit is set.
>> > > >
>> > > > Change the name to ACPI_NFIT_MEM_NOT_ARMED per the spec.
>> > > >
>> > > > Signed-off-by: Toshi Kani <[email protected]>
>> > > > Cc: Dan Williams <[email protected]>
>> > > > Cc: Bob Moore <[email protected]>
>> > > > Cc: Rafael J. Wysocki <[email protected]>
>> > > > ---
>> > > > drivers/acpi/nfit.c | 6 +++---
>> > > > drivers/acpi/nfit.h | 2 +-
>> > > > include/acpi/actbl1.h | 2 +-
>> > >
>> > > This file "include/acpi/actbl1.h" is owned by the ACPICA project so
>> > > any changes need to come through them. But that said, I'm not sure we
>> > > need friendly names at this level.
>> >
>> > I think the name is misleading, but I agree with the process and this
>> > patch2
>> > can be dropped. It'd be nice if the ACPICA project can pick it up later
>> > when they have a chance, though.
>> >
>> > > What I usually say about sysfs name changes to be more human friendly
>> > > is "sysfs is not a UI", i.e. it's not necessarily meant to be user
>> > > friendly. As long as the names for the flags are distinct then
>> > > wrapping descriptive / accurate names around them is the role of
>> > > libndctl and userspace management software.
>> > >
>> > > Similar feedback for patch1 in the sense that I don't think we need to
>> > > update the sysfs naming. For example the API to retrieve the state of
>> > > the "arm" flag in libndctl is ndctl_dimm_failed_arm().
>> >
>> > I agree that we do not want to change sysfs API for friendliness, and I
>> > understand that libndctl already consumes the strings... But I think
>> > they
>> > can be confusing for the long run, i.e. the flags is likely extended for
>> > additional info, and more people may be looking at sysfs for the state.
>> > It'd be a lot harder to change them later.
>>
>> The starting premise though is that this will be nicer for scripts
>> that want to avoid the library. Properly handling the async device
>> registration semantics of the libnvdimm-sysfs interface is hard to get
>> right in a script. I'm trying my best to discourage raw use of sysfs
>> for this reason. Small fixes to the names of flags seems to miss this
>> wider point.
>
> Okay, I guess I will have to jump on the bandwagon and discourage people to
> look at sysfs... ;-P
>
That said, I'm not opposed to looking at something like Python-binding
for libndctl to make scripting easier.
On Wed, Aug 26, 2015 at 11:12 PM, Toshi Kani <[email protected]> wrote:
> On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote:
>> On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]> wrote:
>> > ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
>> > bit 3 as follows.
>> >
>> > Bit [3] set to 1 to indicate that the Memory Device is observed
>> > to be not armed prior to OSPM hand off. A Memory Device is
>> > considered armed if it is able to accept persistent writes.
>> >
>> > This bit is currently defined as ACPI_NFIT_MEM_ARMED, which can be
>> > confusing as if the Memory Device is armed when this bit is set.
>> >
>> > Change the name to ACPI_NFIT_MEM_NOT_ARMED per the spec.
>> >
>> > Signed-off-by: Toshi Kani <[email protected]>
>> > Cc: Dan Williams <[email protected]>
>> > Cc: Bob Moore <[email protected]>
>> > Cc: Rafael J. Wysocki <[email protected]>
>> > ---
>> > drivers/acpi/nfit.c | 6 +++---
>> > drivers/acpi/nfit.h | 2 +-
>> > include/acpi/actbl1.h | 2 +-
>>
>> This file "include/acpi/actbl1.h" is owned by the ACPICA project so
>> any changes need to come through them. But that said, I'm not sure we
>> need friendly names at this level.
>
> I think the name is misleading, but I agree with the process and this patch2
> can be dropped. It'd be nice if the ACPICA project can pick it up later
> when they have a chance, though.
A good way to cause that to happen would be to send a patch to the
ACPICA development list + maintainers as per MAINTAINERS.
Thanks,
Rafael
On Thu, 2015-08-27 at 01:16 +0200, Rafael J. Wysocki wrote:
> On Wed, Aug 26, 2015 at 11:12 PM, Toshi Kani <[email protected]> wrote:
> > On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote:
> > > On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]> wrote:
> > > > ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
> > > > bit 3 as follows.
> > > >
> > > > Bit [3] set to 1 to indicate that the Memory Device is observed
> > > > to be not armed prior to OSPM hand off. A Memory Device is
> > > > considered armed if it is able to accept persistent writes.
> > > >
> > > > This bit is currently defined as ACPI_NFIT_MEM_ARMED, which can be
> > > > confusing as if the Memory Device is armed when this bit is set.
> > > >
> > > > Change the name to ACPI_NFIT_MEM_NOT_ARMED per the spec.
> > > >
> > > > Signed-off-by: Toshi Kani <[email protected]>
> > > > Cc: Dan Williams <[email protected]>
> > > > Cc: Bob Moore <[email protected]>
> > > > Cc: Rafael J. Wysocki <[email protected]>
> > > > ---
> > > > drivers/acpi/nfit.c | 6 +++---
> > > > drivers/acpi/nfit.h | 2 +-
> > > > include/acpi/actbl1.h | 2 +-
> > >
> > > This file "include/acpi/actbl1.h" is owned by the ACPICA project so
> > > any changes need to come through them. But that said, I'm not sure we
> > > need friendly names at this level.
> >
> > I think the name is misleading, but I agree with the process and this
> > patch2 can be dropped. It'd be nice if the ACPICA project can pick it
> > up later when they have a chance, though.
>
> A good way to cause that to happen would be to send a patch to the
> ACPICA development list + maintainers as per MAINTAINERS.
Oh, I see. I did run get_maintainer.pl for this patch, but [email protected]
did not come out in output... So, I did not realize this email list.
Thanks for the suggestion!
-Toshi
On Wed, 2015-08-26 at 17:29 -0600, Toshi Kani wrote:
> On Thu, 2015-08-27 at 01:16 +0200, Rafael J. Wysocki wrote:
> > On Wed, Aug 26, 2015 at 11:12 PM, Toshi Kani <[email protected]> wrote:
> > > On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote:
> > > > On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]>
> > > > wrote:
> > > > > ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
> > > > > bit 3 as follows.
> > > > >
> > > > > Bit [3] set to 1 to indicate that the Memory Device is observed
> > > > > to be not armed prior to OSPM hand off. A Memory Device is
> > > > > considered armed if it is able to accept persistent writes.
> > > > >
> > > > > This bit is currently defined as ACPI_NFIT_MEM_ARMED, which can be
> > > > > confusing as if the Memory Device is armed when this bit is set.
> > > > >
> > > > > Change the name to ACPI_NFIT_MEM_NOT_ARMED per the spec.
> > > > >
> > > > > Signed-off-by: Toshi Kani <[email protected]>
> > > > > Cc: Dan Williams <[email protected]>
> > > > > Cc: Bob Moore <[email protected]>
> > > > > Cc: Rafael J. Wysocki <[email protected]>
> > > > > ---
> > > > > drivers/acpi/nfit.c | 6 +++---
> > > > > drivers/acpi/nfit.h | 2 +-
> > > > > include/acpi/actbl1.h | 2 +-
> > > >
> > > > This file "include/acpi/actbl1.h" is owned by the ACPICA project so
> > > > any changes need to come through them. But that said, I'm not sure
> > > > we
> > > > need friendly names at this level.
> > >
> > > I think the name is misleading, but I agree with the process and this
> > > patch2 can be dropped. It'd be nice if the ACPICA project can pick it
> > > up later when they have a chance, though.
> >
> > A good way to cause that to happen would be to send a patch to the
> > ACPICA development list + maintainers as per MAINTAINERS.
>
> Oh, I see. I did run get_maintainer.pl for this patch, but
> [email protected] did not come out in output... So, I did not realize this
> email list.
Sorry, it was listed in the output. I was simply blinded... :-(
$ scripts/get_maintainer.pl patches-nd-flags/02*
:
[email protected] (open list:LIBNVDIMM BLK: MMIO-APERTURE DRIVER)
[email protected] (open list:ACPI)
[email protected] (open list)
[email protected] (open list:ACPI COMPONENT ARCHITECTURE (ACPICA))
Thanks!
-Toshi
I don’t have any problem changing this in ACPICA if/when you all agree.
> -----Original Message-----
> From: Toshi Kani [mailto:[email protected]]
> Sent: Wednesday, August 26, 2015 4:36 PM
> To: Rafael J. Wysocki
> Cc: Williams, Dan J; Wysocki, Rafael J; Moore, Robert; linux-
> [email protected]; Linux ACPI; [email protected]; Elliott,
> Robert (Server Storage)
> Subject: Re: [PATCH 2/2]: acpica/nfit: Rename not-armed bit definition
>
> On Wed, 2015-08-26 at 17:29 -0600, Toshi Kani wrote:
> > On Thu, 2015-08-27 at 01:16 +0200, Rafael J. Wysocki wrote:
> > > On Wed, Aug 26, 2015 at 11:12 PM, Toshi Kani <[email protected]>
> wrote:
> > > > On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote:
> > > > > On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]>
> > > > > wrote:
> > > > > > ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
> > > > > > bit 3 as follows.
> > > > > >
> > > > > > Bit [3] set to 1 to indicate that the Memory Device is
> observed
> > > > > > to be not armed prior to OSPM hand off. A Memory Device is
> > > > > > considered armed if it is able to accept persistent writes.
> > > > > >
> > > > > > This bit is currently defined as ACPI_NFIT_MEM_ARMED, which
> > > > > > can be confusing as if the Memory Device is armed when this bit
> is set.
> > > > > >
> > > > > > Change the name to ACPI_NFIT_MEM_NOT_ARMED per the spec.
> > > > > >
> > > > > > Signed-off-by: Toshi Kani <[email protected]>
> > > > > > Cc: Dan Williams <[email protected]>
> > > > > > Cc: Bob Moore <[email protected]>
> > > > > > Cc: Rafael J. Wysocki <[email protected]>
> > > > > > ---
> > > > > > drivers/acpi/nfit.c | 6 +++---
> > > > > > drivers/acpi/nfit.h | 2 +-
> > > > > > include/acpi/actbl1.h | 2 +-
> > > > >
> > > > > This file "include/acpi/actbl1.h" is owned by the ACPICA project
> > > > > so any changes need to come through them. But that said, I'm
> > > > > not sure we need friendly names at this level.
> > > >
> > > > I think the name is misleading, but I agree with the process and
> > > > this
> > > > patch2 can be dropped. It'd be nice if the ACPICA project can
> > > > pick it up later when they have a chance, though.
> > >
> > > A good way to cause that to happen would be to send a patch to the
> > > ACPICA development list + maintainers as per MAINTAINERS.
> >
> > Oh, I see. I did run get_maintainer.pl for this patch, but
> > [email protected] did not come out in output... So, I did not realize
> > this email list.
>
> Sorry, it was listed in the output. I was simply blinded... :-(
>
> $ scripts/get_maintainer.pl patches-nd-flags/02*
> :
> [email protected] (open list:LIBNVDIMM BLK: MMIO-APERTURE DRIVER)
> [email protected] (open list:ACPI) [email protected]
> (open list) [email protected] (open list:ACPI COMPONENT ARCHITECTURE
> (ACPICA))
>
> Thanks!
> -Toshi
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, Aug 26, 2015 at 10:20:23AM -0600, Toshi Kani wrote:
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index c3fe206..6993ff2 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -701,12 +701,13 @@ static ssize_t flags_show(struct device *dev,
> {
> u16 flags = to_nfit_memdev(dev)->flags;
>
> - return sprintf(buf, "%s%s%s%s%s\n",
> - flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save " : "",
> - flags & ACPI_NFIT_MEM_RESTORE_FAILED ? "restore " : "",
> - flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush " : "",
> - flags & ACPI_NFIT_MEM_ARMED ? "arm " : "",
> - flags & ACPI_NFIT_MEM_HEALTH_OBSERVED ? "smart " : "");
> + return sprintf(buf, "%s%s%s%s%s%s\n",
> + flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save_fail " : "",
> + flags & ACPI_NFIT_MEM_RESTORE_FAILED ? "restore_fail " : "",
> + flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush_fail " : "",
> + flags & ACPI_NFIT_MEM_ARMED ? "not_arm " : "",
Assuming we do want to update these strings to be more friendly, "not_armed"
probably makes more sense than "not_arm". Also applies to the 2nd hunk below.
> + flags & ACPI_NFIT_MEM_HEALTH_OBSERVED ? "smart_event " : "",
> + flags & ACPI_NFIT_MEM_HEALTH_ENABLED ? "notify_enabled " : "");
> }
> static DEVICE_ATTR_RO(flags);
>
> @@ -834,11 +835,11 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
> continue;
>
> dev_info(acpi_desc->dev, "%s: failed: %s%s%s%s\n",
> - nvdimm_name(nvdimm),
> - mem_flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save " : "",
> - mem_flags & ACPI_NFIT_MEM_RESTORE_FAILED ? "restore " : "",
> - mem_flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush " : "",
> - mem_flags & ACPI_NFIT_MEM_ARMED ? "arm " : "");
> + nvdimm_name(nvdimm),
> + mem_flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save_fail " : "",
> + mem_flags & ACPI_NFIT_MEM_RESTORE_FAILED ? "restore_fail ":"",
> + mem_flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush_fail " : "",
> + mem_flags & ACPI_NFIT_MEM_ARMED ? "not_arm " : "");
While you're in here, is there a reason not to include the last two flags
(smart_event and notify_enabled) in this dev_info() output?
On Wed, 2015-08-26 at 21:07 -0600, Ross Zwisler wrote:
> On Wed, Aug 26, 2015 at 10:20:23AM -0600, Toshi Kani wrote:
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index c3fe206..6993ff2 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -701,12 +701,13 @@ static ssize_t flags_show(struct device *dev,
> > {
> > u16 flags = to_nfit_memdev(dev)->flags;
> >
> > - return sprintf(buf, "%s%s%s%s%s\n",
> > - flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save " :
> > "",
> > - flags & ACPI_NFIT_MEM_RESTORE_FAILED ? "restore
> > " : "",
> > - flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush " :
> > "",
> > - flags & ACPI_NFIT_MEM_ARMED ? "arm " : "",
> > - flags & ACPI_NFIT_MEM_HEALTH_OBSERVED ? "smart
> > " : "");
> > + return sprintf(buf, "%s%s%s%s%s%s\n",
> > + flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save_fail " : "",
> > + flags & ACPI_NFIT_MEM_RESTORE_FAILED ? "restore_fail "
> > : "",
> > + flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush_fail " :
> > "",
> > + flags & ACPI_NFIT_MEM_ARMED ? "not_arm " : "",
>
> Assuming we do want to update these strings to be more friendly,
> "not_armed" probably makes more sense than "not_arm". Also applies to the
> 2nd hunk below.
Agreed. (Will update if this patch gets ever resurrected. :-)
> > + flags & ACPI_NFIT_MEM_HEALTH_OBSERVED ? "smart_event " > > : "",
> > + flags & ACPI_NFIT_MEM_HEALTH_ENABLED ? "notify_enabled
> > " : "");
> > }
> > static DEVICE_ATTR_RO(flags);
> >
> > @@ -834,11 +835,11 @@ static int acpi_nfit_register_dimms(struct
> > acpi_nfit_desc *acpi_desc)
> > continue;
> >
> > dev_info(acpi_desc->dev, "%s: failed: %s%s%s%s\n",
> > - nvdimm_name(nvdimm),
> > - mem_flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save "
> > : "",
> > - mem_flags & ACPI_NFIT_MEM_RESTORE_FAILED ?
> > "restore " : "",
> > - mem_flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush
> > " : "",
> > - mem_flags & ACPI_NFIT_MEM_ARMED ? "arm " : "");
> > + nvdimm_name(nvdimm),
> > + mem_flags & ACPI_NFIT_MEM_SAVE_FAILED ? "save_fail "
> > : "",
> > + mem_flags & ACPI_NFIT_MEM_RESTORE_FAILED ?
> > "restore_fail ":"",
> > + mem_flags & ACPI_NFIT_MEM_FLUSH_FAILED ? "flush_fail
> > " : "",
> > + mem_flags & ACPI_NFIT_MEM_ARMED ? "not_arm " : "");
>
> While you're in here, is there a reason not to include the last two flags
> (smart_event and notify_enabled) in this dev_info() output?
This dev_info() logs any failure in NVDIMM, and the last two flags are not
failure conditions.
Thanks,
-Toshi
On Thu, 2015-08-27 at 01:56 +0000, Moore, Robert wrote:
> I don’t have any problem changing this in ACPICA if/when you all agree.
Great. I will resend this patch alone, assuming the patch can be based on
the Dan's NVDIMM tree (since it changes NFIT files).
Thanks,
-Toshi
On 8/26/2015 6:00 PM, Dan Williams wrote:
> On Wed, Aug 26, 2015 at 2:44 PM, Toshi Kani <[email protected]> wrote:
>> On Wed, 2015-08-26 at 14:30 -0700, Dan Williams wrote:
>>> On Wed, Aug 26, 2015 at 2:12 PM, Toshi Kani <[email protected]> wrote:
>>>> On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote:
>>>>> On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]> wrote:
>>>>>> ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
>>>>>> bit 3 as follows.
>>>>>>
>>>>>> Bit [3] set to 1 to indicate that the Memory Device is observed
>>>>>> to be not armed prior to OSPM hand off. A Memory Device is
>>>>>> considered armed if it is able to accept persistent writes.
>>>>>>
>>>>>> This bit is currently defined as ACPI_NFIT_MEM_ARMED, which can be
>>>>>> confusing as if the Memory Device is armed when this bit is set.
>>>>>>
>>>>>> Change the name to ACPI_NFIT_MEM_NOT_ARMED per the spec.
>>>>>>
>>>>>> Signed-off-by: Toshi Kani <[email protected]>
>>>>>> Cc: Dan Williams <[email protected]>
>>>>>> Cc: Bob Moore <[email protected]>
>>>>>> Cc: Rafael J. Wysocki <[email protected]>
>>>>>> ---
>>>>>> drivers/acpi/nfit.c | 6 +++---
>>>>>> drivers/acpi/nfit.h | 2 +-
>>>>>> include/acpi/actbl1.h | 2 +-
>>>>>
>>>>> This file "include/acpi/actbl1.h" is owned by the ACPICA project so
>>>>> any changes need to come through them. But that said, I'm not sure we
>>>>> need friendly names at this level.
>>>>
>>>> I think the name is misleading, but I agree with the process and this
>>>> patch2
>>>> can be dropped. It'd be nice if the ACPICA project can pick it up later
>>>> when they have a chance, though.
>>>>
>>>>> What I usually say about sysfs name changes to be more human friendly
>>>>> is "sysfs is not a UI", i.e. it's not necessarily meant to be user
>>>>> friendly. As long as the names for the flags are distinct then
>>>>> wrapping descriptive / accurate names around them is the role of
>>>>> libndctl and userspace management software.
>>>>>
>>>>> Similar feedback for patch1 in the sense that I don't think we need to
>>>>> update the sysfs naming. For example the API to retrieve the state of
>>>>> the "arm" flag in libndctl is ndctl_dimm_failed_arm().
>>>>
>>>> I agree that we do not want to change sysfs API for friendliness, and I
>>>> understand that libndctl already consumes the strings... But I think
>>>> they
>>>> can be confusing for the long run, i.e. the flags is likely extended for
>>>> additional info, and more people may be looking at sysfs for the state.
>>>> It'd be a lot harder to change them later.
>>>
>>> The starting premise though is that this will be nicer for scripts
>>> that want to avoid the library. Properly handling the async device
>>> registration semantics of the libnvdimm-sysfs interface is hard to get
>>> right in a script. I'm trying my best to discourage raw use of sysfs
>>> for this reason. Small fixes to the names of flags seems to miss this
>>> wider point.
>>
>> Okay, I guess I will have to jump on the bandwagon and discourage people to
>> look at sysfs... ;-P
>>
>
> That said, I'm not opposed to looking at something like Python-binding
> for libndctl to make scripting easier.
I don't see why we can't fix the names so they make sense now before there
is hardware in the market. People doing testing and debugging look at stuff
in /sys and they write their own scripts too, not necessarily in python.
If they only make sense to someone using your library, I think we've missed the
mark. Toshi is reacting to feedback we're getting from people are starting
to test this stuff.
-- ljk
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Thu, Aug 27, 2015 at 7:43 AM, Linda Knippers <[email protected]> wrote:
> I don't see why we can't fix the names so they make sense now before there
> is hardware in the market. People doing testing and debugging look at stuff
> in /sys and they write their own scripts too, not necessarily in python.
The practical concern at this point is that it is too late in the 4.2
development cycle, in my opinion, to push for a cosmetic change like
this. We also have versions of libndctl starting to leak into
distributions. Changing this in 4.3 means breaking the ABI from 4.2
and managing both ways in future versions of the library as well as
getting all distributions to update. Not insurmountable, but also
something I don't want to take on just for a few characters in a sysfs
file. I think this is better fixed with documentation which I still
owe to the the Documentation/ABI/ directory.
On 8/27/2015 11:30 AM, Dan Williams wrote:
> On Thu, Aug 27, 2015 at 7:43 AM, Linda Knippers <[email protected]> wrote:
>> I don't see why we can't fix the names so they make sense now before there
>> is hardware in the market. People doing testing and debugging look at stuff
>> in /sys and they write their own scripts too, not necessarily in python.
>
> The practical concern at this point is that it is too late in the 4.2
> development cycle, in my opinion, to push for a cosmetic change like
> this. We also have versions of libndctl starting to leak into
> distributions. Changing this in 4.3 means breaking the ABI from 4.2
> and managing both ways in future versions of the library as well as
> getting all distributions to update. Not insurmountable, but also
> something I don't want to take on just for a few characters in a sysfs
> file. I think this is better fixed with documentation which I still
> owe to the the Documentation/ABI/ directory.
Is there any distribution that is going to enable this with 4.2? I know
we're using it for testing now but there is still quite a bit of work
queued up for 4.3 and more still left to be done.
-- ljk
On Thu, Aug 27, 2015 at 8:35 AM, Linda Knippers <[email protected]> wrote:
> On 8/27/2015 11:30 AM, Dan Williams wrote:
>> On Thu, Aug 27, 2015 at 7:43 AM, Linda Knippers <[email protected]> wrote:
>>> I don't see why we can't fix the names so they make sense now before there
>>> is hardware in the market. People doing testing and debugging look at stuff
>>> in /sys and they write their own scripts too, not necessarily in python.
>>
>> The practical concern at this point is that it is too late in the 4.2
>> development cycle, in my opinion, to push for a cosmetic change like
>> this. We also have versions of libndctl starting to leak into
>> distributions. Changing this in 4.3 means breaking the ABI from 4.2
>> and managing both ways in future versions of the library as well as
>> getting all distributions to update. Not insurmountable, but also
>> something I don't want to take on just for a few characters in a sysfs
>> file. I think this is better fixed with documentation which I still
>> owe to the the Documentation/ABI/ directory.
>
> Is there any distribution that is going to enable this with 4.2? I know
> we're using it for testing now but there is still quite a bit of work
> queued up for 4.3 and more still left to be done.
I'm not happy that this is confusing folks, and it is unfortunate that
the polarity is reversed.
However, I'm more concerned about the fact that I'd be knowingly
breaking ABI from 4.2 to 4.3. I originally thought "no one" was using
e820 type-12 for enumerating nvdimm resources, but I got shouted down
when trying to convert that exclusively to the ACPI 6 definition. The
Linus edict of "we don't break released ABI" is ringing in my ears.
The current name is also consistent with the current / released ACPICA
definition.
On 8/27/2015 11:54 AM, Dan Williams wrote:
> On Thu, Aug 27, 2015 at 8:35 AM, Linda Knippers <[email protected]> wrote:
>> On 8/27/2015 11:30 AM, Dan Williams wrote:
>>> On Thu, Aug 27, 2015 at 7:43 AM, Linda Knippers <[email protected]> wrote:
>>>> I don't see why we can't fix the names so they make sense now before there
>>>> is hardware in the market. People doing testing and debugging look at stuff
>>>> in /sys and they write their own scripts too, not necessarily in python.
>>>
>>> The practical concern at this point is that it is too late in the 4.2
>>> development cycle, in my opinion, to push for a cosmetic change like
>>> this. We also have versions of libndctl starting to leak into
>>> distributions. Changing this in 4.3 means breaking the ABI from 4.2
>>> and managing both ways in future versions of the library as well as
>>> getting all distributions to update. Not insurmountable, but also
>>> something I don't want to take on just for a few characters in a sysfs
>>> file. I think this is better fixed with documentation which I still
>>> owe to the the Documentation/ABI/ directory.
>>
>> Is there any distribution that is going to enable this with 4.2? I know
>> we're using it for testing now but there is still quite a bit of work
>> queued up for 4.3 and more still left to be done.
>
> I'm not happy that this is confusing folks, and it is unfortunate that
> the polarity is reversed.
>
> However, I'm more concerned about the fact that I'd be knowingly
> breaking ABI from 4.2 to 4.3. I originally thought "no one" was using
> e820 type-12 for enumerating nvdimm resources, but I got shouted down
> when trying to convert that exclusively to the ACPI 6 definition. The
> Linus edict of "we don't break released ABI" is ringing in my ears.
Maybe not too late for 4.2 then. They're just string changes.
> The current name is also consistent with the current / released ACPICA
> definition.
They seem to be open to fixing it.
I know this seems like a nit but we've going to live with this stuff for
a long time.
-- ljk
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Thu, Aug 27, 2015 at 9:32 AM, Linda Knippers <[email protected]> wrote:
> I know this seems like a nit but we've going to live with this stuff for
> a long time.
Along those lines, Bob just came by my office and gave me a decisive
argument that I think trumps my ABI concerns, and one I'd be willing
to argue for 4.2 inclusion. Some BIOS implementers may do the work to
get the polarity correct others may just read the flag name and get it
wrong. Once that happens the field loses all meaning regardless of
whether Linux interprets it correctly.
On 8/27/2015 1:04 PM, Dan Williams wrote:
> On Thu, Aug 27, 2015 at 9:32 AM, Linda Knippers <[email protected]> wrote:
>> I know this seems like a nit but we've going to live with this stuff for
>> a long time.
>
> Along those lines, Bob just came by my office and gave me a decisive
> argument that I think trumps my ABI concerns, and one I'd be willing
> to argue for 4.2 inclusion. Some BIOS implementers may do the work to
> get the polarity correct others may just read the flag name and get it
> wrong. Once that happens the field loses all meaning regardless of
> whether Linux interprets it correctly.
BIOS developers, BIOS testers, and the OS folks working with those teams
will be grateful.
Thanks,
-- ljk
On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]> wrote:
> ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
> NVDIMM status as follows. These bits indicate multiple info,
> such as failures, pending event, and capability.
>
> Bit [0] set to 1 to indicate that the previous SAVE to the
> Memory Device failed.
> Bit [1] set to 1 to indicate that the last RESTORE from the
> Memory Device failed.
> Bit [2] set to 1 to indicate that platform flush of data to
> Memory Device failed. As a result, the restored data content
> may be inconsistent even if SAVE and RESTORE do not indicate
> failure.
> Bit [3] set to 1 to indicate that the Memory Device is observed
> to be not armed prior to OSPM hand off. A Memory Device is
> considered armed if it is able to accept persistent writes.
> Bit [4] set to 1 to indicate that the Memory Device observed
> SMART and health events prior to OSPM handoff.
> Bit [5] is set to 1 to indicate that platform firmware is
> enabled to notify OSPM on SMART and health events related to
> the memory device using Notify codes as specified in Section
> 5.6.6.
>
> /sys/bus/nd/devices/nmemX/nfit/flags shows this flags info.
> The output strings associated with the bits are "save", "restore",
> "smart", etc., which can be confusing as they may be interpreted
> as positive status, i.e. save succeeded.
>
> Change the strings to be more descriptive per the ACPI spec.
> Also add a string to bit 5 for completeness.
Ok I'm going to push this upstream for 4.2 with the "not_armed" fixup
that Ross suggested, but I'll defer adding bit5 since that is separate
from the urgent fix to get the polarities properly reflected and we
can add support for it later.
On Thu, 2015-08-27 at 11:57 -0700, Dan Williams wrote:
> On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <[email protected]> wrote:
> > ACPI 6.0 NFIT Memory Device State Flags in Table 5-129 defines
> > NVDIMM status as follows. These bits indicate multiple info,
> > such as failures, pending event, and capability.
> >
> > Bit [0] set to 1 to indicate that the previous SAVE to the
> > Memory Device failed.
> > Bit [1] set to 1 to indicate that the last RESTORE from the
> > Memory Device failed.
> > Bit [2] set to 1 to indicate that platform flush of data to
> > Memory Device failed. As a result, the restored data content
> > may be inconsistent even if SAVE and RESTORE do not indicate
> > failure.
> > Bit [3] set to 1 to indicate that the Memory Device is observed
> > to be not armed prior to OSPM hand off. A Memory Device is
> > considered armed if it is able to accept persistent writes.
> > Bit [4] set to 1 to indicate that the Memory Device observed
> > SMART and health events prior to OSPM handoff.
> > Bit [5] is set to 1 to indicate that platform firmware is
> > enabled to notify OSPM on SMART and health events related to
> > the memory device using Notify codes as specified in Section
> > 5.6.6.
> >
> > /sys/bus/nd/devices/nmemX/nfit/flags shows this flags info.
> > The output strings associated with the bits are "save", "restore",
> > "smart", etc., which can be confusing as they may be interpreted
> > as positive status, i.e. save succeeded.
> >
> > Change the strings to be more descriptive per the ACPI spec.
> > Also add a string to bit 5 for completeness.
>
> Ok I'm going to push this upstream for 4.2 with the "not_armed" fixup
> that Ross suggested, but I'll defer adding bit5 since that is separate
> from the urgent fix to get the polarities properly reflected and we
> can add support for it later.
Great!! Thanks Dan! (and sorry for making more work to you...)
-Toshi