Re: [RFC PATCH 1/8] cxl: break out range register decoding from cxl_hdm_decode_init()

[Dave Jiang <dave.jiang@intel.com>]

On 12/19/22 8:59 AM, Jonathan Cameron wrote:
> On Wed, 30 Nov 2022 16:12:20 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> There are 2 scenarios that requires additional handling. 1. A device that
>> has active ranges in DVSEC range registers (RR) but no HDM decoder register
>> block. 2. A device that has both RR active and HDM, but the HDM decoders are
>> not programmed. The goal is to create emulated decoder software structs
>> based on the RR.
>>
>> Move the CXL DVSEC range register decoding code block from
>> cxl_hdm_decode_init() to its own function. Refactor code in preparation for
>> the HDM decoder emulation.  There is no functionality change to the code.
>> Name the new function to cxl_dvsec_rr_decode().
>>
>> The only change is to set range->start to CXL_RESOURCE_NONE if the range is
>> not programmed correctly or active.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> Hi Dave,
> 
> I think this refactor, whilst fairly minimal reveals some places
> where with a slightly more invasive set of changes we can improve the resulting
> code.
> 
> Jonathan
> 
>> ---
> 
> 
>> -int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
>> +
>> +static int cxl_dvsec_rr_decode(struct pci_dev *pdev, int d,
>> +			       struct cxl_endpoint_dvsec_info *info)
>>   {
> ...
> 
>>   	for (i = 0; i < hdm_count; i++) {
>>   		u64 base, size;
>> @@ -426,22 +417,44 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
>>   
>>   		base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
>>   
>> -		info.dvsec_range[i] = (struct range) {
>> +		info->dvsec_range[i] = (struct range) {
>>   			.start = base,
>>   			.end = base + size - 1
>>   		};
>>   
>>   		if (size)
>>   			ranges++;
>> +		else
>> +			info->dvsec_range[i].start = CXL_RESOURCE_NONE;
> 
> The following might become irrelevant after later patches in series...
> 
> Seems a little odd to do it this way for the !size case and set it
> directly above for the case where size is non zero.  Also, is there any
> purpose to setting end?
> Perhaps just sent whole thing down here and set end to the magic flag?
> 
> 		if (size) {
> 			info->dvsec_range[i] = (struct range) {
> 				.start = base,
> 				.end = base + size - 1,
> 			};
> 			ranges++;
> 		} else {
> 			info->dvsec_range[i] = (struct range) {
> 				.start = CXL_RESOURCE_NONE,
> 				.end = CXL_RESOURCE_NONE,
> 			};
> 		}
> 
> or for a more major refactor short cut the !size case and don't bother
> reading the pci registers values that are going to be thrown away
> anyway.
> 
> 		if (!size) {
> 			info->dvsec_range[i] = (struct range) {
> 				.start = CXL_RESOURCE_NONE,
> 				.end = CXL_RESOURCE_NONE,
> 			};
> 			continue;
> 		}
> 
> 		rc = pci_read_config_dword(
> 			pdev, d + CXL_DVSEC_RANGE_BASE_HIGH(i), &temp);
> 		if (rc)
> 			return rc;
> 
> 		...
> 
> 		info->dvsec_range[i] = (struct range) {
> 			.start = base,
> 			.end = base + size - 1,
> 		};
> 		ranges++;
> 	}

Ok I like this optimization. It makes sense not reading the other 
registers if the range is not enabled.

> 
>>   	}
>>   
>> -	info.ranges = ranges;
>> +	info->ranges = ranges;
> 
> Trivial but I would like a blank line here.

ok

> 
>> +	return 0;
>> +}
>> +
>> +/**
>> + * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
>> + * @cxlds: Device state
>> + * @cxlhdm: Mapped HDM decoder Capability
>> + *
>> + * Try to enable the endpoint's HDM Decoder Capability
>> + */
>> +int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>> +	struct cxl_endpoint_dvsec_info info = { 0 };
>> +	int rc;
> 
> Trivial but probably want to reorder these to maintain the reverse xmas tree.

ok

> 
>> +	struct device *dev = &pdev->dev;
>> +	int d = cxlds->cxl_dvsec;
>> +
>> +	rc = cxl_dvsec_rr_decode(pdev, d, &info);
>> +	if (rc < 0)
>> +		return rc;
>>   
>>   	/*
>>   	 * If DVSEC ranges are being used instead of HDM decoder registers there
>>   	 * is no use in trying to manage those.
>>   	 */
>> -hdm_init:
>>   	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, &info)) {
>>   		dev_err(dev,
>>   			"Legacy range registers configuration prevents HDM operation.\n");
>>
>>
>