| Message ID | 151925076435.65338.5271742129529173494.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-02-21 at 15:06 -0700, Dave Jiang wrote: > Adding all option to allow ndctl to update all DIMMs at once. I was going to ask that we should also update the documentation for this command to include the new 'all' behavior, but it looks like during the revisions of the original series, the documentation update to list the options explicitly got dropped? Anyway, can you re-add those, and also talk about --dimm=all there. On a side note, should we make this more inline with other dimm commands, such as xable-dimm or read/write-labels, where the dimm name (or 'all') is a non-option argument? That would make it something like: ndctl update-firmware --firmware=<file> nmem0 (or 'all) > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > ndctl/update.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/ndctl/update.c b/ndctl/update.c > index fc26acf..1b771ff 100644 > --- a/ndctl/update.c > +++ b/ndctl/update.c > @@ -77,6 +77,7 @@ struct update_context { > struct ndctl_dimm *dimm; > struct fw_info dimm_fw; > struct ndctl_cmd *start; > + bool all; > }; > > /* > @@ -471,16 +472,23 @@ static int get_ndctl_dimm(struct update_context > *uctx, void *ctx) > { > struct ndctl_dimm *dimm; > struct ndctl_bus *bus; > + int rc = -ENODEV; > > ndctl_bus_foreach(ctx, bus) > ndctl_dimm_foreach(bus, dimm) { > - if (!util_dimm_filter(dimm, uctx->dimm_id)) > + if (!uctx->all && > + !util_dimm_filter(dimm, uctx- > >dimm_id)) > continue; > uctx->dimm = dimm; > - return 0; > + rc = update_firmware(uctx); > + if (rc < 0) { > + error("Update firmware for dimm %s > failed\n", > + ndctl_dimm_get_devna > me(dimm)); > + continue; > + } > } > > - return -ENODEV; > + return rc; > } > > static int verify_fw_file(struct update_context *uctx) > @@ -573,18 +581,17 @@ int cmd_update_firmware(int argc, const char > **argv, void *ctx) > return rc; > } > > + if (strcmp(uctx.dimm_id, "all") == 0) > + uctx.all = true; > + else > + uctx.all = false; > + > rc = get_ndctl_dimm(&uctx, ctx); > if (rc < 0) { > error("DIMM %s not found", uctx.dimm_id); > return rc; > } > > - rc = update_firmware(&uctx); > - if (rc < 0) { > - error("Update firmware failed"); > - return rc; > - } > - > if (uctx.start) > ndctl_cmd_unref(uctx.start); > >
On Wed, Feb 21, 2018 at 2:06 PM, Dave Jiang <dave.jiang@intel.com> wrote: > Adding all option to allow ndctl to update all DIMMs at once. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > ndctl/update.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/ndctl/update.c b/ndctl/update.c > index fc26acf..1b771ff 100644 > --- a/ndctl/update.c > +++ b/ndctl/update.c > @@ -77,6 +77,7 @@ struct update_context { > struct ndctl_dimm *dimm; > struct fw_info dimm_fw; > struct ndctl_cmd *start; > + bool all; > }; > > /* > @@ -471,16 +472,23 @@ static int get_ndctl_dimm(struct update_context *uctx, void *ctx) > { > struct ndctl_dimm *dimm; > struct ndctl_bus *bus; > + int rc = -ENODEV; > > ndctl_bus_foreach(ctx, bus) > ndctl_dimm_foreach(bus, dimm) { > - if (!util_dimm_filter(dimm, uctx->dimm_id)) > + if (!uctx->all && > + !util_dimm_filter(dimm, uctx->dimm_id)) > continue; > uctx->dimm = dimm; > - return 0; > + rc = update_firmware(uctx); > + if (rc < 0) { > + error("Update firmware for dimm %s failed\n", > + ndctl_dimm_get_devname(dimm)); > + continue; > + } No need to special case "all" support, it's handled explicitly as a supported dimm-id string in util_dimm_filter(). In fact, all the util_<object>_filter() helpers support "all" as a special case.
On Wed, Feb 21, 2018 at 2:28 PM, Verma, Vishal L <vishal.l.verma@intel.com> wrote: > > On Wed, 2018-02-21 at 15:06 -0700, Dave Jiang wrote: >> Adding all option to allow ndctl to update all DIMMs at once. > > I was going to ask that we should also update the documentation for > this command to include the new 'all' behavior, but it looks like > during the revisions of the original series, the documentation update > to list the options explicitly got dropped? > > Anyway, can you re-add those, and also talk about --dimm=all there. > > On a side note, should we make this more inline with other dimm > commands, such as xable-dimm or read/write-labels, where the dimm name > (or 'all') is a non-option argument? That would make it something like: > > ndctl update-firmware --firmware=<file> nmem0 (or 'all) Sounds good to me, but we should still silently support -d for just this command since it appeared in a released version and user interfaces are forever.
diff --git a/ndctl/update.c b/ndctl/update.c index fc26acf..1b771ff 100644 --- a/ndctl/update.c +++ b/ndctl/update.c @@ -77,6 +77,7 @@ struct update_context { struct ndctl_dimm *dimm; struct fw_info dimm_fw; struct ndctl_cmd *start; + bool all; }; /* @@ -471,16 +472,23 @@ static int get_ndctl_dimm(struct update_context *uctx, void *ctx) { struct ndctl_dimm *dimm; struct ndctl_bus *bus; + int rc = -ENODEV; ndctl_bus_foreach(ctx, bus) ndctl_dimm_foreach(bus, dimm) { - if (!util_dimm_filter(dimm, uctx->dimm_id)) + if (!uctx->all && + !util_dimm_filter(dimm, uctx->dimm_id)) continue; uctx->dimm = dimm; - return 0; + rc = update_firmware(uctx); + if (rc < 0) { + error("Update firmware for dimm %s failed\n", + ndctl_dimm_get_devname(dimm)); + continue; + } } - return -ENODEV; + return rc; } static int verify_fw_file(struct update_context *uctx) @@ -573,18 +581,17 @@ int cmd_update_firmware(int argc, const char **argv, void *ctx) return rc; } + if (strcmp(uctx.dimm_id, "all") == 0) + uctx.all = true; + else + uctx.all = false; + rc = get_ndctl_dimm(&uctx, ctx); if (rc < 0) { error("DIMM %s not found", uctx.dimm_id); return rc; } - rc = update_firmware(&uctx); - if (rc < 0) { - error("Update firmware failed"); - return rc; - } - if (uctx.start) ndctl_cmd_unref(uctx.start);
Adding all option to allow ndctl to update all DIMMs at once. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- ndctl/update.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)