[v2 0/4] Fixes for hdm docoder initialization from DVSEC ranges

[v2 0/4] Fixes for hdm docoder initialization from DVSEC ranges

[Yanfei Xu]

The first three patches are functional fixes, the fourth one is for adjustment
of code format.

v1:https://lore.kernel.org/linux-cxl/66b4db4ff3730_c144829418@dwillia2-xfh.jf.intel.com.notmuch/T/#t

v1->v2:
  separate one big patch into four.


Yanfei Xu (4):
  cxl/pci: Fix to record only non-zero ranges
  cxl/pci: Don't set up decoders for disallowed DVSEC ranges
  cxl/pci: Check Mem_info_valid bit for each applicable DVSEC range
  cxl/pci: Simplify the code logic of cxl_hdm_decode_init

 drivers/cxl/core/pci.c        | 114 ++++++++++++----------------------
 drivers/cxl/cxl.h             |   2 +-
 drivers/cxl/port.c            |   2 +-
 tools/testing/cxl/test/mock.c |   4 +-
 4 files changed, 42 insertions(+), 80 deletions(-)

-- 
2.39.2

[v2 1/4] cxl/pci: Fix to record only non-zero ranges

[Yanfei Xu]

The function cxl_dvsec_rr_decode() retrieves and records DVSEC
ranges into info->dvsec_range[], regardless of whether it is
non-zero range, and the variable info->ranges indicates the number
of non-zero ranges. However, in cxl_hdm_decode_init(), the validation
for info->dvsec_range[] occurs in a for loop that iterates based
on info->ranges. It may result in zero range to be validated but
non-zero range not be validated, in turn, the number of allowed
ranges is to be 0. Address it by only record non-zero ranges.

Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
---
 drivers/cxl/core/pci.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index a663e7566c48..2d69340134da 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -390,10 +390,6 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
 
 		size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK;
 		if (!size) {
-			info->dvsec_range[i] = (struct range) {
-				.start = 0,
-				.end = CXL_RESOURCE_NONE,
-			};
 			continue;
 		}
 
@@ -411,12 +407,10 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
 
 		base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
 
-		info->dvsec_range[i] = (struct range) {
+		info->dvsec_range[ranges++] = (struct range) {
 			.start = base,
 			.end = base + size - 1
 		};
-
-		ranges++;
 	}
 
 	info->ranges = ranges;
-- 
2.39.2

Re: [v2 1/4] cxl/pci: Fix to record only non-zero ranges

[Dan Williams]

Yanfei Xu wrote:
> The function cxl_dvsec_rr_decode() retrieves and records DVSEC
> ranges into info->dvsec_range[], regardless of whether it is
> non-zero range, and the variable info->ranges indicates the number
> of non-zero ranges. However, in cxl_hdm_decode_init(), the validation
> for info->dvsec_range[] occurs in a for loop that iterates based
> on info->ranges. It may result in zero range to be validated but
> non-zero range not be validated, in turn, the number of allowed
> ranges is to be 0. Address it by only record non-zero ranges.

When applying this should mention the potential impact of the change,
something like:

"This fix is not urgent as it requires a configuration that zeroes out
the first dvsec range while populating the second. This has not been
observed, but it is theoretically possible. If this gets picked up for
-stable, no harm done, but there is no urgency to backport."

Re: [v2 1/4] cxl/pci: Fix to record only non-zero ranges

[Yanfei Xu]

On 8/10/2024 2:55 AM, Dan Williams wrote:
> Yanfei Xu wrote:
>> The function cxl_dvsec_rr_decode() retrieves and records DVSEC
>> ranges into info->dvsec_range[], regardless of whether it is
>> non-zero range, and the variable info->ranges indicates the number
>> of non-zero ranges. However, in cxl_hdm_decode_init(), the validation
>> for info->dvsec_range[] occurs in a for loop that iterates based
>> on info->ranges. It may result in zero range to be validated but
>> non-zero range not be validated, in turn, the number of allowed
>> ranges is to be 0. Address it by only record non-zero ranges.
> 
> When applying this should mention the potential impact of the change,
> something like:
> 
> "This fix is not urgent as it requires a configuration that zeroes out
> the first dvsec range while populating the second. This has not been
> observed, but it is theoretically possible. If this gets picked up for
> -stable, no harm done, but there is no urgency to backport."

Thanks, Will add in v3.

Yanfei

[v2 2/4] cxl/pci: Don't set up decoders for disallowed DVSEC ranges

[Yanfei Xu]

Since it shouldn't create and configure decoders for disallowed
ranges, move the check of dvsec_range_allowed() earlier into
cxl_dvsec_rr_decode() to filter the disallowed DVSEC ranges out
at firtst.

Fixes: 34e37b4c432c ("cxl/port: Enable HDM Capability after validating DVSEC Ranges")
Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
---
 drivers/cxl/core/pci.c        | 62 +++++++++++++++++------------------
 drivers/cxl/cxl.h             |  2 +-
 drivers/cxl/port.c            |  2 +-
 tools/testing/cxl/test/mock.c |  4 +--
 4 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 2d69340134da..0915fc9e6d70 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -322,11 +322,14 @@ static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
 	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
 }
 
-int cxl_dvsec_rr_decode(struct device *dev, int d,
+int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
 			struct cxl_endpoint_dvsec_info *info)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
+	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
 	int hdm_count, rc, i, ranges = 0;
+	int d = cxlds->cxl_dvsec;
+	struct cxl_port *root;
 	u16 cap, ctrl;
 
 	if (!d) {
@@ -359,6 +362,14 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
 		return rc;
 	}
 
+	root = to_cxl_port(port->dev.parent);
+	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
+		root = to_cxl_port(root->dev.parent);
+	if (!is_cxl_root(root)) {
+		dev_err(dev, "Failed to acquire root port for HDM enable\n");
+		return -ENODEV;
+	}
+
 	/*
 	 * The current DVSEC values are moot if the memory capability is
 	 * disabled, and they will remain moot after the HDM Decoder
@@ -373,6 +384,8 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
 		return 0;
 
 	for (i = 0; i < hdm_count; i++) {
+		struct device *cxld_dev;
+		struct range dvsec_range;
 		u64 base, size;
 		u32 temp;
 
@@ -389,9 +402,8 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
 			return rc;
 
 		size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK;
-		if (!size) {
+		if (!size)
 			continue;
-		}
 
 		rc = pci_read_config_dword(
 			pdev, d + CXL_DVSEC_RANGE_BASE_HIGH(i), &temp);
@@ -407,10 +419,21 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
 
 		base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
 
-		info->dvsec_range[ranges++] = (struct range) {
+		dvsec_range = (struct range) {
 			.start = base,
-			.end = base + size - 1
+			.end = base + size - 1,
 		};
+
+		cxld_dev = device_find_child(&root->dev, &dvsec_range,
+							dvsec_range_allowed);
+		if (!cxld_dev) {
+			dev_dbg(dev, "DVSEC Range%d denied by platform\n", i+1);
+			continue;
+		}
+		dev_dbg(dev, "DVSEC Range%d allowed by platform\n", i+1);
+		put_device(cxld_dev);
+
+		info->dvsec_range[ranges++] = dvsec_range;
 	}
 
 	info->ranges = ranges;
@@ -433,9 +456,8 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
 	struct cxl_port *port = cxlhdm->port;
 	struct device *dev = cxlds->dev;
-	struct cxl_port *root;
-	int i, rc, allowed;
 	u32 global_ctrl = 0;
+	int rc;
 
 	if (hdm)
 		global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
@@ -449,30 +471,8 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	else if (!hdm)
 		return -ENODEV;
 
-	root = to_cxl_port(port->dev.parent);
-	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
-		root = to_cxl_port(root->dev.parent);
-	if (!is_cxl_root(root)) {
-		dev_err(dev, "Failed to acquire root port for HDM enable\n");
-		return -ENODEV;
-	}
-
-	for (i = 0, allowed = 0; info->mem_enabled && i < info->ranges; i++) {
-		struct device *cxld_dev;
-
-		cxld_dev = device_find_child(&root->dev, &info->dvsec_range[i],
-					     dvsec_range_allowed);
-		if (!cxld_dev) {
-			dev_dbg(dev, "DVSEC Range%d denied by platform\n", i);
-			continue;
-		}
-		dev_dbg(dev, "DVSEC Range%d allowed by platform\n", i);
-		put_device(cxld_dev);
-		allowed++;
-	}
-
-	if (!allowed && info->mem_enabled) {
-		dev_err(dev, "Range register decodes outside platform defined CXL ranges.\n");
+	if (!info->ranges && info->mem_enabled) {
+		dev_err(dev, "No available DVSEC register ranges.\n");
 		return -ENXIO;
 	}
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 9afb407d438f..e2e277463794 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -809,7 +809,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 				struct cxl_endpoint_dvsec_info *info);
 int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
-int cxl_dvsec_rr_decode(struct device *dev, int dvsec,
+int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
 			struct cxl_endpoint_dvsec_info *info);
 
 bool is_cxl_region(struct device *dev);
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index d7d5d982ce69..861dde65768f 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -98,7 +98,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
 	struct cxl_port *root;
 	int rc;
 
-	rc = cxl_dvsec_rr_decode(cxlds->dev, cxlds->cxl_dvsec, &info);
+	rc = cxl_dvsec_rr_decode(cxlds->dev, port, &info);
 	if (rc < 0)
 		return rc;
 
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index 6f737941dc0e..79fdfaad49e8 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -228,7 +228,7 @@ int __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_cxl_hdm_decode_init, CXL);
 
-int __wrap_cxl_dvsec_rr_decode(struct device *dev, int dvsec,
+int __wrap_cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
 			       struct cxl_endpoint_dvsec_info *info)
 {
 	int rc = 0, index;
@@ -237,7 +237,7 @@ int __wrap_cxl_dvsec_rr_decode(struct device *dev, int dvsec,
 	if (ops && ops->is_mock_dev(dev))
 		rc = 0;
 	else
-		rc = cxl_dvsec_rr_decode(dev, dvsec, info);
+		rc = cxl_dvsec_rr_decode(dev, port, info);
 	put_cxl_mock_ops(index);
 
 	return rc;
-- 
2.39.2

Re: [v2 2/4] cxl/pci: Don't set up decoders for disallowed DVSEC ranges

[Dan Williams]

Yanfei Xu wrote:
> Since it shouldn't create and configure decoders for disallowed
> ranges, move the check of dvsec_range_allowed() earlier into
> cxl_dvsec_rr_decode() to filter the disallowed DVSEC ranges out
> at firtst.

"Fixes" explain the user visible side effect of the problem. There is no
problem being fixed with this. It is maybe a conceptual cleanup, but to
me the risk and thrash is not worth the reward. Unless I am missing
something this is just code movement for no value.

The "risk" is that I think this breaks cases where BIOS has enabled HDM
decoders. I.e.

        /*
         * If the HDM Decoder Capability is already enabled then assume
         * that some other agent like platform firmware set it up.
         */     
        if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && info->mem_enabled))
                return devm_cxl_enable_mem(&port->dev, cxlds);

...the DVSEC range resgister values do not matter when HDM decoders are
enabled.

Re: [v2 2/4] cxl/pci: Don't set up decoders for disallowed DVSEC ranges

[Yanfei Xu]

On 8/10/2024 3:02 AM, Dan Williams wrote:
> Yanfei Xu wrote:
>> Since it shouldn't create and configure decoders for disallowed
>> ranges, move the check of dvsec_range_allowed() earlier into
>> cxl_dvsec_rr_decode() to filter the disallowed DVSEC ranges out
>> at firtst.
> 
> "Fixes" explain the user visible side effect of the problem. There is no
> problem being fixed with this. It is maybe a conceptual cleanup, but to
> me the risk and thrash is not worth the reward. Unless I am missing
> something this is just code movement for no value.

You are right, this is much like a cleanup as creating decoders for 
disallowed ranges doesn't have side effect for user. Will drop the "Fixes".

I just was aiming to avoid to create decoders for the disallowed ranges, 
since this kind of decoders don't take effect in practice. And we 
already checked and found that, why continued to create that? :)

How about changing the dev_dbg() in below codes to dev_warn() when find 
disallowed ranges to let the user to know what happened.

         cxld_dev = device_find_child(&root->dev, &dvsec_range,
                             dvsec_range_allowed);
         if (!cxld_dev) {
             dev_dbg(dev, "DVSEC Range%d denied by platform\n", i+1);
             continue;
         }
         dev_dbg(dev, "DVSEC Range%d allowed by platform\n", i+1);
         put_device(cxld_dev);

> 
> The "risk" is that I think this breaks cases where BIOS has enabled HDM
> decoders. I.e.
> 
>          /*
>           * If the HDM Decoder Capability is already enabled then assume
>           * that some other agent like platform firmware set it up.
>           */
>          if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && info->mem_enabled))
>                  return devm_cxl_enable_mem(&port->dev, cxlds);
> 
> ...the DVSEC range resgister values do not matter when HDM decoders are
> enabled.

It won't. Even no allowed range, it only keep 0 in info->ranges first. 
And the check for whether info->ranges is 0 is performed after the check 
you pasted.

     if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && 
info->mem_enabled))
         return devm_cxl_enable_mem(&port->dev, cxlds);
     else if (!hdm)
         return -ENODEV;
     ......

     if (!info->ranges) {
         dev_err(dev, "No available DVSEC register ranges.\n");
         return -ENXIO;
     }


Thanks,
Yanfei

[v2 3/4] cxl/pci: Check Mem_info_valid bit for each applicable DVSEC range

[Yanfei Xu]

The right way is to checking Mem_info_valid bit for each applicable
DVSEC range against HDM_COUNT, not only for the DVSEC range 1. Also
the functions to check the Mem_info_valid bit are repeatedly
implemented, drop the rough one.

Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
---
 drivers/cxl/core/pci.c | 41 ++++-------------------------------------
 1 file changed, 4 insertions(+), 37 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 0915fc9e6d70..e822cc9ce315 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -211,37 +211,6 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_await_media_ready, CXL);
 
-static int wait_for_valid(struct pci_dev *pdev, int d)
-{
-	u32 val;
-	int rc;
-
-	/*
-	 * Memory_Info_Valid: When set, indicates that the CXL Range 1 Size high
-	 * and Size Low registers are valid. Must be set within 1 second of
-	 * deassertion of reset to CXL device. Likely it is already set by the
-	 * time this runs, but otherwise give a 1.5 second timeout in case of
-	 * clock skew.
-	 */
-	rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val);
-	if (rc)
-		return rc;
-
-	if (val & CXL_DVSEC_MEM_INFO_VALID)
-		return 0;
-
-	msleep(1500);
-
-	rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val);
-	if (rc)
-		return rc;
-
-	if (val & CXL_DVSEC_MEM_INFO_VALID)
-		return 0;
-
-	return -ETIMEDOUT;
-}
-
 static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
 {
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
@@ -356,12 +325,6 @@ int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
 	if (!hdm_count || hdm_count > 2)
 		return -EINVAL;
 
-	rc = wait_for_valid(pdev, d);
-	if (rc) {
-		dev_dbg(dev, "Failure awaiting MEM_INFO_VALID (%d)\n", rc);
-		return rc;
-	}
-
 	root = to_cxl_port(port->dev.parent);
 	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
 		root = to_cxl_port(root->dev.parent);
@@ -389,6 +352,10 @@ int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
 		u64 base, size;
 		u32 temp;
 
+		rc = cxl_dvsec_mem_range_valid(cxlds, i);
+		if (rc)
+			return rc;
+
 		rc = pci_read_config_dword(
 			pdev, d + CXL_DVSEC_RANGE_SIZE_HIGH(i), &temp);
 		if (rc)
-- 
2.39.2

Re: [v2 3/4] cxl/pci: Check Mem_info_valid bit for each applicable DVSEC range

[Dan Williams]

Yanfei Xu wrote:
> The right way is to checking Mem_info_valid bit for each applicable
> DVSEC range against HDM_COUNT, not only for the DVSEC range 1. Also
> the functions to check the Mem_info_valid bit are repeatedly
> implemented, drop the rough one.

Again, not a fix, as there is no evidence of a device in the wild that
initializes memory_info_valid of range1 on a different timescale than
range2.

A better description of this patch is something like:

"commit ce17ad0d5498 ("cxl: Wait Memory_Info_Valid before access memory
related info") added another implementation of waiting for
memory_info_valid without realizing it duplicated wait_for_valid()"

...I would also *only* do that cleanup and save changing the logic about
when it is called to a separate patch. Neither of those would be marked
as a fix because it depends on odd device behavior for it to be a
problem in practice.

Re: [v2 3/4] cxl/pci: Check Mem_info_valid bit for each applicable DVSEC range

[Yanfei Xu]

On 8/10/2024 3:13 AM, Dan Williams wrote:
> Yanfei Xu wrote:
>> The right way is to checking Mem_info_valid bit for each applicable
>> DVSEC range against HDM_COUNT, not only for the DVSEC range 1. Also
>> the functions to check the Mem_info_valid bit are repeatedly
>> implemented, drop the rough one.
> 
> Again, not a fix, as there is no evidence of a device in the wild that
> initializes memory_info_valid of range1 on a different timescale than
> range2.
> 
> A better description of this patch is something like:
> 
> "commit ce17ad0d5498 ("cxl: Wait Memory_Info_Valid before access memory
> related info") added another implementation of waiting for
> memory_info_valid without realizing it duplicated wait_for_valid()"
> 
> ...I would also *only* do that cleanup and save changing the logic about
> when it is called to a separate patch. Neither of those would be marked
> as a fix because it depends on odd device behavior for it to be a
> problem in practice.

OK. Will split this patch into two and drop the "Fixes"

Thanks,
Yanfei

[v2 4/4] cxl/pci: Simplify the code logic of cxl_hdm_decode_init

[Yanfei Xu]

When HDM decoders exist but is not enabled, the cases can be divided into
two categories: DVSEC range enabled and not enabled. Extract the check of
mem_enabled out to improve code readability. No functional change.

Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
---
 drivers/cxl/core/pci.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index e822cc9ce315..09d63a62f05b 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -438,7 +438,15 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	else if (!hdm)
 		return -ENODEV;
 
-	if (!info->ranges && info->mem_enabled) {
+	if (!info->mem_enabled) {
+		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
+		if (rc)
+			return rc;
+
+		return devm_cxl_enable_mem(&port->dev, cxlds);
+	}
+
+	if (!info->ranges) {
 		dev_err(dev, "No available DVSEC register ranges.\n");
 		return -ENXIO;
 	}
@@ -452,14 +460,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
 	 * Decoder Capability Enable.
 	 */
-	if (info->mem_enabled)
-		return 0;
-
-	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
-	if (rc)
-		return rc;
-
-	return devm_cxl_enable_mem(&port->dev, cxlds);
+	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, CXL);
 
-- 
2.39.2

Re: [v2 4/4] cxl/pci: Simplify the code logic of cxl_hdm_decode_init

[Dan Williams]

Yanfei Xu wrote:
> When HDM decoders exist but is not enabled, the cases can be divided into
> two categories: DVSEC range enabled and not enabled. Extract the check of
> mem_enabled out to improve code readability. No functional change.

I am not convinced that this is clear win, and that there is no
functional change.

Re: [v2 4/4] cxl/pci: Simplify the code logic of cxl_hdm_decode_init

[Yanfei Xu]

On 8/10/2024 3:15 AM, Dan Williams wrote:
> Yanfei Xu wrote:
>> When HDM decoders exist but is not enabled, the cases can be divided into
>> two categories: DVSEC range enabled and not enabled. Extract the check of
>> mem_enabled out to improve code readability. No functional change.
> 
> I am not convinced that this is clear win, and that there is no
> functional change.

Yeah, no functional change.

Before the patch2 which moves the check of dvsec_range_allowed() 
earlier, the previous codes in cxl_hdm_decode_init() is like below:
             ....
         for (i = 0, allowed = 0; info->mem_enabled && i < info->ranges; 
i++) {
             ....
         }

         if (!allowed && info->mem_enabled) {
             ....
         }

             ....
         if (info->mem_enabled)
                 return 0;

It checks "info->mem_enabled" consecutively three times, so if 
extracting the check and replace it with one check of 
"!info->mem_enabled" can improve code readability I was thinking.
However with applying of patch2, maybe this patch is not much necessary, 
we can drop it. :)

Thanks,
Yanfei