| Message ID | 20220714182737.60714-1-sunfishho12@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [ndctl] cxl: Add list all option to the cxl command | expand |
On Thu, 14 Jul 2022, Verma, Vishal L wrote: >On Thu, 2022-07-14 at 11:27 -0700, sunfishho12@gmail.com wrote: >> From: Matthew Ho <sunfishho12@gmail.com> >> >> This adds a new subcommand cxl list --all, which is equivalent to cxl >> list -MBPEDTHIiu. This addition makes it easier to list all the CXL >> devices at once, as one does not need to append a subcommand for each >> device. --all is also easier to remember than -MBPEDTHIiu. When >> region support is added, this will be updated to include it. >> >> Reviewed-by: Adam Manzanares <a.manzanares@samsung.com> >> Signed-off-by: Matthew Ho <sunfishho12@gmail.com> >> --- >> cxl/filter.h | 1 + >> cxl/list.c | 15 +++++++++++++++ >> 2 files changed, 16 insertions(+) You also need to the manpage (which I also missed) via: Documentation/cxl/cxl-list.txt and of course, same for Vishal's suggestions below. > >Hi Matthew, thanks for sending this - I agree it gets tedious trying to >list multiple things. > >However, I think we should do this similar to ndctl-list's --verbose >options, where adding the number of 'v's adds in more and more detail. Yeah I agree, and was not aware that ndctl has this. Furthermore perf tooling also uses this. > >For example: > >cxl list (default) : Regions and memdevs (this isn't there today, but >in a pending series I'm about to send out). > >cxl list -v: -RMBDPT > >cxl list -vv: -RMBDPTi > >cxl list -vvv: -RMBDPTiHI Makes sense to me. > >-u/--human can be excluded from the verbosity levels as that can be >passed in if needed, and there may be use cases where it isn't desired >(in scripts). > >Thoughts on this - do you want to take a shot at implementing it this >way? Sounds good to me. Thanks, Davidlohr
On Thu, 2022-07-14 at 11:27 -0700, sunfishho12@gmail.com wrote: > From: Matthew Ho <sunfishho12@gmail.com> > > This adds a new subcommand cxl list --all, which is equivalent to cxl > list -MBPEDTHIiu. This addition makes it easier to list all the CXL > devices at once, as one does not need to append a subcommand for each > device. --all is also easier to remember than -MBPEDTHIiu. When > region support is added, this will be updated to include it. > > Reviewed-by: Adam Manzanares <a.manzanares@samsung.com> > Signed-off-by: Matthew Ho <sunfishho12@gmail.com> > --- > cxl/filter.h | 1 + > cxl/list.c | 15 +++++++++++++++ > 2 files changed, 16 insertions(+) Hi Matthew, thanks for sending this - I agree it gets tedious trying to list multiple things. However, I think we should do this similar to ndctl-list's --verbose options, where adding the number of 'v's adds in more and more detail. For example: cxl list (default) : Regions and memdevs (this isn't there today, but in a pending series I'm about to send out). cxl list -v: -RMBDPT cxl list -vv: -RMBDPTi cxl list -vvv: -RMBDPTiHI -u/--human can be excluded from the verbosity levels as that can be passed in if needed, and there may be use cases where it isn't desired (in scripts). Thoughts on this - do you want to take a shot at implementing it this way? > > diff --git a/cxl/filter.h b/cxl/filter.h > index 697b7779c08e..7009c00a1c23 100644 > --- a/cxl/filter.h > +++ b/cxl/filter.h > @@ -13,6 +13,7 @@ struct cxl_filter_params { > const char *port_filter; > const char *endpoint_filter; > const char *decoder_filter; > + bool all; > bool single; > bool endpoints; > bool decoders; > diff --git a/cxl/list.c b/cxl/list.c > index 1e9d441190a0..738282e95434 100644 > --- a/cxl/list.c > +++ b/cxl/list.c > @@ -50,6 +50,8 @@ static const struct option options[] = { > "include memory device health information "), > OPT_BOOLEAN('I', "partition", ¶m.partition, > "include memory device partition information "), > + OPT_BOOLEAN(0, "all", ¶m.all, > + "include info on all components of the CXL > hierarchy"), > #ifdef ENABLE_DEBUG > OPT_BOOLEAN(0, "debug", &debug, "debug list walk"), > #endif > @@ -82,6 +84,19 @@ int cmd_list(int argc, const char **argv, struct > cxl_ctx *ctx) > usage_with_options(u, options); > } > > + if (param.all){ > + param.memdevs = true; > + param.buses = true; > + param.ports = true; > + param.endpoints = true; > + param.decoders = true; > + param.partition = true; > + param.health = true; > + param.targets = true; > + param.human = true; > + param.idle = true; > + } > + > if (num_list_flags() == 0) { > if (param.memdev_filter || param.serial_filter) > param.memdevs = true; > -- > 2.34.1 >
On Thu, Jul 14, 2022 at 12:15:21PM -0700, Davidlohr Bueso wrote: > On Thu, 14 Jul 2022, Verma, Vishal L wrote: > > > On Thu, 2022-07-14 at 11:27 -0700, sunfishho12@gmail.com wrote: > > > From: Matthew Ho <sunfishho12@gmail.com> > > > > > > This adds a new subcommand cxl list --all, which is equivalent to cxl > > > list -MBPEDTHIiu. This addition makes it easier to list all the CXL > > > devices at once, as one does not need to append a subcommand for each > > > device. --all is also easier to remember than -MBPEDTHIiu. When > > > region support is added, this will be updated to include it. > > > > > > Reviewed-by: Adam Manzanares <a.manzanares@samsung.com> > > > Signed-off-by: Matthew Ho <sunfishho12@gmail.com> > > > --- > > > cxl/filter.h | 1 + > > > cxl/list.c | 15 +++++++++++++++ > > > 2 files changed, 16 insertions(+) > > You also need to the manpage (which I also missed) via: > > Documentation/cxl/cxl-list.txt > > and of course, same for Vishal's suggestions below. Ah, will add. > > > > > Hi Matthew, thanks for sending this - I agree it gets tedious trying to > > list multiple things. > > > > However, I think we should do this similar to ndctl-list's --verbose > > options, where adding the number of 'v's adds in more and more detail. > > Yeah I agree, and was not aware that ndctl has this. Furthermore perf > tooling also uses this. > > > > > For example: > > > > cxl list (default) : Regions and memdevs (this isn't there today, but > > in a pending series I'm about to send out). > > > > cxl list -v: -RMBDPT > > > > cxl list -vv: -RMBDPTi > > > > cxl list -vvv: -RMBDPTiHI > > Makes sense to me. Sounds good, will implement it this way. > > > > -u/--human can be excluded from the verbosity levels as that can be > > passed in if needed, and there may be use cases where it isn't desired > > (in scripts). > > > > Thoughts on this - do you want to take a shot at implementing it this > > way? > > Sounds good to me. Will do. Thanks for all of the feedback! Best, Matthew
diff --git a/cxl/filter.h b/cxl/filter.h index 697b7779c08e..7009c00a1c23 100644 --- a/cxl/filter.h +++ b/cxl/filter.h @@ -13,6 +13,7 @@ struct cxl_filter_params { const char *port_filter; const char *endpoint_filter; const char *decoder_filter; + bool all; bool single; bool endpoints; bool decoders; diff --git a/cxl/list.c b/cxl/list.c index 1e9d441190a0..738282e95434 100644 --- a/cxl/list.c +++ b/cxl/list.c @@ -50,6 +50,8 @@ static const struct option options[] = { "include memory device health information "), OPT_BOOLEAN('I', "partition", ¶m.partition, "include memory device partition information "), + OPT_BOOLEAN(0, "all", ¶m.all, + "include info on all components of the CXL hierarchy"), #ifdef ENABLE_DEBUG OPT_BOOLEAN(0, "debug", &debug, "debug list walk"), #endif @@ -82,6 +84,19 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx) usage_with_options(u, options); } + if (param.all){ + param.memdevs = true; + param.buses = true; + param.ports = true; + param.endpoints = true; + param.decoders = true; + param.partition = true; + param.health = true; + param.targets = true; + param.human = true; + param.idle = true; + } + if (num_list_flags() == 0) { if (param.memdev_filter || param.serial_filter) param.memdevs = true;