[PATCH] EDAC/amd64: Fix size calculation for Non-Power-of-Two DIMMs

[Naik, Avadhut]

On 4/14/2025 08:11, Yazen Ghannam wrote:
> On Wed, Apr 09, 2025 at 12:48:57AM -0500, Naik, Avadhut wrote:
> 
> [...]
> 
>>>>  
>>>>  	edac_dbg(1, "CS%d DIMM%d AddrMasks:\n", csrow_nr, dimm);
>>>> -	edac_dbg(1, "  Original AddrMask: 0x%x\n", addr_mask_orig);
>>>> -	edac_dbg(1, "  Deinterleaved AddrMask: 0x%x\n", addr_mask_deinterleaved);
>>>> +	edac_dbg(1, "  Primary AddrMask: 0x%x\n", addr_mask);
>>>>  
>>>>  	/* Register [31:1] = Address [39:9]. Size is in kBs here. */
>>>> -	size = (addr_mask_deinterleaved >> 2) + 1;
>>>> +	size = calculate_cs_size(addr_mask, cs_mode);
>>>> +
>>>> +	if (addr_mask_sec) {
>>>
>>> I think we can skip this check.
>>>
>> Commented below on this.
>>
>>> For debug messages, it doesn't hurt to be more explicit. So printing a
>>> 'mask: 0x0' message is more helpful/reassuring than 'no message'.
>>>
>>>> +		edac_dbg(1, "  Secondary AddrMask: 0x%x\n", addr_mask);
>>>
>>> addr_mask -> addr_mask_sec
>>>
>>>> +		size += calculate_cs_size(addr_mask_sec, cs_mode);
>>>
>>> Maybe add a "!mask" check to return early if you want to save some
>>> cycles in this helper function.
>>>
>> In a way, this is the reason why I had added the above condition check.
>> To avoid unnecessary function calls.
>>
>> AFAIK, power-of-2 DIMMs are predominantly used, so the Secondary Address
>> Mask register will seldom be used.
>>
>> Would you agree?
> 
> I don't know. It seems non-power-of-2 (24G, 48G, 96G) is becoming more
> common for individual DIMMs and sets of DIMMs.
> 
> Thinking on it more, I don't think we need to be concerned about saving
> cycles here. These functions are only called during module init,
> correct?
> 
Yes, this should only be executed during module init.
Get your point. Will remove the condition check and move it to the
new helper function.

> Thanks,
> Yazen

-- 
Thanks,
Avadhut Naik