[PATCH v1 1/1] cxl/hdm: Verify HDM decoder capabilities after parsing

[PATCH v1 1/1] cxl/hdm: Verify HDM decoder capabilities after parsing

[Li Ming]

devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM
decoder capability, But according to the implementation of
cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0.

Per CXL specification, the values ranges of decoder_count and
target_count are limited. Adding a checking for the values of them
in case hardware initialized them with wrong values.

Signed-off-by: Li Ming <ming.li@zohomail.com>
---
base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next
---
 drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 70cae4ebf8a4..a98191867c22 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
 	return true;
 }
 
+static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm)
+{
+	/*
+	 * CXL r3.2 section 8.2.4.20.1
+	 * CXL devices shall not advertise more than 10 decoders,
+	 * CXL switches and HBs may advertise up to 32 decoders.
+	 */
+	if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10)
+		return -EINVAL;
+	else if (cxlhdm->decoder_count > 32)
+		return -EINVAL;
+
+	/*
+	 * CXL r3.2 section 8.2.4.20.1
+	 * target count is applicable only to CXL upstream port and HB.
+	 * The number of target ports each decoder supports should be
+	 * one of the numbers 1, 2, 4 or 8.
+	 */
+	if (!is_cxl_endpoint(cxlhdm->port) &&
+	    cxlhdm->target_count != 1 &&
+	    cxlhdm->target_count != 2 &&
+	    cxlhdm->target_count != 4 &&
+	    cxlhdm->target_count != 8)
+		return -EINVAL;
+
+	return 0;
+}
+
 /**
  * devm_cxl_setup_hdm - map HDM decoder component registers
  * @port: cxl_port to map
@@ -182,7 +210,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 	}
 
 	parse_hdm_decoder_caps(cxlhdm);
-	if (cxlhdm->decoder_count == 0) {
+	rc = cxlhdm_decoder_caps_verify(cxlhdm);
+	if (rc) {
 		dev_err(dev, "Spec violation. Caps invalid\n");
 		return ERR_PTR(-ENXIO);
 	}
-- 
2.34.1

Re: [PATCH v1 1/1] cxl/hdm: Verify HDM decoder capabilities after parsing

[Dave Jiang]

On 2/27/25 3:32 AM, Li Ming wrote:
> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM
> decoder capability, But according to the implementation of
> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0.
> 
> Per CXL specification, the values ranges of decoder_count and
> target_count are limited. Adding a checking for the values of them
> in case hardware initialized them with wrong values.
> 
> Signed-off-by: Li Ming <ming.li@zohomail.com>
> ---
> base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next
> ---
>  drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 70cae4ebf8a4..a98191867c22 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>  	return true;
>  }
>  
> +static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm)
> +{
> +	/*
> +	 * CXL r3.2 section 8.2.4.20.1
> +	 * CXL devices shall not advertise more than 10 decoders,
> +	 * CXL switches and HBs may advertise up to 32 decoders.
> +	 */
> +	if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10)

#define the limit please

> +		return -EINVAL;
> +	else if (cxlhdm->decoder_count > 32)

same here

DJ
> +		return -EINVAL;
> +
> +	/*
> +	 * CXL r3.2 section 8.2.4.20.1
> +	 * target count is applicable only to CXL upstream port and HB.
> +	 * The number of target ports each decoder supports should be
> +	 * one of the numbers 1, 2, 4 or 8.
> +	 */
> +	if (!is_cxl_endpoint(cxlhdm->port) &&
> +	    cxlhdm->target_count != 1 &&
> +	    cxlhdm->target_count != 2 &&
> +	    cxlhdm->target_count != 4 &&
> +	    cxlhdm->target_count != 8)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  /**
>   * devm_cxl_setup_hdm - map HDM decoder component registers
>   * @port: cxl_port to map
> @@ -182,7 +210,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>  	}
>  
>  	parse_hdm_decoder_caps(cxlhdm);
> -	if (cxlhdm->decoder_count == 0) {
> +	rc = cxlhdm_decoder_caps_verify(cxlhdm);
> +	if (rc) {
>  		dev_err(dev, "Spec violation. Caps invalid\n");
>  		return ERR_PTR(-ENXIO);
>  	}

Re: [PATCH v1 1/1] cxl/hdm: Verify HDM decoder capabilities after parsing

[Li Ming]

On 2/27/2025 11:21 PM, Dave Jiang wrote:
>
> On 2/27/25 3:32 AM, Li Ming wrote:
>> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM
>> decoder capability, But according to the implementation of
>> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0.
>>
>> Per CXL specification, the values ranges of decoder_count and
>> target_count are limited. Adding a checking for the values of them
>> in case hardware initialized them with wrong values.
>>
>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>> ---
>> base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next
>> ---
>>  drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++-
>>  1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 70cae4ebf8a4..a98191867c22 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>>  	return true;
>>  }
>>  
>> +static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm)
>> +{
>> +	/*
>> +	 * CXL r3.2 section 8.2.4.20.1
>> +	 * CXL devices shall not advertise more than 10 decoders,
>> +	 * CXL switches and HBs may advertise up to 32 decoders.
>> +	 */
>> +	if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10)
> #define the limit please

Will do, thanks.

>
>> +		return -EINVAL;
>> +	else if (cxlhdm->decoder_count > 32)
> same here
>
> DJ

Will do, thanks for review.


Ming

[snip]

Re: [PATCH v1 1/1] cxl/hdm: Verify HDM decoder capabilities after parsing

[Alison Schofield]

On Thu, Feb 27, 2025 at 06:32:51PM +0800, Li Ming wrote:
> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM
> decoder capability, But according to the implementation of
> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0.

How does a check against the spec maximums benefit this driver? Is there
a bad path we avoid by checking and quitting at this point.

Might this catch silly decoder counts that the driver previously
ignored?

> 
> Per CXL specification, the values ranges of decoder_count and
> target_count are limited. Adding a checking for the values of them
> in case hardware initialized them with wrong values.

Similar question - is this catching something sooner, rather than
later?

> 
> Signed-off-by: Li Ming <ming.li@zohomail.com>
> ---
> base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next
> ---
>  drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 70cae4ebf8a4..a98191867c22 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>  	return true;
>  }
>  
> +static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm)
> +{
> +	/*
> +	 * CXL r3.2 section 8.2.4.20.1
> +	 * CXL devices shall not advertise more than 10 decoders,
> +	 * CXL switches and HBs may advertise up to 32 decoders.
> +	 */
> +	if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10)
> +		return -EINVAL;
> +	else if (cxlhdm->decoder_count > 32)
> +		return -EINVAL;
> +
> +	/*
> +	 * CXL r3.2 section 8.2.4.20.1
> +	 * target count is applicable only to CXL upstream port and HB.
> +	 * The number of target ports each decoder supports should be
> +	 * one of the numbers 1, 2, 4 or 8.
> +	 */
> +	if (!is_cxl_endpoint(cxlhdm->port) &&
> +	    cxlhdm->target_count != 1 &&
> +	    cxlhdm->target_count != 2 &&
> +	    cxlhdm->target_count != 4 &&
> +	    cxlhdm->target_count != 8)
> +		return -EINVAL;

Maybe instead of manual bitwise checks try
	(!is_power_of_2(cxlhdm->target_count) || cxlhdm->target_count > 8))


> +
> +	return 0;
> +}
> +
>  /**
>   * devm_cxl_setup_hdm - map HDM decoder component registers
>   * @port: cxl_port to map
> @@ -182,7 +210,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>  	}
>  
>  	parse_hdm_decoder_caps(cxlhdm);
> -	if (cxlhdm->decoder_count == 0) {
> +	rc = cxlhdm_decoder_caps_verify(cxlhdm);
> +	if (rc) {
>  		dev_err(dev, "Spec violation. Caps invalid\n");

Can you move the dev_err to the verify function and include the
specific invalid capability.


--Alison

>  		return ERR_PTR(-ENXIO);
>  	}
> -- 
> 2.34.1
> 

Re: [PATCH v1 1/1] cxl/hdm: Verify HDM decoder capabilities after parsing

[Li Ming]

On 2/28/2025 5:47 AM, Alison Schofield wrote:
> On Thu, Feb 27, 2025 at 06:32:51PM +0800, Li Ming wrote:
>> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM
>> decoder capability, But according to the implementation of
>> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0.
> How does a check against the spec maximums benefit this driver? Is there
> a bad path we avoid by checking and quitting at this point.


My understanding is that no a bad path on driver side if the decoder_count is greater than the maximum number spec defines.

Driver just allocates cxl decoders on the port based on the value of decoder_count. But I am not sure if hardware will have other potential problems when it didn't follow the spec.


>
> Might this catch silly decoder counts that the driver previously
> ignored?
>
>> Per CXL specification, the values ranges of decoder_count and
>> target_count are limited. Adding a checking for the values of them
>> in case hardware initialized them with wrong values.
> Similar question - is this catching something sooner, rather than
> later?


Yes, the check is at the beginning of HDM setup during port probing, if value is wrong, will break HDM setup.

I'm not sure if I fully understand your question, please correct me if I misunderstand it. thanks.


>
>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>> ---
>> base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next
>> ---
>>  drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++-
>>  1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 70cae4ebf8a4..a98191867c22 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>>  	return true;
>>  }
>>  
>> +static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm)
>> +{
>> +	/*
>> +	 * CXL r3.2 section 8.2.4.20.1
>> +	 * CXL devices shall not advertise more than 10 decoders,
>> +	 * CXL switches and HBs may advertise up to 32 decoders.
>> +	 */
>> +	if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10)
>> +		return -EINVAL;
>> +	else if (cxlhdm->decoder_count > 32)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * CXL r3.2 section 8.2.4.20.1
>> +	 * target count is applicable only to CXL upstream port and HB.
>> +	 * The number of target ports each decoder supports should be
>> +	 * one of the numbers 1, 2, 4 or 8.
>> +	 */
>> +	if (!is_cxl_endpoint(cxlhdm->port) &&
>> +	    cxlhdm->target_count != 1 &&
>> +	    cxlhdm->target_count != 2 &&
>> +	    cxlhdm->target_count != 4 &&
>> +	    cxlhdm->target_count != 8)
>> +		return -EINVAL;
> Maybe instead of manual bitwise checks try
> 	(!is_power_of_2(cxlhdm->target_count) || cxlhdm->target_count > 8))


Yes, It is clearer, thanks for that.


>
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * devm_cxl_setup_hdm - map HDM decoder component registers
>>   * @port: cxl_port to map
>> @@ -182,7 +210,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>>  	}
>>  
>>  	parse_hdm_decoder_caps(cxlhdm);
>> -	if (cxlhdm->decoder_count == 0) {
>> +	rc = cxlhdm_decoder_caps_verify(cxlhdm);
>> +	if (rc) {
>>  		dev_err(dev, "Spec violation. Caps invalid\n");
> Can you move the dev_err to the verify function and include the
> specific invalid capability.
>
>
> --Alison

Sure, will do that, thanks.


Ming

Re: [PATCH v1 1/1] cxl/hdm: Verify HDM decoder capabilities after parsing

[Alison Schofield]

On Fri, Feb 28, 2025 at 10:47:12AM +0800, Li Ming wrote:
> On 2/28/2025 5:47 AM, Alison Schofield wrote:
> > On Thu, Feb 27, 2025 at 06:32:51PM +0800, Li Ming wrote:
> >> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM
> >> decoder capability, But according to the implementation of
> >> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0.
> > How does a check against the spec maximums benefit this driver? Is there
> > a bad path we avoid by checking and quitting at this point.
> 
> 
> My understanding is that no a bad path on driver side if the decoder_count is greater than the maximum number spec defines.
> 
> Driver just allocates cxl decoders on the port based on the value of decoder_count. But I am not sure if hardware will have other potential problems when it didn't follow the spec.

I had the general thought that the driver is not responsible for
compliance checking the device, unless it affects function. Excessive
decoder_count's sound like they cause needless allocations, so let's
stop doing that - as best we can. 

Is it sufficient to clamp at the spec defined max values and continue
with only a dev_warn_once or even a dev_info?
ie. for a device: decoder_count = min(decoder_count, EP_DECODER_MAX);

That'll avoid failing a device that previously snuck by with an
excessive decoder_count and protect against excessive allocations
in the driver.

> 
> 
> >
> > Might this catch silly decoder counts that the driver previously
> > ignored?
> >
> >> Per CXL specification, the values ranges of decoder_count and
> >> target_count are limited. Adding a checking for the values of them
> >> in case hardware initialized them with wrong values.
> > Similar question - is this catching something sooner, rather than
> > later?
> 
> 
> Yes, the check is at the beginning of HDM setup during port probing, if value is wrong, will break HDM setup.
> 
> I'm not sure if I fully understand your question, please correct me if I misunderstand it. thanks.

I understand now. This one is different that decoder_count because 
it was heading to failure anyway and seems good to fail sooner.

> 
> 
> >
> >> Signed-off-by: Li Ming <ming.li@zohomail.com>
> >> ---
> >> base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next
> >> ---
> >>  drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++-
> >>  1 file changed, 30 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> >> index 70cae4ebf8a4..a98191867c22 100644
> >> --- a/drivers/cxl/core/hdm.c
> >> +++ b/drivers/cxl/core/hdm.c
> >> @@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> >>  	return true;
> >>  }
> >>  
> >> +static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm)
> >> +{
> >> +	/*
> >> +	 * CXL r3.2 section 8.2.4.20.1
> >> +	 * CXL devices shall not advertise more than 10 decoders,
> >> +	 * CXL switches and HBs may advertise up to 32 decoders.
> >> +	 */
> >> +	if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10)
> >> +		return -EINVAL;
> >> +	else if (cxlhdm->decoder_count > 32)
> >> +		return -EINVAL;
> >> +
> >> +	/*
> >> +	 * CXL r3.2 section 8.2.4.20.1
> >> +	 * target count is applicable only to CXL upstream port and HB.
> >> +	 * The number of target ports each decoder supports should be
> >> +	 * one of the numbers 1, 2, 4 or 8.
> >> +	 */
> >> +	if (!is_cxl_endpoint(cxlhdm->port) &&
> >> +	    cxlhdm->target_count != 1 &&
> >> +	    cxlhdm->target_count != 2 &&
> >> +	    cxlhdm->target_count != 4 &&
> >> +	    cxlhdm->target_count != 8)
> >> +		return -EINVAL;
> > Maybe instead of manual bitwise checks try
> > 	(!is_power_of_2(cxlhdm->target_count) || cxlhdm->target_count > 8))
> 
> 
> Yes, It is clearer, thanks for that.
> 
> 
> >
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  /**
> >>   * devm_cxl_setup_hdm - map HDM decoder component registers
> >>   * @port: cxl_port to map
> >> @@ -182,7 +210,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
> >>  	}
> >>  
> >>  	parse_hdm_decoder_caps(cxlhdm);
> >> -	if (cxlhdm->decoder_count == 0) {
> >> +	rc = cxlhdm_decoder_caps_verify(cxlhdm);
> >> +	if (rc) {
> >>  		dev_err(dev, "Spec violation. Caps invalid\n");
> > Can you move the dev_err to the verify function and include the
> > specific invalid capability.
> >
> >
> > --Alison
> 
> Sure, will do that, thanks.
> 
> 
> Ming
> 
> 

Re: [PATCH v1 1/1] cxl/hdm: Verify HDM decoder capabilities after parsing

[Dan Williams]

Alison Schofield wrote:
> On Fri, Feb 28, 2025 at 10:47:12AM +0800, Li Ming wrote:
> > On 2/28/2025 5:47 AM, Alison Schofield wrote:
> > > On Thu, Feb 27, 2025 at 06:32:51PM +0800, Li Ming wrote:
> > >> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM
> > >> decoder capability, But according to the implementation of
> > >> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0.
> > > How does a check against the spec maximums benefit this driver? Is there
> > > a bad path we avoid by checking and quitting at this point.
> > 
> > 
> > My understanding is that no a bad path on driver side if the decoder_count is greater than the maximum number spec defines.
> > 
> > Driver just allocates cxl decoders on the port based on the value of decoder_count. But I am not sure if hardware will have other potential problems when it didn't follow the spec.
> 
> I had the general thought that the driver is not responsible for
> compliance checking the device, unless it affects function. Excessive
> decoder_count's sound like they cause needless allocations, so let's
> stop doing that - as best we can. 

Only if we see a device in the wild that causes an actual problem.
Otherwise this is a losing theoretical game of adding checks for things
that will likely never be violated. The way to address devices that
violate spec expectations *and* cause end user visible pain is to add
quirks. The allocation of a few extra decoders is does not amount to
that standard.

Lets not add checks for benign issues "just because", or "just in case".
If the check is cheap and we need to do it for the driver's own internal
sanity, fine, but if it's just being strict for strictness sake, please
no.

Re: [PATCH v1 1/1] cxl/hdm: Verify HDM decoder capabilities after parsing

[Li Ming]

On 3/1/2025 7:45 AM, Dan Williams wrote:
> Alison Schofield wrote:
>> On Fri, Feb 28, 2025 at 10:47:12AM +0800, Li Ming wrote:
>>> On 2/28/2025 5:47 AM, Alison Schofield wrote:
>>>> On Thu, Feb 27, 2025 at 06:32:51PM +0800, Li Ming wrote:
>>>>> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM
>>>>> decoder capability, But according to the implementation of
>>>>> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0.
>>>> How does a check against the spec maximums benefit this driver? Is there
>>>> a bad path we avoid by checking and quitting at this point.
>>>
>>> My understanding is that no a bad path on driver side if the decoder_count is greater than the maximum number spec defines.
>>>
>>> Driver just allocates cxl decoders on the port based on the value of decoder_count. But I am not sure if hardware will have other potential problems when it didn't follow the spec.
>> I had the general thought that the driver is not responsible for
>> compliance checking the device, unless it affects function. Excessive
>> decoder_count's sound like they cause needless allocations, so let's
>> stop doing that - as best we can. 
> Only if we see a device in the wild that causes an actual problem.
> Otherwise this is a losing theoretical game of adding checks for things
> that will likely never be violated. The way to address devices that
> violate spec expectations *and* cause end user visible pain is to add
> quirks. The allocation of a few extra decoders is does not amount to
> that standard.
>
> Lets not add checks for benign issues "just because", or "just in case".
> If the check is cheap and we need to do it for the driver's own internal
> sanity, fine, but if it's just being strict for strictness sake, please
> no.

Got it, thanks for explanation.


Ming