[PATCH] ACPI: APEI: Fix ERST record walk looping on a missing record

[PATCH] ACPI: APEI: Fix ERST record walk looping on a missing record

[Breno Leitao]

On some x86 servers with AMI firmware, mounting pstore logs

  ERST: [Firmware Warn]: too many record IDs!

and /sys/fs/pstore stays empty. The firmware keeps returning, from
GET_RECORD_IDENTIFIER, the ID of a record that READ reports as not
found, while GET_RECORD_COUNT reads 0.

Since commit a090931524d0 ("ACPI: APEI: Fix missing ERST record id"),
erst_read_record() drops such an ID from the record ID cache. The next
__erst_record_id_cache_add_one() call no longer recognizes it and adds
it again, until the cache holds ERST_RECORD_ID_CACHE_SIZE_MAX (1024)
IDs.

Remember the last ID that read back as missing and skip it like a
cached one, so the prev_id check ends the walk. Forget it once the last
walker is done.

On an affected machine, each pstore mount goes from 1024 failed READs
and 297 ms to one failed READ and 13 ms.

Fixes: a090931524d0 ("ACPI: APEI: Fix missing ERST record id")
Cc: stable@vger.kernel.org
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/acpi/apei/erst.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 50c98db57e4a5c..0e8c52326d82fc 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -457,11 +457,13 @@ struct erst_record_id_cache {
 	int len;
 	int size;
 	int refcount;
+	u64 missing_id;
 };
 
 static struct erst_record_id_cache erst_record_id_cache = {
 	.lock = __MUTEX_INITIALIZER(erst_record_id_cache.lock),
 	.refcount = 0,
+	.missing_id = APEI_ERST_INVALID_RECORD_ID,
 };
 
 static int __erst_get_next_record_id(u64 *record_id)
@@ -523,6 +525,10 @@ static int __erst_record_id_cache_add_one(void)
 		first_id = id;
 	prev_id = id;
 
+	/* The firmware may keep handing out a record it reported missing */
+	if (id == erst_record_id_cache.missing_id)
+		goto retry;
+
 	entries = erst_record_id_cache.entries;
 	for (i = 0; i < erst_record_id_cache.len; i++) {
 		if (entries[i] == id)
@@ -639,6 +645,9 @@ void erst_get_record_id_end(void)
 	mutex_lock(&erst_record_id_cache.lock);
 	erst_record_id_cache.refcount--;
 	BUG_ON(erst_record_id_cache.refcount < 0);
+	/* The next walk retries it, a new record may reuse the ID */
+	if (!erst_record_id_cache.refcount)
+		erst_record_id_cache.missing_id = APEI_ERST_INVALID_RECORD_ID;
 	__erst_record_id_cache_compact();
 	mutex_unlock(&erst_record_id_cache.lock);
 }
@@ -900,6 +909,7 @@ static void erst_clear_cache(u64 record_id)
 		if (entries[i] == record_id)
 			entries[i] = APEI_ERST_INVALID_RECORD_ID;
 	}
+	erst_record_id_cache.missing_id = record_id;
 	__erst_record_id_cache_compact();
 
 	mutex_unlock(&erst_record_id_cache.lock);

---
base-commit: 630059a5a632377b9b7b487baccde92b7ae1dd47
change-id: 20261110-erst_missing_id-980c46f95950

Best regards,
--  
Breno Leitao <leitao@debian.org>