| Message ID | 1440606024-29873-3-git-send-email-toshi.kani@hp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <toshi.kani@hp.com> 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 <toshi.kani@hp.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Bob Moore <robert.moore@intel.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > 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 <toshi.kani@hp.com> 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 <toshi.kani@hp.com> >> Cc: Dan Williams <dan.j.williams@intel.com> >> Cc: Bob Moore <robert.moore@intel.com> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> --- >> 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 > Linux-nvdimm@lists.01.org > 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 <toshi.kani@hp.com> 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 <toshi.kani@hp.com> > > Cc: Dan Williams <dan.j.williams@intel.com> > > Cc: Bob Moore <robert.moore@intel.com> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > 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 <toshi.kani@hp.com> wrote: > On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote: >> On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <toshi.kani@hp.com> 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 <toshi.kani@hp.com> >> > Cc: Dan Williams <dan.j.williams@intel.com> >> > Cc: Bob Moore <robert.moore@intel.com> >> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> > --- >> > 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 <toshi.kani@hp.com> wrote: > > On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote: > > > On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <toshi.kani@hp.com> 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 <toshi.kani@hp.com> > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > Cc: Bob Moore <robert.moore@intel.com> > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > --- > > > > 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 <toshi.kani@hp.com> wrote: > On Wed, 2015-08-26 at 14:30 -0700, Dan Williams wrote: >> On Wed, Aug 26, 2015 at 2:12 PM, Toshi Kani <toshi.kani@hp.com> wrote: >> > On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote: >> > > On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <toshi.kani@hp.com> 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 <toshi.kani@hp.com> >> > > > Cc: Dan Williams <dan.j.williams@intel.com> >> > > > Cc: Bob Moore <robert.moore@intel.com> >> > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> > > > --- >> > > > 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 <toshi.kani@hp.com> wrote: > On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote: >> On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <toshi.kani@hp.com> 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 <toshi.kani@hp.com> >> > Cc: Dan Williams <dan.j.williams@intel.com> >> > Cc: Bob Moore <robert.moore@intel.com> >> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> > --- >> > 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 <toshi.kani@hp.com> wrote: > > On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote: > > > On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <toshi.kani@hp.com> 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 <toshi.kani@hp.com> > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > Cc: Bob Moore <robert.moore@intel.com> > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > --- > > > > 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 devel@acpica.org 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 <toshi.kani@hp.com> wrote: > > > On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote: > > > > On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <toshi.kani@hp.com> > > > > 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 <toshi.kani@hp.com> > > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > > Cc: Bob Moore <robert.moore@intel.com> > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > --- > > > > > 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 > devel@acpica.org 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* : linux-nvdimm@lists.01.org (open list:LIBNVDIMM BLK: MMIO-APERTURE DRIVER) linux-acpi@vger.kernel.org (open list:ACPI) linux-kernel@vger.kernel.org (open list) devel@acpica.org (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:toshi.kani@hp.com] > Sent: Wednesday, August 26, 2015 4:36 PM > To: Rafael J. Wysocki > Cc: Williams, Dan J; Wysocki, Rafael J; Moore, Robert; linux- > nvdimm@lists.01.org; Linux ACPI; linux-kernel@vger.kernel.org; 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 <toshi.kani@hp.com> > wrote: > > > > On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote: > > > > > On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <toshi.kani@hp.com> > > > > > 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 <toshi.kani@hp.com> > > > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > > > Cc: Bob Moore <robert.moore@intel.com> > > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > --- > > > > > > 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 > > devel@acpica.org 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* > : > linux-nvdimm@lists.01.org (open list:LIBNVDIMM BLK: MMIO-APERTURE DRIVER) > linux-acpi@vger.kernel.org (open list:ACPI) linux-kernel@vger.kernel.org > (open list) devel@acpica.org (open list:ACPI COMPONENT ARCHITECTURE > (ACPICA)) > > 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 <toshi.kani@hp.com> wrote: >> On Wed, 2015-08-26 at 14:30 -0700, Dan Williams wrote: >>> On Wed, Aug 26, 2015 at 2:12 PM, Toshi Kani <toshi.kani@hp.com> wrote: >>>> On Wed, 2015-08-26 at 10:16 -0700, Dan Williams wrote: >>>>> On Wed, Aug 26, 2015 at 9:20 AM, Toshi Kani <toshi.kani@hp.com> 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 <toshi.kani@hp.com> >>>>>> Cc: Dan Williams <dan.j.williams@intel.com> >>>>>> Cc: Bob Moore <robert.moore@intel.com> >>>>>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>>> --- >>>>>> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Thu, Aug 27, 2015 at 7:43 AM, Linda Knippers <linda.knippers@hp.com> 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 <linda.knippers@hp.com> 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 <linda.knippers@hp.com> wrote: > On 8/27/2015 11:30 AM, Dan Williams wrote: >> On Thu, Aug 27, 2015 at 7:43 AM, Linda Knippers <linda.knippers@hp.com> 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 <linda.knippers@hp.com> wrote: >> On 8/27/2015 11:30 AM, Dan Williams wrote: >>> On Thu, Aug 27, 2015 at 7:43 AM, Linda Knippers <linda.knippers@hp.com> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Thu, Aug 27, 2015 at 9:32 AM, Linda Knippers <linda.knippers@hp.com> 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 <linda.knippers@hp.com> 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
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 */
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 <toshi.kani@hp.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Bob Moore <robert.moore@intel.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- 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(-)