From: Borislav Petkov <bp@alien8.de>
To: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
tony.luck@intel.com, x86@kernel.org, avadhut.naik@amd.com,
john.allen@amd.com, william.roche@oracle.com,
muralidhara.mk@amd.com
Subject: Re: [PATCH v3 1/3] RAS: Introduce AMD Address Translation Library
Date: Thu, 14 Dec 2023 15:30:05 +0100 [thread overview]
Message-ID: <20231214143005.GNZXsRbcALa1/TW2OK@fat_crate.local> (raw)
In-Reply-To: <20231210194932.43992-2-yazen.ghannam@amd.com>
On Sun, Dec 10, 2023 at 01:49:30PM -0600, Yazen Ghannam wrote:
> +/*
> + * Some, but not all, cases have asserts.
> + * So use return values to indicate failure where needed.
> + */
No need for that comment.
> +static int get_intlv_mode(struct addr_ctx *ctx)
> +{
> + switch (df_cfg.rev) {
> + case DF2: return df2_get_intlv_mode(ctx);
> + case DF3: return df3_get_intlv_mode(ctx);
> + case DF3p5: return df3p5_get_intlv_mode(ctx);
> + case DF4: return df4_get_intlv_mode(ctx);
> + case DF4p5: return df4p5_get_intlv_mode(ctx);
> + default:
> + warn_on_bad_df_rev();
> + return -EINVAL;
> + }
You can warn once here instead of the callers:
int ret;
switch () {
... ret = ...get_intlv_mode();
...
default:
ret = -EINVAL;
}
if (ret)
warn_on_bad_df_rev();
return ret;
and save some text lines.
> +}
> +
> +static u64 get_hi_addr_offset(u32 reg_dram_offset)
> +{
> + u8 shift = DF_DRAM_BASE_LIMIT_LSB;
> + u64 hi_addr_offset = 0;
Move that assignment to 0...
> +
> + switch (df_cfg.rev) {
> + case DF2:
> + hi_addr_offset = FIELD_GET(DF2_HI_ADDR_OFFSET, reg_dram_offset);
> + break;
> + case DF3:
> + case DF3p5:
> + hi_addr_offset = FIELD_GET(DF3_HI_ADDR_OFFSET, reg_dram_offset);
> + break;
> + case DF4:
> + case DF4p5:
> + hi_addr_offset = FIELD_GET(DF4_HI_ADDR_OFFSET, reg_dram_offset);
> + break;
> + default:
... here.
<---
> + warn_on_bad_df_rev();
> + }
> +
> + return hi_addr_offset << shift;
> +}
> +
> +static int get_dram_offset(struct addr_ctx *ctx, bool *enabled, u64 *norm_offset)
> +{
You don't need *enabled. The retval can be:
< 0: fail
0: disabled
>0: enabled
and then you get rid of the IO param.
> + u32 reg_dram_offset;
> + u8 map_num;
> +
> + /* Should not be called for map 0. */
> + if (!ctx->map.num) {
> + warn_on_assert("Trying to find DRAM offset for map 0");
> + return -EINVAL;
> + }
> +
> + /*
> + * DramOffset registers don't exist for map 0, so the base register
> + * actually refers to map 1.
> + * Adjust the map_num for the register offsets.
> + */
> + map_num = ctx->map.num - 1;
> +
> + if (df_cfg.rev >= DF4) {
> + /* Read D18F7x140 (DramOffset) */
> + if (df_indirect_read_instance(ctx->node_id, 7, 0x140 + (4 * map_num),
> + ctx->inst_id, ®_dram_offset))
> + return -EINVAL;
> +
> + } else {
> + /* Read D18F0x1B4 (DramOffset) */
> + if (df_indirect_read_instance(ctx->node_id, 0, 0x1B4 + (4 * map_num),
> + ctx->inst_id, ®_dram_offset))
> + return -EINVAL;
> + }
> +
> + if (!FIELD_GET(DF_HI_ADDR_OFFSET_EN, reg_dram_offset))
> + return 0;
> +
> + *enabled = true;
> + *norm_offset = get_hi_addr_offset(reg_dram_offset);
> +
> + return 0;
> +}
...
> +static int get_cs_fabric_id(struct addr_ctx *ctx)
> +{
> + return lookup_cs_fabric_id(ctx);
> +}
Get rid of that silly helper.
> +
> +static bool valid_map(struct addr_ctx *ctx)
> +{
> + if (df_cfg.rev >= DF4)
> + return FIELD_GET(DF_ADDR_RANGE_VAL, ctx->map.ctl);
> +
> + return FIELD_GET(DF_ADDR_RANGE_VAL, ctx->map.base);
if (... )
return
else
return
Balanced.
> +int get_address_map(struct addr_ctx *ctx)
> +{
> + int ret = 0;
> +
> + ret = get_address_map_common(ctx);
> + if (ret)
> + return ret;
> +
> + if (get_global_map_data(ctx))
> + return -EINVAL;
Use ret here too.
> +
> + dump_address_map(&ctx->map);
> +
> + return ret;
> +}
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
next prev parent reply other threads:[~2023-12-14 14:30 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-10 19:49 [PATCH v3 0/3] AMD Address Translation Library Yazen Ghannam
2023-12-10 19:49 ` [PATCH v3 1/3] RAS: Introduce " Yazen Ghannam
2023-12-11 14:20 ` Borislav Petkov
2023-12-11 15:28 ` Yazen Ghannam
2023-12-11 19:57 ` Borislav Petkov
2023-12-12 14:23 ` Yazen Ghannam
2023-12-12 15:34 ` Borislav Petkov
2023-12-13 15:35 ` Yazen Ghannam
2023-12-13 16:48 ` Borislav Petkov
2023-12-13 17:04 ` Yazen Ghannam
2023-12-13 17:07 ` Borislav Petkov
2023-12-12 13:29 ` Borislav Petkov
2023-12-12 14:33 ` Yazen Ghannam
2023-12-12 16:07 ` Borislav Petkov
2023-12-14 10:54 ` Borislav Petkov
2023-12-14 14:30 ` Borislav Petkov [this message]
2023-12-10 19:49 ` [PATCH v3 2/3] EDAC/amd64: Use new " Yazen Ghannam
2023-12-10 19:49 ` [PATCH v3 3/3] Documentation: RAS: Add index and address translation section Yazen Ghannam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231214143005.GNZXsRbcALa1/TW2OK@fat_crate.local \
--to=bp@alien8.de \
--cc=avadhut.naik@amd.com \
--cc=john.allen@amd.com \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=muralidhara.mk@amd.com \
--cc=tony.luck@intel.com \
--cc=william.roche@oracle.com \
--cc=x86@kernel.org \
--cc=yazen.ghannam@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).