Received-SPF: pass (google.com: domain of linux-kernel+bounces-65050-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 14 Feb 2024 11:29:26 +0100
From: Borislav Petkov <bp@alien8.de>
To: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: tony.luck@intel.com, linux-edac@vger.kernel.org,
	linux-kernel@vger.kernel.org, avadhut.naik@amd.com,
	john.allen@amd.com, muralidhara.mk@amd.com,
	naveenkrishna.chatradhi@amd.com, sathyapriya.k@amd.com
Subject: Re: [PATCH 2/2] RAS: Introduce the FRU Memory Poison Manager
Message-ID: <20240214102926.GCZcyWBuEBe7WRXWYO@fat_crate.local>
References: <20240214033516.1344948-1-yazen.ghannam@amd.com>
 <20240214033516.1344948-3-yazen.ghannam@amd.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240214033516.1344948-3-yazen.ghannam@amd.com>

On Tue, Feb 13, 2024 at 09:35:16PM -0600, Yazen Ghannam wrote:
> +/* FRU Memory Poison Section, UEFI vX.Y sec N.X.Z */

Whack those:

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index 328e0a962c23..0246b13b5ba1 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -72,7 +72,7 @@
 /* FRU ID Types */
 #define FMP_ID_TYPE_X86_PPIN		0
 
-/* FRU Memory Poison Section, UEFI vX.Y sec N.X.Z */
+/* FRU Memory Poison Section */
 struct cper_sec_fru_mem_poison {
 	u32 checksum;
 	u64 validation_bits;
@@ -89,7 +89,7 @@ struct cper_sec_fru_mem_poison {
 /* FRU Descriptor Address Types */
 #define FPD_ADDR_TYPE_MCA_ADDR		0
 
-/* Memory Poison Descriptor, UEFI vX.Y sec N.X.Y */
+/* Memory Poison Descriptor */
 struct cper_fru_poison_desc {
 	u64 timestamp;
 	u32 hw_id_type;


> +/**
> + * DOC: fru_poison_entries (byte)
> + * Maximum number of descriptor entries possible for each FRU.
> + *
> + * Values between '1' and '255' are valid.
> + * No input or '0' will default to FMPM_DEFAULT_MAX_NR_ENTRIES.
> + */
> +static u8 max_nr_entries;
> +module_param(max_nr_entries, byte, 0644);
> +MODULE_PARM_DESC(max_nr_entries,
> +		 "Maximum number of memory poison descriptor entries per FRU");

Why is there a module parameter?

So that people can brick their BIOSes if it can't handle some size?

Can we read out the max size of the area destined for FRU records from
somewhere and go with it?

> +#define FMPM_DEFAULT_MAX_NR_ENTRIES	8
> +
> +/* Maximum number of FRUs in the system. */
> +static unsigned int max_nr_fru;
> +
> +/* Total length of record including headers and list of descriptor entries. */
> +static size_t max_rec_len;
> +
> +/*
> + * Protect the local cache and prevent concurrent writes to storage.

"local cache"?

> + * This is only needed after init once notifier block registration is done.
> + */
> +static DEFINE_MUTEX(fmpm_update_mutex);
> +
> +#define for_each_fru(i, rec) \
> +	for (i = 0; rec = fru_records[i], i < max_nr_fru; i++)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette