Messages in this thread | | | Date | Thu, 8 Jul 2021 15:53:34 -0400 | From | Yazen Ghannam <> | Subject | Re: [PATCH v2 05/31] EDAC/amd64: Add context struct |
| |
On Wed, Jun 30, 2021 at 07:17:41PM +0200, Borislav Petkov wrote: > On Wed, Jun 23, 2021 at 07:19:36PM +0000, Yazen Ghannam wrote: > > Define an address translation context struct. This will hold values that > > will be passed between multiple functions. > > > > Save return address, Node ID, and the Instance ID number to start. > > Currently, we use the UMC number as the Instance ID, but future DF > > Please use passive voice in your commit message: no "we" or "I", etc. > > And this here is a perfect example: it sounds here like "we" is "AMD" > but we use "we" mostly for the kernel. And there's the confusion. > > So please teach yourself to formulate those commit messages properly - > the future you will thank you, trust me! > > :-) >
Yeah, sorry I always seem to let it slip. :/
...
> > @@ -1097,6 +1100,16 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr > > > > struct df_reg reg; > > > > + struct addr_ctx ctx; > > Those empty-lines spaced-out local vars look weird. >
Okay, I'll fix that.
...
> > @@ -1236,14 +1249,14 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr > > * bits there are. "intlv_addr_bit" tells us how many "Y" bits > > * there are (where "I" starts). > > */ > > - temp_addr_y = ret_addr & GENMASK_ULL(intlv_addr_bit-1, 0); > > + temp_addr_y = ctx.ret_addr & GENMASK_ULL(intlv_addr_bit - 1, 0); > > temp_addr_i = (cs_id << intlv_addr_bit); > > - temp_addr_x = (ret_addr & GENMASK_ULL(63, intlv_addr_bit)) << num_intlv_bits; > > - ret_addr = temp_addr_x | temp_addr_i | temp_addr_y; > > + temp_addr_x = (ctx.ret_addr & GENMASK_ULL(63, intlv_addr_bit)) << num_intlv_bits; > > + ctx.ret_addr = temp_addr_x | temp_addr_i | temp_addr_y; > > You want to align those vertically on the "=" for better readability. >
Most of this will be removed in later patches, so I didn't think to change any more than is necessary to make sure the patch builds.
I'll check the end result to make sure things are aligned nicely.
> > } > > > > /* Add dram base address */ > > - ret_addr += dram_base_addr; > > + ctx.ret_addr += dram_base_addr; > > > > /* If legacy MMIO hole enabled */ > > if (lgcy_mmio_hole_en) { > > @@ -1251,29 +1264,29 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr > > goto out_err; > > > > dram_hole_base = tmp & GENMASK(31, 24); > > - if (ret_addr >= dram_hole_base) > > - ret_addr += (BIT_ULL(32) - dram_hole_base); > > + if (ctx.ret_addr >= dram_hole_base) > > + ctx.ret_addr += (BIT_ULL(32) - dram_hole_base); > > } > > > > if (hash_enabled) { > > /* Save some parentheses and grab ls-bit at the end. */ > > - hashed_bit = (ret_addr >> 12) ^ > > - (ret_addr >> 18) ^ > > - (ret_addr >> 21) ^ > > - (ret_addr >> 30) ^ > > + hashed_bit = (ctx.ret_addr >> 12) ^ > > + (ctx.ret_addr >> 18) ^ > > + (ctx.ret_addr >> 21) ^ > > + (ctx.ret_addr >> 30) ^ > > cs_id; > > > > hashed_bit &= BIT(0); > > > > - if (hashed_bit != ((ret_addr >> intlv_addr_bit) & BIT(0))) > > - ret_addr ^= BIT(intlv_addr_bit); > > + if (hashed_bit != ((ctx.ret_addr >> intlv_addr_bit) & BIT(0))) > > + ctx.ret_addr ^= BIT(intlv_addr_bit); > > } > > > > /* Is calculated system address is above DRAM limit address? */ > > - if (ret_addr > dram_limit_addr) > > + if (ctx.ret_addr > dram_limit_addr) > > goto out_err; > > > > - *sys_addr = ret_addr; > > + *sys_addr = ctx.ret_addr; > > So adding ctx to exchange data between functions - that was a good idea. > > But what this patch does is pointless because you simply replace those > variables with a local struct. > > I guess the aha moment will come with the later patches when you start > passing it around to functions. > > /me waits to see. >
That's true. I was trying to make each patch a single logical change, so there are a few that have a lot of churn. I can squash some of the patches together if that's okay.
Thanks, Yazen
| |