| Message ID | 20220127204115.384161-11-yazen.ghannam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMD MCA Address Translation Updates | expand |
On Thu, Jan 27, 2022 at 08:41:01PM +0000, Yazen Ghannam wrote: > Move code to find the interleave address bit into a separate helper > function. Same question: what's the point of this change?
On Mon, Feb 14, 2022 at 01:10:49PM +0100, Borislav Petkov wrote: > On Thu, Jan 27, 2022 at 08:41:01PM +0000, Yazen Ghannam wrote: > > Move code to find the interleave address bit into a separate helper > > function. > > Same question: what's the point of this change? > Yes, I'll update this. Similar goal as in other places. When the function seems sufficiently long (subjective I know), break it up into helper functions. I've been trying to decide based on logical steps. Do you have any general recommendations or rule-of-thumb? Thanks, Yazen
On Wed, Mar 09, 2022 at 10:12:00PM +0000, Yazen Ghannam wrote: > Similar goal as in other places. When the function seems sufficiently long > (subjective I know), break it up into helper functions. > > I've been trying to decide based on logical steps. Do you have any general > recommendations or rule-of-thumb? Documentation/process/coding-style.rst, 6) Functions has some good ideas about it. To me, a function should do one thing and one thing only but yes, the decision is subjective.
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index d568ad886d35..53d9c4b1c233 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -1182,25 +1182,33 @@ static int get_dram_addr_map(struct addr_ctx *ctx) return 0; } -static int denormalize_addr(struct addr_ctx *ctx) +static int get_intlv_addr_bit(struct addr_ctx *ctx) { - u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask; - u8 intlv_num_dies, intlv_num_chan, intlv_num_sockets; u8 intlv_addr_sel = (ctx->reg_base_addr >> 8) & 0x7; - u8 num_intlv_bits, cs_mask = 0; /* {0, 1, 2, 3} map to address bits {8, 9, 10, 11} respectively */ if (intlv_addr_sel > 3) { - pr_err("%s: Invalid interleave address select %d.\n", - __func__, intlv_addr_sel); + pr_debug("Invalid interleave address select %d.\n", intlv_addr_sel); return -EINVAL; } + ctx->intlv_addr_bit = intlv_addr_sel + 8; + + return 0; +} + +static int denormalize_addr(struct addr_ctx *ctx) +{ + u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask; + u8 intlv_num_dies, intlv_num_chan, intlv_num_sockets; + u8 num_intlv_bits, cs_mask = 0; + + if (get_intlv_addr_bit(ctx)) + return -EINVAL; + intlv_num_sockets = (ctx->reg_limit_addr >> 8) & 0x1; intlv_num_dies = (ctx->reg_limit_addr >> 10) & 0x3; - ctx->intlv_addr_bit = intlv_addr_sel + 8; - /* Re-use intlv_num_chan by setting it equal to log2(#channels) */ switch (intlv_num_chan) { case 0: intlv_num_chan = 0; break;
Move code to find the interleave address bit into a separate helper function. Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> --- Link: https://lore.kernel.org/r/20211028175728.121452-16-yazen.ghannam@amd.com v3->v4: * No change. v2->v3: * Was patch 16 in v2. v1->v2: * Moved from arch/x86 to EDAC. drivers/edac/amd64_edac.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)