* [NDCTL PATCH] cxl: Add helper function to verify port is in memdev hierarchy @ 2025-06-18 20:41 Dave Jiang 2025-06-18 21:54 ` Alison Schofield 0 siblings, 1 reply; 6+ messages in thread From: Dave Jiang @ 2025-06-18 20:41 UTC (permalink / raw) To: linux-cxl, nvdimm; +Cc: alison.schofield 'cxl enable-port -m' uses cxl_port_get_dport_by_memdev() to find the memdevs that are associated with a port in order to enable those associated memdevs. When the kernel switch to delayed dport initialization by enumerating the dports during memdev probe, the dports are no longer valid until the memdev is probed. This means that cxl_port_get_dport_by_memdev() will not find any memdevs under the port. Add a new helper function cxl_port_is_memdev_hierarchy() that checks if a port is in the memdev hierarchy via the memdev->host_path where the sysfs path contains all the devices in the hierarchy. This call is also backward compatible with the old behavior. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- cxl/lib/libcxl.c | 31 +++++++++++++++++++++++++++++++ cxl/lib/libcxl.sym | 5 +++++ cxl/libcxl.h | 3 +++ cxl/port.c | 2 +- 4 files changed, 40 insertions(+), 1 deletion(-) diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c index 5d97023377ec..cafde1cee4e8 100644 --- a/cxl/lib/libcxl.c +++ b/cxl/lib/libcxl.c @@ -2024,6 +2024,37 @@ CXL_EXPORT int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev) return is_enabled(path); } +CXL_EXPORT bool cxl_memdev_is_port_ancestor(struct cxl_memdev *memdev, + struct cxl_port *port) +{ + const char *uport = cxl_port_get_host(port); + const char *start = "devices"; + const char *pstr = "platform"; + char *host, *pos; + + host = strdup(memdev->host_path); + if (!host) + return false; + + pos = strstr(host, start); + pos += strlen(start) + 1; + if (strncmp(pos, pstr, strlen(pstr)) == 0) + pos += strlen(pstr) + 1; + pos = strtok(pos, "/"); + + while (pos) { + if (strcmp(pos, uport) == 0) { + free(host); + return true; + } + pos = strtok(NULL, "/"); + } + + free(host); + + return false; +} + static int cxl_port_init(struct cxl_port *port, struct cxl_port *parent_port, enum cxl_port_type type, struct cxl_ctx *ctx, int id, const char *cxlport_base) diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym index 3ad0cd06e25a..e01a676cdeb9 100644 --- a/cxl/lib/libcxl.sym +++ b/cxl/lib/libcxl.sym @@ -295,3 +295,8 @@ global: cxl_fwctl_get_major; cxl_fwctl_get_minor; } LIBECXL_8; + +LIBCXL_10 { +global: + cxl_memdev_is_port_ancestor; +} LIBCXL_9; diff --git a/cxl/libcxl.h b/cxl/libcxl.h index 54d97d7bb501..54bc025b121d 100644 --- a/cxl/libcxl.h +++ b/cxl/libcxl.h @@ -179,6 +179,9 @@ bool cxl_dport_maps_memdev(struct cxl_dport *dport, struct cxl_memdev *memdev); struct cxl_dport *cxl_port_get_dport_by_memdev(struct cxl_port *port, struct cxl_memdev *memdev); +bool cxl_memdev_is_port_ancestor(struct cxl_memdev *memdev, + struct cxl_port *port); + #define cxl_dport_foreach(port, dport) \ for (dport = cxl_dport_get_first(port); dport != NULL; \ dport = cxl_dport_get_next(dport)) diff --git a/cxl/port.c b/cxl/port.c index 89f3916d85aa..c951c0c771e8 100644 --- a/cxl/port.c +++ b/cxl/port.c @@ -102,7 +102,7 @@ static int action_enable(struct cxl_port *port) return rc; cxl_memdev_foreach(ctx, memdev) - if (cxl_port_get_dport_by_memdev(port, memdev)) + if (cxl_memdev_is_port_ancestor(memdev, port)) cxl_memdev_enable(memdev); return 0; } base-commit: 74b9e411bf13e87df39a517d10143fafa7e2ea92 -- 2.49.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [NDCTL PATCH] cxl: Add helper function to verify port is in memdev hierarchy 2025-06-18 20:41 [NDCTL PATCH] cxl: Add helper function to verify port is in memdev hierarchy Dave Jiang @ 2025-06-18 21:54 ` Alison Schofield 2025-06-18 21:58 ` Dave Jiang 0 siblings, 1 reply; 6+ messages in thread From: Alison Schofield @ 2025-06-18 21:54 UTC (permalink / raw) To: Dave Jiang; +Cc: linux-cxl, nvdimm On Wed, Jun 18, 2025 at 01:41:17PM -0700, Dave Jiang wrote: > 'cxl enable-port -m' uses cxl_port_get_dport_by_memdev() to find the > memdevs that are associated with a port in order to enable those > associated memdevs. When the kernel switch to delayed dport > initialization by enumerating the dports during memdev probe, the > dports are no longer valid until the memdev is probed. This means > that cxl_port_get_dport_by_memdev() will not find any memdevs under > the port. > > Add a new helper function cxl_port_is_memdev_hierarchy() that checks if a > port is in the memdev hierarchy via the memdev->host_path where the sysfs > path contains all the devices in the hierarchy. This call is also backward > compatible with the old behavior. I get how this new function works w the delayed dport init that is coming soon to the CXL driver. I'm not so clear on why we leave the existing function in place when we know it will fail in some use cases. (It is a libcxl fcn afterall) Why not change the behavior of the existing function? How come this usage of cxl_port_get_dport_by_memdev() needs to change to the new helper and not the other usage in action_disable()? If the 'sometimes fails to find' function stays, how about libcxl docs explaining the limitations. Just stirring the pot to better understand ;) --Alison > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > cxl/lib/libcxl.c | 31 +++++++++++++++++++++++++++++++ > cxl/lib/libcxl.sym | 5 +++++ > cxl/libcxl.h | 3 +++ > cxl/port.c | 2 +- > 4 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c > index 5d97023377ec..cafde1cee4e8 100644 > --- a/cxl/lib/libcxl.c > +++ b/cxl/lib/libcxl.c > @@ -2024,6 +2024,37 @@ CXL_EXPORT int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev) > return is_enabled(path); > } > > +CXL_EXPORT bool cxl_memdev_is_port_ancestor(struct cxl_memdev *memdev, > + struct cxl_port *port) > +{ > + const char *uport = cxl_port_get_host(port); > + const char *start = "devices"; > + const char *pstr = "platform"; > + char *host, *pos; > + > + host = strdup(memdev->host_path); > + if (!host) > + return false; > + > + pos = strstr(host, start); > + pos += strlen(start) + 1; > + if (strncmp(pos, pstr, strlen(pstr)) == 0) > + pos += strlen(pstr) + 1; > + pos = strtok(pos, "/"); > + > + while (pos) { > + if (strcmp(pos, uport) == 0) { > + free(host); > + return true; > + } > + pos = strtok(NULL, "/"); > + } > + > + free(host); > + > + return false; > +} > + > static int cxl_port_init(struct cxl_port *port, struct cxl_port *parent_port, > enum cxl_port_type type, struct cxl_ctx *ctx, int id, > const char *cxlport_base) > diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym > index 3ad0cd06e25a..e01a676cdeb9 100644 > --- a/cxl/lib/libcxl.sym > +++ b/cxl/lib/libcxl.sym > @@ -295,3 +295,8 @@ global: > cxl_fwctl_get_major; > cxl_fwctl_get_minor; > } LIBECXL_8; > + > +LIBCXL_10 { > +global: > + cxl_memdev_is_port_ancestor; > +} LIBCXL_9; > diff --git a/cxl/libcxl.h b/cxl/libcxl.h > index 54d97d7bb501..54bc025b121d 100644 > --- a/cxl/libcxl.h > +++ b/cxl/libcxl.h > @@ -179,6 +179,9 @@ bool cxl_dport_maps_memdev(struct cxl_dport *dport, struct cxl_memdev *memdev); > struct cxl_dport *cxl_port_get_dport_by_memdev(struct cxl_port *port, > struct cxl_memdev *memdev); > > +bool cxl_memdev_is_port_ancestor(struct cxl_memdev *memdev, > + struct cxl_port *port); > + > #define cxl_dport_foreach(port, dport) \ > for (dport = cxl_dport_get_first(port); dport != NULL; \ > dport = cxl_dport_get_next(dport)) > diff --git a/cxl/port.c b/cxl/port.c > index 89f3916d85aa..c951c0c771e8 100644 > --- a/cxl/port.c > +++ b/cxl/port.c > @@ -102,7 +102,7 @@ static int action_enable(struct cxl_port *port) > return rc; > > cxl_memdev_foreach(ctx, memdev) > - if (cxl_port_get_dport_by_memdev(port, memdev)) > + if (cxl_memdev_is_port_ancestor(memdev, port)) > cxl_memdev_enable(memdev); > return 0; > } > > base-commit: 74b9e411bf13e87df39a517d10143fafa7e2ea92 > -- > 2.49.0 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [NDCTL PATCH] cxl: Add helper function to verify port is in memdev hierarchy 2025-06-18 21:54 ` Alison Schofield @ 2025-06-18 21:58 ` Dave Jiang 2025-06-18 22:45 ` Alison Schofield 0 siblings, 1 reply; 6+ messages in thread From: Dave Jiang @ 2025-06-18 21:58 UTC (permalink / raw) To: Alison Schofield; +Cc: linux-cxl, nvdimm On 6/18/25 2:54 PM, Alison Schofield wrote: > On Wed, Jun 18, 2025 at 01:41:17PM -0700, Dave Jiang wrote: >> 'cxl enable-port -m' uses cxl_port_get_dport_by_memdev() to find the >> memdevs that are associated with a port in order to enable those >> associated memdevs. When the kernel switch to delayed dport >> initialization by enumerating the dports during memdev probe, the >> dports are no longer valid until the memdev is probed. This means >> that cxl_port_get_dport_by_memdev() will not find any memdevs under >> the port. >> >> Add a new helper function cxl_port_is_memdev_hierarchy() that checks if a >> port is in the memdev hierarchy via the memdev->host_path where the sysfs >> path contains all the devices in the hierarchy. This call is also backward >> compatible with the old behavior. > > I get how this new function works w the delayed dport init that is > coming soon to the CXL driver. I'm not so clear on why we leave the > existing function in place when we know it will fail in some use > cases. (It is a libcxl fcn afterall) > > Why not change the behavior of the existing function? > How come this usage of cxl_port_get_dport_by_memdev() needs to change > to the new helper and not the other usage in action_disable()? > > If the 'sometimes fails to find' function stays, how about libcxl > docs explaining the limitations. > > Just stirring the pot to better understand ;) What's the process of retiring API calls? Add deprecated in the doc? Add warnings when called? > > --Alison > > >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> cxl/lib/libcxl.c | 31 +++++++++++++++++++++++++++++++ >> cxl/lib/libcxl.sym | 5 +++++ >> cxl/libcxl.h | 3 +++ >> cxl/port.c | 2 +- >> 4 files changed, 40 insertions(+), 1 deletion(-) >> >> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c >> index 5d97023377ec..cafde1cee4e8 100644 >> --- a/cxl/lib/libcxl.c >> +++ b/cxl/lib/libcxl.c >> @@ -2024,6 +2024,37 @@ CXL_EXPORT int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev) >> return is_enabled(path); >> } >> >> +CXL_EXPORT bool cxl_memdev_is_port_ancestor(struct cxl_memdev *memdev, >> + struct cxl_port *port) >> +{ >> + const char *uport = cxl_port_get_host(port); >> + const char *start = "devices"; >> + const char *pstr = "platform"; >> + char *host, *pos; >> + >> + host = strdup(memdev->host_path); >> + if (!host) >> + return false; >> + >> + pos = strstr(host, start); >> + pos += strlen(start) + 1; >> + if (strncmp(pos, pstr, strlen(pstr)) == 0) >> + pos += strlen(pstr) + 1; >> + pos = strtok(pos, "/"); >> + >> + while (pos) { >> + if (strcmp(pos, uport) == 0) { >> + free(host); >> + return true; >> + } >> + pos = strtok(NULL, "/"); >> + } >> + >> + free(host); >> + >> + return false; >> +} >> + >> static int cxl_port_init(struct cxl_port *port, struct cxl_port *parent_port, >> enum cxl_port_type type, struct cxl_ctx *ctx, int id, >> const char *cxlport_base) >> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym >> index 3ad0cd06e25a..e01a676cdeb9 100644 >> --- a/cxl/lib/libcxl.sym >> +++ b/cxl/lib/libcxl.sym >> @@ -295,3 +295,8 @@ global: >> cxl_fwctl_get_major; >> cxl_fwctl_get_minor; >> } LIBECXL_8; >> + >> +LIBCXL_10 { >> +global: >> + cxl_memdev_is_port_ancestor; >> +} LIBCXL_9; >> diff --git a/cxl/libcxl.h b/cxl/libcxl.h >> index 54d97d7bb501..54bc025b121d 100644 >> --- a/cxl/libcxl.h >> +++ b/cxl/libcxl.h >> @@ -179,6 +179,9 @@ bool cxl_dport_maps_memdev(struct cxl_dport *dport, struct cxl_memdev *memdev); >> struct cxl_dport *cxl_port_get_dport_by_memdev(struct cxl_port *port, >> struct cxl_memdev *memdev); >> >> +bool cxl_memdev_is_port_ancestor(struct cxl_memdev *memdev, >> + struct cxl_port *port); >> + >> #define cxl_dport_foreach(port, dport) \ >> for (dport = cxl_dport_get_first(port); dport != NULL; \ >> dport = cxl_dport_get_next(dport)) >> diff --git a/cxl/port.c b/cxl/port.c >> index 89f3916d85aa..c951c0c771e8 100644 >> --- a/cxl/port.c >> +++ b/cxl/port.c >> @@ -102,7 +102,7 @@ static int action_enable(struct cxl_port *port) >> return rc; >> >> cxl_memdev_foreach(ctx, memdev) >> - if (cxl_port_get_dport_by_memdev(port, memdev)) >> + if (cxl_memdev_is_port_ancestor(memdev, port)) >> cxl_memdev_enable(memdev); >> return 0; >> } >> >> base-commit: 74b9e411bf13e87df39a517d10143fafa7e2ea92 >> -- >> 2.49.0 >> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [NDCTL PATCH] cxl: Add helper function to verify port is in memdev hierarchy 2025-06-18 21:58 ` Dave Jiang @ 2025-06-18 22:45 ` Alison Schofield 2025-06-18 22:50 ` Dave Jiang 0 siblings, 1 reply; 6+ messages in thread From: Alison Schofield @ 2025-06-18 22:45 UTC (permalink / raw) To: Dave Jiang; +Cc: linux-cxl, nvdimm On Wed, Jun 18, 2025 at 02:58:13PM -0700, Dave Jiang wrote: > > > On 6/18/25 2:54 PM, Alison Schofield wrote: > > On Wed, Jun 18, 2025 at 01:41:17PM -0700, Dave Jiang wrote: > >> 'cxl enable-port -m' uses cxl_port_get_dport_by_memdev() to find the > >> memdevs that are associated with a port in order to enable those > >> associated memdevs. When the kernel switch to delayed dport > >> initialization by enumerating the dports during memdev probe, the > >> dports are no longer valid until the memdev is probed. This means > >> that cxl_port_get_dport_by_memdev() will not find any memdevs under > >> the port. > >> > >> Add a new helper function cxl_port_is_memdev_hierarchy() that checks if a > >> port is in the memdev hierarchy via the memdev->host_path where the sysfs > >> path contains all the devices in the hierarchy. This call is also backward > >> compatible with the old behavior. > > > > I get how this new function works w the delayed dport init that is > > coming soon to the CXL driver. I'm not so clear on why we leave the > > existing function in place when we know it will fail in some use > > cases. (It is a libcxl fcn afterall) > > > > Why not change the behavior of the existing function? > > How come this usage of cxl_port_get_dport_by_memdev() needs to change > > to the new helper and not the other usage in action_disable()? > > > > If the 'sometimes fails to find' function stays, how about libcxl > > docs explaining the limitations. > > > > Just stirring the pot to better understand ;) > > What's the process of retiring API calls? Add deprecated in the doc? Add warnings when called? What is wanted here? Should a v2 of the existing cxl_port_get_dport... be replaced with a v2 that can differentiate btw memdev not probed vs NULL for dport not found. I see example of v2 APIs in ndctl/ndctl lib, so doable, but first need to define what is wanted. > > > > > --Alison > > > > > >> > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > >> --- > >> cxl/lib/libcxl.c | 31 +++++++++++++++++++++++++++++++ > >> cxl/lib/libcxl.sym | 5 +++++ > >> cxl/libcxl.h | 3 +++ > >> cxl/port.c | 2 +- > >> 4 files changed, 40 insertions(+), 1 deletion(-) > >> > >> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c > >> index 5d97023377ec..cafde1cee4e8 100644 > >> --- a/cxl/lib/libcxl.c > >> +++ b/cxl/lib/libcxl.c > >> @@ -2024,6 +2024,37 @@ CXL_EXPORT int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev) > >> return is_enabled(path); > >> } > >> > >> +CXL_EXPORT bool cxl_memdev_is_port_ancestor(struct cxl_memdev *memdev, > >> + struct cxl_port *port) > >> +{ > >> + const char *uport = cxl_port_get_host(port); > >> + const char *start = "devices"; > >> + const char *pstr = "platform"; > >> + char *host, *pos; > >> + > >> + host = strdup(memdev->host_path); > >> + if (!host) > >> + return false; > >> + > >> + pos = strstr(host, start); > >> + pos += strlen(start) + 1; > >> + if (strncmp(pos, pstr, strlen(pstr)) == 0) > >> + pos += strlen(pstr) + 1; > >> + pos = strtok(pos, "/"); > >> + > >> + while (pos) { > >> + if (strcmp(pos, uport) == 0) { > >> + free(host); > >> + return true; > >> + } > >> + pos = strtok(NULL, "/"); > >> + } > >> + > >> + free(host); > >> + > >> + return false; > >> +} > >> + > >> static int cxl_port_init(struct cxl_port *port, struct cxl_port *parent_port, > >> enum cxl_port_type type, struct cxl_ctx *ctx, int id, > >> const char *cxlport_base) > >> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym > >> index 3ad0cd06e25a..e01a676cdeb9 100644 > >> --- a/cxl/lib/libcxl.sym > >> +++ b/cxl/lib/libcxl.sym > >> @@ -295,3 +295,8 @@ global: > >> cxl_fwctl_get_major; > >> cxl_fwctl_get_minor; > >> } LIBECXL_8; > >> + > >> +LIBCXL_10 { > >> +global: > >> + cxl_memdev_is_port_ancestor; > >> +} LIBCXL_9; > >> diff --git a/cxl/libcxl.h b/cxl/libcxl.h > >> index 54d97d7bb501..54bc025b121d 100644 > >> --- a/cxl/libcxl.h > >> +++ b/cxl/libcxl.h > >> @@ -179,6 +179,9 @@ bool cxl_dport_maps_memdev(struct cxl_dport *dport, struct cxl_memdev *memdev); > >> struct cxl_dport *cxl_port_get_dport_by_memdev(struct cxl_port *port, > >> struct cxl_memdev *memdev); > >> > >> +bool cxl_memdev_is_port_ancestor(struct cxl_memdev *memdev, > >> + struct cxl_port *port); > >> + > >> #define cxl_dport_foreach(port, dport) \ > >> for (dport = cxl_dport_get_first(port); dport != NULL; \ > >> dport = cxl_dport_get_next(dport)) > >> diff --git a/cxl/port.c b/cxl/port.c > >> index 89f3916d85aa..c951c0c771e8 100644 > >> --- a/cxl/port.c > >> +++ b/cxl/port.c > >> @@ -102,7 +102,7 @@ static int action_enable(struct cxl_port *port) > >> return rc; > >> > >> cxl_memdev_foreach(ctx, memdev) > >> - if (cxl_port_get_dport_by_memdev(port, memdev)) > >> + if (cxl_memdev_is_port_ancestor(memdev, port)) > >> cxl_memdev_enable(memdev); > >> return 0; > >> } > >> > >> base-commit: 74b9e411bf13e87df39a517d10143fafa7e2ea92 > >> -- > >> 2.49.0 > >> > >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [NDCTL PATCH] cxl: Add helper function to verify port is in memdev hierarchy 2025-06-18 22:45 ` Alison Schofield @ 2025-06-18 22:50 ` Dave Jiang 2025-07-04 0:17 ` Alison Schofield 0 siblings, 1 reply; 6+ messages in thread From: Dave Jiang @ 2025-06-18 22:50 UTC (permalink / raw) To: Alison Schofield; +Cc: linux-cxl, nvdimm On 6/18/25 3:45 PM, Alison Schofield wrote: > On Wed, Jun 18, 2025 at 02:58:13PM -0700, Dave Jiang wrote: >> >> >> On 6/18/25 2:54 PM, Alison Schofield wrote: >>> On Wed, Jun 18, 2025 at 01:41:17PM -0700, Dave Jiang wrote: >>>> 'cxl enable-port -m' uses cxl_port_get_dport_by_memdev() to find the >>>> memdevs that are associated with a port in order to enable those >>>> associated memdevs. When the kernel switch to delayed dport >>>> initialization by enumerating the dports during memdev probe, the >>>> dports are no longer valid until the memdev is probed. This means >>>> that cxl_port_get_dport_by_memdev() will not find any memdevs under >>>> the port. >>>> >>>> Add a new helper function cxl_port_is_memdev_hierarchy() that checks if a >>>> port is in the memdev hierarchy via the memdev->host_path where the sysfs >>>> path contains all the devices in the hierarchy. This call is also backward >>>> compatible with the old behavior. >>> >>> I get how this new function works w the delayed dport init that is >>> coming soon to the CXL driver. I'm not so clear on why we leave the >>> existing function in place when we know it will fail in some use >>> cases. (It is a libcxl fcn afterall) >>> >>> Why not change the behavior of the existing function? >>> How come this usage of cxl_port_get_dport_by_memdev() needs to change >>> to the new helper and not the other usage in action_disable()? >>> >>> If the 'sometimes fails to find' function stays, how about libcxl >>> docs explaining the limitations. >>> >>> Just stirring the pot to better understand ;) >> >> What's the process of retiring API calls? Add deprecated in the doc? Add warnings when called? > > What is wanted here? Should a v2 of the existing cxl_port_get_dport... > be replaced with a v2 that can differentiate btw memdev not probed vs > NULL for dport not found. > > I see example of v2 APIs in ndctl/ndctl lib, so doable, but first need > to define what is wanted. So there are 2 locations using cxl_port_get_dport_by_memdev(). If the usage in cxl/filter.c can be replaced by the new function, then we can drop the call entirely for CXL CLI. Of course if someone else is using this function in their app, then we would be breaking their app. I wonder if we need to add documentation to explain where it would fail and emit warning of deprecation in the next release? > >> >>> >>> --Alison >>> >>> >>>> >>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >>>> --- >>>> cxl/lib/libcxl.c | 31 +++++++++++++++++++++++++++++++ >>>> cxl/lib/libcxl.sym | 5 +++++ >>>> cxl/libcxl.h | 3 +++ >>>> cxl/port.c | 2 +- >>>> 4 files changed, 40 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c >>>> index 5d97023377ec..cafde1cee4e8 100644 >>>> --- a/cxl/lib/libcxl.c >>>> +++ b/cxl/lib/libcxl.c >>>> @@ -2024,6 +2024,37 @@ CXL_EXPORT int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev) >>>> return is_enabled(path); >>>> } >>>> >>>> +CXL_EXPORT bool cxl_memdev_is_port_ancestor(struct cxl_memdev *memdev, >>>> + struct cxl_port *port) >>>> +{ >>>> + const char *uport = cxl_port_get_host(port); >>>> + const char *start = "devices"; >>>> + const char *pstr = "platform"; >>>> + char *host, *pos; >>>> + >>>> + host = strdup(memdev->host_path); >>>> + if (!host) >>>> + return false; >>>> + >>>> + pos = strstr(host, start); >>>> + pos += strlen(start) + 1; >>>> + if (strncmp(pos, pstr, strlen(pstr)) == 0) >>>> + pos += strlen(pstr) + 1; >>>> + pos = strtok(pos, "/"); >>>> + >>>> + while (pos) { >>>> + if (strcmp(pos, uport) == 0) { >>>> + free(host); >>>> + return true; >>>> + } >>>> + pos = strtok(NULL, "/"); >>>> + } >>>> + >>>> + free(host); >>>> + >>>> + return false; >>>> +} >>>> + >>>> static int cxl_port_init(struct cxl_port *port, struct cxl_port *parent_port, >>>> enum cxl_port_type type, struct cxl_ctx *ctx, int id, >>>> const char *cxlport_base) >>>> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym >>>> index 3ad0cd06e25a..e01a676cdeb9 100644 >>>> --- a/cxl/lib/libcxl.sym >>>> +++ b/cxl/lib/libcxl.sym >>>> @@ -295,3 +295,8 @@ global: >>>> cxl_fwctl_get_major; >>>> cxl_fwctl_get_minor; >>>> } LIBECXL_8; >>>> + >>>> +LIBCXL_10 { >>>> +global: >>>> + cxl_memdev_is_port_ancestor; >>>> +} LIBCXL_9; >>>> diff --git a/cxl/libcxl.h b/cxl/libcxl.h >>>> index 54d97d7bb501..54bc025b121d 100644 >>>> --- a/cxl/libcxl.h >>>> +++ b/cxl/libcxl.h >>>> @@ -179,6 +179,9 @@ bool cxl_dport_maps_memdev(struct cxl_dport *dport, struct cxl_memdev *memdev); >>>> struct cxl_dport *cxl_port_get_dport_by_memdev(struct cxl_port *port, >>>> struct cxl_memdev *memdev); >>>> >>>> +bool cxl_memdev_is_port_ancestor(struct cxl_memdev *memdev, >>>> + struct cxl_port *port); >>>> + >>>> #define cxl_dport_foreach(port, dport) \ >>>> for (dport = cxl_dport_get_first(port); dport != NULL; \ >>>> dport = cxl_dport_get_next(dport)) >>>> diff --git a/cxl/port.c b/cxl/port.c >>>> index 89f3916d85aa..c951c0c771e8 100644 >>>> --- a/cxl/port.c >>>> +++ b/cxl/port.c >>>> @@ -102,7 +102,7 @@ static int action_enable(struct cxl_port *port) >>>> return rc; >>>> >>>> cxl_memdev_foreach(ctx, memdev) >>>> - if (cxl_port_get_dport_by_memdev(port, memdev)) >>>> + if (cxl_memdev_is_port_ancestor(memdev, port)) >>>> cxl_memdev_enable(memdev); >>>> return 0; >>>> } >>>> >>>> base-commit: 74b9e411bf13e87df39a517d10143fafa7e2ea92 >>>> -- >>>> 2.49.0 >>>> >>>> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [NDCTL PATCH] cxl: Add helper function to verify port is in memdev hierarchy 2025-06-18 22:50 ` Dave Jiang @ 2025-07-04 0:17 ` Alison Schofield 0 siblings, 0 replies; 6+ messages in thread From: Alison Schofield @ 2025-07-04 0:17 UTC (permalink / raw) To: Dave Jiang; +Cc: linux-cxl, nvdimm On Wed, Jun 18, 2025 at 03:50:33PM -0700, Dave Jiang wrote: > > > On 6/18/25 3:45 PM, Alison Schofield wrote: > > On Wed, Jun 18, 2025 at 02:58:13PM -0700, Dave Jiang wrote: > >> > >> > >> On 6/18/25 2:54 PM, Alison Schofield wrote: > >>> On Wed, Jun 18, 2025 at 01:41:17PM -0700, Dave Jiang wrote: > >>>> 'cxl enable-port -m' uses cxl_port_get_dport_by_memdev() to find the > >>>> memdevs that are associated with a port in order to enable those > >>>> associated memdevs. When the kernel switch to delayed dport > >>>> initialization by enumerating the dports during memdev probe, the > >>>> dports are no longer valid until the memdev is probed. This means > >>>> that cxl_port_get_dport_by_memdev() will not find any memdevs under > >>>> the port. > >>>> > >>>> Add a new helper function cxl_port_is_memdev_hierarchy() that checks if a > >>>> port is in the memdev hierarchy via the memdev->host_path where the sysfs > >>>> path contains all the devices in the hierarchy. This call is also backward > >>>> compatible with the old behavior. > >>> > >>> I get how this new function works w the delayed dport init that is > >>> coming soon to the CXL driver. I'm not so clear on why we leave the > >>> existing function in place when we know it will fail in some use > >>> cases. (It is a libcxl fcn afterall) > >>> > >>> Why not change the behavior of the existing function? > >>> How come this usage of cxl_port_get_dport_by_memdev() needs to change > >>> to the new helper and not the other usage in action_disable()? > >>> > >>> If the 'sometimes fails to find' function stays, how about libcxl > >>> docs explaining the limitations. > >>> > >>> Just stirring the pot to better understand ;) > >> > >> What's the process of retiring API calls? Add deprecated in the doc? Add warnings when called? > > > > What is wanted here? Should a v2 of the existing cxl_port_get_dport... > > be replaced with a v2 that can differentiate btw memdev not probed vs > > NULL for dport not found. > > > > I see example of v2 APIs in ndctl/ndctl lib, so doable, but first need > > to define what is wanted. > > So there are 2 locations using cxl_port_get_dport_by_memdev(). If the usage in cxl/filter.c can be replaced by the new function, then we can drop the call entirely for CXL CLI. Of course if someone else is using this function in their app, then we would be breaking their app. I wonder if we need to add documentation to explain where it would fail and emit warning of deprecation in the next release? OK, back to this: There was no issue or question about adding the new function that only checks for existence. In fact, neither of the call sites of the old cxl_port_get_dport_by_memdev() use the returned dport - so both can switch to use the new function. What to do about the old? Can we do something like this. I'm thinking we don't need to obsolete the function. We can document this alongside the description of the new function. Not so sure of the is_enabled() in this context. It is just checking for the sysfs symbolic link. cxl_port_get_dport_by_memdev(struct cxl_port *port, struct cxl_memdev *memdev) { struct cxl_dport *dport; if (cxl_memdev_is_enabled(memdev)) { error message return NULL; } cxl_dport_foreach(port, dport) if (cxl_dport_maps_memdev(dport, memdev)) return dport; return NULL; } > > > >> > >>> > >>> --Alison > >>> > >>> > >>>> > >>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > >>>> --- > >>>> cxl/lib/libcxl.c | 31 +++++++++++++++++++++++++++++++ > >>>> cxl/lib/libcxl.sym | 5 +++++ > >>>> cxl/libcxl.h | 3 +++ > >>>> cxl/port.c | 2 +- > >>>> 4 files changed, 40 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c > >>>> index 5d97023377ec..cafde1cee4e8 100644 > >>>> --- a/cxl/lib/libcxl.c > >>>> +++ b/cxl/lib/libcxl.c > >>>> @@ -2024,6 +2024,37 @@ CXL_EXPORT int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev) > >>>> return is_enabled(path); > >>>> } > >>>> > >>>> +CXL_EXPORT bool cxl_memdev_is_port_ancestor(struct cxl_memdev *memdev, > >>>> + struct cxl_port *port) > >>>> +{ > >>>> + const char *uport = cxl_port_get_host(port); > >>>> + const char *start = "devices"; > >>>> + const char *pstr = "platform"; > >>>> + char *host, *pos; > >>>> + > >>>> + host = strdup(memdev->host_path); > >>>> + if (!host) > >>>> + return false; > >>>> + > >>>> + pos = strstr(host, start); > >>>> + pos += strlen(start) + 1; > >>>> + if (strncmp(pos, pstr, strlen(pstr)) == 0) > >>>> + pos += strlen(pstr) + 1; > >>>> + pos = strtok(pos, "/"); > >>>> + > >>>> + while (pos) { > >>>> + if (strcmp(pos, uport) == 0) { > >>>> + free(host); > >>>> + return true; > >>>> + } > >>>> + pos = strtok(NULL, "/"); > >>>> + } > >>>> + > >>>> + free(host); > >>>> + > >>>> + return false; > >>>> +} > >>>> + > >>>> static int cxl_port_init(struct cxl_port *port, struct cxl_port *parent_port, > >>>> enum cxl_port_type type, struct cxl_ctx *ctx, int id, > >>>> const char *cxlport_base) > >>>> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym > >>>> index 3ad0cd06e25a..e01a676cdeb9 100644 > >>>> --- a/cxl/lib/libcxl.sym > >>>> +++ b/cxl/lib/libcxl.sym > >>>> @@ -295,3 +295,8 @@ global: > >>>> cxl_fwctl_get_major; > >>>> cxl_fwctl_get_minor; > >>>> } LIBECXL_8; > >>>> + > >>>> +LIBCXL_10 { > >>>> +global: > >>>> + cxl_memdev_is_port_ancestor; > >>>> +} LIBCXL_9; > >>>> diff --git a/cxl/libcxl.h b/cxl/libcxl.h > >>>> index 54d97d7bb501..54bc025b121d 100644 > >>>> --- a/cxl/libcxl.h > >>>> +++ b/cxl/libcxl.h > >>>> @@ -179,6 +179,9 @@ bool cxl_dport_maps_memdev(struct cxl_dport *dport, struct cxl_memdev *memdev); > >>>> struct cxl_dport *cxl_port_get_dport_by_memdev(struct cxl_port *port, > >>>> struct cxl_memdev *memdev); > >>>> > >>>> +bool cxl_memdev_is_port_ancestor(struct cxl_memdev *memdev, > >>>> + struct cxl_port *port); > >>>> + > >>>> #define cxl_dport_foreach(port, dport) \ > >>>> for (dport = cxl_dport_get_first(port); dport != NULL; \ > >>>> dport = cxl_dport_get_next(dport)) > >>>> diff --git a/cxl/port.c b/cxl/port.c > >>>> index 89f3916d85aa..c951c0c771e8 100644 > >>>> --- a/cxl/port.c > >>>> +++ b/cxl/port.c > >>>> @@ -102,7 +102,7 @@ static int action_enable(struct cxl_port *port) > >>>> return rc; > >>>> > >>>> cxl_memdev_foreach(ctx, memdev) > >>>> - if (cxl_port_get_dport_by_memdev(port, memdev)) > >>>> + if (cxl_memdev_is_port_ancestor(memdev, port)) > >>>> cxl_memdev_enable(memdev); > >>>> return 0; > >>>> } > >>>> > >>>> base-commit: 74b9e411bf13e87df39a517d10143fafa7e2ea92 > >>>> -- > >>>> 2.49.0 > >>>> > >>>> > >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-04 0:17 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-06-18 20:41 [NDCTL PATCH] cxl: Add helper function to verify port is in memdev hierarchy Dave Jiang 2025-06-18 21:54 ` Alison Schofield 2025-06-18 21:58 ` Dave Jiang 2025-06-18 22:45 ` Alison Schofield 2025-06-18 22:50 ` Dave Jiang 2025-07-04 0:17 ` Alison Schofield
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox