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

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

[sunfishho12]

From: Matthew Ho <sunfishho12@gmail.com>

This adds the new subcommands cxl list -v, cxl list -vv, and cxl list -vvv.

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.

Acked-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Adam Manzanares <a.manzanares@samsung.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Matthew Ho <sunfishho12@gmail.com>
---
Changes since v3[1]

 - Fixed formatting of patch
 - Amended patch to apply to latest region driver patches [2]
 - Added missed "Acked-by"

[1]: https://lore.kernel.org/linux-cxl/20220722183705.26936-1-sunfishho12@gmail.com/
[2]: https://lore.kernel.org/linux-cxl/20220810230914.549611-1-vishal.l.verma@intel.com/

Maybe this could be added to the region driver series?

 Documentation/cxl/cxl-list.txt | 20 ++++++++++++++++++++
 cxl/filter.h                   |  1 +
 cxl/list.c                     | 21 +++++++++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
index 2906c2f6ef4b..0e7727ca167f 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*
+	  Enable --memdevs, --regions, --buses,
+	  --ports, --decoders, and --targets.
+	- *-vv*
+	  Everything *-v* provides, plus include disabled
+	  devices with --idle.
+	- *-vvv*
+	  Everything *-vvv* provides, plus 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..8c48fbbaaec3 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", &param.partition,
 		    "include memory device partition information"),
+	OPT_INCR('v', "verbose", &param.verbose,
+		 "increase output detail"),
 #ifdef ENABLE_DEBUG
 	OPT_BOOLEAN(0, "debug", &debug, "debug list walk"),
 #endif
@@ -106,6 +108,25 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
 		param.memdevs = true;
 	}

+	switch(param.verbose){
+	default:
+	case 3:
+		param.health = true;
+		param.partition = true;
+		/* fallthrough */
+	case 2:
+		param.idle = true;
+		/* fallthrough */
+	case 1:
+		param.buses = true;
+		param.ports = true;
+		param.decoders = true;
+		param.targets = true;
+		/*fallthrough*/
+	case 0:
+		break;
+	}
+
 	log_init(&param.ctx, "cxl list", "CXL_LIST_LOG");
 	if (debug) {
 		cxl_set_log_priority(ctx, LOG_DEBUG);
--
2.34.1

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

[Verma, Vishal L]

On Fri, 2022-08-12 at 11:12 -0700, 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.
> 
> 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.
> 
> Acked-by: Davidlohr Bueso <dave@stgolabs.net>
> Reviewed-by: Adam Manzanares <a.manzanares@samsung.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Matthew Ho <sunfishho12@gmail.com>
> ---
> Changes since v3[1]
> 
>  - Fixed formatting of patch
>  - Amended patch to apply to latest region driver patches [2]
>  - Added missed "Acked-by"
> 
> [1]: https://lore.kernel.org/linux-cxl/20220722183705.26936-1-sunfishho12@gmail.com/
> [2]: https://lore.kernel.org/linux-cxl/20220810230914.549611-1-vishal.l.verma@intel.com/
> 
> Maybe this could be added to the region driver series?

Hi Matthew,

Thanks for the update. I intend to pick up all pending patches in the
next week or so in preparation for the v74 release, and I will include
this.

One comment and small typo below - otherwise this looks good.

> 
>  Documentation/cxl/cxl-list.txt | 20 ++++++++++++++++++++
>  cxl/filter.h                   |  1 +
>  cxl/list.c                     | 21 +++++++++++++++++++++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
> index 2906c2f6ef4b..0e7727ca167f 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.

I'd say a single example, of -vv is sufficient here.

> +       - *-v*
> +         Enable --memdevs, --regions, --buses,
> +         --ports, --decoders, and --targets.
> +       - *-vv*
> +         Everything *-v* provides, plus include disabled
> +         devices with --idle.
> +       - *-vvv*
> +         Everything *-vvv* provides, plus enable

The above should be -vv

> +         --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..8c48fbbaaec3 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", &param.partition,
>                     "include memory device partition information"),
> +       OPT_INCR('v', "verbose", &param.verbose,
> +                "increase output detail"),
>  #ifdef ENABLE_DEBUG
>         OPT_BOOLEAN(0, "debug", &debug, "debug list walk"),
>  #endif
> @@ -106,6 +108,25 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
>                 param.memdevs = true;
>         }
> 
> +       switch(param.verbose){
> +       default:
> +       case 3:
> +               param.health = true;
> +               param.partition = true;
> +               /* fallthrough */
> +       case 2:
> +               param.idle = true;
> +               /* fallthrough */
> +       case 1:
> +               param.buses = true;
> +               param.ports = true;
> +               param.decoders = true;
> +               param.targets = true;
> +               /*fallthrough*/
> +       case 0:
> +               break;
> +       }
> +
>         log_init(&param.ctx, "cxl list", "CXL_LIST_LOG");
>         if (debug) {
>                 cxl_set_log_priority(ctx, LOG_DEBUG);
> --
> 2.34.1
> 

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

[Matthew Ho]

On Fri, Aug 12, 2022 at 08:04:49PM +0000, Verma, Vishal L wrote:
> On Fri, 2022-08-12 at 11:12 -0700, 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.
> > 
> > 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.
> > 
> > Acked-by: Davidlohr Bueso <dave@stgolabs.net>
> > Reviewed-by: Adam Manzanares <a.manzanares@samsung.com>
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Matthew Ho <sunfishho12@gmail.com>
> > ---
> > Changes since v3[1]
> > 
> >  - Fixed formatting of patch
> >  - Amended patch to apply to latest region driver patches [2]
> >  - Added missed "Acked-by"
> > 
> > [1]: https://lore.kernel.org/linux-cxl/20220722183705.26936-1-sunfishho12@gmail.com/
> > [2]: https://lore.kernel.org/linux-cxl/20220810230914.549611-1-vishal.l.verma@intel.com/
> > 
> > Maybe this could be added to the region driver series?
> 
> Hi Matthew,
> 
> Thanks for the update. I intend to pick up all pending patches in the
> next week or so in preparation for the v74 release, and I will include
> this.

Thanks, sounds great! I'll fix the changes in a v5.

> 
> One comment and small typo below - otherwise this looks good.
> 
> > 
> >  Documentation/cxl/cxl-list.txt | 20 ++++++++++++++++++++
> >  cxl/filter.h                   |  1 +
> >  cxl/list.c                     | 21 +++++++++++++++++++++
> >  3 files changed, 42 insertions(+)
> > 
> > diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
> > index 2906c2f6ef4b..0e7727ca167f 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.
> 
> I'd say a single example, of -vv is sufficient here.

Sounds good, will remove the -vvv example.

> > +       - *-v*
> > +         Enable --memdevs, --regions, --buses,
> > +         --ports, --decoders, and --targets.
> > +       - *-vv*
> > +         Everything *-v* provides, plus include disabled
> > +         devices with --idle.
> > +       - *-vvv*
> > +         Everything *-vvv* provides, plus enable
> 
> The above should be -vv

Oops, thanks for the catch, will fix.

> 
> > +         --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..8c48fbbaaec3 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", &param.partition,
> >                     "include memory device partition information"),
> > +       OPT_INCR('v', "verbose", &param.verbose,
> > +                "increase output detail"),
> >  #ifdef ENABLE_DEBUG
> >         OPT_BOOLEAN(0, "debug", &debug, "debug list walk"),
> >  #endif
> > @@ -106,6 +108,25 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
> >                 param.memdevs = true;
> >         }
> > 
> > +       switch(param.verbose){
> > +       default:
> > +       case 3:
> > +               param.health = true;
> > +               param.partition = true;
> > +               /* fallthrough */
> > +       case 2:
> > +               param.idle = true;
> > +               /* fallthrough */
> > +       case 1:
> > +               param.buses = true;
> > +               param.ports = true;
> > +               param.decoders = true;
> > +               param.targets = true;
> > +               /*fallthrough*/
> > +       case 0:
> > +               break;
> > +       }
> > +
> >         log_init(&param.ctx, "cxl list", "CXL_LIST_LOG");
> >         if (debug) {
> >                 cxl_set_log_priority(ctx, LOG_DEBUG);
> > --
> > 2.34.1
> >
>

Thanks again for the feedback!

Best,
Matthew