| Message ID | 20220721192447.16958-1-sunfishho12@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ndctl,v2] cxl: Add list verbose option to the cxl command | expand |
On Thu, 21 Jul 2022, sunfishho12@gmail.com wrote: >From: Matthew Ho <sunfishho12@gmail.com> > >This adds the new subcommands cxl list -v, cxl list -vv, and cxl list -vvv, and >builds on this pending patch series that adds region listing support: > >https://lore.kernel.org/linux-cxl/20220715062550.789736-1-vishal.l.verma@intel.com/ > >cxl list -v is now equivalent to cxl list -RMBDPT, cxl list -vv is >equivalent to cxl list -RMBDPTi, and cxl list -vvv is equivalent to >cxl list -RMBDPTiHI. These additions make it easier to list all of the CXL >devices without having to remember which subcommand must be appended for each >type of device. > >Reviewed-by: Adam Manzanares <a.manzanares@samsung.com> >Signed-off-by: Matthew Ho <sunfishho12@gmail.com> With the exception of the switch() handling (see below). Acked-by: Davidlohr Bueso <dave@stgolabs.net> >--- > >Changes since v1 (previously titled "Add list all option to the cxl command"): > - cxl list -v, cxl list -vv, cxl list -vvv replace cxl list --all, > providing greater control over detail of the output. --human has been excluded > as it can be appended by the user. > > Documentation/cxl/cxl-list.txt | 20 ++++++++++++++++++++ > cxl/filter.h | 1 + > cxl/list.c | 17 +++++++++++++++++ > 3 files changed, 38 insertions(+) > >diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt >index 2906c2f6ef4b..f069aee8c674 100644 >--- a/Documentation/cxl/cxl-list.txt >+++ b/Documentation/cxl/cxl-list.txt >@@ -336,6 +336,26 @@ OPTIONS > --region:: > Specify the region name to filter the emitted regions. > >+-v:: >+--verbose:: >+ Increase verbosity of the output. This can be specified >+ multiple times to be even more verbose on the >+ informational and miscellaneous output, and can be used >+ to override omitted flags for showing specific >+ information. Note that cxl list --verbose --verbose is >+ equivalent to cxl list -vv, and likewise >+ cxl list --verbose --verbose --verbose is equivalent to >+ cxl list -vvv. >+ - *-v* >+ Automatically enable --memdevs, --regions, --buses, >+ --ports, --decoders, --targets. >+ - *-vv* >+ Everything *-v* provides, plus automatically include >+ disabled devices with --idle. >+ - *-vvv* >+ Everything *-vv* provides, plus automatically enable >+ --health and --partition. >+ > --debug:: > If the cxl tool was built with debug enabled, turn on debug > messages. >diff --git a/cxl/filter.h b/cxl/filter.h >index d22d8b1f798b..256df49c3d0c 100644 >--- a/cxl/filter.h >+++ b/cxl/filter.h >@@ -26,6 +26,7 @@ struct cxl_filter_params { > bool human; > bool health; > bool partition; >+ int verbose; > struct log_ctx ctx; > }; > >diff --git a/cxl/list.c b/cxl/list.c >index 5f604ecddf3c..4763c7c36024 100644 >--- a/cxl/list.c >+++ b/cxl/list.c >@@ -52,6 +52,8 @@ static const struct option options[] = { > "include memory device health information"), > OPT_BOOLEAN('I', "partition", ¶m.partition, > "include memory device partition information"), >+ OPT_INCR('v', "verbose", ¶m.verbose, >+ "increase output detail"), > #ifdef ENABLE_DEBUG > OPT_BOOLEAN(0, "debug", &debug, "debug list walk"), > #endif >@@ -106,6 +108,21 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx) > param.memdevs = true; > } > >+ switch (param.verbose){ >+ default: >+ break; >+ case 3: >+ param.health = true; >+ param.partition = true; >+ case 2: >+ param.idle = true; >+ case 1: >+ param.buses = true; >+ param.ports = true; >+ param.decoders = true; >+ param.targets = true; >+ } >+ I think you want this: switch(param.verbose) { default: case 3: ... case 2: ... case 1: .... case 0: break; } > log_init(¶m.ctx, "cxl list", "CXL_LIST_LOG"); > if (debug) { > cxl_set_log_priority(ctx, LOG_DEBUG); >-- >2.34.1 >
sunfishho12@ wrote: > From: Matthew Ho <sunfishho12@gmail.com> > > This adds the new subcommands cxl list -v, cxl list -vv, and cxl list -vvv, and > builds on this pending patch series that adds region listing support: > > https://lore.kernel.org/linux-cxl/20220715062550.789736-1-vishal.l.verma@intel.com/ > > cxl list -v is now equivalent to cxl list -RMBDPT, cxl list -vv is > equivalent to cxl list -RMBDPTi, and cxl list -vvv is equivalent to > cxl list -RMBDPTiHI. These additions make it easier to list all of the CXL > devices without having to remember which subcommand must be appended for each > type of device. Looks good, minor comments below that after addressing you can add: Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > Reviewed-by: Adam Manzanares <a.manzanares@samsung.com> > Signed-off-by: Matthew Ho <sunfishho12@gmail.com> > --- > > Changes since v1 (previously titled "Add list all option to the cxl command"): > - cxl list -v, cxl list -vv, cxl list -vvv replace cxl list --all, > providing greater control over detail of the output. --human has been excluded > as it can be appended by the user. > > Documentation/cxl/cxl-list.txt | 20 ++++++++++++++++++++ > cxl/filter.h | 1 + > cxl/list.c | 17 +++++++++++++++++ > 3 files changed, 38 insertions(+) > > diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt > index 2906c2f6ef4b..f069aee8c674 100644 > --- a/Documentation/cxl/cxl-list.txt > +++ b/Documentation/cxl/cxl-list.txt > @@ -336,6 +336,26 @@ OPTIONS > --region:: > Specify the region name to filter the emitted regions. > > +-v:: > +--verbose:: > + Increase verbosity of the output. This can be specified > + multiple times to be even more verbose on the > + informational and miscellaneous output, and can be used > + to override omitted flags for showing specific > + information. Note that cxl list --verbose --verbose is > + equivalent to cxl list -vv, and likewise > + cxl list --verbose --verbose --verbose is equivalent to > + cxl list -vvv. > + - *-v* > + Automatically enable --memdevs, --regions, --buses, > + --ports, --decoders, --targets. > + - *-vv* > + Everything *-v* provides, plus automatically include > + disabled devices with --idle. > + - *-vvv* > + Everything *-vv* provides, plus automatically enable > + --health and --partition. These descriptions can drop the word "automatically" without any loss of clarity. > + > --debug:: > If the cxl tool was built with debug enabled, turn on debug > messages. > diff --git a/cxl/filter.h b/cxl/filter.h > index d22d8b1f798b..256df49c3d0c 100644 > --- a/cxl/filter.h > +++ b/cxl/filter.h > @@ -26,6 +26,7 @@ struct cxl_filter_params { > bool human; > bool health; > bool partition; > + int verbose; > struct log_ctx ctx; > }; > > diff --git a/cxl/list.c b/cxl/list.c > index 5f604ecddf3c..4763c7c36024 100644 > --- a/cxl/list.c > +++ b/cxl/list.c > @@ -52,6 +52,8 @@ static const struct option options[] = { > "include memory device health information"), > OPT_BOOLEAN('I', "partition", ¶m.partition, > "include memory device partition information"), > + OPT_INCR('v', "verbose", ¶m.verbose, > + "increase output detail"), > #ifdef ENABLE_DEBUG > OPT_BOOLEAN(0, "debug", &debug, "debug list walk"), > #endif > @@ -106,6 +108,21 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx) > param.memdevs = true; > } > > + switch (param.verbose){ > + default: > + break; > + case 3: > + param.health = true; > + param.partition = true; add a "/* fallthrough */;" statement here... > + case 2: > + param.idle = true; ...and here. > + case 1: > + param.buses = true; > + param.ports = true; > + param.decoders = true; > + param.targets = true; ...and a "break;" here just because the ndctl project tries to follow kernel C-style. > + } > + > log_init(¶m.ctx, "cxl list", "CXL_LIST_LOG"); > if (debug) { > cxl_set_log_priority(ctx, LOG_DEBUG); > -- > 2.34.1 >
On Thu, Jul 21, 2022 at 04:44:32PM -0700, Dan Williams wrote: > sunfishho12@ wrote: > > From: Matthew Ho <sunfishho12@gmail.com> > > > > This adds the new subcommands cxl list -v, cxl list -vv, and cxl list -vvv, and > > builds on this pending patch series that adds region listing support: > > > > https://lore.kernel.org/linux-cxl/20220715062550.789736-1-vishal.l.verma@intel.com/ > > > > cxl list -v is now equivalent to cxl list -RMBDPT, cxl list -vv is > > equivalent to cxl list -RMBDPTi, and cxl list -vvv is equivalent to > > cxl list -RMBDPTiHI. These additions make it easier to list all of the CXL > > devices without having to remember which subcommand must be appended for each > > type of device. > > Looks good, minor comments below that after addressing you can add: > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > > > Reviewed-by: Adam Manzanares <a.manzanares@samsung.com> > > Signed-off-by: Matthew Ho <sunfishho12@gmail.com> > > --- > > > > Changes since v1 (previously titled "Add list all option to the cxl command"): > > - cxl list -v, cxl list -vv, cxl list -vvv replace cxl list --all, > > providing greater control over detail of the output. --human has been excluded > > as it can be appended by the user. > > > > Documentation/cxl/cxl-list.txt | 20 ++++++++++++++++++++ > > cxl/filter.h | 1 + > > cxl/list.c | 17 +++++++++++++++++ > > 3 files changed, 38 insertions(+) > > > > diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt > > index 2906c2f6ef4b..f069aee8c674 100644 > > --- a/Documentation/cxl/cxl-list.txt > > +++ b/Documentation/cxl/cxl-list.txt > > @@ -336,6 +336,26 @@ OPTIONS > > --region:: > > Specify the region name to filter the emitted regions. > > > > +-v:: > > +--verbose:: > > + Increase verbosity of the output. This can be specified > > + multiple times to be even more verbose on the > > + informational and miscellaneous output, and can be used > > + to override omitted flags for showing specific > > + information. Note that cxl list --verbose --verbose is > > + equivalent to cxl list -vv, and likewise > > + cxl list --verbose --verbose --verbose is equivalent to > > + cxl list -vvv. > > + - *-v* > > + Automatically enable --memdevs, --regions, --buses, > > + --ports, --decoders, --targets. > > + - *-vv* > > + Everything *-v* provides, plus automatically include > > + disabled devices with --idle. > > + - *-vvv* > > + Everything *-vv* provides, plus automatically enable > > + --health and --partition. > > These descriptions can drop the word "automatically" without any loss of clarity. > Sounds good, will drop that in a follow up patch. > > + > > --debug:: > > If the cxl tool was built with debug enabled, turn on debug > > messages. > > diff --git a/cxl/filter.h b/cxl/filter.h > > index d22d8b1f798b..256df49c3d0c 100644 > > --- a/cxl/filter.h > > +++ b/cxl/filter.h > > @@ -26,6 +26,7 @@ struct cxl_filter_params { > > bool human; > > bool health; > > bool partition; > > + int verbose; > > struct log_ctx ctx; > > }; > > > > diff --git a/cxl/list.c b/cxl/list.c > > index 5f604ecddf3c..4763c7c36024 100644 > > --- a/cxl/list.c > > +++ b/cxl/list.c > > @@ -52,6 +52,8 @@ static const struct option options[] = { > > "include memory device health information"), > > OPT_BOOLEAN('I', "partition", ¶m.partition, > > "include memory device partition information"), > > + OPT_INCR('v', "verbose", ¶m.verbose, > > + "increase output detail"), > > #ifdef ENABLE_DEBUG > > OPT_BOOLEAN(0, "debug", &debug, "debug list walk"), > > #endif > > @@ -106,6 +108,21 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx) > > param.memdevs = true; > > } > > > > + switch (param.verbose){ > > + default: > > + break; > > + case 3: > > + param.health = true; > > + param.partition = true; > > add a "/* fallthrough */;" statement here... > > > + case 2: > > + param.idle = true; > > ...and here. > > > + case 1: > > + param.buses = true; > > + param.ports = true; > > + param.decoders = true; > > + param.targets = true; > > ...and a "break;" here just because the ndctl project tries to follow > kernel C-style. > Will combine this with David's suggestion to add a "case 0:", thanks for the feedback. Best, Matthew > > + } > > + > > log_init(¶m.ctx, "cxl list", "CXL_LIST_LOG"); > > if (debug) { > > cxl_set_log_priority(ctx, LOG_DEBUG); > > -- > > 2.34.1 > > > >
diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt index 2906c2f6ef4b..f069aee8c674 100644 --- a/Documentation/cxl/cxl-list.txt +++ b/Documentation/cxl/cxl-list.txt @@ -336,6 +336,26 @@ OPTIONS --region:: Specify the region name to filter the emitted regions. +-v:: +--verbose:: + Increase verbosity of the output. This can be specified + multiple times to be even more verbose on the + informational and miscellaneous output, and can be used + to override omitted flags for showing specific + information. Note that cxl list --verbose --verbose is + equivalent to cxl list -vv, and likewise + cxl list --verbose --verbose --verbose is equivalent to + cxl list -vvv. + - *-v* + Automatically enable --memdevs, --regions, --buses, + --ports, --decoders, --targets. + - *-vv* + Everything *-v* provides, plus automatically include + disabled devices with --idle. + - *-vvv* + Everything *-vv* provides, plus automatically enable + --health and --partition. + --debug:: If the cxl tool was built with debug enabled, turn on debug messages. diff --git a/cxl/filter.h b/cxl/filter.h index d22d8b1f798b..256df49c3d0c 100644 --- a/cxl/filter.h +++ b/cxl/filter.h @@ -26,6 +26,7 @@ struct cxl_filter_params { bool human; bool health; bool partition; + int verbose; struct log_ctx ctx; }; diff --git a/cxl/list.c b/cxl/list.c index 5f604ecddf3c..4763c7c36024 100644 --- a/cxl/list.c +++ b/cxl/list.c @@ -52,6 +52,8 @@ static const struct option options[] = { "include memory device health information"), OPT_BOOLEAN('I', "partition", ¶m.partition, "include memory device partition information"), + OPT_INCR('v', "verbose", ¶m.verbose, + "increase output detail"), #ifdef ENABLE_DEBUG OPT_BOOLEAN(0, "debug", &debug, "debug list walk"), #endif @@ -106,6 +108,21 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx) param.memdevs = true; } + switch (param.verbose){ + default: + break; + case 3: + param.health = true; + param.partition = true; + case 2: + param.idle = true; + case 1: + param.buses = true; + param.ports = true; + param.decoders = true; + param.targets = true; + } + log_init(¶m.ctx, "cxl list", "CXL_LIST_LOG"); if (debug) { cxl_set_log_priority(ctx, LOG_DEBUG);