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@xxxxxxxxxxxx>
>> ---
>> 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