[PATCH] cxl: check decoder count for end device

[PATCH] cxl: check decoder count for end device

[Dave Jiang]

CXL spec rev3.0 8.2.4.19.1 added definition for up to 32 decoders. It also
indicates that for devices, only 10 decoders should be advertised. Add
check on number of decoders greater than 10 for devices and reset to 10 to
force spec compliance.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/hdm.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index d1d2caea5c62..1919d99d157e 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -71,9 +71,23 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, CXL);
 static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
 {
 	u32 hdm_cap;
+	int decoder_count;
+	struct device *dev = &cxlhdm->port->dev;
 
 	hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET);
-	cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap);
+	decoder_count = cxl_hdm_decoder_count(hdm_cap);
+	/*
+	 * CXL spec rev3.0 8.2.4.19.1 indicates CXL devices shall not advertise
+	 * more than 10 decoders. Switches and Host Bridges may advertise up to
+	 * 32 decoders. Set the decoders to 10 for devices if more than 10 are
+	 * found.
+	 */
+	if (is_cxl_endpoint(cxlhdm->port) && decoder_count > 10) {
+		dev_warn(dev, "Reset decoders count (%d) to 10, spec violation!\n",
+			 decoder_count);
+		decoder_count = 10;
+	}
+	cxlhdm->decoder_count = decoder_count;
 	cxlhdm->target_count =
 		FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap);
 	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap))

Re: [PATCH] cxl: check decoder count for end device

[Jonathan Cameron]

On Thu, 15 Sep 2022 11:11:07 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> CXL spec rev3.0 8.2.4.19.1 added definition for up to 32 decoders. It also
> indicates that for devices, only 10 decoders should be advertised. Add
> check on number of decoders greater than 10 for devices and reset to 10 to
> force spec compliance.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Hi Dave

Seems reasonable.  I'm a bit curious to whether there is an actual problem if
a device is spec non compliant and advertises more decoders?

Either way does no harm and might let us know something fishy is happening.

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

> ---
>  drivers/cxl/core/hdm.c |   16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index d1d2caea5c62..1919d99d157e 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -71,9 +71,23 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, CXL);
>  static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>  {
>  	u32 hdm_cap;
> +	int decoder_count;
> +	struct device *dev = &cxlhdm->port->dev;
>  
>  	hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET);
> -	cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap);
> +	decoder_count = cxl_hdm_decoder_count(hdm_cap);
> +	/*
> +	 * CXL spec rev3.0 8.2.4.19.1 indicates CXL devices shall not advertise
> +	 * more than 10 decoders. Switches and Host Bridges may advertise up to
> +	 * 32 decoders. Set the decoders to 10 for devices if more than 10 are
> +	 * found.
> +	 */
> +	if (is_cxl_endpoint(cxlhdm->port) && decoder_count > 10) {
> +		dev_warn(dev, "Reset decoders count (%d) to 10, spec violation!\n",
> +			 decoder_count);
> +		decoder_count = 10;
> +	}
> +	cxlhdm->decoder_count = decoder_count;
>  	cxlhdm->target_count =
>  		FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap);
>  	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap))
> 
> 

Re: [PATCH] cxl: check decoder count for end device

[Dave Jiang]

On 10/11/2022 9:45 AM, Jonathan Cameron wrote:
> On Thu, 15 Sep 2022 11:11:07 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
>
>> CXL spec rev3.0 8.2.4.19.1 added definition for up to 32 decoders. It also
>> indicates that for devices, only 10 decoders should be advertised. Add
>> check on number of decoders greater than 10 for devices and reset to 10 to
>> force spec compliance.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Hi Dave
>
> Seems reasonable.  I'm a bit curious to whether there is an actual problem if
> a device is spec non compliant and advertises more decoders?
Not that I'm aware of. Just being cautious.
>
> Either way does no harm and might let us know something fishy is happening.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Thanks!
>
>> ---
>>   drivers/cxl/core/hdm.c |   16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index d1d2caea5c62..1919d99d157e 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -71,9 +71,23 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, CXL);
>>   static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>>   {
>>   	u32 hdm_cap;
>> +	int decoder_count;
>> +	struct device *dev = &cxlhdm->port->dev;
>>   
>>   	hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET);
>> -	cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap);
>> +	decoder_count = cxl_hdm_decoder_count(hdm_cap);
>> +	/*
>> +	 * CXL spec rev3.0 8.2.4.19.1 indicates CXL devices shall not advertise
>> +	 * more than 10 decoders. Switches and Host Bridges may advertise up to
>> +	 * 32 decoders. Set the decoders to 10 for devices if more than 10 are
>> +	 * found.
>> +	 */
>> +	if (is_cxl_endpoint(cxlhdm->port) && decoder_count > 10) {
>> +		dev_warn(dev, "Reset decoders count (%d) to 10, spec violation!\n",
>> +			 decoder_count);
>> +		decoder_count = 10;
>> +	}
>> +	cxlhdm->decoder_count = decoder_count;
>>   	cxlhdm->target_count =
>>   		FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap);
>>   	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap))
>>
>>

RE: [PATCH] cxl: check decoder count for end device

[Dan Williams]

Dave Jiang wrote:
> CXL spec rev3.0 8.2.4.19.1 added definition for up to 32 decoders. It also
> indicates that for devices, only 10 decoders should be advertised. Add
> check on number of decoders greater than 10 for devices and reset to 10 to
> force spec compliance.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/hdm.c |   16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index d1d2caea5c62..1919d99d157e 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -71,9 +71,23 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, CXL);
>  static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>  {
>  	u32 hdm_cap;
> +	int decoder_count;
> +	struct device *dev = &cxlhdm->port->dev;
>  
>  	hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET);
> -	cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap);
> +	decoder_count = cxl_hdm_decoder_count(hdm_cap);
> +	/*
> +	 * CXL spec rev3.0 8.2.4.19.1 indicates CXL devices shall not advertise
> +	 * more than 10 decoders. Switches and Host Bridges may advertise up to
> +	 * 32 decoders. Set the decoders to 10 for devices if more than 10 are
> +	 * found.
> +	 */
> +	if (is_cxl_endpoint(cxlhdm->port) && decoder_count > 10) {
> +		dev_warn(dev, "Reset decoders count (%d) to 10, spec violation!\n",
> +			 decoder_count);
> +		decoder_count = 10;

I agree with the reporting, but not the limitation. The Robustness
Principle says if allowing it does no harm, then let it through.

Re: [PATCH] cxl: check decoder count for end device

[Dave Jiang]

On 10/21/2022 2:17 PM, Dan Williams wrote:
> Dave Jiang wrote:
>> CXL spec rev3.0 8.2.4.19.1 added definition for up to 32 decoders. It also
>> indicates that for devices, only 10 decoders should be advertised. Add
>> check on number of decoders greater than 10 for devices and reset to 10 to
>> force spec compliance.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/hdm.c |   16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index d1d2caea5c62..1919d99d157e 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -71,9 +71,23 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, CXL);
>>   static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>>   {
>>   	u32 hdm_cap;
>> +	int decoder_count;
>> +	struct device *dev = &cxlhdm->port->dev;
>>   
>>   	hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET);
>> -	cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap);
>> +	decoder_count = cxl_hdm_decoder_count(hdm_cap);
>> +	/*
>> +	 * CXL spec rev3.0 8.2.4.19.1 indicates CXL devices shall not advertise
>> +	 * more than 10 decoders. Switches and Host Bridges may advertise up to
>> +	 * 32 decoders. Set the decoders to 10 for devices if more than 10 are
>> +	 * found.
>> +	 */
>> +	if (is_cxl_endpoint(cxlhdm->port) && decoder_count > 10) {
>> +		dev_warn(dev, "Reset decoders count (%d) to 10, spec violation!\n",
>> +			 decoder_count);
>> +		decoder_count = 10;
> I agree with the reporting, but not the limitation. The Robustness
> Principle says if allowing it does no harm, then let it through.
Ok I'll remove the reassigment.