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

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

[Yanfei Xu]

The first and third patch are intended to fix potential issues regarding to
retrieve and record DVSEC ranges. The second and fourth are cleanup.

v2->v3:
- improved the commit message of patch1 to indicate potential impact of
  the change.  (Dan)
- Dropped un-appropriate "Fixes" tag. (Dan)
- Dropped the patch2 which is a code movement in original patchset.
- Separated the original patch3 into cleanup one and logic change one which
  are corresponding to patch2 and patch3 in current patchset. (Dan)

v2:https://lore.kernel.org/linux-cxl/20240809093442.646545-1-yanfei.xu@intel.com/T/#t

Yanfei Xu (4):
  cxl/pci: Fix to record only non-zero ranges
  cxl/pci: Remove duplicated implementation of waiting for
    memory_info_valid
  cxl/pci: Check Mem_info_valid bit for each applicable DVSEC
  cxl/pci: simplify the check of mem_enabled in cxl_hdm_decode_init()

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

-- 
2.39.2

[v3 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.

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.

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: [v3 1/4] cxl/pci: Fix to record only non-zero ranges

[Jonathan Cameron]

On Tue, 13 Aug 2024 19:05:29 +0800
Yanfei Xu <yanfei.xu@intel.com> 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.
> 
> 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.
> 
> Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>

LGTM

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

[v3 2/4] cxl/pci: Remove duplicated implementation of waiting for memory_info_valid

[Yanfei Xu]

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()

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
---
 drivers/cxl/core/pci.c        | 41 +++++------------------------------
 drivers/cxl/cxl.h             |  2 +-
 drivers/cxl/port.c            |  2 +-
 tools/testing/cxl/test/mock.c |  4 ++--
 4 files changed, 9 insertions(+), 40 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 2d69340134da..38c567727dbb 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);
@@ -322,11 +291,13 @@ 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;
 	u16 cap, ctrl;
 
 	if (!d) {
@@ -353,11 +324,9 @@ int cxl_dvsec_rr_decode(struct device *dev, int d,
 	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);
+	rc = cxl_dvsec_mem_range_valid(cxlds, 0);
+	if (rc)
 		return rc;
-	}
 
 	/*
 	 * The current DVSEC values are moot if the memory capability is
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: [v3 2/4] cxl/pci: Remove duplicated implementation of waiting for memory_info_valid

[Jonathan Cameron]

On Tue, 13 Aug 2024 19:05:30 +0800
Yanfei Xu <yanfei.xu@intel.com> wrote:

> 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()
Mention here the name of the other duplicate function

Also good to call out why you picked this duplicate to remove
over the other one.

Otherwise looks good to me. 
So with that info added.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
> ---
>  drivers/cxl/core/pci.c        | 41 +++++------------------------------
>  drivers/cxl/cxl.h             |  2 +-
>  drivers/cxl/port.c            |  2 +-
>  tools/testing/cxl/test/mock.c |  4 ++--
>  4 files changed, 9 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 2d69340134da..38c567727dbb 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;
> -}
> -

Re: [v3 2/4] cxl/pci: Remove duplicated implementation of waiting for memory_info_valid

[Yanfei Xu]

On 8/28/2024 12:16 AM, Jonathan Cameron wrote:
> On Tue, 13 Aug 2024 19:05:30 +0800
> Yanfei Xu <yanfei.xu@intel.com> wrote:
> 
>> 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()
> Mention here the name of the other duplicate function
> 
> Also good to call out why you picked this duplicate to remove
> over the other one.
> 
> Otherwise looks good to me.
> So with that info added.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks! will improve it in next version.

> 
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
>> ---
>>   drivers/cxl/core/pci.c        | 41 +++++------------------------------
>>   drivers/cxl/cxl.h             |  2 +-
>>   drivers/cxl/port.c            |  2 +-
>>   tools/testing/cxl/test/mock.c |  4 ++--
>>   4 files changed, 9 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 2d69340134da..38c567727dbb 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;
>> -}
>> -
> 
> 

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

[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, hence
let's move the check into the "for loop" of handling each DVSEC range.

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

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 38c567727dbb..519989ada48e 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -324,10 +324,6 @@ int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
 	if (!hdm_count || hdm_count > 2)
 		return -EINVAL;
 
-	rc = cxl_dvsec_mem_range_valid(cxlds, 0);
-	if (rc)
-		return rc;
-
 	/*
 	 * The current DVSEC values are moot if the memory capability is
 	 * disabled, and they will remain moot after the HDM Decoder
@@ -345,6 +341,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: [v3 3/4] cxl/pci: Check Mem_info_valid bit for each applicable DVSEC

[Jonathan Cameron]

On Tue, 13 Aug 2024 19:05:31 +0800
Yanfei Xu <yanfei.xu@intel.com> 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, hence
> let's move the check into the "for loop" of handling each DVSEC range.
Say why it's the 'right' way.   I agree it probably is, but more
detail in this patch description would be good.
I assume it's as simple as
"In theory a device might set the mem_info_valid bit for a first range
 after it is ready but before as second range has reached that state."

If so looks fine to me and with that additional detail,
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> 
> Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
> ---
>  drivers/cxl/core/pci.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 38c567727dbb..519989ada48e 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -324,10 +324,6 @@ int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
>  	if (!hdm_count || hdm_count > 2)
>  		return -EINVAL;
>  
> -	rc = cxl_dvsec_mem_range_valid(cxlds, 0);
> -	if (rc)
> -		return rc;
> -
>  	/*
>  	 * The current DVSEC values are moot if the memory capability is
>  	 * disabled, and they will remain moot after the HDM Decoder
> @@ -345,6 +341,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)

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

[Yanfei Xu]

On 8/28/2024 12:22 AM, Jonathan Cameron wrote:
> On Tue, 13 Aug 2024 19:05:31 +0800
> Yanfei Xu <yanfei.xu@intel.com> 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, hence
>> let's move the check into the "for loop" of handling each DVSEC range.
> Say why it's the 'right' way.   I agree it probably is, but more
> detail in this patch description would be good.
> I assume it's as simple as
> "In theory a device might set the mem_info_valid bit for a first range
>   after it is ready but before as second range has reached that state."
> 
> If so looks fine to me and with that additional detail,
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Make sense, will do in v4. Thanks for your review!

> 
> 
>>
>> Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
>> ---
>>   drivers/cxl/core/pci.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 38c567727dbb..519989ada48e 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -324,10 +324,6 @@ int cxl_dvsec_rr_decode(struct device *dev, struct cxl_port *port,
>>   	if (!hdm_count || hdm_count > 2)
>>   		return -EINVAL;
>>   
>> -	rc = cxl_dvsec_mem_range_valid(cxlds, 0);
>> -	if (rc)
>> -		return rc;
>> -
>>   	/*
>>   	 * The current DVSEC values are moot if the memory capability is
>>   	 * disabled, and they will remain moot after the HDM Decoder
>> @@ -345,6 +341,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)
> 
> 

[v3 4/4] cxl/pci: simplify the check of mem_enabled in cxl_hdm_decode_init()

[Yanfei Xu]

Cases can be divided into two categories which are DVSEC range enabled and
not enabled when HDM decoders exist but is not enabled. To avoid checking
info->mem_enabled, which indicates the enablement of DVSEC range, every
time, we can check !info->mem_enabled once in advance. This simplification
can make the code clearer.

No functional change intended.

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

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 519989ada48e..be00266c8907 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -426,7 +426,15 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 		return -ENODEV;
 	}
 
-	for (i = 0, allowed = 0; info->mem_enabled && i < info->ranges; i++) {
+	if (!info->mem_enabled) {
+		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
+		if (rc)
+			return rc;
+
+		return devm_cxl_enable_mem(&port->dev, cxlds);
+	}
+
+	for (i = 0, allowed = 0; i < info->ranges; i++) {
 		struct device *cxld_dev;
 
 		cxld_dev = device_find_child(&root->dev, &info->dvsec_range[i],
@@ -440,7 +448,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 		allowed++;
 	}
 
-	if (!allowed && info->mem_enabled) {
+	if (!allowed) {
 		dev_err(dev, "Range register decodes outside platform defined CXL ranges.\n");
 		return -ENXIO;
 	}
@@ -454,14 +462,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: [v3 4/4] cxl/pci: simplify the check of mem_enabled in cxl_hdm_decode_init()

[Jonathan Cameron]

On Tue, 13 Aug 2024 19:05:32 +0800
Yanfei Xu <yanfei.xu@intel.com> wrote:

> Cases can be divided into two categories which are DVSEC range enabled and
> not enabled when HDM decoders exist but is not enabled. To avoid checking
> info->mem_enabled, which indicates the enablement of DVSEC range, every
> time, we can check !info->mem_enabled once in advance. This simplification
> can make the code clearer.
> 
> No functional change intended.
> 
> Signed-off-by: Yanfei Xu <yanfei.xu@intel.com>
LGTM as definitely simpler if we handled the !mem_enabled case first.

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

Re: [v3 0/4] Fixes for hdm docoder initialization from DVSEC ranges

[Yanfei Xu]

gentle ping

On 8/13/2024 7:05 PM, Yanfei Xu wrote:
> The first and third patch are intended to fix potential issues regarding to
> retrieve and record DVSEC ranges. The second and fourth are cleanup.
> 
> v2->v3:
> - improved the commit message of patch1 to indicate potential impact of
>    the change.  (Dan)
> - Dropped un-appropriate "Fixes" tag. (Dan)
> - Dropped the patch2 which is a code movement in original patchset.
> - Separated the original patch3 into cleanup one and logic change one which
>    are corresponding to patch2 and patch3 in current patchset. (Dan)
> 
> v2:https://lore.kernel.org/linux-cxl/20240809093442.646545-1-yanfei.xu@intel.com/T/#t
> 
> Yanfei Xu (4):
>    cxl/pci: Fix to record only non-zero ranges
>    cxl/pci: Remove duplicated implementation of waiting for
>      memory_info_valid
>    cxl/pci: Check Mem_info_valid bit for each applicable DVSEC
>    cxl/pci: simplify the check of mem_enabled in cxl_hdm_decode_init()
> 
>   drivers/cxl/core/pci.c        | 74 +++++++++--------------------------
>   drivers/cxl/cxl.h             |  2 +-
>   drivers/cxl/port.c            |  2 +-
>   tools/testing/cxl/test/mock.c |  4 +-
>   4 files changed, 23 insertions(+), 59 deletions(-)
> 

Re: [v3 0/4] Fixes for hdm docoder initialization from DVSEC ranges

[Jonathan Cameron]

On Tue, 13 Aug 2024 19:05:28 +0800
Yanfei Xu <yanfei.xu@intel.com> wrote:

> The first and third patch are intended to fix potential issues regarding to
> retrieve and record DVSEC ranges. The second and fourth are cleanup.

typo in patch title.  decoder


Also, good to include cxl: as prefix to the patch title.

> 
> v2->v3:
> - improved the commit message of patch1 to indicate potential impact of
>   the change.  (Dan)
> - Dropped un-appropriate "Fixes" tag. (Dan)
> - Dropped the patch2 which is a code movement in original patchset.
> - Separated the original patch3 into cleanup one and logic change one which
>   are corresponding to patch2 and patch3 in current patchset. (Dan)
> 
> v2:https://lore.kernel.org/linux-cxl/20240809093442.646545-1-yanfei.xu@intel.com/T/#t
> 
> Yanfei Xu (4):
>   cxl/pci: Fix to record only non-zero ranges
>   cxl/pci: Remove duplicated implementation of waiting for
>     memory_info_valid
>   cxl/pci: Check Mem_info_valid bit for each applicable DVSEC
>   cxl/pci: simplify the check of mem_enabled in cxl_hdm_decode_init()
> 
>  drivers/cxl/core/pci.c        | 74 +++++++++--------------------------
>  drivers/cxl/cxl.h             |  2 +-
>  drivers/cxl/port.c            |  2 +-
>  tools/testing/cxl/test/mock.c |  4 +-
>  4 files changed, 23 insertions(+), 59 deletions(-)
> 

Re: [v3 0/4] Fixes for hdm docoder initialization from DVSEC ranges

[Yanfei Xu]

On 8/28/2024 12:08 AM, Jonathan Cameron wrote:
> On Tue, 13 Aug 2024 19:05:28 +0800
> Yanfei Xu <yanfei.xu@intel.com> wrote:
> 
>> The first and third patch are intended to fix potential issues regarding to
>> retrieve and record DVSEC ranges. The second and fourth are cleanup.
> 
> typo in patch title.  decoder
> 
> 
> Also, good to include cxl: as prefix to the patch title.
> 

Got it, will do.

>>
>> v2->v3:
>> - improved the commit message of patch1 to indicate potential impact of
>>    the change.  (Dan)
>> - Dropped un-appropriate "Fixes" tag. (Dan)
>> - Dropped the patch2 which is a code movement in original patchset.
>> - Separated the original patch3 into cleanup one and logic change one which
>>    are corresponding to patch2 and patch3 in current patchset. (Dan)
>>
>> v2:https://lore.kernel.org/linux-cxl/20240809093442.646545-1-yanfei.xu@intel.com/T/#t
>>
>> Yanfei Xu (4):
>>    cxl/pci: Fix to record only non-zero ranges
>>    cxl/pci: Remove duplicated implementation of waiting for
>>      memory_info_valid
>>    cxl/pci: Check Mem_info_valid bit for each applicable DVSEC
>>    cxl/pci: simplify the check of mem_enabled in cxl_hdm_decode_init()
>>
>>   drivers/cxl/core/pci.c        | 74 +++++++++--------------------------
>>   drivers/cxl/cxl.h             |  2 +-
>>   drivers/cxl/port.c            |  2 +-
>>   tools/testing/cxl/test/mock.c |  4 +-
>>   4 files changed, 23 insertions(+), 59 deletions(-)
>>
> 
>