linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] cxl: Random clean ups
@ 2023-05-17 21:28 Ira Weiny
  2023-05-17 21:28 ` [PATCH 1/3] MAINTAINERS: Add additional reviewers for CXL Ira Weiny
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Ira Weiny @ 2023-05-17 21:28 UTC (permalink / raw)
  To: Dan Williams, Alison Schofield, Vishal Verma, Dave Jiang,
	Ben Widawsky, Jonathan Cameron
  Cc: linux-cxl, linux-kernel, Ira Weiny

These 3 clean ups were found while working on other code.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
Ira Weiny (3):
      MAINTAINERS: Add additional reviewers for CXL
      cxl/pci: Update comment
      tools/testing/cxl: Document test configurations

 MAINTAINERS                  |  2 ++
 drivers/cxl/pci.c            |  5 ++-
 tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 79 insertions(+), 3 deletions(-)
---
base-commit: bfe58458fd2557c9a81b89bc0ff10eb03d6c0745
change-id: 20230426-cxl-fixes-85b66bd605e0

Best regards,
-- 
Ira Weiny <ira.weiny@intel.com>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/3] MAINTAINERS: Add additional reviewers for CXL
  2023-05-17 21:28 [PATCH 0/3] cxl: Random clean ups Ira Weiny
@ 2023-05-17 21:28 ` Ira Weiny
  2023-05-17 21:29   ` Dave Jiang
  2023-05-18  9:36   ` Jonathan Cameron
  2023-05-17 21:28 ` [PATCH 2/3] cxl/pci: Update comment Ira Weiny
  2023-05-17 21:28 ` [PATCH 3/3] tools/testing/cxl: Document test configurations Ira Weiny
  2 siblings, 2 replies; 15+ messages in thread
From: Ira Weiny @ 2023-05-17 21:28 UTC (permalink / raw)
  To: Dan Williams, Alison Schofield, Vishal Verma, Dave Jiang,
	Ben Widawsky, Jonathan Cameron
  Cc: linux-cxl, linux-kernel, Ira Weiny

To get better coverage add Dave Jiang and Jonathan Cameron as reviewers.

Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 90abe83c02f3..6f8568ec2731 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5169,6 +5169,8 @@ M:	Vishal Verma <vishal.l.verma@intel.com>
 M:	Ira Weiny <ira.weiny@intel.com>
 M:	Ben Widawsky <bwidawsk@kernel.org>
 M:	Dan Williams <dan.j.williams@intel.com>
+R:	Dave Jiang <dave.jiang@intel.com>
+R:	Jonathan Cameron <Jonathan.Cameron@Huawei.com>
 L:	linux-cxl@vger.kernel.org
 S:	Maintained
 F:	drivers/cxl/

-- 
2.40.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/3] cxl/pci: Update comment
  2023-05-17 21:28 [PATCH 0/3] cxl: Random clean ups Ira Weiny
  2023-05-17 21:28 ` [PATCH 1/3] MAINTAINERS: Add additional reviewers for CXL Ira Weiny
@ 2023-05-17 21:28 ` Ira Weiny
  2023-05-17 21:32   ` Dave Jiang
  2023-05-18  9:38   ` Jonathan Cameron
  2023-05-17 21:28 ` [PATCH 3/3] tools/testing/cxl: Document test configurations Ira Weiny
  2 siblings, 2 replies; 15+ messages in thread
From: Ira Weiny @ 2023-05-17 21:28 UTC (permalink / raw)
  To: Dan Williams, Alison Schofield, Vishal Verma, Dave Jiang,
	Ben Widawsky, Jonathan Cameron
  Cc: linux-cxl, linux-kernel, Ira Weiny

The existence of struct cxl_dev_id containing a single member is odd.
The comment made sense when I wrote it but could be clarified.

Update the comment and place it next to the odd looking structure.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/pci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index f7a5b8e9c102..ad7a1276fdc5 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -469,6 +469,10 @@ static int cxl_alloc_irq_vectors(struct pci_dev *pdev)
 	return 0;
 }
 
+/*
+ * Threaded irq dev_id's must be globally unique.  cxl_dev_id provides a unique
+ * wrapper object for each irq within the same cxlds.
+ */
 struct cxl_dev_id {
 	struct cxl_dev_state *cxlds;
 };
@@ -506,7 +510,6 @@ static int cxl_event_req_irq(struct cxl_dev_state *cxlds, u8 setting)
 	if (FIELD_GET(CXLDEV_EVENT_INT_MODE_MASK, setting) != CXL_INT_MSI_MSIX)
 		return -ENXIO;
 
-	/* dev_id must be globally unique and must contain the cxlds */
 	dev_id = devm_kzalloc(dev, sizeof(*dev_id), GFP_KERNEL);
 	if (!dev_id)
 		return -ENOMEM;

-- 
2.40.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/3] tools/testing/cxl: Document test configurations
  2023-05-17 21:28 [PATCH 0/3] cxl: Random clean ups Ira Weiny
  2023-05-17 21:28 ` [PATCH 1/3] MAINTAINERS: Add additional reviewers for CXL Ira Weiny
  2023-05-17 21:28 ` [PATCH 2/3] cxl/pci: Update comment Ira Weiny
@ 2023-05-17 21:28 ` Ira Weiny
  2023-05-17 21:31   ` Dave Jiang
                     ` (2 more replies)
  2 siblings, 3 replies; 15+ messages in thread
From: Ira Weiny @ 2023-05-17 21:28 UTC (permalink / raw)
  To: Dan Williams, Alison Schofield, Vishal Verma, Dave Jiang,
	Ben Widawsky, Jonathan Cameron
  Cc: linux-cxl, linux-kernel, Ira Weiny

The devices created, their relationship, and intended testing purpose is
not extremely clear, especially for those unfamiliar with cxl-test.

Document the purpose of each hierarchy.  Add ASCII art to show the
relationship of devices.  Group the device declarations together based
on the hierarchies.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 2 deletions(-)

diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index bf00dc52fe96..bd38a5fb60ae 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -23,6 +23,31 @@ static int interleave_arithmetic;
 #define NR_CXL_PORT_DECODERS 8
 #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH)
 
+/*
+ * Interleave testing
+ *
+ *             +---------------+                            +---------------+
+ *             | host_bridge[0]|                            | host_bridge[1]|
+ *             +-/---------\---+                            +--/---------\--+
+ *             /-           -\                               /-           -\
+ *           /-               -\                           /-               -\
+ *   +-------------+         +-------------+       +-------------+      +-------------+
+ *   |root_port[0] |         |root_port[1] |       |root_port[2] |      |root_port[3] |
+ *   +------|------+         +------|------+       +------|------+      +------|------+
+ *          |                       |                     |                    |
+ *  +-------|-------+       +-------|-------+     +-------|-------+    +-------|-------+
+ *  |switch_uport[0]|       |switch_uport[1]|     |switch_uport[2]|    |switch_uport[3]|
+ *  +---|-------|---+       +---/-------|---+     +---/-------|---+    +---|-------\---+
+ *      |       \              /        \            /        \            /        \
+ * +----|----++--|------++---------++----|----++---------++----|----++----|----++---------+
+ * |switch   ||switch   ||switch   ||switch   ||switch   ||switch   ||switch   ||switch   |
+ * |_dport[0]||_dport[1]||_dport[2]||_dport[3]||_dport[4]||_dport[5]||_dport[6]||_dport[7]|
+ * +----|----++--|------++----|----++----|----++----|----++----|----++----|----++----|----+
+ *      |        |            |          |          |          |          |          |
+ *  +---|--+   +-|----+   +---|--+   +---|--+    +--|---+  +---|--+   +---|--+   +---|--+
+ *  |mem[0]|   |mem[1]|   |mem[2]|   |mem[3]|    |mem[4]|  |mem[5]|   |mem[6]|   |mem[7]|
+ *  +------+   +------+   +------+   +------+    +------+  +------+   +------+   +------+
+ */
 static struct platform_device *cxl_acpi;
 static struct platform_device *cxl_host_bridge[NR_CXL_HOST_BRIDGES];
 #define NR_MULTI_ROOT (NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS)
@@ -31,16 +56,51 @@ static struct platform_device *cxl_switch_uport[NR_MULTI_ROOT];
 #define NR_MEM_MULTI \
 	(NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS * NR_CXL_SWITCH_PORTS)
 static struct platform_device *cxl_switch_dport[NR_MEM_MULTI];
+struct platform_device *cxl_mem[NR_MEM_MULTI];
 
+/*
+ * 1) Preconfigured region support (Simulated BIOS configured region)
+ * 2) 'Pass-through' decoder
+ *
+ *       +---------------+
+ *       |  hb_single    |
+ *       +------|--------+
+ *              |
+ *       +------|--------+
+ *       | root_single   |
+ *       +------|--------+
+ *              |
+ *   +----------|----------+
+ *   |     swu_single      |
+ *   +-----|-----------|---+
+ *         |           |
+ *   +-----|-----+  +--|--------+
+ *   |swd_single |  | swd_single|
+ *   +-----|-----+  +----|------+
+ *         |             |
+ *  +------|-----+  +----|-------+
+ *  |mem_single  |  |mem_single  |
+ *  +------------+  +------------+
+ */
 static struct platform_device *cxl_hb_single[NR_CXL_SINGLE_HOST];
 static struct platform_device *cxl_root_single[NR_CXL_SINGLE_HOST];
 static struct platform_device *cxl_swu_single[NR_CXL_SINGLE_HOST];
 #define NR_MEM_SINGLE (NR_CXL_SINGLE_HOST * NR_CXL_SWITCH_PORTS)
 static struct platform_device *cxl_swd_single[NR_MEM_SINGLE];
-
-struct platform_device *cxl_mem[NR_MEM_MULTI];
 struct platform_device *cxl_mem_single[NR_MEM_SINGLE];
 
+/*
+ * 1) RCD
+ * 2) Type-2 (Accelerator)
+ *
+ *  +-----+
+ *  | rch |
+ *  +--|--+
+ *     |
+ *   +-|--+
+ *   |rcd |
+ *   +----+
+ */
 static struct platform_device *cxl_rch[NR_CXL_RCH];
 static struct platform_device *cxl_rcd[NR_CXL_RCH];
 
@@ -64,6 +124,17 @@ static inline bool is_single_bridge(struct device *dev)
 	return false;
 }
 
+/*
+ *  +---------------+ +---------------+
+ *  | host_bridge[0]| | host_bridge[1]|
+ *  +---------------+ +---------------+
+ *  +---------------+
+ *  |  hb_single    | (host_bridge[2])
+ *  +---------------+
+ *  +-----+
+ *  | rch | (host_bridge[3])
+ *  +-----+
+ */
 static struct acpi_device acpi0017_mock;
 static struct acpi_device host_bridge[NR_BRIDGES] = {
 	[0] = {

-- 
2.40.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] MAINTAINERS: Add additional reviewers for CXL
  2023-05-17 21:28 ` [PATCH 1/3] MAINTAINERS: Add additional reviewers for CXL Ira Weiny
@ 2023-05-17 21:29   ` Dave Jiang
  2023-05-18  9:36   ` Jonathan Cameron
  1 sibling, 0 replies; 15+ messages in thread
From: Dave Jiang @ 2023-05-17 21:29 UTC (permalink / raw)
  To: Ira Weiny, Dan Williams, Alison Schofield, Vishal Verma,
	Ben Widawsky, Jonathan Cameron
  Cc: linux-cxl, linux-kernel



On 5/17/23 2:28 PM, Ira Weiny wrote:
> To get better coverage add Dave Jiang and Jonathan Cameron as reviewers.
> 
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Acked-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   MAINTAINERS | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90abe83c02f3..6f8568ec2731 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5169,6 +5169,8 @@ M:	Vishal Verma <vishal.l.verma@intel.com>
>   M:	Ira Weiny <ira.weiny@intel.com>
>   M:	Ben Widawsky <bwidawsk@kernel.org>
>   M:	Dan Williams <dan.j.williams@intel.com>
> +R:	Dave Jiang <dave.jiang@intel.com>
> +R:	Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>   L:	linux-cxl@vger.kernel.org
>   S:	Maintained
>   F:	drivers/cxl/
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] tools/testing/cxl: Document test configurations
  2023-05-17 21:28 ` [PATCH 3/3] tools/testing/cxl: Document test configurations Ira Weiny
@ 2023-05-17 21:31   ` Dave Jiang
  2023-05-18  9:50   ` Jonathan Cameron
  2023-09-16  7:03   ` Dan Williams
  2 siblings, 0 replies; 15+ messages in thread
From: Dave Jiang @ 2023-05-17 21:31 UTC (permalink / raw)
  To: Ira Weiny, Dan Williams, Alison Schofield, Vishal Verma,
	Ben Widawsky, Jonathan Cameron
  Cc: linux-cxl, linux-kernel



On 5/17/23 2:28 PM, Ira Weiny wrote:
> The devices created, their relationship, and intended testing purpose is
> not extremely clear, especially for those unfamiliar with cxl-test.
> 
> Document the purpose of each hierarchy.  Add ASCII art to show the
> relationship of devices.  Group the device declarations together based
> on the hierarchies.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Nice artwork :)

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index bf00dc52fe96..bd38a5fb60ae 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -23,6 +23,31 @@ static int interleave_arithmetic;
>   #define NR_CXL_PORT_DECODERS 8
>   #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH)
>   
> +/*
> + * Interleave testing
> + *
> + *             +---------------+                            +---------------+
> + *             | host_bridge[0]|                            | host_bridge[1]|
> + *             +-/---------\---+                            +--/---------\--+
> + *             /-           -\                               /-           -\
> + *           /-               -\                           /-               -\
> + *   +-------------+         +-------------+       +-------------+      +-------------+
> + *   |root_port[0] |         |root_port[1] |       |root_port[2] |      |root_port[3] |
> + *   +------|------+         +------|------+       +------|------+      +------|------+
> + *          |                       |                     |                    |
> + *  +-------|-------+       +-------|-------+     +-------|-------+    +-------|-------+
> + *  |switch_uport[0]|       |switch_uport[1]|     |switch_uport[2]|    |switch_uport[3]|
> + *  +---|-------|---+       +---/-------|---+     +---/-------|---+    +---|-------\---+
> + *      |       \              /        \            /        \            /        \
> + * +----|----++--|------++---------++----|----++---------++----|----++----|----++---------+
> + * |switch   ||switch   ||switch   ||switch   ||switch   ||switch   ||switch   ||switch   |
> + * |_dport[0]||_dport[1]||_dport[2]||_dport[3]||_dport[4]||_dport[5]||_dport[6]||_dport[7]|
> + * +----|----++--|------++----|----++----|----++----|----++----|----++----|----++----|----+
> + *      |        |            |          |          |          |          |          |
> + *  +---|--+   +-|----+   +---|--+   +---|--+    +--|---+  +---|--+   +---|--+   +---|--+
> + *  |mem[0]|   |mem[1]|   |mem[2]|   |mem[3]|    |mem[4]|  |mem[5]|   |mem[6]|   |mem[7]|
> + *  +------+   +------+   +------+   +------+    +------+  +------+   +------+   +------+
> + */
>   static struct platform_device *cxl_acpi;
>   static struct platform_device *cxl_host_bridge[NR_CXL_HOST_BRIDGES];
>   #define NR_MULTI_ROOT (NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS)
> @@ -31,16 +56,51 @@ static struct platform_device *cxl_switch_uport[NR_MULTI_ROOT];
>   #define NR_MEM_MULTI \
>   	(NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS * NR_CXL_SWITCH_PORTS)
>   static struct platform_device *cxl_switch_dport[NR_MEM_MULTI];
> +struct platform_device *cxl_mem[NR_MEM_MULTI];
>   
> +/*
> + * 1) Preconfigured region support (Simulated BIOS configured region)
> + * 2) 'Pass-through' decoder
> + *
> + *       +---------------+
> + *       |  hb_single    |
> + *       +------|--------+
> + *              |
> + *       +------|--------+
> + *       | root_single   |
> + *       +------|--------+
> + *              |
> + *   +----------|----------+
> + *   |     swu_single      |
> + *   +-----|-----------|---+
> + *         |           |
> + *   +-----|-----+  +--|--------+
> + *   |swd_single |  | swd_single|
> + *   +-----|-----+  +----|------+
> + *         |             |
> + *  +------|-----+  +----|-------+
> + *  |mem_single  |  |mem_single  |
> + *  +------------+  +------------+
> + */
>   static struct platform_device *cxl_hb_single[NR_CXL_SINGLE_HOST];
>   static struct platform_device *cxl_root_single[NR_CXL_SINGLE_HOST];
>   static struct platform_device *cxl_swu_single[NR_CXL_SINGLE_HOST];
>   #define NR_MEM_SINGLE (NR_CXL_SINGLE_HOST * NR_CXL_SWITCH_PORTS)
>   static struct platform_device *cxl_swd_single[NR_MEM_SINGLE];
> -
> -struct platform_device *cxl_mem[NR_MEM_MULTI];
>   struct platform_device *cxl_mem_single[NR_MEM_SINGLE];
>   
> +/*
> + * 1) RCD
> + * 2) Type-2 (Accelerator)
> + *
> + *  +-----+
> + *  | rch |
> + *  +--|--+
> + *     |
> + *   +-|--+
> + *   |rcd |
> + *   +----+
> + */
>   static struct platform_device *cxl_rch[NR_CXL_RCH];
>   static struct platform_device *cxl_rcd[NR_CXL_RCH];
>   
> @@ -64,6 +124,17 @@ static inline bool is_single_bridge(struct device *dev)
>   	return false;
>   }
>   
> +/*
> + *  +---------------+ +---------------+
> + *  | host_bridge[0]| | host_bridge[1]|
> + *  +---------------+ +---------------+
> + *  +---------------+
> + *  |  hb_single    | (host_bridge[2])
> + *  +---------------+
> + *  +-----+
> + *  | rch | (host_bridge[3])
> + *  +-----+
> + */
>   static struct acpi_device acpi0017_mock;
>   static struct acpi_device host_bridge[NR_BRIDGES] = {
>   	[0] = {
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] cxl/pci: Update comment
  2023-05-17 21:28 ` [PATCH 2/3] cxl/pci: Update comment Ira Weiny
@ 2023-05-17 21:32   ` Dave Jiang
  2023-05-18  9:38   ` Jonathan Cameron
  1 sibling, 0 replies; 15+ messages in thread
From: Dave Jiang @ 2023-05-17 21:32 UTC (permalink / raw)
  To: Ira Weiny, Dan Williams, Alison Schofield, Vishal Verma,
	Ben Widawsky, Jonathan Cameron
  Cc: linux-cxl, linux-kernel



On 5/17/23 2:28 PM, Ira Weiny wrote:
> The existence of struct cxl_dev_id containing a single member is odd.
> The comment made sense when I wrote it but could be clarified.
> 
> Update the comment and place it next to the odd looking structure.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/pci.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index f7a5b8e9c102..ad7a1276fdc5 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -469,6 +469,10 @@ static int cxl_alloc_irq_vectors(struct pci_dev *pdev)
>   	return 0;
>   }
>   
> +/*
> + * Threaded irq dev_id's must be globally unique.  cxl_dev_id provides a unique
> + * wrapper object for each irq within the same cxlds.
> + */
>   struct cxl_dev_id {
>   	struct cxl_dev_state *cxlds;
>   };
> @@ -506,7 +510,6 @@ static int cxl_event_req_irq(struct cxl_dev_state *cxlds, u8 setting)
>   	if (FIELD_GET(CXLDEV_EVENT_INT_MODE_MASK, setting) != CXL_INT_MSI_MSIX)
>   		return -ENXIO;
>   
> -	/* dev_id must be globally unique and must contain the cxlds */
>   	dev_id = devm_kzalloc(dev, sizeof(*dev_id), GFP_KERNEL);
>   	if (!dev_id)
>   		return -ENOMEM;
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] MAINTAINERS: Add additional reviewers for CXL
  2023-05-17 21:28 ` [PATCH 1/3] MAINTAINERS: Add additional reviewers for CXL Ira Weiny
  2023-05-17 21:29   ` Dave Jiang
@ 2023-05-18  9:36   ` Jonathan Cameron
  2023-05-18 14:42     ` Ira Weiny
  1 sibling, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2023-05-18  9:36 UTC (permalink / raw)
  To: Ira Weiny
  Cc: Dan Williams, Alison Schofield, Vishal Verma, Dave Jiang,
	Ben Widawsky, linux-cxl, linux-kernel

On Wed, 17 May 2023 14:28:10 -0700
Ira Weiny <ira.weiny@intel.com> wrote:

> To get better coverage add Dave Jiang and Jonathan Cameron as reviewers.
> 
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

One comment inline though on capitalization.

Thanks,

Jonathan

> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90abe83c02f3..6f8568ec2731 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5169,6 +5169,8 @@ M:	Vishal Verma <vishal.l.verma@intel.com>
>  M:	Ira Weiny <ira.weiny@intel.com>
>  M:	Ben Widawsky <bwidawsk@kernel.org>
>  M:	Dan Williams <dan.j.williams@intel.com>
> +R:	Dave Jiang <dave.jiang@intel.com>
> +R:	Jonathan Cameron <Jonathan.Cameron@Huawei.com>

For consistency go with <jonathan.cameron@huawei.com>
which is what I'm using elsewhere in maintainers and
matches other lower case entries here.
 


>  L:	linux-cxl@vger.kernel.org
>  S:	Maintained
>  F:	drivers/cxl/
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/3] cxl/pci: Update comment
  2023-05-17 21:28 ` [PATCH 2/3] cxl/pci: Update comment Ira Weiny
  2023-05-17 21:32   ` Dave Jiang
@ 2023-05-18  9:38   ` Jonathan Cameron
  1 sibling, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2023-05-18  9:38 UTC (permalink / raw)
  To: Ira Weiny
  Cc: Dan Williams, Alison Schofield, Vishal Verma, Dave Jiang,
	Ben Widawsky, linux-cxl, linux-kernel

On Wed, 17 May 2023 14:28:11 -0700
Ira Weiny <ira.weiny@intel.com> wrote:

> The existence of struct cxl_dev_id containing a single member is odd.
> The comment made sense when I wrote it but could be clarified.
> 
> Update the comment and place it next to the odd looking structure.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/pci.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index f7a5b8e9c102..ad7a1276fdc5 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -469,6 +469,10 @@ static int cxl_alloc_irq_vectors(struct pci_dev *pdev)
>  	return 0;
>  }
>  
> +/*
> + * Threaded irq dev_id's must be globally unique.  cxl_dev_id provides a unique
> + * wrapper object for each irq within the same cxlds.
> + */
>  struct cxl_dev_id {
>  	struct cxl_dev_state *cxlds;
>  };
> @@ -506,7 +510,6 @@ static int cxl_event_req_irq(struct cxl_dev_state *cxlds, u8 setting)
>  	if (FIELD_GET(CXLDEV_EVENT_INT_MODE_MASK, setting) != CXL_INT_MSI_MSIX)
>  		return -ENXIO;
>  
> -	/* dev_id must be globally unique and must contain the cxlds */
>  	dev_id = devm_kzalloc(dev, sizeof(*dev_id), GFP_KERNEL);
>  	if (!dev_id)
>  		return -ENOMEM;
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] tools/testing/cxl: Document test configurations
  2023-05-17 21:28 ` [PATCH 3/3] tools/testing/cxl: Document test configurations Ira Weiny
  2023-05-17 21:31   ` Dave Jiang
@ 2023-05-18  9:50   ` Jonathan Cameron
  2023-05-18  9:51     ` Jonathan Cameron
  2023-05-18 14:36     ` Ira Weiny
  2023-09-16  7:03   ` Dan Williams
  2 siblings, 2 replies; 15+ messages in thread
From: Jonathan Cameron @ 2023-05-18  9:50 UTC (permalink / raw)
  To: Ira Weiny
  Cc: Dan Williams, Alison Schofield, Vishal Verma, Dave Jiang,
	Ben Widawsky, linux-cxl, linux-kernel

On Wed, 17 May 2023 14:28:12 -0700
Ira Weiny <ira.weiny@intel.com> wrote:

> The devices created, their relationship, and intended testing purpose is
> not extremely clear, especially for those unfamiliar with cxl-test.
> 
> Document the purpose of each hierarchy.  Add ASCII art to show the
> relationship of devices.  Group the device declarations together based
> on the hierarchies.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Trivial nitpicks below :)

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index bf00dc52fe96..bd38a5fb60ae 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -23,6 +23,31 @@ static int interleave_arithmetic;
>  #define NR_CXL_PORT_DECODERS 8
>  #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH)
>  
> +/*
> + * Interleave testing

Doesn't include the cfmws, which will be tricky to draw, but maybe you could
add something to indicate they interleave over the two HB sometimes?

> + *
> + *             +---------------+                            +---------------+
> + *             | host_bridge[0]|                            | host_bridge[1]|
> + *             +-/---------\---+                            +--/---------\--+
Text for host bridges is right aligned.
> + *             /-           -\                               /-           -\
> + *           /-               -\                           /-               -\
> + *   +-------------+         +-------------+       +-------------+      +-------------+
> + *   |root_port[0] |         |root_port[1] |       |root_port[2] |      |root_port[3] |
> + *   +------|------+         +------|------+       +------|------+      +------|------+
and root ports are left aligned.
I'd shrink both boxes so they are same as the switches below - or expand them to give
a space on either side of the text.


> + *          |                       |                     |                    |
> + *  +-------|-------+       +-------|-------+     +-------|-------+    +-------|-------+
> + *  |switch_uport[0]|       |switch_uport[1]|     |switch_uport[2]|    |switch_uport[3]|
> + *  +---|-------|---+       +---/-------|---+     +---/-------|---+    +---|-------\---+
> + *      |       \              /        \            /        \            /        \
> + * +----|----++--|------++---------++----|----++---------++----|----++----|----++---------+
> + * |switch   ||switch   ||switch   ||switch   ||switch   ||switch   ||switch   ||switch   |
> + * |_dport[0]||_dport[1]||_dport[2]||_dport[3]||_dport[4]||_dport[5]||_dport[6]||_dport[7]|
> + * +----|----++--|------++----|----++----|----++----|----++----|----++----|----++----|----+
> + *      |        |            |          |          |          |          |          |
> + *  +---|--+   +-|----+   +---|--+   +---|--+    +--|---+  +---|--+   +---|--+   +---|--+
> + *  |mem[0]|   |mem[1]|   |mem[2]|   |mem[3]|    |mem[4]|  |mem[5]|   |mem[6]|   |mem[7]|
> + *  +------+   +------+   +------+   +------+    +------+  +------+   +------+   +------+
> + */
>  static struct platform_device *cxl_acpi;
>  static struct platform_device *cxl_host_bridge[NR_CXL_HOST_BRIDGES];
>  #define NR_MULTI_ROOT (NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS)
> @@ -31,16 +56,51 @@ static struct platform_device *cxl_switch_uport[NR_MULTI_ROOT];
>  #define NR_MEM_MULTI \
>  	(NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS * NR_CXL_SWITCH_PORTS)
>  static struct platform_device *cxl_switch_dport[NR_MEM_MULTI];
> +struct platform_device *cxl_mem[NR_MEM_MULTI];
>  
> +/*
> + * 1) Preconfigured region support (Simulated BIOS configured region)
> + * 2) 'Pass-through' decoder
> + *
> + *       +---------------+
> + *       |  hb_single    |
> + *       +------|--------+
> + *              |
> + *       +------|--------+
> + *       | root_single   |
> + *       +------|--------+
> + *              |
> + *   +----------|----------+
> + *   |     swu_single      |
> + *   +-----|-----------|---+
> + *         |           |
> + *   +-----|-----+  +--|--------+
> + *   |swd_single |  | swd_single|
> + *   +-----|-----+  +----|------+
> + *         |             |
> + *  +------|-----+  +----|-------+
> + *  |mem_single  |  |mem_single  |
> + *  +------------+  +------------+
mem[0] etc?  Also swd_single[0] etc?

For consistency with above.

> + */
>  static struct platform_device *cxl_hb_single[NR_CXL_SINGLE_HOST];
>  static struct platform_device *cxl_root_single[NR_CXL_SINGLE_HOST];
>  static struct platform_device *cxl_swu_single[NR_CXL_SINGLE_HOST];
>  #define NR_MEM_SINGLE (NR_CXL_SINGLE_HOST * NR_CXL_SWITCH_PORTS)
>  static struct platform_device *cxl_swd_single[NR_MEM_SINGLE];
> -
> -struct platform_device *cxl_mem[NR_MEM_MULTI];
>  struct platform_device *cxl_mem_single[NR_MEM_SINGLE];
>  
> +/*
> + * 1) RCD
> + * 2) Type-2 (Accelerator)
> + *
> + *  +-----+
> + *  | rch |
> + *  +--|--+
> + *     |
> + *   +-|--+
> + *   |rcd |
> + *   +----+
> + */
>  static struct platform_device *cxl_rch[NR_CXL_RCH];
>  static struct platform_device *cxl_rcd[NR_CXL_RCH];
>  
> @@ -64,6 +124,17 @@ static inline bool is_single_bridge(struct device *dev)
>  	return false;
>  }
>  
> +/*
> + *  +---------------+ +---------------+
> + *  | host_bridge[0]| | host_bridge[1]|
> + *  +---------------+ +---------------+
> + *  +---------------+
> + *  |  hb_single    | (host_bridge[2])
> + *  +---------------+
> + *  +-----+
> + *  | rch | (host_bridge[3])
> + *  +-----+
> + */

Not sure what this diagram is illustrating...

>  static struct acpi_device acpi0017_mock;
>  static struct acpi_device host_bridge[NR_BRIDGES] = {
>  	[0] = {
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] tools/testing/cxl: Document test configurations
  2023-05-18  9:50   ` Jonathan Cameron
@ 2023-05-18  9:51     ` Jonathan Cameron
  2023-05-18 14:36     ` Ira Weiny
  1 sibling, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2023-05-18  9:51 UTC (permalink / raw)
  To: Ira Weiny
  Cc: Dan Williams, Alison Schofield, Vishal Verma, Dave Jiang,
	Ben Widawsky, linux-cxl, linux-kernel

On Thu, 18 May 2023 10:50:20 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 17 May 2023 14:28:12 -0700
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > The devices created, their relationship, and intended testing purpose is
> > not extremely clear, especially for those unfamiliar with cxl-test.
> > 
> > Document the purpose of each hierarchy.  Add ASCII art to show the
> > relationship of devices.  Group the device declarations together based
> > on the hierarchies.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>  
> 
> Trivial nitpicks below :)
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Actually scrub that RB - the last question on what the final diagram means
probably makes an RB inappropriate for now...

> 
> > ---
> >  tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 73 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> > index bf00dc52fe96..bd38a5fb60ae 100644
> > --- a/tools/testing/cxl/test/cxl.c
> > +++ b/tools/testing/cxl/test/cxl.c
> > @@ -23,6 +23,31 @@ static int interleave_arithmetic;
> >  #define NR_CXL_PORT_DECODERS 8
> >  #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH)
> >  
> > +/*
> > + * Interleave testing  
> 
> Doesn't include the cfmws, which will be tricky to draw, but maybe you could
> add something to indicate they interleave over the two HB sometimes?
> 
> > + *
> > + *             +---------------+                            +---------------+
> > + *             | host_bridge[0]|                            | host_bridge[1]|
> > + *             +-/---------\---+                            +--/---------\--+  
> Text for host bridges is right aligned.
> > + *             /-           -\                               /-           -\
> > + *           /-               -\                           /-               -\
> > + *   +-------------+         +-------------+       +-------------+      +-------------+
> > + *   |root_port[0] |         |root_port[1] |       |root_port[2] |      |root_port[3] |
> > + *   +------|------+         +------|------+       +------|------+      +------|------+  
> and root ports are left aligned.
> I'd shrink both boxes so they are same as the switches below - or expand them to give
> a space on either side of the text.
> 
> 
> > + *          |                       |                     |                    |
> > + *  +-------|-------+       +-------|-------+     +-------|-------+    +-------|-------+
> > + *  |switch_uport[0]|       |switch_uport[1]|     |switch_uport[2]|    |switch_uport[3]|
> > + *  +---|-------|---+       +---/-------|---+     +---/-------|---+    +---|-------\---+
> > + *      |       \              /        \            /        \            /        \
> > + * +----|----++--|------++---------++----|----++---------++----|----++----|----++---------+
> > + * |switch   ||switch   ||switch   ||switch   ||switch   ||switch   ||switch   ||switch   |
> > + * |_dport[0]||_dport[1]||_dport[2]||_dport[3]||_dport[4]||_dport[5]||_dport[6]||_dport[7]|
> > + * +----|----++--|------++----|----++----|----++----|----++----|----++----|----++----|----+
> > + *      |        |            |          |          |          |          |          |
> > + *  +---|--+   +-|----+   +---|--+   +---|--+    +--|---+  +---|--+   +---|--+   +---|--+
> > + *  |mem[0]|   |mem[1]|   |mem[2]|   |mem[3]|    |mem[4]|  |mem[5]|   |mem[6]|   |mem[7]|
> > + *  +------+   +------+   +------+   +------+    +------+  +------+   +------+   +------+
> > + */
> >  static struct platform_device *cxl_acpi;
> >  static struct platform_device *cxl_host_bridge[NR_CXL_HOST_BRIDGES];
> >  #define NR_MULTI_ROOT (NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS)
> > @@ -31,16 +56,51 @@ static struct platform_device *cxl_switch_uport[NR_MULTI_ROOT];
> >  #define NR_MEM_MULTI \
> >  	(NR_CXL_HOST_BRIDGES * NR_CXL_ROOT_PORTS * NR_CXL_SWITCH_PORTS)
> >  static struct platform_device *cxl_switch_dport[NR_MEM_MULTI];
> > +struct platform_device *cxl_mem[NR_MEM_MULTI];
> >  
> > +/*
> > + * 1) Preconfigured region support (Simulated BIOS configured region)
> > + * 2) 'Pass-through' decoder
> > + *
> > + *       +---------------+
> > + *       |  hb_single    |
> > + *       +------|--------+
> > + *              |
> > + *       +------|--------+
> > + *       | root_single   |
> > + *       +------|--------+
> > + *              |
> > + *   +----------|----------+
> > + *   |     swu_single      |
> > + *   +-----|-----------|---+
> > + *         |           |
> > + *   +-----|-----+  +--|--------+
> > + *   |swd_single |  | swd_single|
> > + *   +-----|-----+  +----|------+
> > + *         |             |
> > + *  +------|-----+  +----|-------+
> > + *  |mem_single  |  |mem_single  |
> > + *  +------------+  +------------+  
> mem[0] etc?  Also swd_single[0] etc?
> 
> For consistency with above.
> 
> > + */
> >  static struct platform_device *cxl_hb_single[NR_CXL_SINGLE_HOST];
> >  static struct platform_device *cxl_root_single[NR_CXL_SINGLE_HOST];
> >  static struct platform_device *cxl_swu_single[NR_CXL_SINGLE_HOST];
> >  #define NR_MEM_SINGLE (NR_CXL_SINGLE_HOST * NR_CXL_SWITCH_PORTS)
> >  static struct platform_device *cxl_swd_single[NR_MEM_SINGLE];
> > -
> > -struct platform_device *cxl_mem[NR_MEM_MULTI];
> >  struct platform_device *cxl_mem_single[NR_MEM_SINGLE];
> >  
> > +/*
> > + * 1) RCD
> > + * 2) Type-2 (Accelerator)
> > + *
> > + *  +-----+
> > + *  | rch |
> > + *  +--|--+
> > + *     |
> > + *   +-|--+
> > + *   |rcd |
> > + *   +----+
> > + */
> >  static struct platform_device *cxl_rch[NR_CXL_RCH];
> >  static struct platform_device *cxl_rcd[NR_CXL_RCH];
> >  
> > @@ -64,6 +124,17 @@ static inline bool is_single_bridge(struct device *dev)
> >  	return false;
> >  }
> >  
> > +/*
> > + *  +---------------+ +---------------+
> > + *  | host_bridge[0]| | host_bridge[1]|
> > + *  +---------------+ +---------------+
> > + *  +---------------+
> > + *  |  hb_single    | (host_bridge[2])
> > + *  +---------------+
> > + *  +-----+
> > + *  | rch | (host_bridge[3])
> > + *  +-----+
> > + */  
> 
> Not sure what this diagram is illustrating...
> 
> >  static struct acpi_device acpi0017_mock;
> >  static struct acpi_device host_bridge[NR_BRIDGES] = {
> >  	[0] = {
> >   
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] tools/testing/cxl: Document test configurations
  2023-05-18  9:50   ` Jonathan Cameron
  2023-05-18  9:51     ` Jonathan Cameron
@ 2023-05-18 14:36     ` Ira Weiny
  1 sibling, 0 replies; 15+ messages in thread
From: Ira Weiny @ 2023-05-18 14:36 UTC (permalink / raw)
  To: Jonathan Cameron, Ira Weiny
  Cc: Dan Williams, Alison Schofield, Vishal Verma, Dave Jiang,
	Ben Widawsky, linux-cxl, linux-kernel

Jonathan Cameron wrote:
> On Wed, 17 May 2023 14:28:12 -0700
> Ira Weiny <ira.weiny@intel.com> wrote:
> 

[snip]

> > ---
> >  tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 73 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> > index bf00dc52fe96..bd38a5fb60ae 100644
> > --- a/tools/testing/cxl/test/cxl.c
> > +++ b/tools/testing/cxl/test/cxl.c
> > @@ -23,6 +23,31 @@ static int interleave_arithmetic;
> >  #define NR_CXL_PORT_DECODERS 8
> >  #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH)
> >  
> > +/*
> > + * Interleave testing
> 
> Doesn't include the cfmws, which will be tricky to draw, but maybe you could
> add something to indicate they interleave over the two HB sometimes?

I was mainly looking to document the devices below.  Because they are all
'platform_device' and they are assigned type in the code which made things
a bit harder for me to follow when I was going through it the other day.

> 
> > + *
> > + *             +---------------+                            +---------------+
> > + *             | host_bridge[0]|                            | host_bridge[1]|
> > + *             +-/---------\---+                            +--/---------\--+
> Text for host bridges is right aligned.

Ah true.  I used an online ascii editor for these.  :-D  So I did not pay
any attention when I copied pasted.

> > + *             /-           -\                               /-           -\
> > + *           /-               -\                           /-               -\
> > + *   +-------------+         +-------------+       +-------------+      +-------------+
> > + *   |root_port[0] |         |root_port[1] |       |root_port[2] |      |root_port[3] |
> > + *   +------|------+         +------|------+       +------|------+      +------|------+
> and root ports are left aligned.
> I'd shrink both boxes so they are same as the switches below - or expand them to give
> a space on either side of the text.

Done.

> >  
> > +/*
> > + * 1) Preconfigured region support (Simulated BIOS configured region)
> > + * 2) 'Pass-through' decoder
> > + *
> > + *       +---------------+
> > + *       |  hb_single    |
> > + *       +------|--------+
> > + *              |
> > + *       +------|--------+
> > + *       | root_single   |
> > + *       +------|--------+
> > + *              |
> > + *   +----------|----------+
> > + *   |     swu_single      |
> > + *   +-----|-----------|---+
> > + *         |           |
> > + *   +-----|-----+  +--|--------+
> > + *   |swd_single |  | swd_single|
> > + *   +-----|-----+  +----|------+
> > + *         |             |
> > + *  +------|-----+  +----|-------+
> > + *  |mem_single  |  |mem_single  |
> > + *  +------------+  +------------+
> mem[0] etc?  Also swd_single[0] etc?
> 
> For consistency with above.
> 

Actually mem_single[0,1].  yea swd_single[0,1].

> >  
> > +/*
> > + *  +---------------+ +---------------+
> > + *  | host_bridge[0]| | host_bridge[1]|
> > + *  +---------------+ +---------------+
> > + *  +---------------+
> > + *  |  hb_single    | (host_bridge[2])
> > + *  +---------------+
> > + *  +-----+
> > + *  | rch | (host_bridge[3])
> > + *  +-----+
> > + */
> 
> Not sure what this diagram is illustrating...

Just showing how the acpi_devices array below ties in with the above
diagrams.  Mainly that their is not a 1:1 corelation between
cxl_host_bridge[] and host_bridge[].  That index 2 and 3 are other
platform devices as shown.

I could probably make that equivalency note in the diagrams above where
hb_single and rch are defined/documented.

Let me do that.
Ira

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] MAINTAINERS: Add additional reviewers for CXL
  2023-05-18  9:36   ` Jonathan Cameron
@ 2023-05-18 14:42     ` Ira Weiny
  0 siblings, 0 replies; 15+ messages in thread
From: Ira Weiny @ 2023-05-18 14:42 UTC (permalink / raw)
  To: Jonathan Cameron, Ira Weiny
  Cc: Dan Williams, Alison Schofield, Vishal Verma, Dave Jiang,
	Ben Widawsky, linux-cxl, linux-kernel

Jonathan Cameron wrote:
> On Wed, 17 May 2023 14:28:10 -0700
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > To get better coverage add Dave Jiang and Jonathan Cameron as reviewers.
> > 
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> One comment inline though on capitalization.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  MAINTAINERS | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 90abe83c02f3..6f8568ec2731 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5169,6 +5169,8 @@ M:	Vishal Verma <vishal.l.verma@intel.com>
> >  M:	Ira Weiny <ira.weiny@intel.com>
> >  M:	Ben Widawsky <bwidawsk@kernel.org>
> >  M:	Dan Williams <dan.j.williams@intel.com>
> > +R:	Dave Jiang <dave.jiang@intel.com>
> > +R:	Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> 
> For consistency go with <jonathan.cameron@huawei.com>
> which is what I'm using elsewhere in maintainers and
> matches other lower case entries here.

NP on this change.  However, FYI b4 picks up your email capitalized a lot.
For example when you ack/review as you did above.

Ira

>  
> 
> 
> >  L:	linux-cxl@vger.kernel.org
> >  S:	Maintained
> >  F:	drivers/cxl/
> > 
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 3/3] tools/testing/cxl: Document test configurations
  2023-05-17 21:28 ` [PATCH 3/3] tools/testing/cxl: Document test configurations Ira Weiny
  2023-05-17 21:31   ` Dave Jiang
  2023-05-18  9:50   ` Jonathan Cameron
@ 2023-09-16  7:03   ` Dan Williams
  2023-09-18 17:31     ` Ira Weiny
  2 siblings, 1 reply; 15+ messages in thread
From: Dan Williams @ 2023-09-16  7:03 UTC (permalink / raw)
  To: Ira Weiny, Dan Williams, Alison Schofield, Vishal Verma,
	Dave Jiang, Ben Widawsky, Jonathan Cameron
  Cc: linux-cxl, linux-kernel, Ira Weiny

Ira Weiny wrote:
> The devices created, their relationship, and intended testing purpose is
> not extremely clear, especially for those unfamiliar with cxl-test.
> 
> Document the purpose of each hierarchy.  Add ASCII art to show the
> relationship of devices.  Group the device declarations together based
> on the hierarchies.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index bf00dc52fe96..bd38a5fb60ae 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -23,6 +23,31 @@ static int interleave_arithmetic;
>  #define NR_CXL_PORT_DECODERS 8
>  #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH)
>  
> +/*
> + * Interleave testing
> + *
> + *             +---------------+                            +---------------+
> + *             | host_bridge[0]|                            | host_bridge[1]|
> + *             +-/---------\---+                            +--/---------\--+
> + *             /-           -\                               /-           -\
> + *           /-               -\                           /-               -\
> + *   +-------------+         +-------------+       +-------------+      +-------------+
> + *   |root_port[0] |         |root_port[1] |       |root_port[2] |      |root_port[3] |
> + *   +------|------+         +------|------+       +------|------+      +------|------+
> + *          |                       |                     |                    |
> + *  +-------|-------+       +-------|-------+     +-------|-------+    +-------|-------+
> + *  |switch_uport[0]|       |switch_uport[1]|     |switch_uport[2]|    |switch_uport[3]|
> + *  +---|-------|---+       +---/-------|---+     +---/-------|---+    +---|-------\---+
> + *      |       \              /        \            /        \            /        \
> + * +----|----++--|------++---------++----|----++---------++----|----++----|----++---------+
> + * |switch   ||switch   ||switch   ||switch   ||switch   ||switch   ||switch   ||switch   |
> + * |_dport[0]||_dport[1]||_dport[2]||_dport[3]||_dport[4]||_dport[5]||_dport[6]||_dport[7]|
> + * +----|----++--|------++----|----++----|----++----|----++----|----++----|----++----|----+
> + *      |        |            |          |          |          |          |          |
> + *  +---|--+   +-|----+   +---|--+   +---|--+    +--|---+  +---|--+   +---|--+   +---|--+
> + *  |mem[0]|   |mem[1]|   |mem[2]|   |mem[3]|    |mem[4]|  |mem[5]|   |mem[6]|   |mem[7]|
> + *  +------+   +------+   +------+   +------+    +------+  +------+   +------+   +------+
> + */

Circling back to merge this I realize that the numbering is off. For
example a snippet from "cxl list -BPT -b cxl_test"

    "ports:root3":[
      {
        "port":"port5",
        "host":"cxl_host_bridge.1",
        "depth":1,
        "nr_dports":2,
        "dports":[
          {
            "dport":"cxl_root_port.1",
            "id":1
          },
          {
            "dport":"cxl_root_port.3",
            "id":3
          }
        ],

This is due to the modulo math at setup time. I only noticed this
because I wanted a diagram to refer to when doing some recent
extensions.

I wonder if we could just use "cxl list" to maintain this diagram, or
maybe circle back and use this to keep an image up to date on a web page
somewhere:

https://lore.kernel.org/linux-cxl/cover.1660895649.git.sunfishho12@gmail.com/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 3/3] tools/testing/cxl: Document test configurations
  2023-09-16  7:03   ` Dan Williams
@ 2023-09-18 17:31     ` Ira Weiny
  0 siblings, 0 replies; 15+ messages in thread
From: Ira Weiny @ 2023-09-18 17:31 UTC (permalink / raw)
  To: Dan Williams, Ira Weiny, Alison Schofield, Vishal Verma,
	Dave Jiang, Ben Widawsky, Jonathan Cameron, sunfishho12
  Cc: linux-cxl, linux-kernel, Ira Weiny

+Matthew Ho

Dan Williams wrote:
> Ira Weiny wrote:
> > The devices created, their relationship, and intended testing purpose is
> > not extremely clear, especially for those unfamiliar with cxl-test.
> > 
> > Document the purpose of each hierarchy.  Add ASCII art to show the
> > relationship of devices.  Group the device declarations together based
> > on the hierarchies.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  tools/testing/cxl/test/cxl.c | 75 ++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 73 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> > index bf00dc52fe96..bd38a5fb60ae 100644
> > --- a/tools/testing/cxl/test/cxl.c
> > +++ b/tools/testing/cxl/test/cxl.c
> > @@ -23,6 +23,31 @@ static int interleave_arithmetic;
> >  #define NR_CXL_PORT_DECODERS 8
> >  #define NR_BRIDGES (NR_CXL_HOST_BRIDGES + NR_CXL_SINGLE_HOST + NR_CXL_RCH)
> >  
> > +/*
> > + * Interleave testing
> > + *
> > + *             +---------------+                            +---------------+
> > + *             | host_bridge[0]|                            | host_bridge[1]|
> > + *             +-/---------\---+                            +--/---------\--+
> > + *             /-           -\                               /-           -\
> > + *           /-               -\                           /-               -\
> > + *   +-------------+         +-------------+       +-------------+      +-------------+
> > + *   |root_port[0] |         |root_port[1] |       |root_port[2] |      |root_port[3] |
> > + *   +------|------+         +------|------+       +------|------+      +------|------+
> > + *          |                       |                     |                    |
> > + *  +-------|-------+       +-------|-------+     +-------|-------+    +-------|-------+
> > + *  |switch_uport[0]|       |switch_uport[1]|     |switch_uport[2]|    |switch_uport[3]|
> > + *  +---|-------|---+       +---/-------|---+     +---/-------|---+    +---|-------\---+
> > + *      |       \              /        \            /        \            /        \
> > + * +----|----++--|------++---------++----|----++---------++----|----++----|----++---------+
> > + * |switch   ||switch   ||switch   ||switch   ||switch   ||switch   ||switch   ||switch   |
> > + * |_dport[0]||_dport[1]||_dport[2]||_dport[3]||_dport[4]||_dport[5]||_dport[6]||_dport[7]|
> > + * +----|----++--|------++----|----++----|----++----|----++----|----++----|----++----|----+
> > + *      |        |            |          |          |          |          |          |
> > + *  +---|--+   +-|----+   +---|--+   +---|--+    +--|---+  +---|--+   +---|--+   +---|--+
> > + *  |mem[0]|   |mem[1]|   |mem[2]|   |mem[3]|    |mem[4]|  |mem[5]|   |mem[6]|   |mem[7]|
> > + *  +------+   +------+   +------+   +------+    +------+  +------+   +------+   +------+
> > + */
> 
> Circling back to merge this I realize that the numbering is off. For
> example a snippet from "cxl list -BPT -b cxl_test"
> 
>     "ports:root3":[
>       {
>         "port":"port5",
>         "host":"cxl_host_bridge.1",
>         "depth":1,
>         "nr_dports":2,
>         "dports":[
>           {
>             "dport":"cxl_root_port.1",
>             "id":1
>           },
>           {
>             "dport":"cxl_root_port.3",
>             "id":3
>           }
>         ],
> 
> This is due to the modulo math at setup time. I only noticed this
> because I wanted a diagram to refer to when doing some recent
> extensions.

:-(  I did not realize this detail.

> 
> I wonder if we could just use "cxl list" to maintain this diagram, or
> maybe circle back and use this to keep an image up to date on a web page
> somewhere:
> 
> https://lore.kernel.org/linux-cxl/cover.1660895649.git.sunfishho12@gmail.com/

I forgot about this and this is a nice idea.  Did that support land?

Ira

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2023-09-18 17:32 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-17 21:28 [PATCH 0/3] cxl: Random clean ups Ira Weiny
2023-05-17 21:28 ` [PATCH 1/3] MAINTAINERS: Add additional reviewers for CXL Ira Weiny
2023-05-17 21:29   ` Dave Jiang
2023-05-18  9:36   ` Jonathan Cameron
2023-05-18 14:42     ` Ira Weiny
2023-05-17 21:28 ` [PATCH 2/3] cxl/pci: Update comment Ira Weiny
2023-05-17 21:32   ` Dave Jiang
2023-05-18  9:38   ` Jonathan Cameron
2023-05-17 21:28 ` [PATCH 3/3] tools/testing/cxl: Document test configurations Ira Weiny
2023-05-17 21:31   ` Dave Jiang
2023-05-18  9:50   ` Jonathan Cameron
2023-05-18  9:51     ` Jonathan Cameron
2023-05-18 14:36     ` Ira Weiny
2023-09-16  7:03   ` Dan Williams
2023-09-18 17:31     ` Ira Weiny

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).