| Message ID | 20200221032720.33893-16-alastair@au1.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for OpenCAPI Persistent Memory devices | expand |
On 21/2/20 2:27 pm, Alastair D'Silva wrote:> +int ns_response_handled(const struct ocxlpmem *ocxlpmem) > +{ > + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_CHIC, > + OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_CHI_NSCRA); > +} Same comment as on the last patch - I think we're meant to be clearing this bit, not setting it to 1,
On Thu, Feb 20, 2020 at 7:28 PM Alastair D'Silva <alastair@au1.ibm.com> wrote: > > From: Alastair D'Silva <alastair@d-silva.org> > > Similar to the previous patch, this adds support for near storage commands. Similar comment as the last patch. This changelog does not give the reviewer any frame of reference to review the patch.
On Thu, 2020-02-27 at 19:30 +1100, Andrew Donnellan wrote: > On 21/2/20 2:27 pm, Alastair D'Silva wrote:> +int > ns_response_handled(const struct ocxlpmem *ocxlpmem) > > +{ > > + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, > > GLOBAL_MMIO_CHIC, > > + OCXL_LITTLE_ENDIAN, > > GLOBAL_MMIO_CHI_NSCRA); > > +} > > Same comment as on the last patch - I think we're meant to be > clearing > this bit, not setting it to 1, > Same reply :) Writing to the CHIC register clears the bit in CHI. >
Le 21/02/2020 à 04:27, Alastair D'Silva a écrit : > From: Alastair D'Silva <alastair@d-silva.org> > > Similar to the previous patch, this adds support for near storage commands. > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > --- Is any of these new functions ever called? Fred > arch/powerpc/platforms/powernv/pmem/ocxl.c | 6 +++ > .../platforms/powernv/pmem/ocxl_internal.c | 41 +++++++++++++++++++ > .../platforms/powernv/pmem/ocxl_internal.h | 37 +++++++++++++++++ > 3 files changed, 84 insertions(+) > > diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl.c b/arch/powerpc/platforms/powernv/pmem/ocxl.c > index 4e782d22605b..b8bd7e703b19 100644 > --- a/arch/powerpc/platforms/powernv/pmem/ocxl.c > +++ b/arch/powerpc/platforms/powernv/pmem/ocxl.c > @@ -259,12 +259,18 @@ static int setup_command_metadata(struct ocxlpmem *ocxlpmem) > int rc; > > mutex_init(&ocxlpmem->admin_command.lock); > + mutex_init(&ocxlpmem->ns_command.lock); > > rc = extract_command_metadata(ocxlpmem, GLOBAL_MMIO_ACMA_CREQO, > &ocxlpmem->admin_command); > if (rc) > return rc; > > + rc = extract_command_metadata(ocxlpmem, GLOBAL_MMIO_NSCMA_CREQO, > + &ocxlpmem->ns_command); > + if (rc) > + return rc; > + > return 0; > } > > diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c > index 583f48023025..3e0b133feddf 100644 > --- a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c > +++ b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c > @@ -133,6 +133,47 @@ int admin_response_handled(const struct ocxlpmem *ocxlpmem) > OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_CHI_ACRA); > } > > +int ns_command_request(struct ocxlpmem *ocxlpmem, u8 op_code) > +{ > + u64 val; > + int rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_CHI, > + OCXL_LITTLE_ENDIAN, &val); > + if (rc) > + return rc; > + > + if (!(val & GLOBAL_MMIO_CHI_NSCRA)) > + return -EBUSY; > + > + return scm_command_request(ocxlpmem, &ocxlpmem->ns_command, op_code); > +} > + > +int ns_response(const struct ocxlpmem *ocxlpmem) > +{ > + return command_response(ocxlpmem, &ocxlpmem->ns_command); > +} > + > +int ns_command_execute(const struct ocxlpmem *ocxlpmem) > +{ > + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_HCI, > + OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_HCI_NSCRW); > +} > + > +bool ns_command_complete(const struct ocxlpmem *ocxlpmem) > +{ > + u64 val = 0; > + int rc = ocxlpmem_chi(ocxlpmem, &val); > + > + WARN_ON(rc); > + > + return (val & GLOBAL_MMIO_CHI_NSCRA) != 0; > +} > + > +int ns_response_handled(const struct ocxlpmem *ocxlpmem) > +{ > + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_CHIC, > + OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_CHI_NSCRA); > +} > + > void warn_status(const struct ocxlpmem *ocxlpmem, const char *message, > u8 status) > { > diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h > index 2fef68c71271..28e2020f6355 100644 > --- a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h > +++ b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h > @@ -107,6 +107,7 @@ struct ocxlpmem { > struct ocxl_context *ocxl_context; > void *metadata_addr; > struct command_metadata admin_command; > + struct command_metadata ns_command; > struct resource pmem_res; > struct nd_region *nd_region; > char fw_version[8+1]; > @@ -175,6 +176,42 @@ int admin_command_complete_timeout(const struct ocxlpmem *ocxlpmem, > */ > int admin_response_handled(const struct ocxlpmem *ocxlpmem); > > +/** > + * ns_command_request() - Issue a near storage command request > + * @ocxlpmem: the device metadata > + * @op_code: The op-code for the command > + * Returns an identifier for the command, or negative on error > + */ > +int ns_command_request(struct ocxlpmem *ocxlpmem, u8 op_code); > + > +/** > + * ns_response() - Validate a near storage response > + * @ocxlpmem: the device metadata > + * Returns the status code of the command, or negative on error > + */ > +int ns_response(const struct ocxlpmem *ocxlpmem); > + > +/** > + * ns_command_execute() - Notify the controller to start processing a pending near storage command > + * @ocxlpmem: the device metadata > + * Returns 0 on success, negative on error > + */ > +int ns_command_execute(const struct ocxlpmem *ocxlpmem); > + > +/** > + * ns_command_complete() - Is a near storage command executing > + * @ocxlpmem: the device metadata > + * Returns true if the previous admin command has completed > + */ > +bool ns_command_complete(const struct ocxlpmem *ocxlpmem); > + > +/** > + * ns_response_handled() - Notify the controller that the near storage response has been handled > + * @ocxlpmem: the device metadata > + * Returns 0 on success, negative on failure > + */ > +int ns_response_handled(const struct ocxlpmem *ocxlpmem); > + > /** > * warn_status() - Emit a kernel warning showing a command status. > * @ocxlpmem: the device metadata >
On Mon, Mar 2, 2020 at 9:59 AM Frederic Barrat <fbarrat@linux.ibm.com> wrote: > > > > Le 21/02/2020 à 04:27, Alastair D'Silva a écrit : > > From: Alastair D'Silva <alastair@d-silva.org> > > > > Similar to the previous patch, this adds support for near storage commands. > > > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > > --- > > > Is any of these new functions ever called? This is my concern as well. The libnvdimm command support is limited to the commands that Linux will use. Other passthrough commands are supported through a passthrough interface. However, that passthrough interface is explicitly limited to publicly documented command sets so that the kernel has an opportunity to constrain and consolidate command implementations across vendors.
On Mon, 2020-03-02 at 10:42 -0800, Dan Williams wrote: > On Mon, Mar 2, 2020 at 9:59 AM Frederic Barrat <fbarrat@linux.ibm.com > > wrote: > > > > > > Le 21/02/2020 à 04:27, Alastair D'Silva a écrit : > > > From: Alastair D'Silva <alastair@d-silva.org> > > > > > > Similar to the previous patch, this adds support for near storage > > > commands. > > > > > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > > > --- > > > > Is any of these new functions ever called? > > This is my concern as well. The libnvdimm command support is limited > to the commands that Linux will use. Other passthrough commands are > supported through a passthrough interface. However, that passthrough > interface is explicitly limited to publicly documented command sets > so > that the kernel has an opportunity to constrain and consolidate > command implementations across vendors. It will be in the patch that implements overwrite. I moved that patch out of this series, as it needs more testing, so I guess I can submit this alongside it.
diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl.c b/arch/powerpc/platforms/powernv/pmem/ocxl.c index 4e782d22605b..b8bd7e703b19 100644 --- a/arch/powerpc/platforms/powernv/pmem/ocxl.c +++ b/arch/powerpc/platforms/powernv/pmem/ocxl.c @@ -259,12 +259,18 @@ static int setup_command_metadata(struct ocxlpmem *ocxlpmem) int rc; mutex_init(&ocxlpmem->admin_command.lock); + mutex_init(&ocxlpmem->ns_command.lock); rc = extract_command_metadata(ocxlpmem, GLOBAL_MMIO_ACMA_CREQO, &ocxlpmem->admin_command); if (rc) return rc; + rc = extract_command_metadata(ocxlpmem, GLOBAL_MMIO_NSCMA_CREQO, + &ocxlpmem->ns_command); + if (rc) + return rc; + return 0; } diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c index 583f48023025..3e0b133feddf 100644 --- a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c +++ b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.c @@ -133,6 +133,47 @@ int admin_response_handled(const struct ocxlpmem *ocxlpmem) OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_CHI_ACRA); } +int ns_command_request(struct ocxlpmem *ocxlpmem, u8 op_code) +{ + u64 val; + int rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_CHI, + OCXL_LITTLE_ENDIAN, &val); + if (rc) + return rc; + + if (!(val & GLOBAL_MMIO_CHI_NSCRA)) + return -EBUSY; + + return scm_command_request(ocxlpmem, &ocxlpmem->ns_command, op_code); +} + +int ns_response(const struct ocxlpmem *ocxlpmem) +{ + return command_response(ocxlpmem, &ocxlpmem->ns_command); +} + +int ns_command_execute(const struct ocxlpmem *ocxlpmem) +{ + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_HCI, + OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_HCI_NSCRW); +} + +bool ns_command_complete(const struct ocxlpmem *ocxlpmem) +{ + u64 val = 0; + int rc = ocxlpmem_chi(ocxlpmem, &val); + + WARN_ON(rc); + + return (val & GLOBAL_MMIO_CHI_NSCRA) != 0; +} + +int ns_response_handled(const struct ocxlpmem *ocxlpmem) +{ + return ocxl_global_mmio_set64(ocxlpmem->ocxl_afu, GLOBAL_MMIO_CHIC, + OCXL_LITTLE_ENDIAN, GLOBAL_MMIO_CHI_NSCRA); +} + void warn_status(const struct ocxlpmem *ocxlpmem, const char *message, u8 status) { diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h index 2fef68c71271..28e2020f6355 100644 --- a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h +++ b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h @@ -107,6 +107,7 @@ struct ocxlpmem { struct ocxl_context *ocxl_context; void *metadata_addr; struct command_metadata admin_command; + struct command_metadata ns_command; struct resource pmem_res; struct nd_region *nd_region; char fw_version[8+1]; @@ -175,6 +176,42 @@ int admin_command_complete_timeout(const struct ocxlpmem *ocxlpmem, */ int admin_response_handled(const struct ocxlpmem *ocxlpmem); +/** + * ns_command_request() - Issue a near storage command request + * @ocxlpmem: the device metadata + * @op_code: The op-code for the command + * Returns an identifier for the command, or negative on error + */ +int ns_command_request(struct ocxlpmem *ocxlpmem, u8 op_code); + +/** + * ns_response() - Validate a near storage response + * @ocxlpmem: the device metadata + * Returns the status code of the command, or negative on error + */ +int ns_response(const struct ocxlpmem *ocxlpmem); + +/** + * ns_command_execute() - Notify the controller to start processing a pending near storage command + * @ocxlpmem: the device metadata + * Returns 0 on success, negative on error + */ +int ns_command_execute(const struct ocxlpmem *ocxlpmem); + +/** + * ns_command_complete() - Is a near storage command executing + * @ocxlpmem: the device metadata + * Returns true if the previous admin command has completed + */ +bool ns_command_complete(const struct ocxlpmem *ocxlpmem); + +/** + * ns_response_handled() - Notify the controller that the near storage response has been handled + * @ocxlpmem: the device metadata + * Returns 0 on success, negative on failure + */ +int ns_response_handled(const struct ocxlpmem *ocxlpmem); + /** * warn_status() - Emit a kernel warning showing a command status. * @ocxlpmem: the device metadata