Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Thu, 14 Dec 2023 15:30:05 +0100
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
Message-ID: <20231214143005.GNZXsRbcALa1/TW2OK@fat_crate.local>
References: <20231210194932.43992-1-yazen.ghannam@amd.com>
 <20231210194932.43992-2-yazen.ghannam@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20231210194932.43992-2-yazen.ghannam@amd.com>
Precedence: bulk

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, &reg_dram_offset))
> +			return -EINVAL;
> +
> +	} else {
> +		/* Read D18F0x1B4 (DramOffset) */
> +		if (df_indirect_read_instance(ctx->node_id, 0, 0x1B4 + (4 * map_num),
> +					      ctx->inst_id, &reg_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