Jeff Moyer wrote:
> Dan Williams <dan.j.willi...@intel.com> writes:
>
> > Dave Jiang wrote:
> >> Add a CXL bus type, and detect whether a 'dimm' is backed by the CXL
> >> subsystem.
> >>
> >> Reviewed-by: Alison Schofield <alison.schofi...@intel.com>
> >> Signed-off-by: Dave Jiang <dave.ji...@intel.com>
> >>
> >> ---
> >> v2:
> >> - Improve commit log. (Vishal)
> >> ---
> >> ndctl/lib/libndctl.c | 53
> >> ++++++++++++++++++++++++++++++++++++++++++++++++
> >> ndctl/lib/libndctl.sym | 1 +
> >> ndctl/lib/private.h | 1 +
> >> ndctl/libndctl.h | 1 +
> >> 4 files changed, 56 insertions(+)
> >>
> >> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> >> index ad54f0626510..10422e24d38b 100644
> >> --- a/ndctl/lib/libndctl.c
> >> +++ b/ndctl/lib/libndctl.c
> >> @@ -12,6 +12,7 @@
> >> #include <ctype.h>
> >> #include <fcntl.h>
> >> #include <dirent.h>
> >> +#include <libgen.h>
> >
> > This new include had me looking for why below...
>
> man 3 basename
>
> >> #include <sys/stat.h>
> >> #include <sys/types.h>
> >> #include <sys/ioctl.h>
> >> @@ -876,6 +877,48 @@ static enum ndctl_fwa_method
> >> fwa_method_to_method(const char *fwa_method)
> >> return NDCTL_FWA_METHOD_RESET;
> >> }
> >>
> >> +static int is_ndbus_cxl(const char *ctl_base)
> >> +{
> >> + char *path, *ppath, *subsys;
> >> + char tmp_path[PATH_MAX];
> >> + int rc;
> >> +
> >> + /* get the real path of ctl_base */
> >> + path = realpath(ctl_base, NULL);
> >> + if (!path)
> >> + return -errno;
> >> +
> >> + /* setup to get the nd bridge device backing the ctl */
> >> + sprintf(tmp_path, "%s/device", path);
> >> + free(path);
> >> +
> >> + path = realpath(tmp_path, NULL);
> >> + if (!path)
> >> + return -errno;
> >> +
> >> + /* get the parent dir of the ndbus, which should be the nvdimm-bridge */
> >> + ppath = dirname(path);
> >> +
> >> + /* setup to get the subsystem of the nvdimm-bridge */
> >> + sprintf(tmp_path, "%s/%s", ppath, "subsystem");
> >> + free(path);
> >> +
> >> + path = realpath(tmp_path, NULL);
> >> + if (!path)
> >> + return -errno;
> >> +
> >> + subsys = basename(path);
> >> +
> >> + /* check if subsystem is cxl */
> >> + if (!strcmp(subsys, "cxl"))
> >> + rc = 1;
> >> + else
> >> + rc = 0;
> >> +
> >> + free(path);
> >> + return rc;
> >> +}
> >> +
> >> static void *add_bus(void *parent, int id, const char *ctl_base)
> >> {
> >> char buf[SYSFS_ATTR_SIZE];
> >> @@ -919,6 +962,11 @@ static void *add_bus(void *parent, int id, const char
> >> *ctl_base)
> >> else
> >> bus->has_of_node = 1;
> >>
> >> + if (is_ndbus_cxl(ctl_base))
> >> + bus->has_cxl = 1;
> >> + else
> >> + bus->has_cxl = 0;
> >> +
> >
> > I think you can drop is_ndbus_cxl() and just do this:
> >
> > @@ -981,6 +976,11 @@ static void *add_bus(void *parent, int id, const char
> > *ctl_base)
> > if (!bus->provider)
> > goto err_read;
> >
> > + if (strcasestr("cxl", provider))
> > + bus->has_cxl = 1;
> > + else
> > + bus->has_cxl = 0;
> > +
>
> Can you explain why this is preferred?
Just less code to achieve a similar result. I do like the precision of
looking at the subsytem of bus device parent, just not the multiple
calls to realpath() with dirname() and basename() thrown in which struck
me as unnecessary. How about this:
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index c569178b9a3a..76bd7167bc70 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -877,40 +877,16 @@ static enum ndctl_fwa_method fwa_method_to_method(const
char *fwa_method)
return NDCTL_FWA_METHOD_RESET;
}
-static int is_ndbus_cxl(const char *ctl_base)
+static int is_subsys_cxl(const char *subsys)
{
- char *path, *ppath, *subsys;
- char tmp_path[PATH_MAX];
+ char *path;
int rc;
- /* get the real path of ctl_base */
- path = realpath(ctl_base, NULL);
+ path = realpath(subsys, NULL);
if (!path)
return -errno;
- /* setup to get the nd bridge device backing the ctl */
- sprintf(tmp_path, "%s/device", path);
- free(path);
-
- path = realpath(tmp_path, NULL);
- if (!path)
- return -errno;
-
- /* get the parent dir of the ndbus, which should be the nvdimm-bridge */
- ppath = dirname(path);
-
- /* setup to get the subsystem of the nvdimm-bridge */
- sprintf(tmp_path, "%s/%s", ppath, "subsystem");
- free(path);
-
- path = realpath(tmp_path, NULL);
- if (!path)
- return -errno;
-
- subsys = basename(path);
-
- /* check if subsystem is cxl */
- if (!strcmp(subsys, "cxl"))
+ if (!strcmp(subsys, "/sys/bus/cxl"))
rc = 1;
else
rc = 0;
@@ -962,7 +938,8 @@ static void *add_bus(void *parent, int id, const char
*ctl_base)
else
bus->has_of_node = 1;
- if (is_ndbus_cxl(ctl_base))
+ sprintf(path, "%s/device/../subsys", ctl_base);
+ if (is_subsys_cxl(path))
bus->has_cxl = 1;
else
bus->has_cxl = 0;
