| Message ID | 20160913174140.10810-3-brian.boylston@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 13, 2016 at 10:41 AM, Brian Boylston <brian.boylston@hpe.com> wrote: > The recorded DSM family can be used to provide family-specific > functionality. > > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Brian Boylston <brian.boylston@hpe.com> > --- > ndctl/lib/libndctl.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c > index 9a9dc74..c824d83 100644 > --- a/ndctl/lib/libndctl.c > +++ b/ndctl/lib/libndctl.c > @@ -136,6 +136,7 @@ struct ndctl_dimm { > unsigned short subsystem_revision_id; > unsigned short manufacturing_date; > unsigned char manufacturing_location; > + unsigned long dsm_family; > unsigned long dsm_mask; > char *unique_id; > char *dimm_path; > @@ -1145,6 +1146,11 @@ static int add_dimm(void *parent, int id, const char *dimm_base) > if (sscanf(buf, "%d:%d", &dimm->major, &dimm->minor) != 2) > goto err_read; > > + sprintf(path, "%s/nfit/family", dimm_base); > + if (sysfs_read_attr(ctx, path, buf) < 0) > + goto err_read; > + dimm->dsm_family = strtoul(buf, NULL, 0); Hmm I don't think it should it be fatal to add_dimm() for a failure to have a family. It's reasonable for a DIMM to exist in the NFIT, but have no DSMs or a DSM implementation that is new to the kernel. Can we make this continue on and teach consumers of ->dsm_family that it might be an error value?
Dan Williams wrote on 2016-09-14: > On Tue, Sep 13, 2016 at 10:41 AM, Brian Boylston <brian.boylston@hpe.com> wrote: >> The recorded DSM family can be used to provide family-specific >> functionality. >> >> Cc: Dan Williams <dan.j.williams@intel.com> >> Signed-off-by: Brian Boylston <brian.boylston@hpe.com> >> --- >> ndctl/lib/libndctl.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c >> index 9a9dc74..c824d83 100644 >> --- a/ndctl/lib/libndctl.c >> +++ b/ndctl/lib/libndctl.c >> @@ -136,6 +136,7 @@ struct ndctl_dimm { >> unsigned short subsystem_revision_id; >> unsigned short manufacturing_date; >> unsigned char manufacturing_location; >> + unsigned long dsm_family; >> unsigned long dsm_mask; >> char *unique_id; >> char *dimm_path; >> @@ -1145,6 +1146,11 @@ static int add_dimm(void *parent, int id, const char *dimm_base) >> if (sscanf(buf, "%d:%d", &dimm->major, &dimm->minor) != 2) >> goto err_read; >> >> + sprintf(path, "%s/nfit/family", dimm_base); >> + if (sysfs_read_attr(ctx, path, buf) < 0) >> + goto err_read; >> + dimm->dsm_family = strtoul(buf, NULL, 0); > > > Hmm I don't think it should it be fatal to add_dimm() for a failure to > have a family. It's reasonable for a DIMM to exist in the NFIT, but > have no DSMs or a DSM implementation that is new to the kernel. Can > we make this continue on and teach consumers of ->dsm_family that it > might be an error value? Sure. Some of the other values collected in add_dimm() use -1 as an error or not available value; ok to use that for dsm_family? Also, should I add a public accessor function such as ndctl_dimm_get_dsm_family()? (although there wouldn't be a consumer yet) Thanks! Brian
On Thu, Sep 15, 2016 at 11:17 AM, Boylston, Brian <brian.boylston@hpe.com> wrote: > Dan Williams wrote on 2016-09-14: >> On Tue, Sep 13, 2016 at 10:41 AM, Brian Boylston <brian.boylston@hpe.com> wrote: >>> The recorded DSM family can be used to provide family-specific >>> functionality. >>> >>> Cc: Dan Williams <dan.j.williams@intel.com> >>> Signed-off-by: Brian Boylston <brian.boylston@hpe.com> >>> --- >>> ndctl/lib/libndctl.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c >>> index 9a9dc74..c824d83 100644 >>> --- a/ndctl/lib/libndctl.c >>> +++ b/ndctl/lib/libndctl.c >>> @@ -136,6 +136,7 @@ struct ndctl_dimm { >>> unsigned short subsystem_revision_id; >>> unsigned short manufacturing_date; >>> unsigned char manufacturing_location; >>> + unsigned long dsm_family; >>> unsigned long dsm_mask; >>> char *unique_id; >>> char *dimm_path; >>> @@ -1145,6 +1146,11 @@ static int add_dimm(void *parent, int id, const char *dimm_base) >>> if (sscanf(buf, "%d:%d", &dimm->major, &dimm->minor) != 2) >>> goto err_read; >>> >>> + sprintf(path, "%s/nfit/family", dimm_base); >>> + if (sysfs_read_attr(ctx, path, buf) < 0) >>> + goto err_read; >>> + dimm->dsm_family = strtoul(buf, NULL, 0); >> >> >> Hmm I don't think it should it be fatal to add_dimm() for a failure to >> have a family. It's reasonable for a DIMM to exist in the NFIT, but >> have no DSMs or a DSM implementation that is new to the kernel. Can >> we make this continue on and teach consumers of ->dsm_family that it >> might be an error value? > > Sure. Some of the other values collected in add_dimm() use -1 as an > error or not available value; ok to use that for dsm_family? Sounds good. > Also, should I add a public accessor function such as > ndctl_dimm_get_dsm_family()? (although there wouldn't be a consumer yet) I'm not opposed to it for informational purposes, but I like the current direction where it doesn't really matter and the other ndctl routines are taught how to multiplex for different families.
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index 9a9dc74..c824d83 100644 --- a/ndctl/lib/libndctl.c +++ b/ndctl/lib/libndctl.c @@ -136,6 +136,7 @@ struct ndctl_dimm { unsigned short subsystem_revision_id; unsigned short manufacturing_date; unsigned char manufacturing_location; + unsigned long dsm_family; unsigned long dsm_mask; char *unique_id; char *dimm_path; @@ -1145,6 +1146,11 @@ static int add_dimm(void *parent, int id, const char *dimm_base) if (sscanf(buf, "%d:%d", &dimm->major, &dimm->minor) != 2) goto err_read; + sprintf(path, "%s/nfit/family", dimm_base); + if (sysfs_read_attr(ctx, path, buf) < 0) + goto err_read; + dimm->dsm_family = strtoul(buf, NULL, 0); + sprintf(path, "%s/commands", dimm_base); if (sysfs_read_attr(ctx, path, buf) < 0) goto err_read;
The recorded DSM family can be used to provide family-specific functionality. Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Brian Boylston <brian.boylston@hpe.com> --- ndctl/lib/libndctl.c | 6 ++++++ 1 file changed, 6 insertions(+)