Re: [PATCH v2 03/31] EDAC/amd64: Don't use naked values for DF registers

[Borislav Petkov]

On Wed, Jun 23, 2021 at 07:19:34PM +0000, Yazen Ghannam wrote:
> +static struct df_reg df_regs[] = {
> +	/* D18F0x50 (FabricBlockInstanceInformation3_CS) */
> +	[FAB_BLK_INST_INFO_3]	=	{0, 0x50},
> +	/* D18F0x104 (DramHoleControl) */
> +	[DRAM_HOLE_CTL]		=	{0, 0x104},
> +	/* D18F0x110 (DramBaseAddress) */
> +	[DRAM_BASE_ADDR]	=	{0, 0x110},
> +	/* D18F0x114 (DramLimitAddress) */
> +	[DRAM_LIMIT_ADDR]	=	{0, 0x114},
> +	/* D18F0x1B4 (DramOffset) */
> +	[DRAM_OFFSET]		=	{0, 0x1B4},
> +	/* D18F1x208 (SystemFabricIdMask) */
> +	[SYS_FAB_ID_MASK]	=	{1, 0x208},
> +};
> +
>  static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
>  {
>  	u64 dram_base_addr, dram_limit_addr, dram_hole_base;
> @@ -1059,8 +1091,9 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
>  	u8 cs_mask, cs_id = 0;
>  	bool hash_enabled = false;
>  
> -	/* Read D18F0x1B4 (DramOffset), check if base 1 is used. */
> -	if (amd_df_indirect_read(nid, 0, 0x1B4, umc, &tmp))
> +	struct df_reg reg;
> +
> +	if (amd_df_indirect_read(nid, df_regs[DRAM_OFFSET], umc, &tmp))
>  		goto out_err;
>  
>  	/* Remove HiAddrOffset from normalized address, if enabled: */
> @@ -1073,8 +1106,9 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
>  		}
>  	}
>  
> -	/* Read D18F0x110 (DramBaseAddress). */
> -	if (amd_df_indirect_read(nid, 0, 0x110 + (8 * base), umc, &tmp))
> +	reg = df_regs[DRAM_BASE_ADDR];
> +	reg.offset += base * 8;

So this looks weird: you have a df_regs[] array of all those different
DF registers which I'd assume is a read-only thing because, well, those
func and offset things are immutable, i.e., hw registers offsets etc.

But then here you go and and modify the offset.

And that df_regs array is globally visible in the driver and if some
later functionality decides to use it, it'll see the modified offset.

IOW, I'd make that array read only (const) and use local vars instead to
pass down to amd_df_indirect_read().

And I'm also questioning what the point is for that df_reg thing?

You have them defined but then you have to change them.

I.e., you can just as well pass in func and offset separately and be
done with it.

But maybe there's something else happening in the patches which comes
later and which will make me go, ahaa.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette