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

[Yazen Ghannam]

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?

Thanks,
Yazen