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

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

On 2/7/23 2:58 PM, Dan Williams wrote:
> Dan Williams wrote:
>> Dave Jiang 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 and range->end to CXL_RESOURCE_NONE
>>> and skipping the reading of base registers if the range size is 0, which
>>> equates to range not active.
>>>
>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>
>>> ---
>>> v2:
>>> - Refactor to continue when size is 0. (Jonathan)
>>> ---
>>>   drivers/cxl/core/pci.c |   63 ++++++++++++++++++++++++++++++------------------
>>>   1 file changed, 40 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>>> index 57764e9cd19d..a8ecc6ddb3d7 100644
>>> --- a/drivers/cxl/core/pci.c
>>> +++ b/drivers/cxl/core/pci.c
>> [..]
>>> @@ -410,6 +400,13 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
>>>   			return rc;
>>>   
>>>   		size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK;
>>> +		if (!size) {
>>> +			info->dvsec_range[i] = (struct range) {
>>> +				.start = CXL_RESOURCE_NONE,
>>> +				.end = CXL_RESOURCE_NONE,
>>> +			};
>>
>> This should just be .start = 0, and .end = -1, or .start =
>> CXL_RESOURCE_NONE, and .end = CXL_RESOURCE_NONE - 1, right? Otherwise,
>> range_len() on this range will be non-zero. Likely moot because the
>> range will be skipped because @ranges++ gets skipped, but it looks
>> strange written this way.
> 
> Actually, IIUC, this update can just be deleted altogether because
> @ranges++ being skipped already invalidates this entry.

Ok, I'll remove and fix the follow on code bits that relies on this.