diff mbox series

[v4,06/12] tools/testing/cxl: Make mock CEDT parsing more robust

Message ID 166931491054.2104015.16722069708509650823.stgit@dwillia2-xfh.jf.intel.com
State Superseded
Headers show
Series cxl: Add support for Restricted CXL hosts (RCD mode) | expand

Commit Message

Dan Williams Nov. 24, 2022, 6:35 p.m. UTC 
Accept any cxl_test topology device as the first argument in
cxl_chbs_context. This is in preparation for reworking the detection of
the component registers across VH and RCH topologies. Move
mock_acpi_table_parse_cedt() beneath the definition of is_mock_port()
and use is_mock_port() instead of the explicit mock cxl_acpi device
check.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 tools/testing/cxl/test/cxl.c |   80 +++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 40 deletions(-)

Comments

Robert Richter Nov. 28, 2022, 11:13 a.m. UTC | #1 
On 24.11.22 10:35:10, Dan Williams wrote:
> Accept any cxl_test topology device as the first argument in
> cxl_chbs_context. This is in preparation for reworking the detection of
> the component registers across VH and RCH topologies. Move
> mock_acpi_table_parse_cedt() beneath the definition of is_mock_port()
> and use is_mock_port() instead of the explicit mock cxl_acpi device
> check.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Robert Richter <rrichter@amd.com>

> ---
>  tools/testing/cxl/test/cxl.c |   80 +++++++++++++++++++++---------------------
>  1 file changed, 40 insertions(+), 40 deletions(-)
Alison Schofield Nov. 28, 2022, 6:20 p.m. UTC | #2 
On Thu, Nov 24, 2022 at 10:35:10AM -0800, Dan Williams wrote:
> Accept any cxl_test topology device as the first argument in
> cxl_chbs_context. This is in preparation for reworking the detection of
> the component registers across VH and RCH topologies. Move
> mock_acpi_table_parse_cedt() beneath the definition of is_mock_port()
> and use is_mock_port() instead of the explicit mock cxl_acpi device
> check.

I'll ACK this change, alhtough I don't appreciate the code move and modify
in the same patch. It hides the diff.

The commit msg seems needlessly vague. Why not state what was done?
Something like: 'Accept any topology device in cxl_chbs_context'

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  tools/testing/cxl/test/cxl.c |   80 +++++++++++++++++++++---------------------
>  1 file changed, 40 insertions(+), 40 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index facfcd11cb67..42a34650dd2f 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -320,46 +320,6 @@ static int populate_cedt(void)
>  	return 0;
>  }
>  
> -/*
> - * WARNING, this hack assumes the format of 'struct
> - * cxl_cfmws_context' and 'struct cxl_chbs_context' share the property that
> - * the first struct member is the device being probed by the cxl_acpi
> - * driver.
> - */
> -struct cxl_cedt_context {
> -	struct device *dev;
> -};
> -
> -static int mock_acpi_table_parse_cedt(enum acpi_cedt_type id,
> -				      acpi_tbl_entry_handler_arg handler_arg,
> -				      void *arg)
> -{
> -	struct cxl_cedt_context *ctx = arg;
> -	struct device *dev = ctx->dev;
> -	union acpi_subtable_headers *h;
> -	unsigned long end;
> -	int i;
> -
> -	if (dev != &cxl_acpi->dev)
> -		return acpi_table_parse_cedt(id, handler_arg, arg);
> -
> -	if (id == ACPI_CEDT_TYPE_CHBS)
> -		for (i = 0; i < ARRAY_SIZE(mock_cedt.chbs); i++) {
> -			h = (union acpi_subtable_headers *)&mock_cedt.chbs[i];
> -			end = (unsigned long)&mock_cedt.chbs[i + 1];
> -			handler_arg(h, arg, end);
> -		}
> -
> -	if (id == ACPI_CEDT_TYPE_CFMWS)
> -		for (i = 0; i < ARRAY_SIZE(mock_cfmws); i++) {
> -			h = (union acpi_subtable_headers *) mock_cfmws[i];
> -			end = (unsigned long) h + mock_cfmws[i]->header.length;
> -			handler_arg(h, arg, end);
> -		}
> -
> -	return 0;
> -}
> -
>  static bool is_mock_bridge(struct device *dev)
>  {
>  	int i;
> @@ -410,6 +370,46 @@ static bool is_mock_port(struct device *dev)
>  	return false;
>  }
>  
> +/*
> + * WARNING, this hack assumes the format of 'struct cxl_cfmws_context'
> + * and 'struct cxl_chbs_context' share the property that the first
> + * struct member is cxl_test device being probed by the cxl_acpi
> + * driver.
> + */
> +struct cxl_cedt_context {
> +	struct device *dev;
> +};
> +
> +static int mock_acpi_table_parse_cedt(enum acpi_cedt_type id,
> +				      acpi_tbl_entry_handler_arg handler_arg,
> +				      void *arg)
> +{
> +	struct cxl_cedt_context *ctx = arg;
> +	struct device *dev = ctx->dev;
> +	union acpi_subtable_headers *h;
> +	unsigned long end;
> +	int i;
> +
> +	if (!is_mock_port(dev) && !is_mock_dev(dev))
> +		return acpi_table_parse_cedt(id, handler_arg, arg);
> +
> +	if (id == ACPI_CEDT_TYPE_CHBS)
> +		for (i = 0; i < ARRAY_SIZE(mock_cedt.chbs); i++) {
> +			h = (union acpi_subtable_headers *)&mock_cedt.chbs[i];
> +			end = (unsigned long)&mock_cedt.chbs[i + 1];
> +			handler_arg(h, arg, end);
> +		}
> +
> +	if (id == ACPI_CEDT_TYPE_CFMWS)
> +		for (i = 0; i < ARRAY_SIZE(mock_cfmws); i++) {
> +			h = (union acpi_subtable_headers *) mock_cfmws[i];
> +			end = (unsigned long) h + mock_cfmws[i]->header.length;
> +			handler_arg(h, arg, end);
> +		}
> +
> +	return 0;
> +}
> +
>  static int host_bridge_index(struct acpi_device *adev)
>  {
>  	return adev - host_bridge;
>
Dan Williams Nov. 28, 2022, 10:10 p.m. UTC | #3 
Alison Schofield wrote:
> On Thu, Nov 24, 2022 at 10:35:10AM -0800, Dan Williams wrote:
> > Accept any cxl_test topology device as the first argument in
> > cxl_chbs_context. This is in preparation for reworking the detection of
> > the component registers across VH and RCH topologies. Move
> > mock_acpi_table_parse_cedt() beneath the definition of is_mock_port()
> > and use is_mock_port() instead of the explicit mock cxl_acpi device
> > check.
> 
> I'll ACK this change, alhtough I don't appreciate the code move and modify
> in the same patch. It hides the diff.

That's fair. I'll go ahead and just forward declare is_mock_port() which
makes the diff easier to read:

@@ -320,10 +320,12 @@ static int populate_cedt(void)
        return 0;
 }
 
+static bool is_mock_port(struct device *dev);
+
 /*
- * WARNING, this hack assumes the format of 'struct
- * cxl_cfmws_context' and 'struct cxl_chbs_context' share the property that
- * the first struct member is the device being probed by the cxl_acpi
+ * WARNING, this hack assumes the format of 'struct cxl_cfmws_context'
+ * and 'struct cxl_chbs_context' share the property that the first
+ * struct member is cxl_test device being probed by the cxl_acpi
  * driver.
  */
 struct cxl_cedt_context {
@@ -340,7 +342,7 @@ static int mock_acpi_table_parse_cedt(enum acpi_cedt_type id,
        unsigned long end;
        int i;
 
-       if (dev != &cxl_acpi->dev)
+       if (!is_mock_port(dev) && !is_mock_dev(dev))
                return acpi_table_parse_cedt(id, handler_arg, arg);
 
        if (id == ACPI_CEDT_TYPE_CHBS)


> The commit msg seems needlessly vague. Why not state what was done?
> Something like: 'Accept any topology device in cxl_chbs_context'

I do start off with : "Accept any cxl_test topology device as the first
argument in cxl_chbs_context", but I can move the rest to its own
paragraph.
diff mbox series

Patch

diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index facfcd11cb67..42a34650dd2f 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -320,46 +320,6 @@  static int populate_cedt(void)
 	return 0;
 }
 
-/*
- * WARNING, this hack assumes the format of 'struct
- * cxl_cfmws_context' and 'struct cxl_chbs_context' share the property that
- * the first struct member is the device being probed by the cxl_acpi
- * driver.
- */
-struct cxl_cedt_context {
-	struct device *dev;
-};
-
-static int mock_acpi_table_parse_cedt(enum acpi_cedt_type id,
-				      acpi_tbl_entry_handler_arg handler_arg,
-				      void *arg)
-{
-	struct cxl_cedt_context *ctx = arg;
-	struct device *dev = ctx->dev;
-	union acpi_subtable_headers *h;
-	unsigned long end;
-	int i;
-
-	if (dev != &cxl_acpi->dev)
-		return acpi_table_parse_cedt(id, handler_arg, arg);
-
-	if (id == ACPI_CEDT_TYPE_CHBS)
-		for (i = 0; i < ARRAY_SIZE(mock_cedt.chbs); i++) {
-			h = (union acpi_subtable_headers *)&mock_cedt.chbs[i];
-			end = (unsigned long)&mock_cedt.chbs[i + 1];
-			handler_arg(h, arg, end);
-		}
-
-	if (id == ACPI_CEDT_TYPE_CFMWS)
-		for (i = 0; i < ARRAY_SIZE(mock_cfmws); i++) {
-			h = (union acpi_subtable_headers *) mock_cfmws[i];
-			end = (unsigned long) h + mock_cfmws[i]->header.length;
-			handler_arg(h, arg, end);
-		}
-
-	return 0;
-}
-
 static bool is_mock_bridge(struct device *dev)
 {
 	int i;
@@ -410,6 +370,46 @@  static bool is_mock_port(struct device *dev)
 	return false;
 }
 
+/*
+ * WARNING, this hack assumes the format of 'struct cxl_cfmws_context'
+ * and 'struct cxl_chbs_context' share the property that the first
+ * struct member is cxl_test device being probed by the cxl_acpi
+ * driver.
+ */
+struct cxl_cedt_context {
+	struct device *dev;
+};
+
+static int mock_acpi_table_parse_cedt(enum acpi_cedt_type id,
+				      acpi_tbl_entry_handler_arg handler_arg,
+				      void *arg)
+{
+	struct cxl_cedt_context *ctx = arg;
+	struct device *dev = ctx->dev;
+	union acpi_subtable_headers *h;
+	unsigned long end;
+	int i;
+
+	if (!is_mock_port(dev) && !is_mock_dev(dev))
+		return acpi_table_parse_cedt(id, handler_arg, arg);
+
+	if (id == ACPI_CEDT_TYPE_CHBS)
+		for (i = 0; i < ARRAY_SIZE(mock_cedt.chbs); i++) {
+			h = (union acpi_subtable_headers *)&mock_cedt.chbs[i];
+			end = (unsigned long)&mock_cedt.chbs[i + 1];
+			handler_arg(h, arg, end);
+		}
+
+	if (id == ACPI_CEDT_TYPE_CFMWS)
+		for (i = 0; i < ARRAY_SIZE(mock_cfmws); i++) {
+			h = (union acpi_subtable_headers *) mock_cfmws[i];
+			end = (unsigned long) h + mock_cfmws[i]->header.length;
+			handler_arg(h, arg, end);
+		}
+
+	return 0;
+}
+
 static int host_bridge_index(struct acpi_device *adev)
 {
 	return adev - host_bridge;