| Message ID | 20160220224648.20667.43800.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4577b0665515 |
| Headers | show |
On 2/20/2016 5:46 PM, Dan Williams wrote: > The original format of these commands from the "NVDIMM DSM Interface > Example" [1] are superseded by the ACPI 6.1 definition of the "NVDIMM Root > Device _DSMs" [2]. > > [1]: http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf > [2]: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf > "9.20.7 NVDIMM Root Device _DSMs" > > Changes include: > 1/ New 'restart' fields in ars_status, unfortunately these are > implemented in the middle of the existing definition so this change > is not backwards compatible. The expectation is that shipping > platforms will only ever support the ACPI 6.1 definition. I agree with that. > > 2/ New status values for ars_start ('busy') and ars_status ('overflow'). > > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Linda Knippers <linda.knippers@hpe.com> > Cc: <stable@vger.kernel.org> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/acpi/nfit.c | 6 +++--- > drivers/nvdimm/bus.c | 12 ++++++------ > include/linux/libnvdimm.h | 2 +- > include/uapi/linux/ndctl.h | 11 +++++++++-- > 4 files changed, 19 insertions(+), 12 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index 424b362e8fdc..3edb1324b9d5 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -1503,9 +1503,7 @@ static int ars_do_start(struct nvdimm_bus_descriptor *nd_desc, > case 1: > /* ARS unsupported, but we should never get here */ > return 0; > - case 2: > - return -EINVAL; > - case 3: > + case 6: > /* ARS is in progress */ > msleep(1000); > break; > @@ -1537,6 +1535,8 @@ static int ars_get_status(struct nvdimm_bus_descriptor *nd_desc, > case 2: > /* No ARS performed for the current boot */ > return 0; > + case 3: > + /* TODO: error list overflow support */ > default: > return -ENXIO; > } > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > index 99953b34fa1d..5d28e9405f32 100644 > --- a/drivers/nvdimm/bus.c > +++ b/drivers/nvdimm/bus.c > @@ -382,14 +382,14 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = { > [ND_CMD_ARS_CAP] = { > .in_num = 2, > .in_sizes = { 8, 8, }, > - .out_num = 2, > - .out_sizes = { 4, 4, }, > + .out_num = 4, > + .out_sizes = { 4, 4, 4, 4, }, The status was 4 bytes but now it's 2 bytes of status and 2 bytes of extended status. Where things are didn't actually change but should the two status fields be defined separately to match the spec? It would save some shifting and anding. Maybe a nit... > }, > [ND_CMD_ARS_START] = { > - .in_num = 4, > - .in_sizes = { 8, 8, 2, 6, }, > - .out_num = 1, > - .out_sizes = { 4, }, > + .in_num = 5, > + .in_sizes = { 8, 8, 2, 1, 5, }, > + .out_num = 2, > + .out_sizes = { 4, 4, }, > }, > [ND_CMD_ARS_STATUS] = { > .out_num = 3, > diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h > index c736382fd260..141ffdd59960 100644 > --- a/include/linux/libnvdimm.h > +++ b/include/linux/libnvdimm.h > @@ -26,7 +26,7 @@ enum { > > /* need to set a limit somewhere, but yes, this is likely overkill */ > ND_IOCTL_MAX_BUFLEN = SZ_4M, > - ND_CMD_MAX_ELEM = 4, > + ND_CMD_MAX_ELEM = 5, > ND_CMD_MAX_ENVELOPE = 16, > ND_MAX_MAPPINGS = 32, > > diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h > index 5b4a4be06e2b..cc68b92124d4 100644 > --- a/include/uapi/linux/ndctl.h > +++ b/include/uapi/linux/ndctl.h > @@ -66,14 +66,18 @@ struct nd_cmd_ars_cap { > __u64 length; > __u32 status; > __u32 max_ars_out; > + __u32 clear_err_unit; > + __u32 reserved; > } __packed; > > struct nd_cmd_ars_start { > __u64 address; > __u64 length; > __u16 type; > - __u8 reserved[6]; > + __u8 flags; > + __u8 reserved[5]; > __u32 status; > + __u32 scrub_time; > } __packed; > > struct nd_cmd_ars_status { > @@ -81,11 +85,14 @@ struct nd_cmd_ars_status { > __u32 out_length; > __u64 address; > __u64 length; > + __u64 restart_address; > + __u64 restart_length; > __u16 type; > + __u16 flags; > __u32 num_records; > struct nd_ars_record { > __u32 handle; > - __u32 flags; > + __u32 reserved; > __u64 err_address; > __u64 length; > } __packed records[0]; >
On Mon, Feb 22, 2016 at 2:22 PM, Linda Knippers <linda.knippers@hpe.com> wrote: > > > On 2/20/2016 5:46 PM, Dan Williams wrote: >> The original format of these commands from the "NVDIMM DSM Interface >> Example" [1] are superseded by the ACPI 6.1 definition of the "NVDIMM Root >> Device _DSMs" [2]. >> >> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf >> [2]: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf >> "9.20.7 NVDIMM Root Device _DSMs" >> >> Changes include: >> 1/ New 'restart' fields in ars_status, unfortunately these are >> implemented in the middle of the existing definition so this change >> is not backwards compatible. The expectation is that shipping >> platforms will only ever support the ACPI 6.1 definition. > > I agree with that. > >> >> 2/ New status values for ars_start ('busy') and ars_status ('overflow'). >> >> Cc: Vishal Verma <vishal.l.verma@intel.com> >> Cc: Linda Knippers <linda.knippers@hpe.com> >> Cc: <stable@vger.kernel.org> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> --- [..] >> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c >> index 99953b34fa1d..5d28e9405f32 100644 >> --- a/drivers/nvdimm/bus.c >> +++ b/drivers/nvdimm/bus.c >> @@ -382,14 +382,14 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = { >> [ND_CMD_ARS_CAP] = { >> .in_num = 2, >> .in_sizes = { 8, 8, }, >> - .out_num = 2, >> - .out_sizes = { 4, 4, }, >> + .out_num = 4, >> + .out_sizes = { 4, 4, 4, 4, }, > > The status was 4 bytes but now it's 2 bytes of status and 2 bytes of extended > status. Where things are didn't actually change but should the two status > fields be defined separately to match the spec? It would save some shifting and > anding. Maybe a nit... For this patch, since I'm tagging it for -stable and ndctl.h is exported to userspace, I don't want "status" to have a different meaning depending on which version of the kernel header an application happened to be compiled against. I think we're stuck with the unified u32.
On 2/22/2016 5:41 PM, Dan Williams wrote: > On Mon, Feb 22, 2016 at 2:22 PM, Linda Knippers <linda.knippers@hpe.com> wrote: >> >> >> On 2/20/2016 5:46 PM, Dan Williams wrote: >>> The original format of these commands from the "NVDIMM DSM Interface >>> Example" [1] are superseded by the ACPI 6.1 definition of the "NVDIMM Root >>> Device _DSMs" [2]. >>> >>> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf >>> [2]: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf >>> "9.20.7 NVDIMM Root Device _DSMs" >>> >>> Changes include: >>> 1/ New 'restart' fields in ars_status, unfortunately these are >>> implemented in the middle of the existing definition so this change >>> is not backwards compatible. The expectation is that shipping >>> platforms will only ever support the ACPI 6.1 definition. >> >> I agree with that. >> >>> >>> 2/ New status values for ars_start ('busy') and ars_status ('overflow'). >>> >>> Cc: Vishal Verma <vishal.l.verma@intel.com> >>> Cc: Linda Knippers <linda.knippers@hpe.com> >>> Cc: <stable@vger.kernel.org> >>> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >>> --- > [..] >>> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c >>> index 99953b34fa1d..5d28e9405f32 100644 >>> --- a/drivers/nvdimm/bus.c >>> +++ b/drivers/nvdimm/bus.c >>> @@ -382,14 +382,14 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = { >>> [ND_CMD_ARS_CAP] = { >>> .in_num = 2, >>> .in_sizes = { 8, 8, }, >>> - .out_num = 2, >>> - .out_sizes = { 4, 4, }, >>> + .out_num = 4, >>> + .out_sizes = { 4, 4, 4, 4, }, >> >> The status was 4 bytes but now it's 2 bytes of status and 2 bytes of extended >> status. Where things are didn't actually change but should the two status >> fields be defined separately to match the spec? It would save some shifting and >> anding. Maybe a nit... > > For this patch, since I'm tagging it for -stable and ndctl.h is > exported to userspace, I don't want "status" to have a different > meaning depending on which version of the kernel header an application > happened to be compiled against. I think we're stuck with the unified > u32. I was originally thinking that this would be a good time to change since some of the data formats were changing too but I've reconsidered based on some other draft/example DSMs I've seen. The one thing that's consistent is that there are 4 bytes of status. Sometimes the bytes are split into sub-status fields but not necessarily into 2 2-byte chunks. It's more flexible the way it is. -- ljk
diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index 424b362e8fdc..3edb1324b9d5 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -1503,9 +1503,7 @@ static int ars_do_start(struct nvdimm_bus_descriptor *nd_desc, case 1: /* ARS unsupported, but we should never get here */ return 0; - case 2: - return -EINVAL; - case 3: + case 6: /* ARS is in progress */ msleep(1000); break; @@ -1537,6 +1535,8 @@ static int ars_get_status(struct nvdimm_bus_descriptor *nd_desc, case 2: /* No ARS performed for the current boot */ return 0; + case 3: + /* TODO: error list overflow support */ default: return -ENXIO; } diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c index 99953b34fa1d..5d28e9405f32 100644 --- a/drivers/nvdimm/bus.c +++ b/drivers/nvdimm/bus.c @@ -382,14 +382,14 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = { [ND_CMD_ARS_CAP] = { .in_num = 2, .in_sizes = { 8, 8, }, - .out_num = 2, - .out_sizes = { 4, 4, }, + .out_num = 4, + .out_sizes = { 4, 4, 4, 4, }, }, [ND_CMD_ARS_START] = { - .in_num = 4, - .in_sizes = { 8, 8, 2, 6, }, - .out_num = 1, - .out_sizes = { 4, }, + .in_num = 5, + .in_sizes = { 8, 8, 2, 1, 5, }, + .out_num = 2, + .out_sizes = { 4, 4, }, }, [ND_CMD_ARS_STATUS] = { .out_num = 3, diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h index c736382fd260..141ffdd59960 100644 --- a/include/linux/libnvdimm.h +++ b/include/linux/libnvdimm.h @@ -26,7 +26,7 @@ enum { /* need to set a limit somewhere, but yes, this is likely overkill */ ND_IOCTL_MAX_BUFLEN = SZ_4M, - ND_CMD_MAX_ELEM = 4, + ND_CMD_MAX_ELEM = 5, ND_CMD_MAX_ENVELOPE = 16, ND_MAX_MAPPINGS = 32, diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h index 5b4a4be06e2b..cc68b92124d4 100644 --- a/include/uapi/linux/ndctl.h +++ b/include/uapi/linux/ndctl.h @@ -66,14 +66,18 @@ struct nd_cmd_ars_cap { __u64 length; __u32 status; __u32 max_ars_out; + __u32 clear_err_unit; + __u32 reserved; } __packed; struct nd_cmd_ars_start { __u64 address; __u64 length; __u16 type; - __u8 reserved[6]; + __u8 flags; + __u8 reserved[5]; __u32 status; + __u32 scrub_time; } __packed; struct nd_cmd_ars_status { @@ -81,11 +85,14 @@ struct nd_cmd_ars_status { __u32 out_length; __u64 address; __u64 length; + __u64 restart_address; + __u64 restart_length; __u16 type; + __u16 flags; __u32 num_records; struct nd_ars_record { __u32 handle; - __u32 flags; + __u32 reserved; __u64 err_address; __u64 length; } __packed records[0];
The original format of these commands from the "NVDIMM DSM Interface Example" [1] are superseded by the ACPI 6.1 definition of the "NVDIMM Root Device _DSMs" [2]. [1]: http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf [2]: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf "9.20.7 NVDIMM Root Device _DSMs" Changes include: 1/ New 'restart' fields in ars_status, unfortunately these are implemented in the middle of the existing definition so this change is not backwards compatible. The expectation is that shipping platforms will only ever support the ACPI 6.1 definition. 2/ New status values for ars_start ('busy') and ars_status ('overflow'). Cc: Vishal Verma <vishal.l.verma@intel.com> Cc: Linda Knippers <linda.knippers@hpe.com> Cc: <stable@vger.kernel.org> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/acpi/nfit.c | 6 +++--- drivers/nvdimm/bus.c | 12 ++++++------ include/linux/libnvdimm.h | 2 +- include/uapi/linux/ndctl.h | 11 +++++++++-- 4 files changed, 19 insertions(+), 12 deletions(-)