[PATCH ndctl] cxl: Add list all option to the cxl command

[PATCH ndctl] cxl: Add list all option to the cxl command

[sunfishho12]

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(+)

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", &param.partition,
 		    "include memory device partition information "),
+	OPT_BOOLEAN(0, "all", &param.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

Re: [PATCH ndctl] cxl: Add list all option to the cxl command

[Verma, Vishal L]

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", &param.partition,
>                     "include memory device partition information "),
> +       OPT_BOOLEAN(0, "all", &param.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
> 

Re: [PATCH ndctl] cxl: Add list all option to the cxl command

[Davidlohr Bueso]

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

Re: [PATCH ndctl] cxl: Add list all option to the cxl command

[Matthew Ho]

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