[ndctl PATCH 00/15] cxl-cli test and usability updates

[ndctl PATCH 00/15] cxl-cli test and usability updates

[Dan Williams]

The v6.1-rc4 kernel picks up new cxl_test infrastructure for a switch
attached to a single-port host-bridge. While extending the region
creation and topology tests for that change, some additional updates
were identified. The main one is the ability to elide the "ways"
"memdevs" arguments to 'cxl create-region'. Those parameters are now
derived by the result of a topology walk under the given root decoder.
I.e.:

    cxl create-region -d decoder3.4

...will internally perform a:

    cxl list -M -d decoder3.4

...operation and use those memdevs as region targets.

This also updates 'create-region' size detection to be maximum free
extent in the decoder, or the available capacity in the target memdevs,
whichever is smaller.

Some miscellaneous fixes and updates are included as well.

---

Dan Williams (15):
      ndctl/test: Move firmware-update.sh to the 'descructive' set
      ndctl/test: Add kernel backtrace detection to some dax tests
      ndctl/clang-format: Move minimum version to 6
      ndctl/clang-format: Fix space after for_each macros
      cxl/list: Always attempt to collect child objects
      cxl/list: Skip emitting pmem_size when it is zero
      cxl/filter: Return json-c topology
      cxl/list: Record cxl objects in json objects
      cxl/region: Make ways an integer argument
      cxl/region: Make granularity an integer argument
      cxl/region: Use cxl_filter_walk() to gather create-region targets
      cxl/region: Trim region size by max available extent
      cxl/region: Default to memdev mode for create with no arguments
      cxl/test: Extend cxl-topology.sh for a single root-port host-bridge
      cxl/test: Test single-port host-bridge region creation


 .clang-format             |   38 ++---
 cxl/filter.c              |   36 +----
 cxl/filter.h              |   22 +++
 cxl/json.c                |   29 +++-
 cxl/list.c                |    7 +
 cxl/region.c              |  346 +++++++++++++++++++++++++++------------------
 test/common               |   10 +
 test/cxl-create-region.sh |   28 ++++
 test/cxl-region-sysfs.sh  |    4 -
 test/cxl-topology.sh      |   53 ++++---
 test/dax.sh               |    2 
 test/daxdev-errors.sh     |    2 
 test/meson.build          |    2 
 test/multi-dax.sh         |    2 
 14 files changed, 363 insertions(+), 218 deletions(-)

base-commit: 1d4dbf6ff6eb988864d154792aaa098a2b11a244

[ndctl PATCH 01/15] ndctl/test: Move firmware-update.sh to the 'descructive' set

[Dan Williams]

The firmware update test attempts a system-suspend test which may break
systems that have a broken driver, or otherwise are not prepared to support
suspend.

Link: https://github.com/pmem/ndctl/issues/221
Reported-by: Yi Zhang <yi.zhang@redhat.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 test/meson.build |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/meson.build b/test/meson.build
index 5953c286d13f..c31d8eac66c5 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -170,7 +170,6 @@ tests = [
   [ 'btt-errors.sh',          btt_errors,	  'ndctl' ],
   [ 'hugetlb',                hugetlb,		  'ndctl' ],
   [ 'btt-pad-compat.sh',      btt_pad_compat,	  'ndctl' ],
-  [ 'firmware-update.sh',     firmware_update,	  'ndctl' ],
   [ 'ack-shutdown-count-set', ack_shutdown_count, 'ndctl' ],
   [ 'rescan-partitions.sh',   rescan_partitions,  'ndctl' ],
   [ 'inject-smart.sh',        inject_smart,	  'ndctl' ],
@@ -196,6 +195,7 @@ if get_option('destructive').enabled()
   mmap_test = find_program('mmap.sh')
 
   tests += [
+    [ 'firmware-update.sh',     firmware_update,	  'ndctl' ],
     [ 'pmem-ns',           pmem_ns,	   'ndctl' ],
     [ 'sub-section.sh',    sub_section,	   'dax'   ],
     [ 'dax-dev',           dax_dev,	   'dax'   ],

[ndctl PATCH 02/15] ndctl/test: Add kernel backtrace detection to some dax tests

[Dan Williams]

It is useful to fail a test if it triggers a backtrace. Generalize the
mechanism from test/cxl-topology.sh and add it to tests that want
to validate clean kernel logs.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 test/common              |   10 ++++++++++
 test/cxl-region-sysfs.sh |    4 +---
 test/cxl-topology.sh     |    5 +----
 test/dax.sh              |    2 ++
 test/daxdev-errors.sh    |    2 ++
 test/multi-dax.sh        |    2 ++
 6 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/test/common b/test/common
index 65615cc09a3e..44cc352f6009 100644
--- a/test/common
+++ b/test/common
@@ -132,3 +132,13 @@ json2var()
 {
 	sed -e "s/[{}\",]//g; s/\[//g; s/\]//g; s/:/=/g"
 }
+
+# check_dmesg
+# $1: line number where this is called
+check_dmesg()
+{
+	# validate no WARN or lockdep report during the run
+	log=$(journalctl -r -k --since "-$((SECONDS+1))s")
+	grep -q "Call Trace" <<< $log && err $1
+	true
+}
diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh
index 63186b60dfec..e128406cd8c8 100644
--- a/test/cxl-region-sysfs.sh
+++ b/test/cxl-region-sysfs.sh
@@ -164,8 +164,6 @@ readarray -t endpoint < <($CXL free-dpa -t pmem ${mem[*]} |
 			  jq -r ".[] | .decoder.decoder")
 echo "$region released ${#endpoint[@]} targets: ${endpoint[@]}"
 
-# validate no WARN or lockdep report during the run
-log=$(journalctl -r -k --since "-$((SECONDS+1))s")
-grep -q "Call Trace" <<< $log && err "$LINENO"
+check_dmesg "$LINENO"
 
 modprobe -r cxl_test
diff --git a/test/cxl-topology.sh b/test/cxl-topology.sh
index f7e390d22680..1f15d29f0600 100644
--- a/test/cxl-topology.sh
+++ b/test/cxl-topology.sh
@@ -169,9 +169,6 @@ done
 # validate that the bus can be disabled without issue
 $CXL disable-bus $root -f
 
-
-# validate no WARN or lockdep report during the run
-log=$(journalctl -r -k --since "-$((SECONDS+1))s")
-grep -q "Call Trace" <<< $log && err "$LINENO"
+check_dmesg "$LINENO"
 
 modprobe -r cxl_test
diff --git a/test/dax.sh b/test/dax.sh
index bb9848b10ecc..3ffbc8079eba 100755
--- a/test/dax.sh
+++ b/test/dax.sh
@@ -118,4 +118,6 @@ else
 	run_xfs
 fi
 
+check_dmesg "$LINENO"
+
 exit 0
diff --git a/test/daxdev-errors.sh b/test/daxdev-errors.sh
index 7f79718113d0..84ef93499acf 100755
--- a/test/daxdev-errors.sh
+++ b/test/daxdev-errors.sh
@@ -71,6 +71,8 @@ if read sector len < /sys/bus/platform/devices/nfit_test.0/$busdev/$region/badbl
 fi
 [ -n "$sector" ] && echo "fail: $LINENO" && exit 1
 
+check_dmesg "$LINENO"
+
 _cleanup
 
 exit 0
diff --git a/test/multi-dax.sh b/test/multi-dax.sh
index 04070adb18e4..d471e1c96b5e 100755
--- a/test/multi-dax.sh
+++ b/test/multi-dax.sh
@@ -28,6 +28,8 @@ chardev1=$(echo $json | jq ". | select(.mode == \"devdax\") | .daxregion.devices
 json=$($NDCTL create-namespace -b $NFIT_TEST_BUS0 -r $region -t pmem -m devdax -a $ALIGN_SIZE -s 16M)
 chardev2=$(echo $json | jq ". | select(.mode == \"devdax\") | .daxregion.devices[0].chardev")
 
+check_dmesg "$LINENO"
+
 _cleanup
 
 exit 0

Re: [ndctl PATCH 02/15] ndctl/test: Add kernel backtrace detection to some dax tests

[Alison Schofield]

On Sun, Nov 06, 2022 at 03:46:57PM -0800, Dan Williams wrote:
> It is useful to fail a test if it triggers a backtrace. Generalize the
> mechanism from test/cxl-topology.sh and add it to tests that want
> to validate clean kernel logs.

Useful!  

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  test/common              |   10 ++++++++++
>  test/cxl-region-sysfs.sh |    4 +---
>  test/cxl-topology.sh     |    5 +----
>  test/dax.sh              |    2 ++
>  test/daxdev-errors.sh    |    2 ++
>  test/multi-dax.sh        |    2 ++
>  6 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/test/common b/test/common
> index 65615cc09a3e..44cc352f6009 100644
> --- a/test/common
> +++ b/test/common
> @@ -132,3 +132,13 @@ json2var()
>  {
>  	sed -e "s/[{}\",]//g; s/\[//g; s/\]//g; s/:/=/g"
>  }
> +
> +# check_dmesg
> +# $1: line number where this is called
> +check_dmesg()
> +{
> +	# validate no WARN or lockdep report during the run
> +	log=$(journalctl -r -k --since "-$((SECONDS+1))s")
> +	grep -q "Call Trace" <<< $log && err $1
> +	true
> +}
> diff --git a/test/cxl-region-sysfs.sh b/test/cxl-region-sysfs.sh
> index 63186b60dfec..e128406cd8c8 100644
> --- a/test/cxl-region-sysfs.sh
> +++ b/test/cxl-region-sysfs.sh
> @@ -164,8 +164,6 @@ readarray -t endpoint < <($CXL free-dpa -t pmem ${mem[*]} |
>  			  jq -r ".[] | .decoder.decoder")
>  echo "$region released ${#endpoint[@]} targets: ${endpoint[@]}"
>  
> -# validate no WARN or lockdep report during the run
> -log=$(journalctl -r -k --since "-$((SECONDS+1))s")
> -grep -q "Call Trace" <<< $log && err "$LINENO"
> +check_dmesg "$LINENO"
>  
>  modprobe -r cxl_test
> diff --git a/test/cxl-topology.sh b/test/cxl-topology.sh
> index f7e390d22680..1f15d29f0600 100644
> --- a/test/cxl-topology.sh
> +++ b/test/cxl-topology.sh
> @@ -169,9 +169,6 @@ done
>  # validate that the bus can be disabled without issue
>  $CXL disable-bus $root -f
>  
> -
> -# validate no WARN or lockdep report during the run
> -log=$(journalctl -r -k --since "-$((SECONDS+1))s")
> -grep -q "Call Trace" <<< $log && err "$LINENO"
> +check_dmesg "$LINENO"
>  
>  modprobe -r cxl_test
> diff --git a/test/dax.sh b/test/dax.sh
> index bb9848b10ecc..3ffbc8079eba 100755
> --- a/test/dax.sh
> +++ b/test/dax.sh
> @@ -118,4 +118,6 @@ else
>  	run_xfs
>  fi
>  
> +check_dmesg "$LINENO"
> +
>  exit 0
> diff --git a/test/daxdev-errors.sh b/test/daxdev-errors.sh
> index 7f79718113d0..84ef93499acf 100755
> --- a/test/daxdev-errors.sh
> +++ b/test/daxdev-errors.sh
> @@ -71,6 +71,8 @@ if read sector len < /sys/bus/platform/devices/nfit_test.0/$busdev/$region/badbl
>  fi
>  [ -n "$sector" ] && echo "fail: $LINENO" && exit 1
>  
> +check_dmesg "$LINENO"
> +
>  _cleanup
>  
>  exit 0
> diff --git a/test/multi-dax.sh b/test/multi-dax.sh
> index 04070adb18e4..d471e1c96b5e 100755
> --- a/test/multi-dax.sh
> +++ b/test/multi-dax.sh
> @@ -28,6 +28,8 @@ chardev1=$(echo $json | jq ". | select(.mode == \"devdax\") | .daxregion.devices
>  json=$($NDCTL create-namespace -b $NFIT_TEST_BUS0 -r $region -t pmem -m devdax -a $ALIGN_SIZE -s 16M)
>  chardev2=$(echo $json | jq ". | select(.mode == \"devdax\") | .daxregion.devices[0].chardev")
>  
> +check_dmesg "$LINENO"
> +
>  _cleanup
>  
>  exit 0
> 

[ndctl PATCH 03/15] ndctl/clang-format: Move minimum version to 6

[Dan Williams]

Follow the kernel change that did the same:

sed -i 's/^\(\s*\)#\(\S*\s\+\S*\) # Unknown to clang-format.*/\1\2/' .clang-format

commit 96232c7d4f84 ("clang-format: Update to clang-format >= 6")

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 .clang-format |   36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/.clang-format b/.clang-format
index b6169e15097c..f372823c3248 100644
--- a/.clang-format
+++ b/.clang-format
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 #
-# clang-format configuration file. Intended for clang-format >= 4.
+# clang-format configuration file. Intended for clang-format >= 6.
 # Copied from Linux's .clang-format
 #
 # For more information, see:
@@ -13,7 +13,7 @@ AccessModifierOffset: -4
 AlignAfterOpenBracket: Align
 AlignConsecutiveAssignments: false
 AlignConsecutiveDeclarations: false
-#AlignEscapedNewlines: Left # Unknown to clang-format-4.0
+AlignEscapedNewlines: Left
 AlignOperands: true
 AlignTrailingComments: false
 AllowAllParametersOfDeclarationOnNextLine: false
@@ -37,24 +37,24 @@ BraceWrapping:
   AfterObjCDeclaration: false
   AfterStruct: false
   AfterUnion: false
-  #AfterExternBlock: false # Unknown to clang-format-5.0
+  AfterExternBlock: false
   BeforeCatch: false
   BeforeElse: false
   IndentBraces: false
-  #SplitEmptyFunction: true # Unknown to clang-format-4.0
-  #SplitEmptyRecord: true # Unknown to clang-format-4.0
-  #SplitEmptyNamespace: true # Unknown to clang-format-4.0
+  SplitEmptyFunction: true
+  SplitEmptyRecord: true
+  SplitEmptyNamespace: true
 BreakBeforeBinaryOperators: None
 BreakBeforeBraces: Custom
-#BreakBeforeInheritanceComma: false # Unknown to clang-format-4.0
+BreakBeforeInheritanceComma: false
 BreakBeforeTernaryOperators: false
 BreakConstructorInitializersBeforeComma: false
-#BreakConstructorInitializers: BeforeComma # Unknown to clang-format-4.0
+BreakConstructorInitializers: BeforeComma
 BreakAfterJavaFieldAnnotations: false
 BreakStringLiterals: false
 ColumnLimit: 80
 CommentPragmas: '^ IWYU pragma:'
-#CompactNamespaces: false # Unknown to clang-format-4.0
+CompactNamespaces: false
 ConstructorInitializerAllOnOneLineOrOnePerLine: false
 ConstructorInitializerIndentWidth: 8
 ContinuationIndentWidth: 8
@@ -62,7 +62,7 @@ Cpp11BracedListStyle: false
 DerivePointerAlignment: false
 DisableFormat: false
 ExperimentalAutoDetectBinPacking: false
-#FixNamespaceComments: false # Unknown to clang-format-4.0
+FixNamespaceComments: false
 
 # Taken from:
 # while read -r sym; do
@@ -118,13 +118,13 @@ ForEachMacros:
   - 'ndctl_region_foreach'
   - 'udev_list_entry_foreach'
 
-#IncludeBlocks: Preserve # Unknown to clang-format-5.0
+IncludeBlocks: Preserve
 IncludeCategories:
   - Regex: '.*'
     Priority: 1
 IncludeIsMainRegex: '(Test)?$'
 IndentCaseLabels: false
-#IndentPPDirectives: None # Unknown to clang-format-5.0
+IndentPPDirectives: None
 IndentWidth: 8
 IndentWrappedFunctionNames: false
 JavaScriptQuotes: Leave
@@ -134,13 +134,13 @@ MacroBlockBegin: ''
 MacroBlockEnd: ''
 MaxEmptyLinesToKeep: 1
 NamespaceIndentation: None
-#ObjCBinPackProtocolList: Auto # Unknown to clang-format-5.0
+ObjCBinPackProtocolList: Auto
 ObjCBlockIndentWidth: 8
 ObjCSpaceAfterProperty: true
 ObjCSpaceBeforeProtocolList: true
 
 # Taken from git's rules
-#PenaltyBreakAssignment: 10 # Unknown to clang-format-4.0
+PenaltyBreakAssignment: 10
 PenaltyBreakBeforeFirstCallParameter: 30
 PenaltyBreakComment: 10
 PenaltyBreakFirstLessLess: 0
@@ -151,14 +151,14 @@ PenaltyReturnTypeOnItsOwnLine: 60
 PointerAlignment: Right
 ReflowComments: false
 SortIncludes: false
-#SortUsingDeclarations: false # Unknown to clang-format-4.0
+SortUsingDeclarations: false
 SpaceAfterCStyleCast: false
 SpaceAfterTemplateKeyword: true
 SpaceBeforeAssignmentOperators: true
-#SpaceBeforeCtorInitializerColon: true # Unknown to clang-format-5.0
-#SpaceBeforeInheritanceColon: true # Unknown to clang-format-5.0
+SpaceBeforeCtorInitializerColon: true
+SpaceBeforeInheritanceColon: true
 SpaceBeforeParens: ControlStatements
-#SpaceBeforeRangeBasedForLoopColon: true # Unknown to clang-format-5.0
+SpaceBeforeRangeBasedForLoopColon: true
 SpaceInEmptyParentheses: false
 SpacesBeforeTrailingComments: 1
 SpacesInAngles: false

[ndctl PATCH 04/15] ndctl/clang-format: Fix space after for_each macros

[Dan Williams]

Copy the approach taken in the kernel via:

commit 781121a7f6d1 ("clang-format: Fix space after for_each macros")

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 .clang-format |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.clang-format b/.clang-format
index f372823c3248..448b7e7211ae 100644
--- a/.clang-format
+++ b/.clang-format
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 #
-# clang-format configuration file. Intended for clang-format >= 6.
+# clang-format configuration file. Intended for clang-format >= 11.
 # Copied from Linux's .clang-format
 #
 # For more information, see:
@@ -157,7 +157,7 @@ SpaceAfterTemplateKeyword: true
 SpaceBeforeAssignmentOperators: true
 SpaceBeforeCtorInitializerColon: true
 SpaceBeforeInheritanceColon: true
-SpaceBeforeParens: ControlStatements
+SpaceBeforeParens: ControlStatementsExceptForEachMacros
 SpaceBeforeRangeBasedForLoopColon: true
 SpaceInEmptyParentheses: false
 SpacesBeforeTrailingComments: 1

[ndctl PATCH 05/15] cxl/list: Always attempt to collect child objects

[Dan Williams]

The evolution of the hierarchical listing left warts like the following:

	if (p->memdevs && !p->ports && !p->endpoints) {
		jchilddevs = json_object_new_array();

...whereby it tried to avoid creating a container for child devices if
another container deeper in the hierarchy might supersede the upper-level
containers. I.e. if endpoints are included in the listing then there will
be nothing to report at the bus level. The protection is unnnecessary
because cond_add_put_array_suffix() already handles the case of dropping
empty containers when a lower level container subsumes all the objects.

Moreover, it's a broken check when adding objects at new levels of the
topology. CXL devices attached to an RCH cause memdevs to appear directly
beneath a bus object, and not an intervening port. So in preparation for
that change, delete all the unnecessary special casing for "jchildobj"
container creation.

Reported-by: Vishal Verma <vishal.l.verma@intel.com>
Fixes: 41d6769393f4 ("cxl/list: Move enabled memdevs underneath their endpoint")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 cxl/filter.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cxl/filter.c b/cxl/filter.c
index 56c659965891..040e7deefb3e 100644
--- a/cxl/filter.c
+++ b/cxl/filter.c
@@ -971,7 +971,7 @@ walk_child_ports(struct cxl_port *parent_port, struct cxl_filter_params *p,
 				continue;
 			}
 
-			if (p->memdevs && !p->endpoints) {
+			if (p->memdevs) {
 				jchilddevs = json_object_new_array();
 				if (!jchilddevs) {
 					err(p,
@@ -1151,7 +1151,7 @@ int cxl_filter_walk(struct cxl_ctx *ctx, struct cxl_filter_params *p)
 				}
 			}
 
-			if (p->memdevs && !p->ports && !p->endpoints) {
+			if (p->memdevs) {
 				jchilddevs = json_object_new_array();
 				if (!jchilddevs) {
 					err(p,
@@ -1169,7 +1169,7 @@ int cxl_filter_walk(struct cxl_ctx *ctx, struct cxl_filter_params *p)
 					continue;
 				}
 			}
-			if (p->regions && !p->decoders) {
+			if (p->regions) {
 				jchildregions = json_object_new_array();
 				if (!jchildregions) {
 					err(p,

[ndctl PATCH 06/15] cxl/list: Skip emitting pmem_size when it is zero

[Dan Williams]

The typical case is that CXL devices are pure ram devices. Only emit
capacity sizes when they are non-zero to avoid confusion around whether
pmem is available via partitioning or not.

Do the same for ram_size on the odd case that someone builds a pure pmem
device.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 cxl/json.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/cxl/json.c b/cxl/json.c
index 63c17519aba1..1b1669ab021d 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -305,7 +305,7 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
 {
 	const char *devname = cxl_memdev_get_devname(memdev);
 	struct json_object *jdev, *jobj;
-	unsigned long long serial;
+	unsigned long long serial, size;
 	int numa_node;
 
 	jdev = json_object_new_object();
@@ -316,13 +316,19 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
 	if (jobj)
 		json_object_object_add(jdev, "memdev", jobj);
 
-	jobj = util_json_object_size(cxl_memdev_get_pmem_size(memdev), flags);
-	if (jobj)
-		json_object_object_add(jdev, "pmem_size", jobj);
+	size = cxl_memdev_get_pmem_size(memdev);
+	if (size) {
+		jobj = util_json_object_size(size, flags);
+		if (jobj)
+			json_object_object_add(jdev, "pmem_size", jobj);
+	}
 
-	jobj = util_json_object_size(cxl_memdev_get_ram_size(memdev), flags);
-	if (jobj)
-		json_object_object_add(jdev, "ram_size", jobj);
+	size = cxl_memdev_get_ram_size(memdev);
+	if (size) {
+		jobj = util_json_object_size(size, flags);
+		if (jobj)
+			json_object_object_add(jdev, "ram_size", jobj);
+	}
 
 	if (flags & UTIL_JSON_HEALTH) {
 		jobj = util_cxl_memdev_health_to_json(memdev, flags);

Re: [ndctl PATCH 06/15] cxl/list: Skip emitting pmem_size when it is zero

[Alison Schofield]

On Sun, Nov 06, 2022 at 03:47:20PM -0800, Dan Williams wrote:
> The typical case is that CXL devices are pure ram devices. Only emit
> capacity sizes when they are non-zero to avoid confusion around whether
> pmem is available via partitioning or not.
> 
> Do the same for ram_size on the odd case that someone builds a pure pmem
> device.

Maybe a few more words around what confusion this seeks to avoid.
The confusion being that a user may assign more meaning to the zero
size value than it actually deserves. A zero value for either 
pmem or ram, doesn't indicate the devices capability for either mode.
Use the -I option to cxl list to include paritition info in the
memdev listing. That will explicitly show the ram and pmem capabilities
of the device.


> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  cxl/json.c |   20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/cxl/json.c b/cxl/json.c
> index 63c17519aba1..1b1669ab021d 100644
> --- a/cxl/json.c
> +++ b/cxl/json.c
> @@ -305,7 +305,7 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>  {
>  	const char *devname = cxl_memdev_get_devname(memdev);
>  	struct json_object *jdev, *jobj;
> -	unsigned long long serial;
> +	unsigned long long serial, size;
>  	int numa_node;
>  
>  	jdev = json_object_new_object();
> @@ -316,13 +316,19 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>  	if (jobj)
>  		json_object_object_add(jdev, "memdev", jobj);
>  
> -	jobj = util_json_object_size(cxl_memdev_get_pmem_size(memdev), flags);
> -	if (jobj)
> -		json_object_object_add(jdev, "pmem_size", jobj);
> +	size = cxl_memdev_get_pmem_size(memdev);
> +	if (size) {
> +		jobj = util_json_object_size(size, flags);
> +		if (jobj)
> +			json_object_object_add(jdev, "pmem_size", jobj);
> +	}
>  
> -	jobj = util_json_object_size(cxl_memdev_get_ram_size(memdev), flags);
> -	if (jobj)
> -		json_object_object_add(jdev, "ram_size", jobj);
> +	size = cxl_memdev_get_ram_size(memdev);
> +	if (size) {
> +		jobj = util_json_object_size(size, flags);
> +		if (jobj)
> +			json_object_object_add(jdev, "ram_size", jobj);
> +	}
>  
>  	if (flags & UTIL_JSON_HEALTH) {
>  		jobj = util_cxl_memdev_health_to_json(memdev, flags);
> 

Re: [ndctl PATCH 06/15] cxl/list: Skip emitting pmem_size when it is zero

[Dan Williams]

Alison Schofield wrote:
> On Sun, Nov 06, 2022 at 03:47:20PM -0800, Dan Williams wrote:
> > The typical case is that CXL devices are pure ram devices. Only emit
> > capacity sizes when they are non-zero to avoid confusion around whether
> > pmem is available via partitioning or not.
> > 
> > Do the same for ram_size on the odd case that someone builds a pure pmem
> > device.
> 
> Maybe a few more words around what confusion this seeks to avoid.
> The confusion being that a user may assign more meaning to the zero
> size value than it actually deserves. A zero value for either 
> pmem or ram, doesn't indicate the devices capability for either mode.
> Use the -I option to cxl list to include paritition info in the
> memdev listing. That will explicitly show the ram and pmem capabilities
> of the device.

Nice, I like it. Will append for v2, or maybe Vishal can grab it when
applying? Depends on if I need to respin other patches.

Re: [ndctl PATCH 06/15] cxl/list: Skip emitting pmem_size when it is zero

[Dan Williams]

Alison Schofield wrote:
> On Sun, Nov 06, 2022 at 03:47:20PM -0800, Dan Williams wrote:
> > The typical case is that CXL devices are pure ram devices. Only emit
> > capacity sizes when they are non-zero to avoid confusion around whether
> > pmem is available via partitioning or not.
> > 
> > Do the same for ram_size on the odd case that someone builds a pure pmem
> > device.
> 
> Maybe a few more words around what confusion this seeks to avoid.
> The confusion being that a user may assign more meaning to the zero
> size value than it actually deserves. A zero value for either 
> pmem or ram, doesn't indicate the devices capability for either mode.
> Use the -I option to cxl list to include paritition info in the
> memdev listing. That will explicitly show the ram and pmem capabilities
> of the device.
> 

I went ahead and added this verbatim to the changelog, thanks!

Re: [ndctl PATCH 06/15] cxl/list: Skip emitting pmem_size when it is zero

[Alison Schofield]

On Sun, Nov 06, 2022 at 03:47:20PM -0800, Dan Williams wrote:
> The typical case is that CXL devices are pure ram devices. Only emit
> capacity sizes when they are non-zero to avoid confusion around whether
> pmem is available via partitioning or not.
> 
> Do the same for ram_size on the odd case that someone builds a pure pmem
> device.

The cxl list man page needs a couple of examples updated.

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  cxl/json.c |   20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/cxl/json.c b/cxl/json.c
> index 63c17519aba1..1b1669ab021d 100644
> --- a/cxl/json.c
> +++ b/cxl/json.c
> @@ -305,7 +305,7 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>  {
>  	const char *devname = cxl_memdev_get_devname(memdev);
>  	struct json_object *jdev, *jobj;
> -	unsigned long long serial;
> +	unsigned long long serial, size;
>  	int numa_node;
>  
>  	jdev = json_object_new_object();
> @@ -316,13 +316,19 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>  	if (jobj)
>  		json_object_object_add(jdev, "memdev", jobj);
>  
> -	jobj = util_json_object_size(cxl_memdev_get_pmem_size(memdev), flags);
> -	if (jobj)
> -		json_object_object_add(jdev, "pmem_size", jobj);
> +	size = cxl_memdev_get_pmem_size(memdev);
> +	if (size) {
> +		jobj = util_json_object_size(size, flags);
> +		if (jobj)
> +			json_object_object_add(jdev, "pmem_size", jobj);
> +	}
>  
> -	jobj = util_json_object_size(cxl_memdev_get_ram_size(memdev), flags);
> -	if (jobj)
> -		json_object_object_add(jdev, "ram_size", jobj);
> +	size = cxl_memdev_get_ram_size(memdev);
> +	if (size) {
> +		jobj = util_json_object_size(size, flags);
> +		if (jobj)
> +			json_object_object_add(jdev, "ram_size", jobj);
> +	}
>  
>  	if (flags & UTIL_JSON_HEALTH) {
>  		jobj = util_cxl_memdev_health_to_json(memdev, flags);
> 

Re: [ndctl PATCH 06/15] cxl/list: Skip emitting pmem_size when it is zero

[Dan Williams]

Alison Schofield wrote:
> On Sun, Nov 06, 2022 at 03:47:20PM -0800, Dan Williams wrote:
> > The typical case is that CXL devices are pure ram devices. Only emit
> > capacity sizes when they are non-zero to avoid confusion around whether
> > pmem is available via partitioning or not.
> > 
> > Do the same for ram_size on the odd case that someone builds a pure pmem
> > device.
> 
> The cxl list man page needs a couple of examples updated.

Ah, good catch, will fix.

Re: [ndctl PATCH 06/15] cxl/list: Skip emitting pmem_size when it is zero

[Dan Williams]

Alison Schofield wrote:
> On Sun, Nov 06, 2022 at 03:47:20PM -0800, Dan Williams wrote:
> > The typical case is that CXL devices are pure ram devices. Only emit
> > capacity sizes when they are non-zero to avoid confusion around whether
> > pmem is available via partitioning or not.
> > 
> > Do the same for ram_size on the odd case that someone builds a pure pmem
> > device.
> 
> The cxl list man page needs a couple of examples updated.

Good point, done.

[ndctl PATCH 07/15] cxl/filter: Return json-c topology

[Dan Williams]

In preparation for cxl_filter_walk() to be used to collect and publish cxl
objects for other utilities, return the resulting json_object directly.
Move the responsibility of freeing and optionally printing the object to
the caller.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 cxl/filter.c |   30 ++++++------------------------
 cxl/filter.h |   22 +++++++++++++++++++++-
 cxl/list.c   |    7 ++++++-
 3 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/cxl/filter.c b/cxl/filter.c
index 040e7deefb3e..8499450ded01 100644
--- a/cxl/filter.c
+++ b/cxl/filter.c
@@ -672,23 +672,6 @@ util_cxl_decoder_filter_by_region(struct cxl_decoder *decoder,
 	return decoder;
 }
 
-static unsigned long params_to_flags(struct cxl_filter_params *param)
-{
-	unsigned long flags = 0;
-
-	if (param->idle)
-		flags |= UTIL_JSON_IDLE;
-	if (param->human)
-		flags |= UTIL_JSON_HUMAN;
-	if (param->health)
-		flags |= UTIL_JSON_HEALTH;
-	if (param->targets)
-		flags |= UTIL_JSON_TARGETS;
-	if (param->partition)
-		flags |= UTIL_JSON_PARTITION;
-	return flags;
-}
-
 static void splice_array(struct cxl_filter_params *p, struct json_object *jobjs,
 			 struct json_object *platform,
 			 const char *container_name, bool do_container)
@@ -1027,11 +1010,12 @@ walk_children:
 	}
 }
 
-int cxl_filter_walk(struct cxl_ctx *ctx, struct cxl_filter_params *p)
+struct json_object *cxl_filter_walk(struct cxl_ctx *ctx,
+				    struct cxl_filter_params *p)
 {
 	struct json_object *jdevs = NULL, *jbuses = NULL, *jports = NULL;
 	struct json_object *jplatform = json_object_new_array();
-	unsigned long flags = params_to_flags(p);
+	unsigned long flags = cxl_filter_to_flags(p);
 	struct json_object *jportdecoders = NULL;
 	struct json_object *jbusdecoders = NULL;
 	struct json_object *jepdecoders = NULL;
@@ -1044,7 +1028,7 @@ int cxl_filter_walk(struct cxl_ctx *ctx, struct cxl_filter_params *p)
 
 	if (!jplatform) {
 		dbg(p, "platform object allocation failure\n");
-		return -ENOMEM;
+		return NULL;
 	}
 
 	janondevs = json_object_new_array();
@@ -1232,9 +1216,7 @@ walk_children:
 		     top_level_objs > 1);
 	splice_array(p, jregions, jplatform, "regions", top_level_objs > 1);
 
-	util_display_json_array(stdout, jplatform, flags);
-
-	return 0;
+	return jplatform;
 err:
 	json_object_put(janondevs);
 	json_object_put(jbuses);
@@ -1246,5 +1228,5 @@ err:
 	json_object_put(jepdecoders);
 	json_object_put(jregions);
 	json_object_put(jplatform);
-	return -ENOMEM;
+	return NULL;
 }
diff --git a/cxl/filter.h b/cxl/filter.h
index 256df49c3d0c..2bda6ddd77ca 100644
--- a/cxl/filter.h
+++ b/cxl/filter.h
@@ -5,6 +5,7 @@
 
 #include <stdbool.h>
 #include <util/log.h>
+#include <util/json.h>
 
 struct cxl_filter_params {
 	const char *memdev_filter;
@@ -59,6 +60,25 @@ struct cxl_dport *util_cxl_dport_filter_by_memdev(struct cxl_dport *dport,
 						  const char *serial);
 struct cxl_decoder *util_cxl_decoder_filter(struct cxl_decoder *decoder,
 					    const char *__ident);
-int cxl_filter_walk(struct cxl_ctx *ctx, struct cxl_filter_params *param);
+struct json_object *cxl_filter_walk(struct cxl_ctx *ctx,
+				    struct cxl_filter_params *param);
+
+static inline unsigned long cxl_filter_to_flags(struct cxl_filter_params *param)
+{
+	unsigned long flags = 0;
+
+	if (param->idle)
+		flags |= UTIL_JSON_IDLE;
+	if (param->human)
+		flags |= UTIL_JSON_HUMAN;
+	if (param->health)
+		flags |= UTIL_JSON_HEALTH;
+	if (param->targets)
+		flags |= UTIL_JSON_TARGETS;
+	if (param->partition)
+		flags |= UTIL_JSON_PARTITION;
+	return flags;
+}
+
 bool cxl_filter_has(const char *needle, const char *__filter);
 #endif /* _CXL_UTIL_FILTER_H_ */
diff --git a/cxl/list.c b/cxl/list.c
index 8c48fbbaaec3..2026de2b548b 100644
--- a/cxl/list.c
+++ b/cxl/list.c
@@ -72,6 +72,7 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
 		"cxl list [<options>]",
 		NULL
 	};
+	struct json_object *jtopology;
 	int i;
 
 	argc = parse_options(argc, argv, options, u, 0);
@@ -140,5 +141,9 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
 		param.endpoints = true;
 
 	dbg(&param, "walk topology\n");
-	return cxl_filter_walk(ctx, &param);
+	jtopology = cxl_filter_walk(ctx, &param);
+	if (!jtopology)
+		return -ENOMEM;
+	util_display_json_array(stdout, jtopology, cxl_filter_to_flags(&param));
+	return 0;
 }

[ndctl PATCH 08/15] cxl/list: Record cxl objects in json objects

[Dan Williams]

In preparation for reusing 'cxl list' object selection in other utilities,
like 'cxl create-region', record the associated cxl object in the json
object. For example, enable 'cxl create-region -d decoderX.Y' to lookup the
memdevs that result from 'cxl list -M -d decoderX.Y'.

This sets up future design decisions for code that wants to walk the
topology. It can either open-code its own object walk, or get the json-c
representation of a query and use that. Unless the use case knows exactly
the object it wants it is likely more powerful to specify a
cxl_filter_walk() query and then walk the topology result.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 cxl/json.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/cxl/json.c b/cxl/json.c
index 1b1669ab021d..9264f5fcb21e 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -365,6 +365,8 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
 		if (jobj)
 			json_object_object_add(jdev, "partition_info", jobj);
 	}
+
+	json_object_set_userdata(jdev, memdev, NULL);
 	return jdev;
 }
 
@@ -416,6 +418,7 @@ void util_cxl_dports_append_json(struct json_object *jport,
 			json_object_object_add(jdport, "id", jobj);
 
 		json_object_array_add(jdports, jdport);
+		json_object_set_userdata(jdport, dport, NULL);
 	}
 
 	json_object_object_add(jport, "dports", jdports);
@@ -439,6 +442,7 @@ struct json_object *util_cxl_bus_to_json(struct cxl_bus *bus,
 	if (jobj)
 		json_object_object_add(jbus, "provider", jobj);
 
+	json_object_set_userdata(jbus, bus, NULL);
 	return jbus;
 }
 
@@ -563,6 +567,7 @@ struct json_object *util_cxl_decoder_to_json(struct cxl_decoder *decoder,
 					       jobj);
 	}
 
+	json_object_set_userdata(jdecoder, decoder, NULL);
 	return jdecoder;
 }
 
@@ -621,6 +626,7 @@ void util_cxl_mappings_append_json(struct json_object *jregion,
 			json_object_object_add(jmapping, "decoder", jobj);
 
 		json_object_array_add(jmappings, jmapping);
+		json_object_set_userdata(jmapping, mapping, NULL);
 	}
 
 	json_object_object_add(jregion, "mappings", jmappings);
@@ -686,6 +692,7 @@ struct json_object *util_cxl_region_to_json(struct cxl_region *region,
 
 	util_cxl_mappings_append_json(jregion, region, flags);
 
+	json_object_set_userdata(jregion, region, NULL);
 	return jregion;
 }
 
@@ -751,6 +758,7 @@ void util_cxl_targets_append_json(struct json_object *jdecoder,
 			json_object_object_add(jtarget, "id", jobj);
 
 		json_object_array_add(jtargets, jtarget);
+		json_object_set_userdata(jtarget, target, NULL);
 	}
 
 	json_object_object_add(jdecoder, "targets", jtargets);
@@ -785,6 +793,7 @@ static struct json_object *__util_cxl_port_to_json(struct cxl_port *port,
 			json_object_object_add(jport, "state", jobj);
 	}
 
+	json_object_set_userdata(jport, port, NULL);
 	return jport;
 }
 

[ndctl PATCH 09/15] cxl/region: Make ways an integer argument

[Dan Williams]

Since --ways does not take a unit value like --size, just make it an
integer argument directly and skip the hand coded conversion.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 cxl/region.c |   41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/cxl/region.c b/cxl/region.c
index 334fcc291de7..494da5139c05 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -21,21 +21,23 @@
 static struct region_params {
 	const char *bus;
 	const char *size;
-	const char *ways;
 	const char *granularity;
 	const char *type;
 	const char *root_decoder;
 	const char *region;
+	int ways;
 	bool memdevs;
 	bool force;
 	bool human;
 	bool debug;
-} param;
+} param = {
+	.ways = INT_MAX,
+};
 
 struct parsed_params {
 	u64 size;
 	u64 ep_min_size;
-	unsigned int ways;
+	int ways;
 	unsigned int granularity;
 	const char **targets;
 	int num_targets;
@@ -63,9 +65,8 @@ OPT_BOOLEAN(0, "debug", &param.debug, "turn on debug")
 OPT_STRING('s', "size", &param.size, \
 	   "size in bytes or with a K/M/G etc. suffix", \
 	   "total size desired for the resulting region."), \
-OPT_STRING('w', "ways", &param.ways, \
-	   "number of interleave ways", \
-	   "number of memdevs participating in the regions interleave set"), \
+OPT_INTEGER('w', "ways", &param.ways, \
+	    "number of memdevs participating in the regions interleave set"), \
 OPT_STRING('g', "granularity", \
 	   &param.granularity, "interleave granularity", \
 	   "granularity of the interleave set"), \
@@ -126,15 +127,11 @@ static int parse_create_options(int argc, const char **argv,
 		}
 	}
 
-	if (param.ways) {
-		unsigned long ways = strtoul(param.ways, NULL, 0);
-
-		if (ways == ULONG_MAX || (int)ways <= 0) {
-			log_err(&rl, "Invalid interleave ways: %s\n",
-				param.ways);
-			return -EINVAL;
-		}
-		p->ways = ways;
+	if (param.ways <= 0) {
+		log_err(&rl, "Invalid interleave ways: %d\n", param.ways);
+		return -EINVAL;
+	} else if (param.ways < INT_MAX) {
+		p->ways = param.ways;
 	} else if (argc) {
 		p->ways = argc;
 	} else {
@@ -155,13 +152,13 @@ static int parse_create_options(int argc, const char **argv,
 	}
 
 
-	if (argc > (int)p->ways) {
+	if (argc > p->ways) {
 		for (i = p->ways; i < argc; i++)
 			log_err(&rl, "extra argument: %s\n", p->targets[i]);
 		return -EINVAL;
 	}
 
-	if (argc < (int)p->ways) {
+	if (argc < p->ways) {
 		log_err(&rl,
 			"too few target arguments (%d) for interleave ways (%u)\n",
 			argc, p->ways);
@@ -253,7 +250,7 @@ static bool validate_memdev(struct cxl_memdev *memdev, const char *target,
 
 static int validate_config_memdevs(struct cxl_ctx *ctx, struct parsed_params *p)
 {
-	unsigned int i, matched = 0;
+	int i, matched = 0;
 
 	for (i = 0; i < p->ways; i++) {
 		struct cxl_memdev *memdev;
@@ -393,7 +390,8 @@ static int cxl_region_determine_granularity(struct cxl_region *region,
 					    struct parsed_params *p)
 {
 	const char *devname = cxl_region_get_devname(region);
-	unsigned int granularity, ways;
+	unsigned int granularity;
+	int ways;
 
 	/* Default granularity will be the root decoder's granularity */
 	granularity = cxl_decoder_get_interleave_granularity(p->root_decoder);
@@ -408,7 +406,7 @@ static int cxl_region_determine_granularity(struct cxl_region *region,
 		return granularity;
 
 	ways = cxl_decoder_get_interleave_ways(p->root_decoder);
-	if (ways == 0 || ways == UINT_MAX) {
+	if (ways == 0 || ways == -1) {
 		log_err(&rl, "%s: unable to determine root decoder ways\n",
 			devname);
 		return -ENXIO;
@@ -436,12 +434,11 @@ static int create_region(struct cxl_ctx *ctx, int *count,
 {
 	unsigned long flags = UTIL_JSON_TARGETS;
 	struct json_object *jregion;
-	unsigned int i, granularity;
 	struct cxl_region *region;
+	int i, rc, granularity;
 	u64 size, max_extent;
 	const char *devname;
 	uuid_t uuid;
-	int rc;
 
 	rc = create_region_validate_config(ctx, p);
 	if (rc)

Re: [ndctl PATCH 09/15] cxl/region: Make ways an integer argument

[Alison Schofield]

On Sun, Nov 06, 2022 at 03:47:37PM -0800, Dan Williams wrote:
> Since --ways does not take a unit value like --size, just make it an
> integer argument directly and skip the hand coded conversion.

Just curious why not unsigned int for this and granularity?


> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  cxl/region.c |   41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 334fcc291de7..494da5139c05 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -21,21 +21,23 @@
>  static struct region_params {
>  	const char *bus;
>  	const char *size;
> -	const char *ways;
>  	const char *granularity;
>  	const char *type;
>  	const char *root_decoder;
>  	const char *region;
> +	int ways;
>  	bool memdevs;
>  	bool force;
>  	bool human;
>  	bool debug;
> -} param;
> +} param = {
> +	.ways = INT_MAX,
> +};
>  
>  struct parsed_params {
>  	u64 size;
>  	u64 ep_min_size;
> -	unsigned int ways;
> +	int ways;
>  	unsigned int granularity;
>  	const char **targets;
>  	int num_targets;
> @@ -63,9 +65,8 @@ OPT_BOOLEAN(0, "debug", &param.debug, "turn on debug")
>  OPT_STRING('s', "size", &param.size, \
>  	   "size in bytes or with a K/M/G etc. suffix", \
>  	   "total size desired for the resulting region."), \
> -OPT_STRING('w', "ways", &param.ways, \
> -	   "number of interleave ways", \
> -	   "number of memdevs participating in the regions interleave set"), \
> +OPT_INTEGER('w', "ways", &param.ways, \
> +	    "number of memdevs participating in the regions interleave set"), \
>  OPT_STRING('g', "granularity", \
>  	   &param.granularity, "interleave granularity", \
>  	   "granularity of the interleave set"), \
> @@ -126,15 +127,11 @@ static int parse_create_options(int argc, const char **argv,
>  		}
>  	}
>  
> -	if (param.ways) {
> -		unsigned long ways = strtoul(param.ways, NULL, 0);
> -
> -		if (ways == ULONG_MAX || (int)ways <= 0) {
> -			log_err(&rl, "Invalid interleave ways: %s\n",
> -				param.ways);
> -			return -EINVAL;
> -		}
> -		p->ways = ways;
> +	if (param.ways <= 0) {
> +		log_err(&rl, "Invalid interleave ways: %d\n", param.ways);
> +		return -EINVAL;
> +	} else if (param.ways < INT_MAX) {
> +		p->ways = param.ways;
>  	} else if (argc) {
>  		p->ways = argc;
>  	} else {
> @@ -155,13 +152,13 @@ static int parse_create_options(int argc, const char **argv,
>  	}
>  
>  
> -	if (argc > (int)p->ways) {
> +	if (argc > p->ways) {
>  		for (i = p->ways; i < argc; i++)
>  			log_err(&rl, "extra argument: %s\n", p->targets[i]);
>  		return -EINVAL;
>  	}
>  
> -	if (argc < (int)p->ways) {
> +	if (argc < p->ways) {
>  		log_err(&rl,
>  			"too few target arguments (%d) for interleave ways (%u)\n",
>  			argc, p->ways);
> @@ -253,7 +250,7 @@ static bool validate_memdev(struct cxl_memdev *memdev, const char *target,
>  
>  static int validate_config_memdevs(struct cxl_ctx *ctx, struct parsed_params *p)
>  {
> -	unsigned int i, matched = 0;
> +	int i, matched = 0;
>  
>  	for (i = 0; i < p->ways; i++) {
>  		struct cxl_memdev *memdev;
> @@ -393,7 +390,8 @@ static int cxl_region_determine_granularity(struct cxl_region *region,
>  					    struct parsed_params *p)
>  {
>  	const char *devname = cxl_region_get_devname(region);
> -	unsigned int granularity, ways;
> +	unsigned int granularity;
> +	int ways;
>  
>  	/* Default granularity will be the root decoder's granularity */
>  	granularity = cxl_decoder_get_interleave_granularity(p->root_decoder);
> @@ -408,7 +406,7 @@ static int cxl_region_determine_granularity(struct cxl_region *region,
>  		return granularity;
>  
>  	ways = cxl_decoder_get_interleave_ways(p->root_decoder);
> -	if (ways == 0 || ways == UINT_MAX) {
> +	if (ways == 0 || ways == -1) {
>  		log_err(&rl, "%s: unable to determine root decoder ways\n",
>  			devname);
>  		return -ENXIO;
> @@ -436,12 +434,11 @@ static int create_region(struct cxl_ctx *ctx, int *count,
>  {
>  	unsigned long flags = UTIL_JSON_TARGETS;
>  	struct json_object *jregion;
> -	unsigned int i, granularity;
>  	struct cxl_region *region;
> +	int i, rc, granularity;
>  	u64 size, max_extent;
>  	const char *devname;
>  	uuid_t uuid;
> -	int rc;
>  
>  	rc = create_region_validate_config(ctx, p);
>  	if (rc)
> 

Re: [ndctl PATCH 09/15] cxl/region: Make ways an integer argument

[Dan Williams]

Alison Schofield wrote:
> On Sun, Nov 06, 2022 at 03:47:37PM -0800, Dan Williams wrote:
> > Since --ways does not take a unit value like --size, just make it an
> > integer argument directly and skip the hand coded conversion.
> 
> Just curious why not unsigned int for this and granularity?

Mainly to avoid signed vs unsigned integer comparison with a typical
'int i' iterator variable.

Re: [ndctl PATCH 09/15] cxl/region: Make ways an integer argument

[Verma, Vishal L]

On Sun, 2022-11-06 at 15:47 -0800, Dan Williams wrote:
> Since --ways does not take a unit value like --size, just make it an
> integer argument directly and skip the hand coded conversion.

Ah I had gone back and forth between using an int vs. unsigned. I had
gone with unsigned because the kernel treats it as an unsigned too
behind the sysfs ABI. But I guess in practice it doesn't matter, since
the values are clamped to [256, 16K]. Certainly using an int here makes
things cleaner!

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  cxl/region.c |   41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 334fcc291de7..494da5139c05 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -21,21 +21,23 @@
>  static struct region_params {
>         const char *bus;
>         const char *size;
> -       const char *ways;
>         const char *granularity;
>         const char *type;
>         const char *root_decoder;
>         const char *region;
> +       int ways;
>         bool memdevs;
>         bool force;
>         bool human;
>         bool debug;
> -} param;
> +} param = {
> +       .ways = INT_MAX,
> +};
>  
>  struct parsed_params {
>         u64 size;
>         u64 ep_min_size;
> -       unsigned int ways;
> +       int ways;
>         unsigned int granularity;
>         const char **targets;
>         int num_targets;
> @@ -63,9 +65,8 @@ OPT_BOOLEAN(0, "debug", &param.debug, "turn on
> debug")
>  OPT_STRING('s', "size", &param.size, \
>            "size in bytes or with a K/M/G etc. suffix", \
>            "total size desired for the resulting region."), \
> -OPT_STRING('w', "ways", &param.ways, \
> -          "number of interleave ways", \
> -          "number of memdevs participating in the regions interleave
> set"), \
> +OPT_INTEGER('w', "ways", &param.ways, \
> +           "number of memdevs participating in the regions
> interleave set"), \
>  OPT_STRING('g', "granularity", \
>            &param.granularity, "interleave granularity", \
>            "granularity of the interleave set"), \
> @@ -126,15 +127,11 @@ static int parse_create_options(int argc, const
> char **argv,
>                 }
>         }
>  
> -       if (param.ways) {
> -               unsigned long ways = strtoul(param.ways, NULL, 0);
> -
> -               if (ways == ULONG_MAX || (int)ways <= 0) {
> -                       log_err(&rl, "Invalid interleave ways: %s\n",
> -                               param.ways);
> -                       return -EINVAL;
> -               }
> -               p->ways = ways;
> +       if (param.ways <= 0) {
> +               log_err(&rl, "Invalid interleave ways: %d\n",
> param.ways);
> +               return -EINVAL;
> +       } else if (param.ways < INT_MAX) {
> +               p->ways = param.ways;
>         } else if (argc) {
>                 p->ways = argc;
>         } else {
> @@ -155,13 +152,13 @@ static int parse_create_options(int argc, const
> char **argv,
>         }
>  
>  
> -       if (argc > (int)p->ways) {
> +       if (argc > p->ways) {
>                 for (i = p->ways; i < argc; i++)
>                         log_err(&rl, "extra argument: %s\n", p-
> >targets[i]);
>                 return -EINVAL;
>         }
>  
> -       if (argc < (int)p->ways) {
> +       if (argc < p->ways) {
>                 log_err(&rl,
>                         "too few target arguments (%d) for interleave
> ways (%u)\n",
>                         argc, p->ways);
> @@ -253,7 +250,7 @@ static bool validate_memdev(struct cxl_memdev
> *memdev, const char *target,
>  
>  static int validate_config_memdevs(struct cxl_ctx *ctx, struct
> parsed_params *p)
>  {
> -       unsigned int i, matched = 0;
> +       int i, matched = 0;
>  
>         for (i = 0; i < p->ways; i++) {
>                 struct cxl_memdev *memdev;
> @@ -393,7 +390,8 @@ static int
> cxl_region_determine_granularity(struct cxl_region *region,
>                                             struct parsed_params *p)
>  {
>         const char *devname = cxl_region_get_devname(region);
> -       unsigned int granularity, ways;
> +       unsigned int granularity;
> +       int ways;
>  
>         /* Default granularity will be the root decoder's granularity
> */
>         granularity = cxl_decoder_get_interleave_granularity(p-
> >root_decoder);
> @@ -408,7 +406,7 @@ static int
> cxl_region_determine_granularity(struct cxl_region *region,
>                 return granularity;
>  
>         ways = cxl_decoder_get_interleave_ways(p->root_decoder);
> -       if (ways == 0 || ways == UINT_MAX) {
> +       if (ways == 0 || ways == -1) {
>                 log_err(&rl, "%s: unable to determine root decoder
> ways\n",
>                         devname);
>                 return -ENXIO;
> @@ -436,12 +434,11 @@ static int create_region(struct cxl_ctx *ctx,
> int *count,
>  {
>         unsigned long flags = UTIL_JSON_TARGETS;
>         struct json_object *jregion;
> -       unsigned int i, granularity;
>         struct cxl_region *region;
> +       int i, rc, granularity;
>         u64 size, max_extent;
>         const char *devname;
>         uuid_t uuid;
> -       int rc;
>  
>         rc = create_region_validate_config(ctx, p);
>         if (rc)
> 

[ndctl PATCH 10/15] cxl/region: Make granularity an integer argument

[Dan Williams]

Since --granularity does not take a unit value like --size, just make it an
integer argument directly and skip the hand coded conversion.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 cxl/region.c |   26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/cxl/region.c b/cxl/region.c
index 494da5139c05..c6d7d1a973a8 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -21,24 +21,25 @@
 static struct region_params {
 	const char *bus;
 	const char *size;
-	const char *granularity;
 	const char *type;
 	const char *root_decoder;
 	const char *region;
 	int ways;
+	int granularity;
 	bool memdevs;
 	bool force;
 	bool human;
 	bool debug;
 } param = {
 	.ways = INT_MAX,
+	.granularity = INT_MAX,
 };
 
 struct parsed_params {
 	u64 size;
 	u64 ep_min_size;
 	int ways;
-	unsigned int granularity;
+	int granularity;
 	const char **targets;
 	int num_targets;
 	struct cxl_decoder *root_decoder;
@@ -67,9 +68,8 @@ OPT_STRING('s', "size", &param.size, \
 	   "total size desired for the resulting region."), \
 OPT_INTEGER('w', "ways", &param.ways, \
 	    "number of memdevs participating in the regions interleave set"), \
-OPT_STRING('g', "granularity", \
-	   &param.granularity, "interleave granularity", \
-	   "granularity of the interleave set"), \
+OPT_INTEGER('g', "granularity", &param.granularity,  \
+	    "granularity of the interleave set"), \
 OPT_STRING('t', "type", &param.type, \
 	   "region type", "region type - 'pmem' or 'ram'"), \
 OPT_BOOLEAN('m', "memdevs", &param.memdevs, \
@@ -140,18 +140,15 @@ static int parse_create_options(int argc, const char **argv,
 		return -EINVAL;
 	}
 
-	if (param.granularity) {
-		unsigned long granularity = strtoul(param.granularity, NULL, 0);
-
-		if (granularity == ULONG_MAX || (int)granularity <= 0) {
-			log_err(&rl, "Invalid interleave granularity: %s\n",
+	if (param.granularity < INT_MAX) {
+		if (param.granularity <= 0) {
+			log_err(&rl, "Invalid interleave granularity: %d\n",
 				param.granularity);
 			return -EINVAL;
 		}
-		p->granularity = granularity;
+		p->granularity = param.granularity;
 	}
 
-
 	if (argc > p->ways) {
 		for (i = p->ways; i < argc; i++)
 			log_err(&rl, "extra argument: %s\n", p->targets[i]);
@@ -390,12 +387,11 @@ static int cxl_region_determine_granularity(struct cxl_region *region,
 					    struct parsed_params *p)
 {
 	const char *devname = cxl_region_get_devname(region);
-	unsigned int granularity;
-	int ways;
+	int granularity, ways;
 
 	/* Default granularity will be the root decoder's granularity */
 	granularity = cxl_decoder_get_interleave_granularity(p->root_decoder);
-	if (granularity == 0 || granularity == UINT_MAX) {
+	if (granularity == 0 || granularity == -1) {
 		log_err(&rl, "%s: unable to determine root decoder granularity\n",
 			devname);
 		return -ENXIO;

[ndctl PATCH 11/15] cxl/region: Use cxl_filter_walk() to gather create-region targets

[Dan Williams]

The core of 'cxl list' knows, among other things, how to filter memdevs by
their connectivity to a root decoder, enabled status, and how to identify
memdevs by name, id, serial number. Use the fact that the json-c object
array returned by cxl_filter_walk() also includes the corresponding libcxl
objects to populate and validate the memdev target list for 'cxl
create-region'.

With this in place a default set of memdev targets can be derived from the
specified root decoder, and the connectivity is validated by the same logic
that prepares the hierarchical json topology. The argument list becomes
as tolerant of different id formats as 'cxl list'. For example "mem9" and
"9" are equivalent.

Comma separated lists are also allowed, e.g. "mem9,mem10". However the
sorting of memdevs by filter position falls back to the 'cxl list' order in
that case. In other words:

arg order     region position
mem9 mem10 => [0]: mem9  [1]: mem10
mem10 mem9 => [0]: mem10 [1]: mem9
mem9,mem10 => [0]: mem9  [1]: mem10
mem10,mem9 => [0]: mem9  [1]: mem10

Note that 'cxl list' order groups memdevs by port, later this will need to
augmented with a sort implementation that orders memdevs by a topology
compatible decode order.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 cxl/region.c |  274 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 175 insertions(+), 99 deletions(-)

diff --git a/cxl/region.c b/cxl/region.c
index c6d7d1a973a8..e47709754447 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -40,8 +40,10 @@ struct parsed_params {
 	u64 ep_min_size;
 	int ways;
 	int granularity;
-	const char **targets;
-	int num_targets;
+	struct json_object *memdevs;
+	int num_memdevs;
+	int argc;
+	const char **argv;
 	struct cxl_decoder *root_decoder;
 	enum cxl_decoder_mode mode;
 };
@@ -99,16 +101,148 @@ static const struct option destroy_options[] = {
 	OPT_END(),
 };
 
-static int parse_create_options(int argc, const char **argv,
-				struct parsed_params *p)
+/*
+ * Convert an array of strings into a single comma-separated-value
+ * string that can be passed as a single 'filter' string to
+ * cxl_filter_walk()
+ */
+static const char *to_csv(int count, const char **strings)
 {
+	ssize_t len = count + 1, cursor = 0;
+	char *csv;
 	int i;
 
+	if (!count)
+		return NULL;
+
+	for (i = 0; i < count; i++)
+		len += strlen(strings[i]);
+	csv = calloc(1, len);
+	if (!csv)
+		return NULL;
+	for (i = 0; i < count; i++) {
+		cursor += snprintf(csv + cursor, len - cursor, "%s%s",
+				   strings[i], i + 1 < count ? "," : "");
+		if (cursor >= len) {
+			csv[len] = 0;
+			break;
+		}
+	}
+	return csv;
+}
+
+static struct sort_context {
+	int count;
+	const char **sort;
+} sort_context;
+
+static int memdev_filter_pos(struct json_object *jobj, int count, const char **sort)
+{
+	struct cxl_memdev *memdev = json_object_get_userdata(jobj);
+	int pos;
+
+	for (pos = 0; pos < count; pos++)
+		if (util_cxl_memdev_filter(memdev, sort[pos], NULL))
+			return pos;
+	return count;
+}
+
+static int memdev_sort(const void *a, const void *b)
+{
+	int a_pos, b_pos, count = sort_context.count;
+	const char **sort = sort_context.sort;
+	struct json_object **a_obj, **b_obj;
+
+	a_obj = (struct json_object **) a;
+	b_obj = (struct json_object **) b;
+
+	a_pos = memdev_filter_pos(*a_obj, count, sort);
+	b_pos = memdev_filter_pos(*b_obj, count, sort);
+
+	return a_pos - b_pos;
+}
+
+static struct json_object *collect_memdevs(struct cxl_ctx *ctx,
+					   const char *decoder, int count,
+					   const char **mems)
+{
+	const char *csv = to_csv(count, mems);
+	struct cxl_filter_params filter_params = {
+		.decoder_filter = decoder,
+		.memdevs = true,
+		.memdev_filter = csv,
+	};
+	struct json_object *jmemdevs;
+
+	jmemdevs = cxl_filter_walk(ctx, &filter_params);
+
+	if (!jmemdevs) {
+		log_err(&rl, "failed to retrieve memdevs\n");
+		goto out;
+	}
+
+	if (json_object_array_length(jmemdevs) == 0) {
+		log_err(&rl,
+			"no active memdevs found: decoder: %s filter: %s\n",
+			decoder, csv ? csv : "none");
+		json_object_put(jmemdevs);
+		jmemdevs = NULL;
+		goto out;
+	}
+
+	sort_context = (struct sort_context){
+		.count = count,
+		.sort = mems,
+	};
+	json_object_array_sort(jmemdevs, memdev_sort);
+
+out:
+	free((void *)csv);
+	return jmemdevs;
+}
+
+static bool validate_ways(struct parsed_params *p, int count)
+{
+	/*
+	 * Validate interleave ways against targets found in the topology. If
+	 * the targets were specified, then non-default param.ways must equal
+	 * that number of targets.
+	 */
+	if (p->ways > p->num_memdevs || (count && p->ways != p->num_memdevs)) {
+		log_err(&rl,
+			"Interleave ways %d is %s than number of memdevs %s: %d\n",
+			p->ways, p->ways > p->num_memdevs ? "greater" : "less",
+			count ? "specified" : "found", p->num_memdevs);
+		return false;
+	}
+	return true;
+}
+
+static int parse_create_options(struct cxl_ctx *ctx, int count,
+				const char **mems, struct parsed_params *p)
+{
 	if (!param.root_decoder) {
 		log_err(&rl, "no root decoder specified\n");
 		return -EINVAL;
 	}
 
+	/*
+	 * For all practical purposes, -m is the default target type, but
+	 * hold off on actively making that decision until a second target
+	 * option is available.
+	 */
+	if (!param.memdevs) {
+		log_err(&rl,
+			"must specify option for target object types (-m)\n");
+		return -EINVAL;
+	}
+
+	/* Collect the valid memdevs relative to the given root decoder */
+	p->memdevs = collect_memdevs(ctx, param.root_decoder, count, mems);
+	if (!p->memdevs)
+		return -ENXIO;
+	p->num_memdevs = json_object_array_length(p->memdevs);
+
 	if (param.type) {
 		p->mode = cxl_decoder_mode_from_ident(param.type);
 		if (p->mode == CXL_DECODER_MODE_NONE) {
@@ -132,8 +266,12 @@ static int parse_create_options(int argc, const char **argv,
 		return -EINVAL;
 	} else if (param.ways < INT_MAX) {
 		p->ways = param.ways;
-	} else if (argc) {
-		p->ways = argc;
+		if (!validate_ways(p, count))
+			return -EINVAL;
+	} else if (count) {
+		p->ways = count;
+		if (!validate_ways(p, count))
+			return -EINVAL;
 	} else {
 		log_err(&rl,
 			"couldn't determine interleave ways from options or arguments\n");
@@ -149,19 +287,6 @@ static int parse_create_options(int argc, const char **argv,
 		p->granularity = param.granularity;
 	}
 
-	if (argc > p->ways) {
-		for (i = p->ways; i < argc; i++)
-			log_err(&rl, "extra argument: %s\n", p->targets[i]);
-		return -EINVAL;
-	}
-
-	if (argc < p->ways) {
-		log_err(&rl,
-			"too few target arguments (%d) for interleave ways (%u)\n",
-			argc, p->ways);
-		return -EINVAL;
-	}
-
 	if (p->size && p->ways) {
 		if (p->size % p->ways) {
 			log_err(&rl,
@@ -171,17 +296,6 @@ static int parse_create_options(int argc, const char **argv,
 		}
 	}
 
-	/*
-	 * For all practical purposes, -m is the default target type, but
-	 * hold off on actively making that decision until a second target
-	 * option is available.
-	 */
-	if (!param.memdevs) {
-		log_err(&rl,
-			"must specify option for target object types (-m)\n");
-		return -EINVAL;
-	}
-
 	return 0;
 }
 
@@ -196,8 +310,8 @@ static int parse_region_options(int argc, const char **argv,
 	};
 
 	argc = parse_options(argc, argv, options, u, 0);
-	p->targets = argv;
-	p->num_targets = argc;
+	p->argc = argc;
+	p->argv = argv;
 
 	if (param.debug) {
 		cxl_set_log_priority(ctx, LOG_DEBUG);
@@ -207,62 +321,27 @@ static int parse_region_options(int argc, const char **argv,
 
 	switch(action) {
 	case ACTION_CREATE:
-		return parse_create_options(argc, argv, p);
+		return parse_create_options(ctx, argc, argv, p);
 	default:
 		return 0;
 	}
 }
 
-/**
- * validate_memdev() - match memdev with the target provided,
- *                     and determine its size contribution
- * @memdev: cxl_memdev being tested for a match against the named target
- * @target: target memdev
- * @p:      params structure
- *
- * This is called for each memdev in the system, and only returns 'true' if
- * the memdev name matches the target argument being tested. Additionally,
- * it sets an ep_min_size attribute that always contains the size of the
- * smallest target in the provided list. This is used during the automatic
- * size determination later, to ensure that all targets contribute equally
- * to the region in case of unevenly sized memdevs.
- */
-static bool validate_memdev(struct cxl_memdev *memdev, const char *target,
-			    struct parsed_params *p)
+static void collect_minsize(struct cxl_ctx *ctx, struct parsed_params *p)
 {
-	const char *devname = cxl_memdev_get_devname(memdev);
-	u64 size;
-
-	if (strcmp(devname, target) != 0)
-		return false;
-
-	size = cxl_memdev_get_pmem_size(memdev);
-	if (!p->ep_min_size)
-		p->ep_min_size = size;
-	else
-		p->ep_min_size = min(p->ep_min_size, size);
-
-	return true;
-}
-
-static int validate_config_memdevs(struct cxl_ctx *ctx, struct parsed_params *p)
-{
-	int i, matched = 0;
+	int i;
 
 	for (i = 0; i < p->ways; i++) {
-		struct cxl_memdev *memdev;
+		struct json_object *jobj =
+			json_object_array_get_idx(p->memdevs, i);
+		struct cxl_memdev *memdev = json_object_get_userdata(jobj);
+		u64 size = cxl_memdev_get_pmem_size(memdev);
 
-		cxl_memdev_foreach(ctx, memdev)
-			if (validate_memdev(memdev, p->targets[i], p))
-				matched++;
+		if (!p->ep_min_size)
+			p->ep_min_size = size;
+		else
+			p->ep_min_size = min(p->ep_min_size, size);
 	}
-	if (matched != p->ways) {
-		log_err(&rl,
-			"one or more memdevs not found in CXL topology\n");
-		return -ENXIO;
-	}
-
-	return 0;
 }
 
 static int validate_decoder(struct cxl_decoder *decoder,
@@ -330,26 +409,18 @@ found:
 	if (rc)
 		return rc;
 
-	return validate_config_memdevs(ctx, p);
+	collect_minsize(ctx, p);
+	return 0;
 }
 
-static struct cxl_decoder *
-cxl_memdev_target_find_decoder(struct cxl_ctx *ctx, const char *memdev_name)
+static struct cxl_decoder *cxl_memdev_find_decoder(struct cxl_memdev *memdev)
 {
-	struct cxl_endpoint *ep = NULL;
+	const char *memdev_name = cxl_memdev_get_devname(memdev);
 	struct cxl_decoder *decoder;
-	struct cxl_memdev *memdev;
+	struct cxl_endpoint *ep;
 	struct cxl_port *port;
 
-	cxl_memdev_foreach(ctx, memdev) {
-		const char *devname = cxl_memdev_get_devname(memdev);
-
-		if (strcmp(devname, memdev_name) != 0)
-			continue;
-
-		ep = cxl_memdev_get_endpoint(memdev);
-	}
-
+	ep = cxl_memdev_get_endpoint(memdev);
 	if (!ep) {
 		log_err(&rl, "could not get an endpoint for %s\n",
 			memdev_name);
@@ -488,9 +559,12 @@ static int create_region(struct cxl_ctx *ctx, int *count,
 	try(cxl_region, set_size, region, size);
 
 	for (i = 0; i < p->ways; i++) {
-		struct cxl_decoder *ep_decoder = NULL;
+		struct cxl_decoder *ep_decoder;
+		struct json_object *jobj =
+			json_object_array_get_idx(p->memdevs, i);
+		struct cxl_memdev *memdev = json_object_get_userdata(jobj);
 
-		ep_decoder = cxl_memdev_target_find_decoder(ctx, p->targets[i]);
+		ep_decoder = cxl_memdev_find_decoder(memdev);
 		if (!ep_decoder) {
 			rc = -ENXIO;
 			goto err_delete;
@@ -508,7 +582,7 @@ static int create_region(struct cxl_ctx *ctx, int *count,
 		rc = cxl_region_set_target(region, i, ep_decoder);
 		if (rc) {
 			log_err(&rl, "%s: failed to set target%d to %s\n",
-				devname, i, p->targets[i]);
+				devname, i, cxl_memdev_get_devname(memdev));
 			goto err_delete;
 		}
 	}
@@ -630,8 +704,8 @@ static int decoder_region_action(struct parsed_params *p,
 	cxl_region_foreach_safe (decoder, region, _r) {
 		int i, match = 0;
 
-		for (i = 0; i < p->num_targets; i++) {
-			if (util_cxl_region_filter(region, p->targets[i])) {
+		for (i = 0; i < p->argc; i++) {
+			if (util_cxl_region_filter(region, p->argv[i])) {
 				match = 1;
 				break;
 			}
@@ -664,8 +738,10 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
 	if (rc)
 		return rc;
 
-	if (action == ACTION_CREATE)
-		return create_region(ctx, count, p);
+	if (action == ACTION_CREATE) {
+		rc = create_region(ctx, count, p);
+		json_object_put(p->memdevs);
+	}
 
 	cxl_bus_foreach(ctx, bus) {
 		struct cxl_decoder *decoder;

Re: [ndctl PATCH 11/15] cxl/region: Use cxl_filter_walk() to gather create-region targets

[Verma, Vishal L]

On Sun, 2022-11-06 at 15:47 -0800, Dan Williams wrote:
> > The core of 'cxl list' knows, among other things, how to filter memdevs by
> > their connectivity to a root decoder, enabled status, and how to identify
> > memdevs by name, id, serial number. Use the fact that the json-c object
> > array returned by cxl_filter_walk() also includes the corresponding libcxl
> > objects to populate and validate the memdev target list for 'cxl
> > create-region'.
> > 
> > With this in place a default set of memdev targets can be derived from the
> > specified root decoder, and the connectivity is validated by the same logic
> > that prepares the hierarchical json topology. The argument list becomes
> > as tolerant of different id formats as 'cxl list'. For example "mem9" and
> > "9" are equivalent.
> > 
> > Comma separated lists are also allowed, e.g. "mem9,mem10". However the
> > sorting of memdevs by filter position falls back to the 'cxl list' order in
> > that case. In other words:
> > 
> > arg order     region position
> > mem9 mem10 => [0]: mem9  [1]: mem10
> > mem10 mem9 => [0]: mem10 [1]: mem9
> > mem9,mem10 => [0]: mem9  [1]: mem10
> > mem10,mem9 => [0]: mem9  [1]: mem10

Hm, this does create an awkward situation where

 cxl create-region -m mem10 mem9 (same as first arg order above)

can behave differently from

 cxl create-region -m "mem10 mem9" (behaves like 4th arg order above)

i.e. if the args happen to get quoted into a single space separated
list, then it switches back to cxl-list ordering as "mem10 mem9" gets
treated as a single filter string.

I wonder if we should add another step after collect_memdevs() (or
change memdev_sort()) to return the arg order by default, so:

 mem9 mem10 => [0]: mem9 [1]: mem10
 mem10 mem9 => [0]: mem10 [1]: mem9
 mem9,mem10 => [0]: mem9 [1]: mem10
 mem10,mem9 => [0]: mem10 [1]: mem9
 "mem9 mem10" => [0]: mem9 [1]: mem10
 "mem10 mem9" => [0]: mem10 [1]: mem9

i.e. region positioning stays consistent no matter what combination of
field separators, or quoting, or word splitting ends up happening.

Then (future patches) add a --relax-ordering or --allow-reordering
option that can fix up the order for creating a region successfully
with the given set.

With this, the only two options create-region has are either try the
user-specified order exactly, or reorder to something that wil be
successful. The cxl-list default order doesn't seem as a useful 'mode'
to have as it can be hit-or-miss.
> > 
> > Note that 'cxl list' order groups memdevs by port, later this will need to
> > augmented with a sort implementation that orders memdevs by a topology
> > compatible decode order.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  cxl/region.c |  274 +++++++++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 175 insertions(+), 99 deletions(-)
> > 
> > diff --git a/cxl/region.c b/cxl/region.c
> > index c6d7d1a973a8..e47709754447 100644
> > --- a/cxl/region.c
> > +++ b/cxl/region.c
> > @@ -40,8 +40,10 @@ struct parsed_params {
> >         u64 ep_min_size;
> >         int ways;
> >         int granularity;
> > -       const char **targets;
> > -       int num_targets;
> > +       struct json_object *memdevs;
> > +       int num_memdevs;
> > +       int argc;
> > +       const char **argv;
> >         struct cxl_decoder *root_decoder;
> >         enum cxl_decoder_mode mode;
> >  };
> > @@ -99,16 +101,148 @@ static const struct option destroy_options[] = {
> >         OPT_END(),
> >  };
> >  
> > -static int parse_create_options(int argc, const char **argv,
> > -                               struct parsed_params *p)
> > +/*
> > + * Convert an array of strings into a single comma-separated-value
> > + * string that can be passed as a single 'filter' string to
> > + * cxl_filter_walk()
> > + */
> > +static const char *to_csv(int count, const char **strings)
> >  {
> > +       ssize_t len = count + 1, cursor = 0;
> > +       char *csv;
> >         int i;
> >  
> > +       if (!count)
> > +               return NULL;
> > +
> > +       for (i = 0; i < count; i++)
> > +               len += strlen(strings[i]);
> > +       csv = calloc(1, len);
> > +       if (!csv)
> > +               return NULL;
> > +       for (i = 0; i < count; i++) {
> > +               cursor += snprintf(csv + cursor, len - cursor, "%s%s",
> > +                                  strings[i], i + 1 < count ? "," : "");
> > +               if (cursor >= len) {
> > +                       csv[len] = 0;
> > +                       break;
> > +               }
> > +       }
> > +       return csv;
> > +}
> > +
> > +static struct sort_context {
> > +       int count;
> > +       const char **sort;
> > +} sort_context;
> > +
> > +static int memdev_filter_pos(struct json_object *jobj, int count, const char **sort)
> > +{
> > +       struct cxl_memdev *memdev = json_object_get_userdata(jobj);
> > +       int pos;
> > +
> > +       for (pos = 0; pos < count; pos++)
> > +               if (util_cxl_memdev_filter(memdev, sort[pos], NULL))
> > +                       return pos;
> > +       return count;
> > +}
> > +
> > +static int memdev_sort(const void *a, const void *b)
> > +{
> > +       int a_pos, b_pos, count = sort_context.count;
> > +       const char **sort = sort_context.sort;
> > +       struct json_object **a_obj, **b_obj;
> > +
> > +       a_obj = (struct json_object **) a;
> > +       b_obj = (struct json_object **) b;
> > +
> > +       a_pos = memdev_filter_pos(*a_obj, count, sort);
> > +       b_pos = memdev_filter_pos(*b_obj, count, sort);
> > +
> > +       return a_pos - b_pos;
> > +}
> > +
> > +static struct json_object *collect_memdevs(struct cxl_ctx *ctx,
> > +                                          const char *decoder, int count,
> > +                                          const char **mems)
> > +{
> > +       const char *csv = to_csv(count, mems);
> > +       struct cxl_filter_params filter_params = {
> > +               .decoder_filter = decoder,
> > +               .memdevs = true,
> > +               .memdev_filter = csv,
> > +       };
> > +       struct json_object *jmemdevs;
> > +
> > +       jmemdevs = cxl_filter_walk(ctx, &filter_params);
> > +
> > +       if (!jmemdevs) {
> > +               log_err(&rl, "failed to retrieve memdevs\n");
> > +               goto out;
> > +       }
> > +
> > +       if (json_object_array_length(jmemdevs) == 0) {
> > +               log_err(&rl,
> > +                       "no active memdevs found: decoder: %s filter: %s\n",
> > +                       decoder, csv ? csv : "none");
> > +               json_object_put(jmemdevs);
> > +               jmemdevs = NULL;
> > +               goto out;
> > +       }
> > +
> > +       sort_context = (struct sort_context){
> > +               .count = count,
> > +               .sort = mems,
> > +       };
> > +       json_object_array_sort(jmemdevs, memdev_sort);
> > +
> > +out:
> > +       free((void *)csv);
> > +       return jmemdevs;
> > +}
> > +
> > +static bool validate_ways(struct parsed_params *p, int count)
> > +{
> > +       /*
> > +        * Validate interleave ways against targets found in the topology. If
> > +        * the targets were specified, then non-default param.ways must equal
> > +        * that number of targets.
> > +        */
> > +       if (p->ways > p->num_memdevs || (count && p->ways != p->num_memdevs)) {

This falls over when doing something like

 cxl create-region -m mem0,mem1

as @count ends up being '1' from the single filter spec passed in.

I think doing a 'count = p->num_memdevs' (below) should fix it - we
don't care about how many separate filter specs were passed in, once
they have been combined, and the resulting memdevfs collected.

> > +               log_err(&rl,
> > +                       "Interleave ways %d is %s than number of memdevs %s: %d\n",
> > +                       p->ways, p->ways > p->num_memdevs ? "greater" : "less",
> > +                       count ? "specified" : "found", p->num_memdevs);
> > +               return false;
> > +       }
> > +       return true;
> > +}
> > +
> > +static int parse_create_options(struct cxl_ctx *ctx, int count,
> > +                               const char **mems, struct parsed_params *p)
> > +{
> >         if (!param.root_decoder) {
> >                 log_err(&rl, "no root decoder specified\n");
> >                 return -EINVAL;
> >         }
> >  
> > +       /*
> > +        * For all practical purposes, -m is the default target type, but
> > +        * hold off on actively making that decision until a second target
> > +        * option is available.
> > +        */
> > +       if (!param.memdevs) {
> > +               log_err(&rl,
> > +                       "must specify option for target object types (-m)\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       /* Collect the valid memdevs relative to the given root decoder */
> > +       p->memdevs = collect_memdevs(ctx, param.root_decoder, count, mems);
> > +       if (!p->memdevs)
> > +               return -ENXIO;
> > +       p->num_memdevs = json_object_array_length(p->memdevs);

Per the above comment, either

 count = p->num_memdevs;

here, or alternatively, the interleave ways validation shouldn't use
@count anymore.

> > +
> >         if (param.type) {
> >                 p->mode = cxl_decoder_mode_from_ident(param.type);
> >                 if (p->mode == CXL_DECODER_MODE_NONE) {

<snip>

> > 
> > @@ -664,8 +738,10 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
> >         if (rc)
> >                 return rc;
> >  
> > -       if (action == ACTION_CREATE)
> > -               return create_region(ctx, count, p);
> > +       if (action == ACTION_CREATE) {
> > +               rc = create_region(ctx, count, p);
> > +               json_object_put(p->memdevs);

Should this dereference happen just before returning from
create_region() since that is where cxl_filter_walk() was called from,
and is also where p->memdevs is last used.

> > +       }
> >  
> >         cxl_bus_foreach(ctx, bus) {
> >                 struct cxl_decoder *decoder;
> > 

Re: [ndctl PATCH 11/15] cxl/region: Use cxl_filter_walk() to gather create-region targets

[Dan Williams]

Verma, Vishal L wrote:
> On Sun, 2022-11-06 at 15:47 -0800, Dan Williams wrote:
> > > The core of 'cxl list' knows, among other things, how to filter memdevs by
> > > their connectivity to a root decoder, enabled status, and how to identify
> > > memdevs by name, id, serial number. Use the fact that the json-c object
> > > array returned by cxl_filter_walk() also includes the corresponding libcxl
> > > objects to populate and validate the memdev target list for 'cxl
> > > create-region'.
> > > 
> > > With this in place a default set of memdev targets can be derived from the
> > > specified root decoder, and the connectivity is validated by the same logic
> > > that prepares the hierarchical json topology. The argument list becomes
> > > as tolerant of different id formats as 'cxl list'. For example "mem9" and
> > > "9" are equivalent.
> > > 
> > > Comma separated lists are also allowed, e.g. "mem9,mem10". However the
> > > sorting of memdevs by filter position falls back to the 'cxl list' order in
> > > that case. In other words:
> > > 
> > > arg order     region position
> > > mem9 mem10 => [0]: mem9  [1]: mem10
> > > mem10 mem9 => [0]: mem10 [1]: mem9
> > > mem9,mem10 => [0]: mem9  [1]: mem10
> > > mem10,mem9 => [0]: mem9  [1]: mem10
> 
> Hm, this does create an awkward situation where
> 
>  cxl create-region -m mem10 mem9 (same as first arg order above)
> 
> can behave differently from
> 
>  cxl create-region -m "mem10 mem9" (behaves like 4th arg order above)
> 
> i.e. if the args happen to get quoted into a single space separated
> list, then it switches back to cxl-list ordering as "mem10 mem9" gets
> treated as a single filter string.
> 
> I wonder if we should add another step after collect_memdevs() (or
> change memdev_sort()) to return the arg order by default, so:
> 
>  mem9 mem10 => [0]: mem9 [1]: mem10
>  mem10 mem9 => [0]: mem10 [1]: mem9
>  mem9,mem10 => [0]: mem9 [1]: mem10
>  mem10,mem9 => [0]: mem10 [1]: mem9
>  "mem9 mem10" => [0]: mem9 [1]: mem10
>  "mem10 mem9" => [0]: mem10 [1]: mem9
> 
> i.e. region positioning stays consistent no matter what combination of
> field separators, or quoting, or word splitting ends up happening.
> 
> Then (future patches) add a --relax-ordering or --allow-reordering
> option that can fix up the order for creating a region successfully
> with the given set.
> 
> With this, the only two options create-region has are either try the
> user-specified order exactly, or reorder to something that wil be
> successful. The cxl-list default order doesn't seem as a useful 'mode'
> to have as it can be hit-or-miss.

Yeah, that timebomb is probably not something I should leave ticking.
Fixed up now, but with the 'fun' of C string manipulation.

> > > 
> > > Note that 'cxl list' order groups memdevs by port, later this will need to
> > > augmented with a sort implementation that orders memdevs by a topology
> > > compatible decode order.
> > > 
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > ---
> > >  cxl/region.c |  274 +++++++++++++++++++++++++++++++++++++---------------------
> > >  1 file changed, 175 insertions(+), 99 deletions(-)
> > > 
> > > diff --git a/cxl/region.c b/cxl/region.c
> > > index c6d7d1a973a8..e47709754447 100644
> > > --- a/cxl/region.c
> > > +++ b/cxl/region.c
> > > @@ -40,8 +40,10 @@ struct parsed_params {
> > >         u64 ep_min_size;
> > >         int ways;
> > >         int granularity;
> > > -       const char **targets;
> > > -       int num_targets;
> > > +       struct json_object *memdevs;
> > > +       int num_memdevs;
> > > +       int argc;
> > > +       const char **argv;
> > >         struct cxl_decoder *root_decoder;
> > >         enum cxl_decoder_mode mode;
> > >  };
> > > @@ -99,16 +101,148 @@ static const struct option destroy_options[] = {
> > >         OPT_END(),
> > >  };
> > >  
> > > -static int parse_create_options(int argc, const char **argv,
> > > -                               struct parsed_params *p)
> > > +/*
> > > + * Convert an array of strings into a single comma-separated-value
> > > + * string that can be passed as a single 'filter' string to
> > > + * cxl_filter_walk()
> > > + */
> > > +static const char *to_csv(int count, const char **strings)
> > >  {
> > > +       ssize_t len = count + 1, cursor = 0;
> > > +       char *csv;
> > >         int i;
> > >  
> > > +       if (!count)
> > > +               return NULL;
> > > +
> > > +       for (i = 0; i < count; i++)
> > > +               len += strlen(strings[i]);
> > > +       csv = calloc(1, len);
> > > +       if (!csv)
> > > +               return NULL;
> > > +       for (i = 0; i < count; i++) {
> > > +               cursor += snprintf(csv + cursor, len - cursor, "%s%s",
> > > +                                  strings[i], i + 1 < count ? "," : "");
> > > +               if (cursor >= len) {
> > > +                       csv[len] = 0;
> > > +                       break;
> > > +               }
> > > +       }
> > > +       return csv;
> > > +}
> > > +
> > > +static struct sort_context {
> > > +       int count;
> > > +       const char **sort;
> > > +} sort_context;
> > > +
> > > +static int memdev_filter_pos(struct json_object *jobj, int count, const char **sort)
> > > +{
> > > +       struct cxl_memdev *memdev = json_object_get_userdata(jobj);
> > > +       int pos;
> > > +
> > > +       for (pos = 0; pos < count; pos++)
> > > +               if (util_cxl_memdev_filter(memdev, sort[pos], NULL))
> > > +                       return pos;
> > > +       return count;
> > > +}
> > > +
> > > +static int memdev_sort(const void *a, const void *b)
> > > +{
> > > +       int a_pos, b_pos, count = sort_context.count;
> > > +       const char **sort = sort_context.sort;
> > > +       struct json_object **a_obj, **b_obj;
> > > +
> > > +       a_obj = (struct json_object **) a;
> > > +       b_obj = (struct json_object **) b;
> > > +
> > > +       a_pos = memdev_filter_pos(*a_obj, count, sort);
> > > +       b_pos = memdev_filter_pos(*b_obj, count, sort);
> > > +
> > > +       return a_pos - b_pos;
> > > +}
> > > +
> > > +static struct json_object *collect_memdevs(struct cxl_ctx *ctx,
> > > +                                          const char *decoder, int count,
> > > +                                          const char **mems)
> > > +{
> > > +       const char *csv = to_csv(count, mems);
> > > +       struct cxl_filter_params filter_params = {
> > > +               .decoder_filter = decoder,
> > > +               .memdevs = true,
> > > +               .memdev_filter = csv,
> > > +       };
> > > +       struct json_object *jmemdevs;
> > > +
> > > +       jmemdevs = cxl_filter_walk(ctx, &filter_params);
> > > +
> > > +       if (!jmemdevs) {
> > > +               log_err(&rl, "failed to retrieve memdevs\n");
> > > +               goto out;
> > > +       }
> > > +
> > > +       if (json_object_array_length(jmemdevs) == 0) {
> > > +               log_err(&rl,
> > > +                       "no active memdevs found: decoder: %s filter: %s\n",
> > > +                       decoder, csv ? csv : "none");
> > > +               json_object_put(jmemdevs);
> > > +               jmemdevs = NULL;
> > > +               goto out;
> > > +       }
> > > +
> > > +       sort_context = (struct sort_context){
> > > +               .count = count,
> > > +               .sort = mems,
> > > +       };
> > > +       json_object_array_sort(jmemdevs, memdev_sort);
> > > +
> > > +out:
> > > +       free((void *)csv);
> > > +       return jmemdevs;
> > > +}
> > > +
> > > +static bool validate_ways(struct parsed_params *p, int count)
> > > +{
> > > +       /*
> > > +        * Validate interleave ways against targets found in the topology. If
> > > +        * the targets were specified, then non-default param.ways must equal
> > > +        * that number of targets.
> > > +        */
> > > +       if (p->ways > p->num_memdevs || (count && p->ways != p->num_memdevs)) {
> 
> This falls over when doing something like
> 
>  cxl create-region -m mem0,mem1
> 
> as @count ends up being '1' from the single filter spec passed in.
> 
> I think doing a 'count = p->num_memdevs' (below) should fix it - we
> don't care about how many separate filter specs were passed in, once
> they have been combined, and the resulting memdevfs collected.

I think it matters because if someone typos something like "mem8, mem8,
mem10" we want the tool to validate with @count at 3 even though
num_memdevs would only return 2 in that case.

I am solving this by turning the @count argument into an in/out that
gets updated to be the number of individual elements of a target list.
Where @count is 4 for this target list on entry:

  "mem1,mem2" mem3 mem4 "mem5 mem6"

...and 6 on return from collect_memdevs().

> 
> > > +               log_err(&rl,
> > > +                       "Interleave ways %d is %s than number of memdevs %s: %d\n",
> > > +                       p->ways, p->ways > p->num_memdevs ? "greater" : "less",
> > > +                       count ? "specified" : "found", p->num_memdevs);
> > > +               return false;
> > > +       }
> > > +       return true;
> > > +}
> > > +
> > > +static int parse_create_options(struct cxl_ctx *ctx, int count,
> > > +                               const char **mems, struct parsed_params *p)
> > > +{
> > >         if (!param.root_decoder) {
> > >                 log_err(&rl, "no root decoder specified\n");
> > >                 return -EINVAL;
> > >         }
> > >  
> > > +       /*
> > > +        * For all practical purposes, -m is the default target type, but
> > > +        * hold off on actively making that decision until a second target
> > > +        * option is available.
> > > +        */
> > > +       if (!param.memdevs) {
> > > +               log_err(&rl,
> > > +                       "must specify option for target object types (-m)\n");
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       /* Collect the valid memdevs relative to the given root decoder */
> > > +       p->memdevs = collect_memdevs(ctx, param.root_decoder, count, mems);
> > > +       if (!p->memdevs)
> > > +               return -ENXIO;
> > > +       p->num_memdevs = json_object_array_length(p->memdevs);
> 
> Per the above comment, either
> 
>  count = p->num_memdevs;
> 
> here, or alternatively, the interleave ways validation shouldn't use
> @count anymore.
> 
> > > +
> > >         if (param.type) {
> > >                 p->mode = cxl_decoder_mode_from_ident(param.type);
> > >                 if (p->mode == CXL_DECODER_MODE_NONE) {
> 
> <snip>
> 
> > > 
> > > @@ -664,8 +738,10 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
> > >         if (rc)
> > >                 return rc;
> > >  
> > > -       if (action == ACTION_CREATE)
> > > -               return create_region(ctx, count, p);
> > > +       if (action == ACTION_CREATE) {
> > > +               rc = create_region(ctx, count, p);
> > > +               json_object_put(p->memdevs);
> 
> Should this dereference happen just before returning from
> create_region() since that is where cxl_filter_walk() was called from,
> and is also where p->memdevs is last used.

Yeah, not sure why I had it outside.

[ndctl PATCH 12/15] cxl/region: Trim region size by max available extent

[Dan Williams]

When a size is not specified, limit the size to either the available DPA
capacity, or the max available extent in the root decoder, whichever is
smaller.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 cxl/region.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/cxl/region.c b/cxl/region.c
index e47709754447..aa0735194fa1 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -502,6 +502,7 @@ static int create_region(struct cxl_ctx *ctx, int *count,
 	unsigned long flags = UTIL_JSON_TARGETS;
 	struct json_object *jregion;
 	struct cxl_region *region;
+	bool default_size = true;
 	int i, rc, granularity;
 	u64 size, max_extent;
 	const char *devname;
@@ -513,6 +514,7 @@ static int create_region(struct cxl_ctx *ctx, int *count,
 
 	if (p->size) {
 		size = p->size;
+		default_size = false;
 	} else if (p->ep_min_size) {
 		size = p->ep_min_size * p->ways;
 	} else {
@@ -525,13 +527,16 @@ static int create_region(struct cxl_ctx *ctx, int *count,
 			cxl_decoder_get_devname(p->root_decoder));
 		return -EINVAL;
 	}
-	if (size > max_extent) {
+	if (!default_size && size > max_extent) {
 		log_err(&rl,
 			"%s: region size %#lx exceeds max available space\n",
 			cxl_decoder_get_devname(p->root_decoder), size);
 		return -ENOSPC;
 	}
 
+	if (size > max_extent)
+		size = ALIGN_DOWN(max_extent, SZ_256M * p->ways);
+
 	if (p->mode == CXL_DECODER_MODE_PMEM) {
 		region = cxl_decoder_create_pmem_region(p->root_decoder);
 		if (!region) {

[ndctl PATCH 13/15] cxl/region: Default to memdev mode for create with no arguments

[Dan Williams]

Allow for:

   cxl create-region -d decoderX.Y

...to assume (-m -w $(count of memdevs beneath decoderX.Y))

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 cxl/region.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/cxl/region.c b/cxl/region.c
index aa0735194fa1..c0cf4ab350da 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -227,10 +227,13 @@ static int parse_create_options(struct cxl_ctx *ctx, int count,
 	}
 
 	/*
-	 * For all practical purposes, -m is the default target type, but
-	 * hold off on actively making that decision until a second target
-	 * option is available.
+	 * For all practical purposes, -m is the default target type, but hold
+	 * off on actively making that decision until a second target option is
+	 * available. Unless there are no arguments then just assume memdevs.
 	 */
+	if (!count)
+		param.memdevs = true;
+
 	if (!param.memdevs) {
 		log_err(&rl,
 			"must specify option for target object types (-m)\n");
@@ -272,11 +275,8 @@ static int parse_create_options(struct cxl_ctx *ctx, int count,
 		p->ways = count;
 		if (!validate_ways(p, count))
 			return -EINVAL;
-	} else {
-		log_err(&rl,
-			"couldn't determine interleave ways from options or arguments\n");
-		return -EINVAL;
-	}
+	} else
+		p->ways = p->num_memdevs;
 
 	if (param.granularity < INT_MAX) {
 		if (param.granularity <= 0) {

Re: [ndctl PATCH 13/15] cxl/region: Default to memdev mode for create with no arguments

[Alison Schofield]

On Sun, Nov 06, 2022 at 03:48:01PM -0800, Dan Williams wrote:
> Allow for:
> 
>    cxl create-region -d decoderX.Y
> 
> ...to assume (-m -w $(count of memdevs beneath decoderX.Y))

I'm not understanding what the change is here. Poked around a bit
and still didn't get it. Help!

Leaving out the -m leads to this:
$ cxl create-region -d decoder3.3 mem0 mem1
cxl region: parse_create_options: must specify option for target object types (-m)
cxl region: cmd_create_region: created 0 regions

Leaving out the the -m and the memdevs fails because the memdev order is
not correct. 
$ cxl create-region -d decoder3.3
cxl region: create_region: region5: failed to set target0 to mem1
cxl region: cmd_create_region: created 0 regions

This still works, where I give the -m and the correct order of memdevs.
cxl create-region -m -d decoder3.3 mem0 mem1

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  cxl/region.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index aa0735194fa1..c0cf4ab350da 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -227,10 +227,13 @@ static int parse_create_options(struct cxl_ctx *ctx, int count,
>  	}
>  
>  	/*
> -	 * For all practical purposes, -m is the default target type, but
> -	 * hold off on actively making that decision until a second target
> -	 * option is available.
> +	 * For all practical purposes, -m is the default target type, but hold
> +	 * off on actively making that decision until a second target option is
> +	 * available. Unless there are no arguments then just assume memdevs.
>  	 */
> +	if (!count)
> +		param.memdevs = true;
> +
>  	if (!param.memdevs) {
>  		log_err(&rl,
>  			"must specify option for target object types (-m)\n");
> @@ -272,11 +275,8 @@ static int parse_create_options(struct cxl_ctx *ctx, int count,
>  		p->ways = count;
>  		if (!validate_ways(p, count))
>  			return -EINVAL;
> -	} else {
> -		log_err(&rl,
> -			"couldn't determine interleave ways from options or arguments\n");
> -		return -EINVAL;
> -	}
> +	} else
> +		p->ways = p->num_memdevs;
>  
>  	if (param.granularity < INT_MAX) {
>  		if (param.granularity <= 0) {
> 

Re: [ndctl PATCH 13/15] cxl/region: Default to memdev mode for create with no arguments

[Dan Williams]

Alison Schofield wrote:
> On Sun, Nov 06, 2022 at 03:48:01PM -0800, Dan Williams wrote:
> > Allow for:
> > 
> >    cxl create-region -d decoderX.Y
> > 
> > ...to assume (-m -w $(count of memdevs beneath decoderX.Y))
> 
> I'm not understanding what the change is here. Poked around a bit
> and still didn't get it. Help!
> 
> Leaving out the -m leads to this:
> $ cxl create-region -d decoder3.3 mem0 mem1
> cxl region: parse_create_options: must specify option for target object types (-m)
> cxl region: cmd_create_region: created 0 regions
> 
> Leaving out the the -m and the memdevs fails because the memdev order is
> not correct. 
> $ cxl create-region -d decoder3.3
> cxl region: create_region: region5: failed to set target0 to mem1
> cxl region: cmd_create_region: created 0 regions
> 
> This still works, where I give the -m and the correct order of memdevs.
> cxl create-region -m -d decoder3.3 mem0 mem1

Oh, I was not expecting the lack of automatic memdev sorting to rear its
head so quickly, and thought that "cxl list" order was good enough for
most configurations.

Can provide more details on your configuration in this case? If this is
current cxl_test then I already do not expect it to work with anything
but decoder3.4 since the other decoders have more complicated ordering
constraints.

I.e. your:

cxl create-region -d decoder3.3

...worked as expected in that it found some memdevs to attempt to create
the region, but you got unlucky in the sense that the default order that
'cxl list' returns memdevs was incompatible with creating a region.

Re: [ndctl PATCH 13/15] cxl/region: Default to memdev mode for create with no arguments

[Alison Schofield]

On Mon, Nov 07, 2022 at 03:48:43PM -0800, Dan Williams wrote:
> Alison Schofield wrote:
> > On Sun, Nov 06, 2022 at 03:48:01PM -0800, Dan Williams wrote:
> > > Allow for:
> > > 
> > >    cxl create-region -d decoderX.Y
> > > 
> > > ...to assume (-m -w $(count of memdevs beneath decoderX.Y))
> > 
> > I'm not understanding what the change is here. Poked around a bit
> > and still didn't get it. Help!
> > 
> > Leaving out the -m leads to this:
> > $ cxl create-region -d decoder3.3 mem0 mem1
> > cxl region: parse_create_options: must specify option for target object types (-m)
> > cxl region: cmd_create_region: created 0 regions
> > 
> > Leaving out the the -m and the memdevs fails because the memdev order is
> > not correct. 
> > $ cxl create-region -d decoder3.3
> > cxl region: create_region: region5: failed to set target0 to mem1
> > cxl region: cmd_create_region: created 0 regions
> > 
> > This still works, where I give the -m and the correct order of memdevs.
> > cxl create-region -m -d decoder3.3 mem0 mem1
> 
> Oh, I was not expecting the lack of automatic memdev sorting to rear its
> head so quickly, and thought that "cxl list" order was good enough for
> most configurations.

I wasn't clear on what was being advertised as supported with this
change. I didn't read this as an announcement of automatic region
creation, but it seemed you were hinting at it.

> 
> Can provide more details on your configuration in this case? If this is
> current cxl_test then I already do not expect it to work with anything
> but decoder3.4 since the other decoders have more complicated ordering
> constraints.
> 
> I.e. your:
> 
> cxl create-region -d decoder3.3
> 
> ...worked as expected in that it found some memdevs to attempt to create
> the region, but you got unlucky in the sense that the default order that
> 'cxl list' returns memdevs was incompatible with creating a region.

Pretty much exactly as you say above. My example was w cxl_test
decoder3.3, w 2 HB's. The automagic worked fine w decoder3.4 w 1 HB.

Re: [ndctl PATCH 13/15] cxl/region: Default to memdev mode for create with no arguments

[Dan Williams]

Alison Schofield wrote:
> On Sun, Nov 06, 2022 at 03:48:01PM -0800, Dan Williams wrote:
> > Allow for:
> > 
> >    cxl create-region -d decoderX.Y
> > 
> > ...to assume (-m -w $(count of memdevs beneath decoderX.Y))
> 
> I'm not understanding what the change is here. Poked around a bit
> and still didn't get it. Help!
> 
> Leaving out the -m leads to this:
> $ cxl create-region -d decoder3.3 mem0 mem1
> cxl region: parse_create_options: must specify option for target object types (-m)
> cxl region: cmd_create_region: created 0 regions
> 
> Leaving out the the -m and the memdevs fails because the memdev order is
> not correct. 
> $ cxl create-region -d decoder3.3
> cxl region: create_region: region5: failed to set target0 to mem1
> cxl region: cmd_create_region: created 0 regions
> 
> This still works, where I give the -m and the correct order of memdevs.
> cxl create-region -m -d decoder3.3 mem0 mem1

I fixed up the man page to clarify that this is a possibility when
eliding the target list:

diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
index e0e6818cfdd1..286779eff9ed 100644
--- a/Documentation/cxl/cxl-create-region.txt
+++ b/Documentation/cxl/cxl-create-region.txt
@@ -53,16 +53,18 @@ OPTIONS
 -------
 <target(s)>::
 The CXL targets that should be used to form the region. The number of
-'target' arguments must match the '--ways' option (if provided). The
-targets are memdev names such as 'mem0', 'mem1' etc.
+'target' arguments must match the '--ways' option (if provided).
 
 include::bus-option.txt[]
 
 -m::
 --memdevs::
        Indicate that the non-option arguments for 'target(s)' refer to memdev
-       names. Currently this is the only option supported, and must be
-       specified.
+       device names. If this option is omitted and no targets are specified
+       then create-region uses the equivalent of 'cxl list -M -d $decoder'
+       internally as the target list. Note that depending on the topology, for
+       example with switches, the automatic target list ordering may not be
+       valid and manual specification of the target list is required.
 
 -s::
 --size=::

> 
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  cxl/region.c |   16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/cxl/region.c b/cxl/region.c
> > index aa0735194fa1..c0cf4ab350da 100644
> > --- a/cxl/region.c
> > +++ b/cxl/region.c
> > @@ -227,10 +227,13 @@ static int parse_create_options(struct cxl_ctx *ctx, int count,
> >  	}
> >  
> >  	/*
> > -	 * For all practical purposes, -m is the default target type, but
> > -	 * hold off on actively making that decision until a second target
> > -	 * option is available.
> > +	 * For all practical purposes, -m is the default target type, but hold
> > +	 * off on actively making that decision until a second target option is
> > +	 * available. Unless there are no arguments then just assume memdevs.
> >  	 */
> > +	if (!count)
> > +		param.memdevs = true;
> > +
> >  	if (!param.memdevs) {
> >  		log_err(&rl,
> >  			"must specify option for target object types (-m)\n");
> > @@ -272,11 +275,8 @@ static int parse_create_options(struct cxl_ctx *ctx, int count,
> >  		p->ways = count;
> >  		if (!validate_ways(p, count))
> >  			return -EINVAL;
> > -	} else {
> > -		log_err(&rl,
> > -			"couldn't determine interleave ways from options or arguments\n");
> > -		return -EINVAL;
> > -	}
> > +	} else
> > +		p->ways = p->num_memdevs;
> >  
> >  	if (param.granularity < INT_MAX) {
> >  		if (param.granularity <= 0) {
> > 

[ndctl PATCH 14/15] cxl/test: Extend cxl-topology.sh for a single root-port host-bridge

[Dan Williams]

A recent extension of cxl_test adds 2 memory devices attached through a
switch to a single ported host-bridge to reproduce a bug report.

Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Link: http://lore.kernel.org/r/20221010172057.00001559@huawei.com
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 test/cxl-topology.sh |   48 +++++++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/test/cxl-topology.sh b/test/cxl-topology.sh
index 1f15d29f0600..f1e0a2b01e98 100644
--- a/test/cxl-topology.sh
+++ b/test/cxl-topology.sh
@@ -29,27 +29,30 @@ count=$(jq "length" <<< $json)
 root=$(jq -r ".[] | .bus" <<< $json)
 
 
-# validate 2 host bridges under a root port
+# validate 2 or 3 host bridges under a root port
 port_sort="sort_by(.port | .[4:] | tonumber)"
 json=$($CXL list -b cxl_test -BP)
 count=$(jq ".[] | .[\"ports:$root\"] | length" <<< $json)
-((count == 2)) || err "$LINENO"
+((count == 2)) || ((count == 3)) || err "$LINENO"
+bridges=$count
 
 bridge[0]=$(jq -r ".[] | .[\"ports:$root\"] | $port_sort | .[0].port" <<< $json)
 bridge[1]=$(jq -r ".[] | .[\"ports:$root\"] | $port_sort | .[1].port" <<< $json)
+((bridges > 2)) && bridge[2]=$(jq -r ".[] | .[\"ports:$root\"] | $port_sort | .[2].port" <<< $json)
 
+# validate root ports per host bridge
+check_host_bridge()
+{
+	json=$($CXL list -b cxl_test -T -p $1)
+	count=$(jq ".[] | .dports | length" <<< $json)
+	((count == $2)) || err "$3"
+}
 
-# validate 2 root ports per host bridge
-json=$($CXL list -b cxl_test -T -p ${bridge[0]})
-count=$(jq ".[] | .dports | length" <<< $json)
-((count == 2)) || err "$LINENO"
-
-json=$($CXL list -b cxl_test -T -p ${bridge[1]})
-count=$(jq ".[] | .dports | length" <<< $json)
-((count == 2)) || err "$LINENO"
+check_host_bridge ${bridge[0]} 2 $LINENO
+check_host_bridge ${bridge[1]} 2 $LINENO
+((bridges > 2)) && check_host_bridge ${bridge[2]} 1 $LINENO
 
-
-# validate 2 switches per-root port
+# validate 2 switches per root-port
 json=$($CXL list -b cxl_test -P -p ${bridge[0]})
 count=$(jq ".[] | .[\"ports:${bridge[0]}\"] | length" <<< $json)
 ((count == 2)) || err "$LINENO"
@@ -65,9 +68,9 @@ switch[2]=$(jq -r ".[] | .[\"ports:${bridge[1]}\"] | $port_sort | .[0].host" <<<
 switch[3]=$(jq -r ".[] | .[\"ports:${bridge[1]}\"] | $port_sort | .[1].host" <<< $json)
 
 
-# validate the expected properties of the 4 root decoders
-# use the size of the first decoder to determine the cxl_test version /
-# properties
+# validate the expected properties of the 4 or 5 root decoders
+# use the size of the first decoder to determine the
+# cxl_test version / properties
 json=$($CXL list -b cxl_test -D -d root)
 port_id=${root:4}
 port_id_len=${#port_id}
@@ -103,12 +106,19 @@ count=$(jq "[ $decoder_sort | .[3] |
 	select(.nr_targets == 2) ] | length" <<< $json)
 ((count == 1)) || err "$LINENO"
 
+if [ $bridges -eq 3 ]; then
+	count=$(jq "[ $decoder_sort | .[4] |
+		select(.pmem_capable == true) |
+		select(.size == $decoder_base_size) |
+		select(.nr_targets == 1) ] | length" <<< $json)
+	((count == 1)) || err "$LINENO"
+fi
 
-# check that all 8 cxl_test memdevs are enabled by default and have a
+# check that all 8 or 10 cxl_test memdevs are enabled by default and have a
 # pmem size of 256M, or 1G
 json=$($CXL list -b cxl_test -M)
 count=$(jq "map(select(.pmem_size == $pmem_size)) | length" <<< $json)
-((count == 8)) || err "$LINENO"
+((bridges == 2 && count == 8 || bridges == 3 && count == 10)) || err "$LINENO"
 
 
 # check that switch ports disappear after all of their memdevs have been
@@ -151,8 +161,8 @@ do
 done
 
 
-# validate host bridge tear down
-for b in ${bridge[@]}
+# validate host bridge tear down for the first 2 bridges
+for b in ${bridge[0]} ${bridge[1]}
 do
 	$CXL disable-port $b -f
 	json=$($CXL list -M -i -p $b)

Re: [ndctl PATCH 14/15] cxl/test: Extend cxl-topology.sh for a single root-port host-bridge

[Alison Schofield]

On Sun, Nov 06, 2022 at 03:48:07PM -0800, Dan Williams wrote:
> A recent extension of cxl_test adds 2 memory devices attached through a
> switch to a single ported host-bridge to reproduce a bug report.
> 
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Link: http://lore.kernel.org/r/20221010172057.00001559@huawei.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Tested-by: Alison Schofield <alison.schofield@intel.com>

> ---
>  test/cxl-topology.sh |   48 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/test/cxl-topology.sh b/test/cxl-topology.sh
> index 1f15d29f0600..f1e0a2b01e98 100644
> --- a/test/cxl-topology.sh
> +++ b/test/cxl-topology.sh
> @@ -29,27 +29,30 @@ count=$(jq "length" <<< $json)
>  root=$(jq -r ".[] | .bus" <<< $json)
>  
>  
> -# validate 2 host bridges under a root port
> +# validate 2 or 3 host bridges under a root port
>  port_sort="sort_by(.port | .[4:] | tonumber)"
>  json=$($CXL list -b cxl_test -BP)
>  count=$(jq ".[] | .[\"ports:$root\"] | length" <<< $json)
> -((count == 2)) || err "$LINENO"
> +((count == 2)) || ((count == 3)) || err "$LINENO"
> +bridges=$count
>  
>  bridge[0]=$(jq -r ".[] | .[\"ports:$root\"] | $port_sort | .[0].port" <<< $json)
>  bridge[1]=$(jq -r ".[] | .[\"ports:$root\"] | $port_sort | .[1].port" <<< $json)
> +((bridges > 2)) && bridge[2]=$(jq -r ".[] | .[\"ports:$root\"] | $port_sort | .[2].port" <<< $json)
>  
> +# validate root ports per host bridge
> +check_host_bridge()
> +{
> +	json=$($CXL list -b cxl_test -T -p $1)
> +	count=$(jq ".[] | .dports | length" <<< $json)
> +	((count == $2)) || err "$3"
> +}
>  
> -# validate 2 root ports per host bridge
> -json=$($CXL list -b cxl_test -T -p ${bridge[0]})
> -count=$(jq ".[] | .dports | length" <<< $json)
> -((count == 2)) || err "$LINENO"
> -
> -json=$($CXL list -b cxl_test -T -p ${bridge[1]})
> -count=$(jq ".[] | .dports | length" <<< $json)
> -((count == 2)) || err "$LINENO"
> +check_host_bridge ${bridge[0]} 2 $LINENO
> +check_host_bridge ${bridge[1]} 2 $LINENO
> +((bridges > 2)) && check_host_bridge ${bridge[2]} 1 $LINENO
>  
> -
> -# validate 2 switches per-root port
> +# validate 2 switches per root-port
>  json=$($CXL list -b cxl_test -P -p ${bridge[0]})
>  count=$(jq ".[] | .[\"ports:${bridge[0]}\"] | length" <<< $json)
>  ((count == 2)) || err "$LINENO"
> @@ -65,9 +68,9 @@ switch[2]=$(jq -r ".[] | .[\"ports:${bridge[1]}\"] | $port_sort | .[0].host" <<<
>  switch[3]=$(jq -r ".[] | .[\"ports:${bridge[1]}\"] | $port_sort | .[1].host" <<< $json)
>  
>  
> -# validate the expected properties of the 4 root decoders
> -# use the size of the first decoder to determine the cxl_test version /
> -# properties
> +# validate the expected properties of the 4 or 5 root decoders
> +# use the size of the first decoder to determine the
> +# cxl_test version / properties
>  json=$($CXL list -b cxl_test -D -d root)
>  port_id=${root:4}
>  port_id_len=${#port_id}
> @@ -103,12 +106,19 @@ count=$(jq "[ $decoder_sort | .[3] |
>  	select(.nr_targets == 2) ] | length" <<< $json)
>  ((count == 1)) || err "$LINENO"
>  
> +if [ $bridges -eq 3 ]; then
> +	count=$(jq "[ $decoder_sort | .[4] |
> +		select(.pmem_capable == true) |
> +		select(.size == $decoder_base_size) |
> +		select(.nr_targets == 1) ] | length" <<< $json)
> +	((count == 1)) || err "$LINENO"
> +fi
>  
> -# check that all 8 cxl_test memdevs are enabled by default and have a
> +# check that all 8 or 10 cxl_test memdevs are enabled by default and have a
>  # pmem size of 256M, or 1G
>  json=$($CXL list -b cxl_test -M)
>  count=$(jq "map(select(.pmem_size == $pmem_size)) | length" <<< $json)
> -((count == 8)) || err "$LINENO"
> +((bridges == 2 && count == 8 || bridges == 3 && count == 10)) || err "$LINENO"
>  
>  
>  # check that switch ports disappear after all of their memdevs have been
> @@ -151,8 +161,8 @@ do
>  done
>  
>  
> -# validate host bridge tear down
> -for b in ${bridge[@]}
> +# validate host bridge tear down for the first 2 bridges
> +for b in ${bridge[0]} ${bridge[1]}
>  do
>  	$CXL disable-port $b -f
>  	json=$($CXL list -M -i -p $b)
> 

Re: [ndctl PATCH 14/15] cxl/test: Extend cxl-topology.sh for a single root-port host-bridge

[Verma, Vishal L]

On Sun, 2022-11-06 at 15:48 -0800, Dan Williams wrote:
> A recent extension of cxl_test adds 2 memory devices attached through a
> switch to a single ported host-bridge to reproduce a bug report.
> 
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Link: http://lore.kernel.org/r/20221010172057.00001559@huawei.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  test/cxl-topology.sh |   48 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 19 deletions(-)

This looks good, just a minor nit below.

> 
> diff --git a/test/cxl-topology.sh b/test/cxl-topology.sh
> index 1f15d29f0600..f1e0a2b01e98 100644
> --- a/test/cxl-topology.sh
> +++ b/test/cxl-topology.sh
> @@ -29,27 +29,30 @@ count=$(jq "length" <<< $json)
>  root=$(jq -r ".[] | .bus" <<< $json)
>  
>  
> -# validate 2 host bridges under a root port
> +# validate 2 or 3 host bridges under a root port
>  port_sort="sort_by(.port | .[4:] | tonumber)"
>  json=$($CXL list -b cxl_test -BP)
>  count=$(jq ".[] | .[\"ports:$root\"] | length" <<< $json)
> -((count == 2)) || err "$LINENO"
> +((count == 2)) || ((count == 3)) || err "$LINENO"
> +bridges=$count
>  
>  bridge[0]=$(jq -r ".[] | .[\"ports:$root\"] | $port_sort | .[0].port" <<< $json)
>  bridge[1]=$(jq -r ".[] | .[\"ports:$root\"] | $port_sort | .[1].port" <<< $json)
> +((bridges > 2)) && bridge[2]=$(jq -r ".[] | .[\"ports:$root\"] | $port_sort | .[2].port" <<< $json)
>  
> +# validate root ports per host bridge
> +check_host_bridge()
> +{
> +       json=$($CXL list -b cxl_test -T -p $1)
> +       count=$(jq ".[] | .dports | length" <<< $json)
> +       ((count == $2)) || err "$3"
> +}
>  
> -# validate 2 root ports per host bridge
> -json=$($CXL list -b cxl_test -T -p ${bridge[0]})
> -count=$(jq ".[] | .dports | length" <<< $json)
> -((count == 2)) || err "$LINENO"
> -
> -json=$($CXL list -b cxl_test -T -p ${bridge[1]})
> -count=$(jq ".[] | .dports | length" <<< $json)
> -((count == 2)) || err "$LINENO"
> +check_host_bridge ${bridge[0]} 2 $LINENO
> +check_host_bridge ${bridge[1]} 2 $LINENO
> +((bridges > 2)) && check_host_bridge ${bridge[2]} 1 $LINENO
>  
> -
> -# validate 2 switches per-root port
> +# validate 2 switches per root-port
>  json=$($CXL list -b cxl_test -P -p ${bridge[0]})
>  count=$(jq ".[] | .[\"ports:${bridge[0]}\"] | length" <<< $json)
>  ((count == 2)) || err "$LINENO"
> @@ -65,9 +68,9 @@ switch[2]=$(jq -r ".[] | .[\"ports:${bridge[1]}\"] | $port_sort | .[0].host" <<<
>  switch[3]=$(jq -r ".[] | .[\"ports:${bridge[1]}\"] | $port_sort | .[1].host" <<< $json)
>  
>  
> -# validate the expected properties of the 4 root decoders
> -# use the size of the first decoder to determine the cxl_test version /
> -# properties
> +# validate the expected properties of the 4 or 5 root decoders
> +# use the size of the first decoder to determine the
> +# cxl_test version / properties
>  json=$($CXL list -b cxl_test -D -d root)
>  port_id=${root:4}
>  port_id_len=${#port_id}
> @@ -103,12 +106,19 @@ count=$(jq "[ $decoder_sort | .[3] |
>         select(.nr_targets == 2) ] | length" <<< $json)
>  ((count == 1)) || err "$LINENO"
>  
> +if [ $bridges -eq 3 ]; then

The $bridges should be quoted if using the posix sh style [ ],
or use the bash style without quoting: if [[ $bridges == "3" ]]
or use a 'math' context: if (( bridges == 3 ))

> +       count=$(jq "[ $decoder_sort | .[4] |
> +               select(.pmem_capable == true) |
> +               select(.size == $decoder_base_size) |
> +               select(.nr_targets == 1) ] | length" <<< $json)
> +       ((count == 1)) || err "$LINENO"
> +fi
>  
> -# check that all 8 cxl_test memdevs are enabled by default and have a
> +# check that all 8 or 10 cxl_test memdevs are enabled by default and have a
>  # pmem size of 256M, or 1G
>  json=$($CXL list -b cxl_test -M)
>  count=$(jq "map(select(.pmem_size == $pmem_size)) | length" <<< $json)
> -((count == 8)) || err "$LINENO"
> +((bridges == 2 && count == 8 || bridges == 3 && count == 10)) || err "$LINENO"
>  
>  
>  # check that switch ports disappear after all of their memdevs have been
> @@ -151,8 +161,8 @@ do
>  done
>  
>  
> -# validate host bridge tear down
> -for b in ${bridge[@]}
> +# validate host bridge tear down for the first 2 bridges
> +for b in ${bridge[0]} ${bridge[1]}
>  do
>         $CXL disable-port $b -f
>         json=$($CXL list -M -i -p $b)
> 

Re: [ndctl PATCH 14/15] cxl/test: Extend cxl-topology.sh for a single root-port host-bridge

[Dan Williams]

Verma, Vishal L wrote:
> On Sun, 2022-11-06 at 15:48 -0800, Dan Williams wrote:
> > A recent extension of cxl_test adds 2 memory devices attached through a
> > switch to a single ported host-bridge to reproduce a bug report.
> > 
> > Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Link: http://lore.kernel.org/r/20221010172057.00001559@huawei.com
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  test/cxl-topology.sh |   48 +++++++++++++++++++++++++++++-------------------
> >  1 file changed, 29 insertions(+), 19 deletions(-)
> 
> This looks good, just a minor nit below.
> 
> > 
> > diff --git a/test/cxl-topology.sh b/test/cxl-topology.sh
> > index 1f15d29f0600..f1e0a2b01e98 100644
> > --- a/test/cxl-topology.sh
> > +++ b/test/cxl-topology.sh
> > @@ -29,27 +29,30 @@ count=$(jq "length" <<< $json)
> >  root=$(jq -r ".[] | .bus" <<< $json)
> >  
> >  
> > -# validate 2 host bridges under a root port
> > +# validate 2 or 3 host bridges under a root port
> >  port_sort="sort_by(.port | .[4:] | tonumber)"
> >  json=$($CXL list -b cxl_test -BP)
> >  count=$(jq ".[] | .[\"ports:$root\"] | length" <<< $json)
> > -((count == 2)) || err "$LINENO"
> > +((count == 2)) || ((count == 3)) || err "$LINENO"
> > +bridges=$count
> >  
> >  bridge[0]=$(jq -r ".[] | .[\"ports:$root\"] | $port_sort | .[0].port" <<< $json)
> >  bridge[1]=$(jq -r ".[] | .[\"ports:$root\"] | $port_sort | .[1].port" <<< $json)
> > +((bridges > 2)) && bridge[2]=$(jq -r ".[] | .[\"ports:$root\"] | $port_sort | .[2].port" <<< $json)
> >  
> > +# validate root ports per host bridge
> > +check_host_bridge()
> > +{
> > +       json=$($CXL list -b cxl_test -T -p $1)
> > +       count=$(jq ".[] | .dports | length" <<< $json)
> > +       ((count == $2)) || err "$3"
> > +}
> >  
> > -# validate 2 root ports per host bridge
> > -json=$($CXL list -b cxl_test -T -p ${bridge[0]})
> > -count=$(jq ".[] | .dports | length" <<< $json)
> > -((count == 2)) || err "$LINENO"
> > -
> > -json=$($CXL list -b cxl_test -T -p ${bridge[1]})
> > -count=$(jq ".[] | .dports | length" <<< $json)
> > -((count == 2)) || err "$LINENO"
> > +check_host_bridge ${bridge[0]} 2 $LINENO
> > +check_host_bridge ${bridge[1]} 2 $LINENO
> > +((bridges > 2)) && check_host_bridge ${bridge[2]} 1 $LINENO
> >  
> > -
> > -# validate 2 switches per-root port
> > +# validate 2 switches per root-port
> >  json=$($CXL list -b cxl_test -P -p ${bridge[0]})
> >  count=$(jq ".[] | .[\"ports:${bridge[0]}\"] | length" <<< $json)
> >  ((count == 2)) || err "$LINENO"
> > @@ -65,9 +68,9 @@ switch[2]=$(jq -r ".[] | .[\"ports:${bridge[1]}\"] | $port_sort | .[0].host" <<<
> >  switch[3]=$(jq -r ".[] | .[\"ports:${bridge[1]}\"] | $port_sort | .[1].host" <<< $json)
> >  
> >  
> > -# validate the expected properties of the 4 root decoders
> > -# use the size of the first decoder to determine the cxl_test version /
> > -# properties
> > +# validate the expected properties of the 4 or 5 root decoders
> > +# use the size of the first decoder to determine the
> > +# cxl_test version / properties
> >  json=$($CXL list -b cxl_test -D -d root)
> >  port_id=${root:4}
> >  port_id_len=${#port_id}
> > @@ -103,12 +106,19 @@ count=$(jq "[ $decoder_sort | .[3] |
> >         select(.nr_targets == 2) ] | length" <<< $json)
> >  ((count == 1)) || err "$LINENO"
> >  
> > +if [ $bridges -eq 3 ]; then
> 
> The $bridges should be quoted if using the posix sh style [ ],
> or use the bash style without quoting: if [[ $bridges == "3" ]]
> or use a 'math' context: if (( bridges == 3 ))

Ah, got it.

[ndctl PATCH 15/15] cxl/test: Test single-port host-bridge region creation

[Dan Williams]

The original port decoder programming algorithm in the kernel failed to
acommodate the corner case of a passthrough port connected to a fan-out
port. Use the 5th cxl_test decoder to regression test this scenario.

Reported-by: Bobo WL <lmw.bobo@gmail.com>
Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Link: http://lore.kernel.org/r/20221010172057.00001559@huawei.com
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 test/cxl-create-region.sh |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/test/cxl-create-region.sh b/test/cxl-create-region.sh
index 82aad3a7285a..47aed44848ab 100644
--- a/test/cxl-create-region.sh
+++ b/test/cxl-create-region.sh
@@ -110,6 +110,34 @@ create_subregions()
 	done
 }
 
+create_single()
+{
+	# the 5th cxl_test decoder is expected to target a single-port
+	# host-bridge. Older cxl_test implementations may not define it,
+	# so skip the test in that case.
+	decoder=$($CXL list -b cxl_test -D -d root |
+		  jq -r ".[4] |
+		  select(.pmem_capable == true) |
+		  select(.nr_targets == 1) |
+		  .decoder")
+
+        if [[ ! $decoder ]]; then
+                echo "no single-port host-bridge decoder found, skipping"
+                return
+        fi
+
+	region=$($CXL create-region -d "$decoder" | jq -r ".region")
+	if [[ ! $region ]]; then
+		echo "failed to create single-port host-bridge region"
+		err "$LINENO"
+	fi
+
+	destroy_regions "$region"
+}
+
+# test region creation on devices behind a single-port host-bridge
+create_single
+
 # test reading labels directly through cxl-cli
 readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
 

Re: [ndctl PATCH 15/15] cxl/test: Test single-port host-bridge region creation

[Alison Schofield]

On Sun, Nov 06, 2022 at 03:48:13PM -0800, Dan Williams wrote:
> The original port decoder programming algorithm in the kernel failed to
> acommodate the corner case of a passthrough port connected to a fan-out
> port. Use the 5th cxl_test decoder to regression test this scenario.
> 
> Reported-by: Bobo WL <lmw.bobo@gmail.com>
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Link: http://lore.kernel.org/r/20221010172057.00001559@huawei.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Tested-by: Alison Schofield <alison.schofield@intel.com>

> ---
>  test/cxl-create-region.sh |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/test/cxl-create-region.sh b/test/cxl-create-region.sh
> index 82aad3a7285a..47aed44848ab 100644
> --- a/test/cxl-create-region.sh
> +++ b/test/cxl-create-region.sh
> @@ -110,6 +110,34 @@ create_subregions()
>  	done
>  }
>  
> +create_single()
> +{
> +	# the 5th cxl_test decoder is expected to target a single-port
> +	# host-bridge. Older cxl_test implementations may not define it,
> +	# so skip the test in that case.
> +	decoder=$($CXL list -b cxl_test -D -d root |
> +		  jq -r ".[4] |
> +		  select(.pmem_capable == true) |
> +		  select(.nr_targets == 1) |
> +		  .decoder")
> +
> +        if [[ ! $decoder ]]; then
> +                echo "no single-port host-bridge decoder found, skipping"
> +                return
> +        fi
> +
> +	region=$($CXL create-region -d "$decoder" | jq -r ".region")
> +	if [[ ! $region ]]; then
> +		echo "failed to create single-port host-bridge region"
> +		err "$LINENO"
> +	fi
> +
> +	destroy_regions "$region"
> +}
> +
> +# test region creation on devices behind a single-port host-bridge
> +create_single
> +
>  # test reading labels directly through cxl-cli
>  readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
>  
>