| Message ID | 20211111204436.1560365-11-vishal.l.verma@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial CXL support | expand |
On Thu, Nov 11, 2021 at 12:45 PM Vishal Verma <vishal.l.verma@intel.com> wrote: > > Add an nvdimm bridge object representation internal to libcxl. A bridge > object is ited to its parent memdev object, and this patch adds its s/ited/tied/ > first interface, which checks whether a bridge is 'active' - i.e. > implying the label space on the memdev is owned by the kernel. Just some minor fixups below and you can add: Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > cxl/lib/private.h | 9 ++++++ > cxl/lib/libcxl.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ > cxl/libcxl.h | 1 + > cxl/lib/libcxl.sym | 1 + > 4 files changed, 84 insertions(+) > > diff --git a/cxl/lib/private.h b/cxl/lib/private.h > index c4ed741..2f0b6ea 100644 > --- a/cxl/lib/private.h > +++ b/cxl/lib/private.h > @@ -10,6 +10,14 @@ > > #define CXL_EXPORT __attribute__ ((visibility("default"))) > > + > +struct cxl_nvdimm_br { Might as well spell out "bridge". > + int id; > + void *dev_buf; > + size_t buf_len; > + char *dev_path; > +}; > + > struct cxl_memdev { > int id, major, minor; > void *dev_buf; > @@ -23,6 +31,7 @@ struct cxl_memdev { > int payload_max; > size_t lsa_size; > struct kmod_module *module; > + struct cxl_nvdimm_br *bridge; > }; > > enum cxl_cmd_query_status { > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c > index def3a97..7bc0696 100644 > --- a/cxl/lib/libcxl.c > +++ b/cxl/lib/libcxl.c > @@ -45,11 +45,19 @@ struct cxl_ctx { > void *private_data; > }; > > +static void free_bridge(struct cxl_nvdimm_br *bridge) > +{ > + free(bridge->dev_buf); > + free(bridge->dev_path); > + free(bridge); > +} > + > static void free_memdev(struct cxl_memdev *memdev, struct list_head *head) > { > if (head) > list_del_from(head, &memdev->list); > kmod_module_unref(memdev->module); > + free_bridge(memdev->bridge); > free(memdev->firmware_version); > free(memdev->dev_buf); > free(memdev->dev_path); > @@ -205,6 +213,40 @@ CXL_EXPORT void cxl_set_log_priority(struct cxl_ctx *ctx, int priority) > ctx->ctx.log_priority = priority; > } > > +static void *add_cxl_bridge(void *parent, int id, const char *br_base) > +{ > + const char *devname = devpath_to_devname(br_base); > + struct cxl_memdev *memdev = parent; > + struct cxl_ctx *ctx = memdev->ctx; > + struct cxl_nvdimm_br *bridge; > + > + dbg(ctx, "%s: bridge_base: \'%s\'\n", devname, br_base); > + > + bridge = calloc(1, sizeof(*bridge)); > + if (!bridge) > + goto err_dev; > + bridge->id = id; > + > + bridge->dev_path = strdup(br_base); > + if (!bridge->dev_path) > + goto err_read; > + > + bridge->dev_buf = calloc(1, strlen(br_base) + 50); > + if (!bridge->dev_buf) > + goto err_read; > + bridge->buf_len = strlen(br_base) + 50; > + > + memdev->bridge = bridge; > + return bridge; > + > + err_read: > + free(bridge->dev_buf); > + free(bridge->dev_path); > + free(bridge); > + err_dev: > + return NULL; > +} > + > static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base) > { > const char *devname = devpath_to_devname(cxlmem_base); > @@ -271,6 +313,8 @@ static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base) > goto err_read; > memdev->buf_len = strlen(cxlmem_base) + 50; > > + sysfs_device_parse(ctx, cxlmem_base, "pmem", memdev, add_cxl_bridge); > + > cxl_memdev_foreach(ctx, memdev_dup) > if (memdev_dup->id == memdev->id) { > free_memdev(memdev, NULL); > @@ -362,6 +406,35 @@ CXL_EXPORT size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev) > return memdev->lsa_size; > } > > +static int is_enabled(const char *drvpath) > +{ > + struct stat st; > + > + if (lstat(drvpath, &st) < 0 || !S_ISLNK(st.st_mode)) > + return 0; > + else > + return 1; > +} > + > +CXL_EXPORT int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev) > +{ > + struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev); > + struct cxl_nvdimm_br *bridge = memdev->bridge; > + char *path = memdev->dev_buf; Should this be: bridge->dev_buf? > + int len = memdev->buf_len; ...and this should bridge->buf_len? Not strictly a bug, but might as well use the 'bridge' version of these attributes, right? > + > + if (!bridge) > + return 0; > + > + if (snprintf(path, len, "%s/driver", bridge->dev_path) >= len) { > + err(ctx, "%s: nvdimm bridge buffer too small!\n", > + cxl_memdev_get_devname(memdev)); > + return 0; > + } > + > + return is_enabled(path); > +} > + > CXL_EXPORT void cxl_cmd_unref(struct cxl_cmd *cmd) > { > if (!cmd) > diff --git a/cxl/libcxl.h b/cxl/libcxl.h > index d3b97a1..535e349 100644 > --- a/cxl/libcxl.h > +++ b/cxl/libcxl.h > @@ -43,6 +43,7 @@ unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev); > unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev); > const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev); > size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev); > +int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev); > > #define cxl_memdev_foreach(ctx, memdev) \ > for (memdev = cxl_memdev_get_first(ctx); \ > diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym > index 858e953..f3b0c63 100644 > --- a/cxl/lib/libcxl.sym > +++ b/cxl/lib/libcxl.sym > @@ -65,6 +65,7 @@ global: > cxl_cmd_new_read_label; > cxl_cmd_read_label_get_payload; > cxl_memdev_get_label_size; > + cxl_memdev_nvdimm_bridge_active; > local: > *; > }; > -- > 2.31.1 >
On Thu, 2021-11-11 at 15:49 -0800, Dan Williams wrote: > On Thu, Nov 11, 2021 at 12:45 PM Vishal Verma <vishal.l.verma@intel.com> wrote: > > > > Add an nvdimm bridge object representation internal to libcxl. A bridge > > object is ited to its parent memdev object, and this patch adds its > > s/ited/tied/ > > > first interface, which checks whether a bridge is 'active' - i.e. > > implying the label space on the memdev is owned by the kernel. > > Just some minor fixups below and you can add: Thanks Dan - fixed all these up in v6 > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > --- > > cxl/lib/private.h | 9 ++++++ > > cxl/lib/libcxl.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ > > cxl/libcxl.h | 1 + > > cxl/lib/libcxl.sym | 1 + > > 4 files changed, 84 insertions(+) > > > > diff --git a/cxl/lib/private.h b/cxl/lib/private.h > > index c4ed741..2f0b6ea 100644 > > --- a/cxl/lib/private.h > > +++ b/cxl/lib/private.h > > @@ -10,6 +10,14 @@ > > > > #define CXL_EXPORT __attribute__ ((visibility("default"))) > > > > + > > +struct cxl_nvdimm_br { > > Might as well spell out "bridge". > > > + int id; > > + void *dev_buf; > > + size_t buf_len; > > + char *dev_path; > > +}; > > + > > struct cxl_memdev { > > int id, major, minor; > > void *dev_buf; > > @@ -23,6 +31,7 @@ struct cxl_memdev { > > int payload_max; > > size_t lsa_size; > > struct kmod_module *module; > > + struct cxl_nvdimm_br *bridge; > > }; > > > > enum cxl_cmd_query_status { > > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c > > index def3a97..7bc0696 100644 > > --- a/cxl/lib/libcxl.c > > +++ b/cxl/lib/libcxl.c > > @@ -45,11 +45,19 @@ struct cxl_ctx { > > void *private_data; > > }; > > > > +static void free_bridge(struct cxl_nvdimm_br *bridge) > > +{ > > + free(bridge->dev_buf); > > + free(bridge->dev_path); > > + free(bridge); > > +} > > + > > static void free_memdev(struct cxl_memdev *memdev, struct list_head *head) > > { > > if (head) > > list_del_from(head, &memdev->list); > > kmod_module_unref(memdev->module); > > + free_bridge(memdev->bridge); > > free(memdev->firmware_version); > > free(memdev->dev_buf); > > free(memdev->dev_path); > > @@ -205,6 +213,40 @@ CXL_EXPORT void cxl_set_log_priority(struct cxl_ctx *ctx, int priority) > > ctx->ctx.log_priority = priority; > > } > > > > +static void *add_cxl_bridge(void *parent, int id, const char *br_base) > > +{ > > + const char *devname = devpath_to_devname(br_base); > > + struct cxl_memdev *memdev = parent; > > + struct cxl_ctx *ctx = memdev->ctx; > > + struct cxl_nvdimm_br *bridge; > > + > > + dbg(ctx, "%s: bridge_base: \'%s\'\n", devname, br_base); > > + > > + bridge = calloc(1, sizeof(*bridge)); > > + if (!bridge) > > + goto err_dev; > > + bridge->id = id; > > + > > + bridge->dev_path = strdup(br_base); > > + if (!bridge->dev_path) > > + goto err_read; > > + > > + bridge->dev_buf = calloc(1, strlen(br_base) + 50); > > + if (!bridge->dev_buf) > > + goto err_read; > > + bridge->buf_len = strlen(br_base) + 50; > > + > > + memdev->bridge = bridge; > > + return bridge; > > + > > + err_read: > > + free(bridge->dev_buf); > > + free(bridge->dev_path); > > + free(bridge); > > + err_dev: > > + return NULL; > > +} > > + > > static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base) > > { > > const char *devname = devpath_to_devname(cxlmem_base); > > @@ -271,6 +313,8 @@ static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base) > > goto err_read; > > memdev->buf_len = strlen(cxlmem_base) + 50; > > > > + sysfs_device_parse(ctx, cxlmem_base, "pmem", memdev, add_cxl_bridge); > > + > > cxl_memdev_foreach(ctx, memdev_dup) > > if (memdev_dup->id == memdev->id) { > > free_memdev(memdev, NULL); > > @@ -362,6 +406,35 @@ CXL_EXPORT size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev) > > return memdev->lsa_size; > > } > > > > +static int is_enabled(const char *drvpath) > > +{ > > + struct stat st; > > + > > + if (lstat(drvpath, &st) < 0 || !S_ISLNK(st.st_mode)) > > + return 0; > > + else > > + return 1; > > +} > > + > > +CXL_EXPORT int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev) > > +{ > > + struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev); > > + struct cxl_nvdimm_br *bridge = memdev->bridge; > > + char *path = memdev->dev_buf; > > Should this be: bridge->dev_buf? > > > + int len = memdev->buf_len; > > ...and this should bridge->buf_len? > > Not strictly a bug, but might as well use the 'bridge' version of > these attributes, right? > > > + > > + if (!bridge) > > + return 0; > > + > > + if (snprintf(path, len, "%s/driver", bridge->dev_path) >= len) { > > + err(ctx, "%s: nvdimm bridge buffer too small!\n", > > + cxl_memdev_get_devname(memdev)); > > + return 0; > > + } > > + > > + return is_enabled(path); > > +} > > + > > CXL_EXPORT void cxl_cmd_unref(struct cxl_cmd *cmd) > > { > > if (!cmd) > > diff --git a/cxl/libcxl.h b/cxl/libcxl.h > > index d3b97a1..535e349 100644 > > --- a/cxl/libcxl.h > > +++ b/cxl/libcxl.h > > @@ -43,6 +43,7 @@ unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev); > > unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev); > > const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev); > > size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev); > > +int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev); > > > > #define cxl_memdev_foreach(ctx, memdev) \ > > for (memdev = cxl_memdev_get_first(ctx); \ > > diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym > > index 858e953..f3b0c63 100644 > > --- a/cxl/lib/libcxl.sym > > +++ b/cxl/lib/libcxl.sym > > @@ -65,6 +65,7 @@ global: > > cxl_cmd_new_read_label; > > cxl_cmd_read_label_get_payload; > > cxl_memdev_get_label_size; > > + cxl_memdev_nvdimm_bridge_active; > > local: > > *; > > }; > > -- > > 2.31.1 > >
diff --git a/cxl/lib/private.h b/cxl/lib/private.h index c4ed741..2f0b6ea 100644 --- a/cxl/lib/private.h +++ b/cxl/lib/private.h @@ -10,6 +10,14 @@ #define CXL_EXPORT __attribute__ ((visibility("default"))) + +struct cxl_nvdimm_br { + int id; + void *dev_buf; + size_t buf_len; + char *dev_path; +}; + struct cxl_memdev { int id, major, minor; void *dev_buf; @@ -23,6 +31,7 @@ struct cxl_memdev { int payload_max; size_t lsa_size; struct kmod_module *module; + struct cxl_nvdimm_br *bridge; }; enum cxl_cmd_query_status { diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c index def3a97..7bc0696 100644 --- a/cxl/lib/libcxl.c +++ b/cxl/lib/libcxl.c @@ -45,11 +45,19 @@ struct cxl_ctx { void *private_data; }; +static void free_bridge(struct cxl_nvdimm_br *bridge) +{ + free(bridge->dev_buf); + free(bridge->dev_path); + free(bridge); +} + static void free_memdev(struct cxl_memdev *memdev, struct list_head *head) { if (head) list_del_from(head, &memdev->list); kmod_module_unref(memdev->module); + free_bridge(memdev->bridge); free(memdev->firmware_version); free(memdev->dev_buf); free(memdev->dev_path); @@ -205,6 +213,40 @@ CXL_EXPORT void cxl_set_log_priority(struct cxl_ctx *ctx, int priority) ctx->ctx.log_priority = priority; } +static void *add_cxl_bridge(void *parent, int id, const char *br_base) +{ + const char *devname = devpath_to_devname(br_base); + struct cxl_memdev *memdev = parent; + struct cxl_ctx *ctx = memdev->ctx; + struct cxl_nvdimm_br *bridge; + + dbg(ctx, "%s: bridge_base: \'%s\'\n", devname, br_base); + + bridge = calloc(1, sizeof(*bridge)); + if (!bridge) + goto err_dev; + bridge->id = id; + + bridge->dev_path = strdup(br_base); + if (!bridge->dev_path) + goto err_read; + + bridge->dev_buf = calloc(1, strlen(br_base) + 50); + if (!bridge->dev_buf) + goto err_read; + bridge->buf_len = strlen(br_base) + 50; + + memdev->bridge = bridge; + return bridge; + + err_read: + free(bridge->dev_buf); + free(bridge->dev_path); + free(bridge); + err_dev: + return NULL; +} + static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base) { const char *devname = devpath_to_devname(cxlmem_base); @@ -271,6 +313,8 @@ static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base) goto err_read; memdev->buf_len = strlen(cxlmem_base) + 50; + sysfs_device_parse(ctx, cxlmem_base, "pmem", memdev, add_cxl_bridge); + cxl_memdev_foreach(ctx, memdev_dup) if (memdev_dup->id == memdev->id) { free_memdev(memdev, NULL); @@ -362,6 +406,35 @@ CXL_EXPORT size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev) return memdev->lsa_size; } +static int is_enabled(const char *drvpath) +{ + struct stat st; + + if (lstat(drvpath, &st) < 0 || !S_ISLNK(st.st_mode)) + return 0; + else + return 1; +} + +CXL_EXPORT int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev) +{ + struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev); + struct cxl_nvdimm_br *bridge = memdev->bridge; + char *path = memdev->dev_buf; + int len = memdev->buf_len; + + if (!bridge) + return 0; + + if (snprintf(path, len, "%s/driver", bridge->dev_path) >= len) { + err(ctx, "%s: nvdimm bridge buffer too small!\n", + cxl_memdev_get_devname(memdev)); + return 0; + } + + return is_enabled(path); +} + CXL_EXPORT void cxl_cmd_unref(struct cxl_cmd *cmd) { if (!cmd) diff --git a/cxl/libcxl.h b/cxl/libcxl.h index d3b97a1..535e349 100644 --- a/cxl/libcxl.h +++ b/cxl/libcxl.h @@ -43,6 +43,7 @@ unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev); unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev); const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev); size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev); +int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev); #define cxl_memdev_foreach(ctx, memdev) \ for (memdev = cxl_memdev_get_first(ctx); \ diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym index 858e953..f3b0c63 100644 --- a/cxl/lib/libcxl.sym +++ b/cxl/lib/libcxl.sym @@ -65,6 +65,7 @@ global: cxl_cmd_new_read_label; cxl_cmd_read_label_get_payload; cxl_memdev_get_label_size; + cxl_memdev_nvdimm_bridge_active; local: *; };
Add an nvdimm bridge object representation internal to libcxl. A bridge object is ited to its parent memdev object, and this patch adds its first interface, which checks whether a bridge is 'active' - i.e. implying the label space on the memdev is owned by the kernel. Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- cxl/lib/private.h | 9 ++++++ cxl/lib/libcxl.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ cxl/libcxl.h | 1 + cxl/lib/libcxl.sym | 1 + 4 files changed, 84 insertions(+)