Re: [PATCH v4 4/4] RAS/AMD/ATL: Implement DF 4.5 NP2 denormalization

[John Allen]

On Tue, May 28, 2024 at 12:23:44PM +0200, Borislav Petkov wrote:
> On Mon, May 06, 2024 at 03:46:05PM +0000, John Allen wrote:
> > +	switch (ctx->map.intlv_mode) {
> > +	case DF4p5_NPS0_24CHAN_1K_HASH:
> > +		cs_id = FIELD_GET(GENMASK_ULL(63, 13), denorm_ctx->current_spa) << 3;
> > +		cs_id %= denorm_ctx->mod_value;
> > +		cs_id <<= 2;
> > +		cs_id |= (hash_pa9 | (hash_pa12 << 1));
> > +		cs_id |= hash_pa8 << df_cfg.socket_id_shift;
> > +		break;
> 
> Newlines after those "break"s pls.
> 
> > +	case DF4p5_NPS0_24CHAN_2K_HASH:
> > +		cs_id = FIELD_GET(GENMASK_ULL(63, 14), denorm_ctx->current_spa) << 4;
> > +		cs_id %= denorm_ctx->mod_value;
> > +		cs_id <<= 2;
> > +		cs_id |= (hash_pa12 | (hash_pa13 << 1));
> > +		cs_id |= hash_pa8 << df_cfg.socket_id_shift;
> > +		break;
> 
> ...
> 
> > +static u64 normalize_addr_df4p5_np2(struct addr_ctx *ctx, struct df4p5_denorm_ctx *denorm_ctx,
> > +				    u64 addr)
> > +{
> > +	u64 temp_addr_a = 0, temp_addr_b = 0;
> > +
> > +	switch (ctx->map.intlv_mode) {
> > +	case DF4p5_NPS0_24CHAN_1K_HASH:
> > +	case DF4p5_NPS1_12CHAN_1K_HASH:
> > +	case DF4p5_NPS2_6CHAN_1K_HASH:
> > +	case DF4p5_NPS4_3CHAN_1K_HASH:
> > +	case DF4p5_NPS1_10CHAN_1K_HASH:
> > +	case DF4p5_NPS2_5CHAN_1K_HASH:
> > +		temp_addr_a = FIELD_GET(GENMASK_ULL(11, 10), addr) << 8;
> > +		break;
> 
> Also linebreaks pls.
> 
> Please change this in all your patches.
> 
> > diff --git a/drivers/ras/amd/atl/internal.h b/drivers/ras/amd/atl/internal.h
> > index 05b870fcb24e..946e36c053c5 100644
> > --- a/drivers/ras/amd/atl/internal.h
> > +++ b/drivers/ras/amd/atl/internal.h
> > @@ -34,6 +34,8 @@
> >  #define DF_DRAM_BASE_LIMIT_LSB		28
> >  #define MI300_DRAM_LIMIT_LSB		20
> >  
> > +#define INVALID_SPA ~0ULL
> 
> No lazy definitions pls:
> 
> drivers/ras/amd/atl/internal.h:37:#define INVALID_SPA ~0ULL
> drivers/ras/amd/fmpm.c:119:#define INVALID_SPA  ~0ULL
> 
> Unify them.

Is there a particular existing header that you suggest we pull this into
or just create a new one under drivers/ras/amd?

Thanks,
John

> 
> > +
> >  enum df_revisions {
> >  	UNKNOWN,
> >  	DF2,
> > @@ -90,6 +92,44 @@ enum intlv_modes {
> >  	DF4p5_NPS1_10CHAN_2K_HASH	= 0x49,
> >  };
> >  
> > +struct df4p5_denorm_ctx {
> > +	/* perm_shift: Indicates the number of "lost" bits. This will be 1, 2, or 3. */
> 
> No need to repeat the variable name in the comment.
> 
> ...
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette