[PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
From: Alastair D'Silva
This driver exposes LPC memory on OpenCAPI SCM cards
as an NVDIMM, allowing the existing nvram infrastructure
to be used.
Signed-off-by: Alastair D'Silva
---
drivers/nvdimm/Kconfig | 17 +
drivers/nvdimm/Makefile|3 +
drivers/nvdimm/ocxl-scm.c | 2210
drivers/nvdimm/ocxl-scm_internal.c | 232 +++
drivers/nvdimm/ocxl-scm_internal.h | 331 +
drivers/nvdimm/ocxl-scm_sysfs.c| 219 +++
include/uapi/linux/ocxl-scm.h | 128 ++
mm/memory_hotplug.c|2 +-
8 files changed, 3141 insertions(+), 1 deletion(-)
create mode 100644 drivers/nvdimm/ocxl-scm.c
create mode 100644 drivers/nvdimm/ocxl-scm_internal.c
create mode 100644 drivers/nvdimm/ocxl-scm_internal.h
create mode 100644 drivers/nvdimm/ocxl-scm_sysfs.c
create mode 100644 include/uapi/linux/ocxl-scm.h
diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
index 36af7af6b7cf..e4f7b6b08efd 100644
--- a/drivers/nvdimm/Kconfig
+++ b/drivers/nvdimm/Kconfig
@@ -130,4 +130,21 @@ config NVDIMM_TEST_BUILD
core devm_memremap_pages() implementation and other
infrastructure.
+config OCXL_SCM
+ tristate "OpenCAPI Storage Class Memory"
+ depends on LIBNVDIMM
+ select ZONE_DEVICE
+ select OCXL
+ help
+ Exposes devices that implement the OpenCAPI Storage Class Memory
+ specification as persistent memory regions.
+
+ Select N if unsure.
+
+config OCXL_SCM_DEBUG
+ bool "OpenCAPI Storage Class Memory debugging"
+ depends on OCXL_SCM
+ help
+ Enables low level IOCTLs for OpenCAPI SCM firmware development
+
endif
diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile
index 29203f3d3069..43d826397bfc 100644
--- a/drivers/nvdimm/Makefile
+++ b/drivers/nvdimm/Makefile
@@ -6,6 +6,9 @@ obj-$(CONFIG_ND_BLK) += nd_blk.o
obj-$(CONFIG_X86_PMEM_LEGACY) += nd_e820.o
obj-$(CONFIG_OF_PMEM) += of_pmem.o
obj-$(CONFIG_VIRTIO_PMEM) += virtio_pmem.o nd_virtio.o
+obj-$(CONFIG_OCXL_SCM) += ocxlscm.o
+
+ocxlscm-y := ocxl-scm.o ocxl-scm_internal.o ocxl-scm_sysfs.o
nd_pmem-y := pmem.o
diff --git a/drivers/nvdimm/ocxl-scm.c b/drivers/nvdimm/ocxl-scm.c
new file mode 100644
index ..f4e6cc022de8
--- /dev/null
+++ b/drivers/nvdimm/ocxl-scm.c
@@ -0,0 +1,2210 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright 2019 IBM Corp.
+
+/*
+ * A driver for Storage Class Memory, connected via OpenCAPI
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "ocxl-scm_internal.h"
+
+
+static const struct pci_device_id scm_pci_tbl[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_IBM, 0x0625), },
+ { }
+};
+
+MODULE_DEVICE_TABLE(pci, scm_pci_tbl);
+
+#define SCM_NUM_MINORS 256 // Total to reserve
+#define SCM_USABLE_TIMEOUT 120 // seconds
+
+static dev_t scm_dev;
+static struct class *scm_class;
+static struct mutex minors_idr_lock;
+static struct idr minors_idr;
+
+static const struct attribute_group *scm_pmem_attribute_groups[] = {
+ &nvdimm_bus_attribute_group,
+ NULL,
+};
+
+static const struct attribute_group *scm_pmem_region_attribute_groups[] = {
+ &nd_region_attribute_group,
+ &nd_device_attribute_group,
+ &nd_mapping_attribute_group,
+ &nd_numa_attribute_group,
+ NULL,
+};
+
+/**
+ * scm_ndctl_config_write() - Handle a ND_CMD_SET_CONFIG_DATA command from
ndctl
+ * @scm_data: the SCM metadata
+ * @command: the incoming data to write
+ * Return: 0 on success, negative on failure
+ */
+static int scm_ndctl_config_write(struct scm_data *scm_data,
+ struct nd_cmd_set_config_hdr *command)
+{
+ if (command->in_offset + command->in_length > SCM_LABEL_AREA_SIZE)
+ return -EINVAL;
+
+ memcpy_flushcache(scm_data->metadata_addr + command->in_offset,
command->in_buf,
+ command->in_length);
+
+ return 0;
+}
+
+/**
+ * scm_ndctl_config_read() - Handle a ND_CMD_GET_CONFIG_DATA command from ndctl
+ * @scm_data: the SCM metadata
+ * @command: the read request
+ * Return: 0 on success, negative on failure
+ */
+static int scm_ndctl_config_read(struct scm_data *scm_data,
+struct nd_cmd_get_config_data_hdr *command)
+{
+ if (command->in_offset + command->in_length > SCM_LABEL_AREA_SIZE)
+ return -EINVAL;
+
+ memcpy(command->out_buf, scm_data->metadata_addr + command->in_offset,
+ command->in_length);
+
+ return 0;
+}
+
+/**
+ * scm_ndctl_config_size() - Handle a ND_CMD_GET_CONFIG_SIZE command from ndctl
+ * @scm_data: the SCM metadata
+ * @command: the read request
+ * Return: 0 on success, negative on failure
+ */
+static int scm_ndctl_config_size(struct nd_cmd_get_config_size *command)
+{
+ command->status = 0;
+ command->config_size = SCM_LABEL_AREA_SIZE;
+ command->max_xfer = PAGE_SIZE;
+
+
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
Hi Alastair,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on char-misc/char-misc-testing]
[cannot apply to v5.4-rc5 next-20191025]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Alastair-D-Silva/Add-support-for-OpenCAPI-SCM-devices/20191028-043750
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
da80d2e516eb858eb5bcca7fa5f5a13ed86930e4
config: s390-allmodconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 7.4.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=s390
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
drivers/misc/ocxl/main.c: In function 'init_ocxl':
>> drivers/misc/ocxl/main.c:12:7: error: 'tlbie_capable' undeclared (first use
>> in this function); did you mean 'iommu_capable'?
if (!tlbie_capable)
^
iommu_capable
drivers/misc/ocxl/main.c:12:7: note: each undeclared identifier is reported
only once for each function it appears in
--
drivers/misc/ocxl/core.c: In function 'ocxl_function_open':
>> drivers/misc/ocxl/core.c:546:7: error: implicit declaration of function
>> 'radix_enabled'; did you mean 'zdev_enabled'?
>> [-Werror=implicit-function-declaration]
if (!radix_enabled()) {
^
zdev_enabled
cc1: some warnings being treated as errors
vim +12 drivers/misc/ocxl/main.c
5ef3166e8a32d7 Frederic Barrat 2018-01-23 7
5ef3166e8a32d7 Frederic Barrat 2018-01-23 8 static int __init init_ocxl(void)
5ef3166e8a32d7 Frederic Barrat 2018-01-23 9 {
5ef3166e8a32d7 Frederic Barrat 2018-01-23 10 int rc = 0;
5ef3166e8a32d7 Frederic Barrat 2018-01-23 11
2275d7b5754a57 Nicholas Piggin 2019-09-03 @12 if (!tlbie_capable)
2275d7b5754a57 Nicholas Piggin 2019-09-03 13 return -EINVAL;
2275d7b5754a57 Nicholas Piggin 2019-09-03 14
5ef3166e8a32d7 Frederic Barrat 2018-01-23 15 rc = ocxl_file_init();
5ef3166e8a32d7 Frederic Barrat 2018-01-23 16 if (rc)
5ef3166e8a32d7 Frederic Barrat 2018-01-23 17 return rc;
5ef3166e8a32d7 Frederic Barrat 2018-01-23 18
5ef3166e8a32d7 Frederic Barrat 2018-01-23 19 rc =
pci_register_driver(&ocxl_pci_driver);
5ef3166e8a32d7 Frederic Barrat 2018-01-23 20 if (rc) {
5ef3166e8a32d7 Frederic Barrat 2018-01-23 21 ocxl_file_exit();
5ef3166e8a32d7 Frederic Barrat 2018-01-23 22 return rc;
5ef3166e8a32d7 Frederic Barrat 2018-01-23 23 }
5ef3166e8a32d7 Frederic Barrat 2018-01-23 24 return 0;
5ef3166e8a32d7 Frederic Barrat 2018-01-23 25 }
5ef3166e8a32d7 Frederic Barrat 2018-01-23 26
:: The code at line 12 was first introduced by commit
:: 2275d7b5754a573ffb2ca9e40bd0546eeb986696 powerpc/64s/radix: introduce
options to disable use of the tlbie instruction
:: TO: Nicholas Piggin
:: CC: Michael Ellerman
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
Hi Alastair,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on char-misc/char-misc-testing]
[cannot apply to v5.4-rc5 next-20191025]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Alastair-D-Silva/Add-support-for-OpenCAPI-SCM-devices/20191028-043750
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
da80d2e516eb858eb5bcca7fa5f5a13ed86930e4
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-14) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
drivers/nvdimm/ocxl-scm.c: In function 'scm_register_lpc_mem':
>> drivers/nvdimm/ocxl-scm.c:476:16: error: implicit declaration of function
>> 'of_node_to_nid'; did you mean 'zone_to_nid'?
>> [-Werror=implicit-function-declaration]
target_node = of_node_to_nid(scm_data->pdev->dev.of_node);
^~
zone_to_nid
cc1: some warnings being treated as errors
--
drivers/misc/ocxl/main.c: In function 'init_ocxl':
>> drivers/misc/ocxl/main.c:12:7: error: 'tlbie_capable' undeclared (first use
>> in this function); did you mean 'ptracer_capable'?
if (!tlbie_capable)
^
ptracer_capable
drivers/misc/ocxl/main.c:12:7: note: each undeclared identifier is reported
only once for each function it appears in
--
>> drivers/misc/ocxl/config.c:4:10: fatal error: asm/pnv-ocxl.h: No such file
>> or directory
#include
^~~~
compilation terminated.
--
>> drivers/misc/ocxl/file.c:9:10: fatal error: asm/reg.h: No such file or
>> directory
#include
^~~
compilation terminated.
--
drivers/misc/ocxl/mmio.c: In function 'ocxl_global_mmio_read32':
>> drivers/misc/ocxl/mmio.c:20:10: error: implicit declaration of function
>> 'readl_be'; did you mean 'readsb'? [-Werror=implicit-function-declaration]
*val = readl_be((char *)afu->global_mmio_ptr + offset);
^~~~
readsb
drivers/misc/ocxl/mmio.c: In function 'ocxl_global_mmio_read64':
>> drivers/misc/ocxl/mmio.c:45:10: error: implicit declaration of function
>> 'readq_be'; did you mean 'readsb'? [-Werror=implicit-function-declaration]
*val = readq_be((char *)afu->global_mmio_ptr + offset);
^~~~
readsb
drivers/misc/ocxl/mmio.c: In function 'ocxl_global_mmio_write32':
>> drivers/misc/ocxl/mmio.c:70:3: error: implicit declaration of function
>> 'writel_be'; did you mean 'writesb'? [-Werror=implicit-function-declaration]
writel_be(val, (char *)afu->global_mmio_ptr + offset);
^
writesb
drivers/misc/ocxl/mmio.c: In function 'ocxl_global_mmio_write64':
>> drivers/misc/ocxl/mmio.c:96:3: error: implicit declaration of function
>> 'writeq_be'; did you mean 'writesb'? [-Werror=implicit-function-declaration]
writeq_be(val, (char *)afu->global_mmio_ptr + offset);
^
writesb
cc1: some warnings being treated as errors
--
>> drivers/misc/ocxl/link.c:7:10: fatal error: asm/copro.h: No such file or
>> directory
#include
^
compilation terminated.
--
drivers/misc/ocxl/context.c: In function 'ocxl_context_attach':
>> drivers/misc/ocxl/context.c:82:21: error: 'mm_context_t {aka struct
>> }' has no member named 'id'
pidr = mm->context.id;
^
--
>> drivers/misc/ocxl/afu_irq.c:4:10: fatal error: asm/pnv-ocxl.h: No such file
>> or directory
#include
^~~~
compilation terminated.
--
drivers/misc/ocxl/core.c: In function 'ocxl_function_open':
>> drivers/misc/ocxl/core.c:546:7: error: implicit declaration of function
>> 'radix_enabled'; did you mean 'pat_enabled'?
>> [-Werror=implicit-function-declaration]
if (!radix_enabled()) {
^
pat_enabled
cc1: some warnings being treated as errors
vim +476 drivers/nvdimm/ocxl-scm.c
402
403 /**
404 * scm_register_lpc_mem() - Discover persistent memory on a device and
register it with the NVDIMM subsystem
405 * @scm_data: The SCM device data
406 * Return: 0 on success
407 */
408 static int scm_register_lpc_mem(struct scm_data *scm_data)
409 {
410 struct nd_region_desc region_desc;
411 struct nd_mapping_desc nd_mapping_desc;
412 struct resource *lpc_mem;
413 const struct ocxl_afu_config *config;
414 const struct ocxl_fn_config *fn_config;
415 int rc;
416 unsigned long nvdimm_cmd_mask = 0;
417
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
"Alastair D'Silva" writes:
> From: Alastair D'Silva
>
> This driver exposes LPC memory on OpenCAPI SCM cards
> as an NVDIMM, allowing the existing nvram infrastructure
> to be used.
>
> Signed-off-by: Alastair D'Silva
> ---
> drivers/nvdimm/Kconfig | 17 +
> drivers/nvdimm/Makefile|3 +
> drivers/nvdimm/ocxl-scm.c | 2210
> drivers/nvdimm/ocxl-scm_internal.c | 232 +++
> drivers/nvdimm/ocxl-scm_internal.h | 331 +
> drivers/nvdimm/ocxl-scm_sysfs.c| 219 +++
> include/uapi/linux/ocxl-scm.h | 128 ++
> mm/memory_hotplug.c|2 +-
> 8 files changed, 3141 insertions(+), 1 deletion(-)
> create mode 100644 drivers/nvdimm/ocxl-scm.c
> create mode 100644 drivers/nvdimm/ocxl-scm_internal.c
> create mode 100644 drivers/nvdimm/ocxl-scm_internal.h
> create mode 100644 drivers/nvdimm/ocxl-scm_sysfs.c
> create mode 100644 include/uapi/linux/ocxl-scm.h
>
> diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
> index 36af7af6b7cf..e4f7b6b08efd 100644
> --- a/drivers/nvdimm/Kconfig
> +++ b/drivers/nvdimm/Kconfig
> @@ -130,4 +130,21 @@ config NVDIMM_TEST_BUILD
> core devm_memremap_pages() implementation and other
> infrastructure.
>
> +config OCXL_SCM
> + tristate "OpenCAPI Storage Class Memory"
> + depends on LIBNVDIMM
> + select ZONE_DEVICE
> + select OCXL
> + help
> + Exposes devices that implement the OpenCAPI Storage Class Memory
> + specification as persistent memory regions.
> +
> + Select N if unsure.
> +
> +config OCXL_SCM_DEBUG
> + bool "OpenCAPI Storage Class Memory debugging"
> + depends on OCXL_SCM
> + help
> + Enables low level IOCTLs for OpenCAPI SCM firmware development
> +
> endif
> diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile
> index 29203f3d3069..43d826397bfc 100644
> --- a/drivers/nvdimm/Makefile
> +++ b/drivers/nvdimm/Makefile
> @@ -6,6 +6,9 @@ obj-$(CONFIG_ND_BLK) += nd_blk.o
> obj-$(CONFIG_X86_PMEM_LEGACY) += nd_e820.o
> obj-$(CONFIG_OF_PMEM) += of_pmem.o
> obj-$(CONFIG_VIRTIO_PMEM) += virtio_pmem.o nd_virtio.o
> +obj-$(CONFIG_OCXL_SCM) += ocxlscm.o
> +
> +ocxlscm-y := ocxl-scm.o ocxl-scm_internal.o ocxl-scm_sysfs.o
>
> nd_pmem-y := pmem.o
>
> diff --git a/drivers/nvdimm/ocxl-scm.c b/drivers/nvdimm/ocxl-scm.c
> new file mode 100644
> index ..f4e6cc022de8
> --- /dev/null
> +++ b/drivers/nvdimm/ocxl-scm.c
how about drivers/nvdimm/ocxl/scm.c ? Or may be place the driver as an
ocxl driver.
> @@ -0,0 +1,2210 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2019 IBM Corp.
> +
> +/*
> + * A driver for Storage Class Memory, connected via OpenCAPI
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "ocxl-scm_internal.h"
> +
> +
> +static const struct pci_device_id scm_pci_tbl[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_IBM, 0x0625), },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, scm_pci_tbl);
> +
> +#define SCM_NUM_MINORS 256 // Total to reserve
> +#define SCM_USABLE_TIMEOUT 120 // seconds
> +
> +static dev_t scm_dev;
> +static struct class *scm_class;
> +static struct mutex minors_idr_lock;
> +static struct idr minors_idr;
> +
> +static const struct attribute_group *scm_pmem_attribute_groups[] = {
> + &nvdimm_bus_attribute_group,
> + NULL,
> +};
> +
> +static const struct attribute_group *scm_pmem_region_attribute_groups[] = {
> + &nd_region_attribute_group,
> + &nd_device_attribute_group,
> + &nd_mapping_attribute_group,
> + &nd_numa_attribute_group,
> + NULL,
> +};
> +
There is another patch series that is moving this from device driver to
nvdimm core.
https://lore.kernel.org/linux-mm/157309899529.1582359.15358067933360719580.st...@dwillia2-desk3.amr.corp.intel.com
> +/**
> + * scm_ndctl_config_write() - Handle a ND_CMD_SET_CONFIG_DATA command from
> ndctl
> + * @scm_data: the SCM metadata
> + * @command: the incoming data to write
> + * Return: 0 on success, negative on failure
> + */
> +static int scm_ndctl_config_write(struct scm_data *scm_data,
> + struct nd_cmd_set_config_hdr *command)
> +{
> + if (command->in_offset + command->in_length > SCM_LABEL_AREA_SIZE)
> + return -EINVAL;
> +
> + memcpy_flushcache(scm_data->metadata_addr + command->in_offset,
> command->in_buf,
> + command->in_length);
> +
> + return 0;
> +}
> +
> +/**
> + * scm_ndctl_config_read() - Handle a ND_CMD_GET_CONFIG_DATA command from
> ndctl
> + * @scm_data: the SCM metadata
> + * @command: the read request
> + * Return: 0 on success, negative on failure
> + */
> +static int scm_ndctl_config_read(struct scm_data *scm_data,
> + struct nd_cmd_get_config_data_hdr *command)
> +{
> + if (command->in_offset + command->in_length > SCM_LABEL_AREA_SIZE)
> + return -EIN
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
On Mon, Nov 11, 2019 at 3:34 AM Aneesh Kumar K.V
wrote:
>
> "Alastair D'Silva" writes:
>
> > From: Alastair D'Silva
> >
> > This driver exposes LPC memory on OpenCAPI SCM cards
> > as an NVDIMM, allowing the existing nvram infrastructure
> > to be used.
> >
> > Signed-off-by: Alastair D'Silva
> > ---
> > drivers/nvdimm/Kconfig | 17 +
> > drivers/nvdimm/Makefile|3 +
> > drivers/nvdimm/ocxl-scm.c | 2210
> > drivers/nvdimm/ocxl-scm_internal.c | 232 +++
> > drivers/nvdimm/ocxl-scm_internal.h | 331 +
> > drivers/nvdimm/ocxl-scm_sysfs.c| 219 +++
> > include/uapi/linux/ocxl-scm.h | 128 ++
> > mm/memory_hotplug.c|2 +-
> > 8 files changed, 3141 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/nvdimm/ocxl-scm.c
> > create mode 100644 drivers/nvdimm/ocxl-scm_internal.c
> > create mode 100644 drivers/nvdimm/ocxl-scm_internal.h
> > create mode 100644 drivers/nvdimm/ocxl-scm_sysfs.c
> > create mode 100644 include/uapi/linux/ocxl-scm.h
> >
> > diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
> > index 36af7af6b7cf..e4f7b6b08efd 100644
> > --- a/drivers/nvdimm/Kconfig
> > +++ b/drivers/nvdimm/Kconfig
> > @@ -130,4 +130,21 @@ config NVDIMM_TEST_BUILD
> > core devm_memremap_pages() implementation and other
> > infrastructure.
> >
> > +config OCXL_SCM
> > + tristate "OpenCAPI Storage Class Memory"
> > + depends on LIBNVDIMM
> > + select ZONE_DEVICE
> > + select OCXL
> > + help
> > + Exposes devices that implement the OpenCAPI Storage Class Memory
> > + specification as persistent memory regions.
> > +
> > + Select N if unsure.
> > +
> > +config OCXL_SCM_DEBUG
> > + bool "OpenCAPI Storage Class Memory debugging"
> > + depends on OCXL_SCM
> > + help
> > + Enables low level IOCTLs for OpenCAPI SCM firmware development
> > +
> > endif
> > diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile
> > index 29203f3d3069..43d826397bfc 100644
> > --- a/drivers/nvdimm/Makefile
> > +++ b/drivers/nvdimm/Makefile
> > @@ -6,6 +6,9 @@ obj-$(CONFIG_ND_BLK) += nd_blk.o
> > obj-$(CONFIG_X86_PMEM_LEGACY) += nd_e820.o
> > obj-$(CONFIG_OF_PMEM) += of_pmem.o
> > obj-$(CONFIG_VIRTIO_PMEM) += virtio_pmem.o nd_virtio.o
> > +obj-$(CONFIG_OCXL_SCM) += ocxlscm.o
> > +
> > +ocxlscm-y := ocxl-scm.o ocxl-scm_internal.o ocxl-scm_sysfs.o
> >
> > nd_pmem-y := pmem.o
> >
> > diff --git a/drivers/nvdimm/ocxl-scm.c b/drivers/nvdimm/ocxl-scm.c
> > new file mode 100644
> > index ..f4e6cc022de8
> > --- /dev/null
> > +++ b/drivers/nvdimm/ocxl-scm.c
>
> how about drivers/nvdimm/ocxl/scm.c ? Or may be place the driver as an
> ocxl driver.
>
>
> > @@ -0,0 +1,2210 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +// Copyright 2019 IBM Corp.
> > +
> > +/*
> > + * A driver for Storage Class Memory, connected via OpenCAPI
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include "ocxl-scm_internal.h"
> > +
> > +
> > +static const struct pci_device_id scm_pci_tbl[] = {
> > + { PCI_DEVICE(PCI_VENDOR_ID_IBM, 0x0625), },
> > + { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(pci, scm_pci_tbl);
> > +
> > +#define SCM_NUM_MINORS 256 // Total to reserve
> > +#define SCM_USABLE_TIMEOUT 120 // seconds
> > +
> > +static dev_t scm_dev;
> > +static struct class *scm_class;
> > +static struct mutex minors_idr_lock;
> > +static struct idr minors_idr;
> > +
> > +static const struct attribute_group *scm_pmem_attribute_groups[] = {
> > + &nvdimm_bus_attribute_group,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group *scm_pmem_region_attribute_groups[] = {
> > + &nd_region_attribute_group,
> > + &nd_device_attribute_group,
> > + &nd_mapping_attribute_group,
> > + &nd_numa_attribute_group,
> > + NULL,
> > +};
> > +
>
>
> There is another patch series that is moving this from device driver to
> nvdimm core.
>
> https://lore.kernel.org/linux-mm/157309899529.1582359.15358067933360719580.st...@dwillia2-desk3.amr.corp.intel.com
>
>
> > +/**
> > + * scm_ndctl_config_write() - Handle a ND_CMD_SET_CONFIG_DATA command from
> > ndctl
> > + * @scm_data: the SCM metadata
> > + * @command: the incoming data to write
> > + * Return: 0 on success, negative on failure
> > + */
> > +static int scm_ndctl_config_write(struct scm_data *scm_data,
> > + struct nd_cmd_set_config_hdr *command)
> > +{
> > + if (command->in_offset + command->in_length > SCM_LABEL_AREA_SIZE)
> > + return -EINVAL;
> > +
> > + memcpy_flushcache(scm_data->metadata_addr + command->in_offset,
> > command->in_buf,
> > + command->in_length);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * scm_ndctl_config_read() - Handle a ND_CMD_GET_CONFIG_DATA command from
> > ndctl
> > + * @scm_data: t
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
On Mon, Nov 11, 2019 at 9:37 PM Dan Williams wrote:
>
> On Mon, Nov 11, 2019 at 3:34 AM Aneesh Kumar K.V
> wrote:
> >
> > "Alastair D'Silva" writes:
> >
> > > From: Alastair D'Silva
> > >
> > > This driver exposes LPC memory on OpenCAPI SCM cards
> > > as an NVDIMM, allowing the existing nvram infrastructure
> > > to be used.
> > >
> > > Signed-off-by: Alastair D'Silva
> > > ---
> > > drivers/nvdimm/Kconfig | 17 +
> > > drivers/nvdimm/Makefile|3 +
> > > drivers/nvdimm/ocxl-scm.c | 2210
> > > drivers/nvdimm/ocxl-scm_internal.c | 232 +++
> > > drivers/nvdimm/ocxl-scm_internal.h | 331 +
> > > drivers/nvdimm/ocxl-scm_sysfs.c| 219 +++
> > > include/uapi/linux/ocxl-scm.h | 128 ++
> > > mm/memory_hotplug.c|2 +-
> > > 8 files changed, 3141 insertions(+), 1 deletion(-)
> > > create mode 100644 drivers/nvdimm/ocxl-scm.c
> > > create mode 100644 drivers/nvdimm/ocxl-scm_internal.c
> > > create mode 100644 drivers/nvdimm/ocxl-scm_internal.h
> > > create mode 100644 drivers/nvdimm/ocxl-scm_sysfs.c
> > > create mode 100644 include/uapi/linux/ocxl-scm.h
> > >
> > > diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
> > > index 36af7af6b7cf..e4f7b6b08efd 100644
> > > --- a/drivers/nvdimm/Kconfig
> > > +++ b/drivers/nvdimm/Kconfig
> > > @@ -130,4 +130,21 @@ config NVDIMM_TEST_BUILD
> > > core devm_memremap_pages() implementation and other
> > > infrastructure.
> > >
> > > +config OCXL_SCM
> > > + tristate "OpenCAPI Storage Class Memory"
> > > + depends on LIBNVDIMM
> > > + select ZONE_DEVICE
> > > + select OCXL
> > > + help
> > > + Exposes devices that implement the OpenCAPI Storage Class Memory
> > > + specification as persistent memory regions.
> > > +
> > > + Select N if unsure.
> > > +
> > > +config OCXL_SCM_DEBUG
> > > + bool "OpenCAPI Storage Class Memory debugging"
> > > + depends on OCXL_SCM
> > > + help
> > > + Enables low level IOCTLs for OpenCAPI SCM firmware development
> > > +
> > > endif
> > > diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile
> > > index 29203f3d3069..43d826397bfc 100644
> > > --- a/drivers/nvdimm/Makefile
> > > +++ b/drivers/nvdimm/Makefile
> > > @@ -6,6 +6,9 @@ obj-$(CONFIG_ND_BLK) += nd_blk.o
> > > obj-$(CONFIG_X86_PMEM_LEGACY) += nd_e820.o
> > > obj-$(CONFIG_OF_PMEM) += of_pmem.o
> > > obj-$(CONFIG_VIRTIO_PMEM) += virtio_pmem.o nd_virtio.o
> > > +obj-$(CONFIG_OCXL_SCM) += ocxlscm.o
> > > +
> > > +ocxlscm-y := ocxl-scm.o ocxl-scm_internal.o ocxl-scm_sysfs.o
> > >
> > > nd_pmem-y := pmem.o
> > >
> > > diff --git a/drivers/nvdimm/ocxl-scm.c b/drivers/nvdimm/ocxl-scm.c
> > > new file mode 100644
> > > index ..f4e6cc022de8
> > > --- /dev/null
> > > +++ b/drivers/nvdimm/ocxl-scm.c
> >
> > how about drivers/nvdimm/ocxl/scm.c ? Or may be place the driver as an
> > ocxl driver.
> >
> >
> > > @@ -0,0 +1,2210 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +// Copyright 2019 IBM Corp.
> > > +
> > > +/*
> > > + * A driver for Storage Class Memory, connected via OpenCAPI
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include "ocxl-scm_internal.h"
> > > +
> > > +
> > > +static const struct pci_device_id scm_pci_tbl[] = {
> > > + { PCI_DEVICE(PCI_VENDOR_ID_IBM, 0x0625), },
> > > + { }
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(pci, scm_pci_tbl);
> > > +
> > > +#define SCM_NUM_MINORS 256 // Total to reserve
> > > +#define SCM_USABLE_TIMEOUT 120 // seconds
> > > +
> > > +static dev_t scm_dev;
> > > +static struct class *scm_class;
> > > +static struct mutex minors_idr_lock;
> > > +static struct idr minors_idr;
> > > +
> > > +static const struct attribute_group *scm_pmem_attribute_groups[] = {
> > > + &nvdimm_bus_attribute_group,
> > > + NULL,
> > > +};
> > > +
> > > +static const struct attribute_group *scm_pmem_region_attribute_groups[]
> > > = {
> > > + &nd_region_attribute_group,
> > > + &nd_device_attribute_group,
> > > + &nd_mapping_attribute_group,
> > > + &nd_numa_attribute_group,
> > > + NULL,
> > > +};
> > > +
> >
> >
> > There is another patch series that is moving this from device driver to
> > nvdimm core.
> >
> > https://lore.kernel.org/linux-mm/157309899529.1582359.15358067933360719580.st...@dwillia2-desk3.amr.corp.intel.com
> >
> >
> > > +/**
> > > + * scm_ndctl_config_write() - Handle a ND_CMD_SET_CONFIG_DATA command
> > > from ndctl
> > > + * @scm_data: the SCM metadata
> > > + * @command: the incoming data to write
> > > + * Return: 0 on success, negative on failure
> > > + */
> > > +static int scm_ndctl_config_write(struct scm_data *scm_data,
> > > + struct nd_cmd_set_config_hdr *command)
> > > +{
> > > + if (command->in_offset + command->in_length > SCM_LABEL_AREA
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
Hi Alastair, The patch is huge and could/should probably be split in smaller pieces to ease the review. However, having sinned on that same topic in the past, I made a first pass anyway. I haven't covered everything but tried to focus on the general setup of the driver for now. Since the patch is very long, I'm writing all the comments in one chunk here instead of spreading them over a few thousand lines, where some would be easy to miss. Update MAINTAINERS for the new files Have you discussed with the directory owner if it's ok to split the driver over several files? Kconfig === Does it make sense to keep OCXL_SCM_DEBUG separate? Why not enabling it by default? ocxl_scm.c == scm_file_init() --- on error paths, should call idr_destroy() (same pb found in base ocxl driver) scm_probe() and _function_0() - The different init between function 0 and 1 looks a bit off and it seems they could be unified. Function 1 does the same thing as function 0 (call ocxl_function_open()) then some AFU-specific init, which we could skip if there's no AFU found on the function. And log a WARN if we find an AFU on something else than function 1, since, unlike ocxl, we know exactly what the device is like for SCM. But keep an common probe() function. It would also simplify the flow on scm_remove() and avoid problems (currently ocxl_function_close() is not called for function 1). scm-data->timeouts: array of size 9 declared, we only init 7 members. With the zalloc() the others are at 0, is that correct? Something looks wrong regarding data release in the error path(s). IIUC, we register the device early and rely on the release callback of the device to free all resources (in free_scm_dev()). We should probably have a comment in probe() to make it obvious. Or maybe better, have a subfunction to keep doing the rest of the inits which are dependent on the device release to be cleaned up. In the subsequent error paths in scm_probe(), we are missing a device_unregister() Could log 120 times the same "Waiting for SCM to become usable" message, which is not really helping much. free_scm() - Related to above comment in probe(), it would help to be able to easily match the what's done in probe vs. undone here. For example, in probe(), there's scm_setup_irq(), where we do all things related to interrupts. But we don't have a subfunction to clean the interrupts state. It would help for readability and track potential misses. I didn't tried to match all of them, but the following calls seem missing: ocxl_context_detach() ocxl_afu_irq_free() ocxl_remove() - see comment above about unifying function 0 and 1 case. Why is nvdimm_bus_unregister() treated separately? Can't it be part of the "normal" freeing of resources done implicitly when calling device_unregister() in the free_scm() callback? scm_setup_device_metadata() --- function doesn't do any setup, so the name is misleading. for (i = 0; i < 8; i++) scm_data->fw_version[i] = (val >> (i * 8)) & 0xff; => looks like an endianess conversion? Can't we use the OCXL_BIG_ENDIAN when doing the mmio read? scm_setup_irq() --- if ocxl_afu_irq_get_addr(irq 1) or the ioremap(irq 1) fail, we jump to the label 'out_irq0' and will exit the function with rc = 0, instead of failing. scm_setup_command_metadata() it would make sense to initialize the mutex in the struct command_metadata in this function instead of the top of scm_probe(), to group all the related data inits. scm_probe_function_0() -- comment above function: * This is important as it enables higher than 0 across all other functions, * which in turn enables higher bandwidth accesses "higher than 0"? I'm guessing you want to say function 0 configures the link, to ensure maximum bandwidth EFAULT is usually reserved for an invalid memory access. Why not PTR_ERR() of the returned value from ocxl_function_open()? struct scm_fops has a wrong indentation (spaces between .open and '=') scm_heartbeat() --- the "goto out" at the end of the good path is useless and unusual in the kernel, I think. scm_register_lpc_mem() -- lpc_mem = ocxl_afu_lpc_mem(scm_data->ocxl_afu); => lpc_mem is allocated as part of the afu structure in ocxl, so that shouldn't be NULL. Still worth keeping, but I think lpc_mem->start is what really needs testing scm_imn0_handler() - I don't think we should return IRQ_NONE. As far as the kernel is concerned, an interrupt was raised. So it should be acknowledged, even if the fgpa is somehow in an incorrect state. So the IRQ_NONE should be IRQ_HANDLED scm_imn1_handler() -- for the sake of clarity, the potential error when calling scm_chi() should be treated the same in the 2 handlers. What
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
Some quick feedback on your intro concerns... On Thu, Nov 14, 2019 at 5:41 AM Frederic Barrat wrote: > > Hi Alastair, > > The patch is huge and could/should probably be split in smaller pieces Yeah, it's a must. Split the minimum viable infrastructure by topic and then follow on with per-feature topic patches. > to ease the review. However, having sinned on that same topic in the > past, I made a first pass anyway. I haven't covered everything but tried > to focus on the general setup of the driver for now. > Since the patch is very long, I'm writing all the comments in one chunk > here instead of spreading them over a few thousand lines, where some > would be easy to miss. > > > Update MAINTAINERS for the new files > > Have you discussed with the directory owner if it's ok to split the > driver over several files? My thought is to establish drivers/opencapi/ and move this and the existing drivers/misc/ocxl/ bits there.
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
On Thu, 2019-11-14 at 14:41 +0100, Frederic Barrat wrote: > Hi Alastair, > > The patch is huge and could/should probably be split in smaller > pieces > to ease the review. However, having sinned on that same topic in the > past, I made a first pass anyway. I haven't covered everything but > tried > to focus on the general setup of the driver for now. > Since the patch is very long, I'm writing all the comments in one > chunk > here instead of spreading them over a few thousand lines, where some > would be easy to miss. > > > Update MAINTAINERS for the new files > > Have you discussed with the directory owner if it's ok to split the > driver over several files? > Not yet, I do have a request as to whether drivers/nvdimm is actually the right place for this though. > > Kconfig > === > Does it make sense to keep OCXL_SCM_DEBUG separate? Why not enabling > it > by default? I think so, these features are a bit dangerous for general users, but very useful for developers. > > > ocxl_scm.c > == > > scm_file_init() > --- > on error paths, should call idr_destroy() (same pb found in base > ocxl > driver) > Ok. > > scm_probe() and _function_0() > - > > The different init between function 0 and 1 looks a bit off and it > seems > they could be unified. Function 1 does the same thing as function 0 > (call ocxl_function_open()) then some AFU-specific init, which we > could > skip if there's no AFU found on the function. And log a WARN if we > find > an AFU on something else than function 1, since, unlike ocxl, we > know > exactly what the device is like for SCM. But keep an common probe() > function. It would also simplify the flow on scm_remove() and avoid > problems (currently ocxl_function_close() is not called for function > 1). Hmm, the 2 functions do very different things. Function 0 only exists for link negotiation, and there is a huge amount of other bits & pieces in the scm_data struct that will never be used for it. ocxl_function_close() is called in free_scm() via the release handler for the device. > > scm-data->timeouts: array of size 9 declared, we only init 7 > members. > With the zalloc() the others are at 0, is that correct? > Yes, these will be queried dynamically from the card in a later patch, but that feature is not yet ready for testing. The timeouts that are currently 0 are never read in the current implementation of the driver. > > Something looks wrong regarding data release in the error path(s). > IIUC, > we register the device early and rely on the release callback of the > device to free all resources (in free_scm_dev()). We should probably > have a comment in probe() to make it obvious. Or maybe better, have > a I'll add a comment. > subfunction to keep doing the rest of the inits which are dependent > on > the device release to be cleaned up. In the subsequent error paths > in > scm_probe(), we are missing a device_unregister() > This is intentional, I want to keep the device online (but not registered with libnvdimm) in the event of an error as the card can be interrogated via IOCTLs to find out what wrong. > Could log 120 times the same "Waiting for SCM to become usable" > message, > which is not really helping much. > I'll quieten that, it was useful during development to identify whether the machine had locked up or was still waiting on the card. > > free_scm() > - > Related to above comment in probe(), it would help to be able to > easily > match the what's done in probe vs. undone here. For example, in > probe(), > there's scm_setup_irq(), where we do all things related to > interrupts. > But we don't have a subfunction to clean the interrupts state. It > would > help for readability and track potential misses. I didn't tried to > match > all of them, but the following calls seem missing: > > ocxl_context_detach() > ocxl_afu_irq_free() Hmm, we call ocxl_context_detach_all() in ocxl/core.c:remove_afu() (via ocxl_function_close), but by that stage, I've already called ocxl_context_free(), so that's clearly a bug. I'll add in the missing ocxl_context_detach call in the scm_driver, and in a seperate patch, free the context in ocxl_context_detach_all(). I'll also add in the missing calls to ocxl_afu_irq_free(), but I wonder whether we should also clean all the IRQ allocations in remove_afu() too? > > > ocxl_remove() > - > see comment above about unifying function 0 and 1 case. > Why is nvdimm_bus_unregister() treated separately? Can't it be part > of > the "normal" freeing of resources done implicitly when calling > device_unregister() in the free_scm() callback? > Yeah, good observation. > > scm_setup_device_metadata() > --- > function doesn't do any setup, so the name is misleading. > renamed to read_device_metadata(). > for (i = 0; i < 8; i++) > scm_data->fw_version[i] = (val >> (i * 8)) & 0xff;
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
On 15/11/19 3:35 am, Dan Williams wrote: Have you discussed with the directory owner if it's ok to split the driver over several files? My thought is to establish drivers/opencapi/ and move this and the existing drivers/misc/ocxl/ bits there. Is there any other justification for this we can think of apart from not wanting to put this driver in the nvdimm directory? OpenCAPI drivers aren't really a category of driver unto themselves. -- Andrew Donnellan OzLabs, ADL Canberra a...@linux.ibm.com IBM Australia Limited
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
On Mon, Nov 18, 2019 at 3:48 PM Andrew Donnellan wrote: > > On 15/11/19 3:35 am, Dan Williams wrote: > >> Have you discussed with the directory owner if it's ok to split the > >> driver over several files? > > > > My thought is to establish drivers/opencapi/ and move this and the > > existing drivers/misc/ocxl/ bits there. > > Is there any other justification for this we can think of apart from not > wanting to put this driver in the nvdimm directory? OpenCAPI drivers > aren't really a category of driver unto themselves. The concern is less about adding to drivers/nvdimm/ and more about the proper location to house opencapi specific transport and enumeration details. The organization I'm looking for is to group platform transport and enumeration code together similar to how drivers/pci/ exists independent of all pci drivers that use that common core. For libnvdimm the enumeration is platform specific and calls into the nvdimm core. This is why the x86 platform persistent memory bus driver lives under drivers/acpi/nfit/ instead of drivers/nvdimm/. The nfit driver is an ACPI extension that translates ACPI details into libnvdimm core objects. The usage of "ocxl" in the source leads me to think part of this driver belongs in a directory that has other opencapi specific considerations.
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
On Tue, 2019-11-19 at 10:47 +1100, Andrew Donnellan wrote: > On 15/11/19 3:35 am, Dan Williams wrote: > > > Have you discussed with the directory owner if it's ok to split > > > the > > > driver over several files? > > > > My thought is to establish drivers/opencapi/ and move this and the > > existing drivers/misc/ocxl/ bits there. > > Is there any other justification for this we can think of apart from > not > wanting to put this driver in the nvdimm directory? OpenCAPI drivers > aren't really a category of driver unto themselves. > There is a precedent for bus-based dirs, eg. drivers/(ide|w1|spi) all contain drivers for both controllers & connected devices. Fred, how do you feel about moving the generic OpenCAPI driver out of drivers/misc? -- Alastair D'Silva Open Source Developer Linux Technology Centre, IBM Australia mob: 0423 762 819
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
On 19/11/19 1:48 pm, Alastair D'Silva wrote: On Tue, 2019-11-19 at 10:47 +1100, Andrew Donnellan wrote: On 15/11/19 3:35 am, Dan Williams wrote: Have you discussed with the directory owner if it's ok to split the driver over several files? My thought is to establish drivers/opencapi/ and move this and the existing drivers/misc/ocxl/ bits there. Is there any other justification for this we can think of apart from not wanting to put this driver in the nvdimm directory? OpenCAPI drivers aren't really a category of driver unto themselves. There is a precedent for bus-based dirs, eg. drivers/(ide|w1|spi) all contain drivers for both controllers & connected devices. Fred, how do you feel about moving the generic OpenCAPI driver out of drivers/misc? Instinctively I don't like the idea of creating a whole opencapi directory, as OpenCAPI is a generic bus which is not tightly coupled to any particular application area, and drivers for other OpenCAPI devices are already spread throughout the tree (e.g. cxlflash in drivers/scsi). -- Andrew Donnellan OzLabs, ADL Canberra a...@linux.ibm.com IBM Australia Limited
Re: [PATCH 08/10] nvdimm: Add driver for OpenCAPI Storage Class Memory
On Mon, Nov 18, 2019 at 7:29 PM Andrew Donnellan wrote: > > On 19/11/19 1:48 pm, Alastair D'Silva wrote: > > On Tue, 2019-11-19 at 10:47 +1100, Andrew Donnellan wrote: > >> On 15/11/19 3:35 am, Dan Williams wrote: > Have you discussed with the directory owner if it's ok to split > the > driver over several files? > >>> > >>> My thought is to establish drivers/opencapi/ and move this and the > >>> existing drivers/misc/ocxl/ bits there. > >> > >> Is there any other justification for this we can think of apart from > >> not > >> wanting to put this driver in the nvdimm directory? OpenCAPI drivers > >> aren't really a category of driver unto themselves. > >> > > > > There is a precedent for bus-based dirs, eg. drivers/(ide|w1|spi) all > > contain drivers for both controllers & connected devices. > > > > Fred, how do you feel about moving the generic OpenCAPI driver out of > > drivers/misc? > > Instinctively I don't like the idea of creating a whole opencapi > directory, as OpenCAPI is a generic bus which is not tightly coupled to > any particular application area, and drivers for other OpenCAPI devices > are already spread throughout the tree (e.g. cxlflash in drivers/scsi). I'm not suggesting all opencapi drivers go there, nor the entirety of this driver, just common infrastructure. That said, it's hard to talk about specifics given the current state of the patch set. I have not even taken a deeper look past the changelog as this 3K lines-of-code submission needs to be broken up into smaller pieces before we settle on what pieces belong where. Just looking at the diffstat, at a minimum it's not appropriate for them to live in drivers/nvdimm/ directly, drivers/nvdimm/oxcl/ would be an acceptable starting point.
