Re: [NDCTL PATCH] cxl: Add helper function to verify port is in memdev hierarchy

[Alison Schofield <alison.schofield@intel.com>]

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
> >>
> >>
>