[Skiboot] [PATCH 2/2] libflash: Don't merge ECC-protected ranges

[Samuel Mendoza-Jonas]

Libflash currently merges contiguous ECC-protected ranges, but doesn't
check that the ECC bytes at the end of the first and start of the second
range actually match sanely. More importantly, if blocklevel_read() is
called with a position at the start of a partition that is contained
somewhere within a region that has been merged it will update the
position assuming ECC wasn't being accounted for. This results in the
position being somewhere well after the actual start of the partition
which is incorrect.

For now, remove the code merging ranges. This means more ranges must be
held and checked however it prevents incorrectly reading ECC-correct
regions like below:

[  174.334119453,7] FLASH: CAPP partition has ECC
[  174.437349574,3] ECC: uncorrectable error: ffffffffffffffff ff
[  174.437426306,3] FLASH: failed to read the first 0x1000 from CAPP partition, rc 14
[  174.439919343,3] CAPP: Error loading ucode lid. index=201d1

Signed-off-by: Samuel Mendoza-Jonas <sam at mendozajonas.com>
---
Ideally I'd improve the ecc magic in blocklevel_read() and keep the
merging functionality, but I need to have more of a think about what
that entails. The biggest issue is that just because two regions are
marked ECC and are contiguous, doesn't mean that the ECC matches between
them unless every range satisfies total_size % 9 == 0 (and they don't).
Even with that worked around we'd still need to be able to treat some
addresses as "no really, this position, but please still strip ECC".
However if I've missed something obvious please let me know :)

 libflash/blocklevel.c           | 26 +++-----------------------
 libflash/test/test-blocklevel.c | 18 ------------------
 2 files changed, 3 insertions(+), 41 deletions(-)

diff --git a/libflash/blocklevel.c b/libflash/blocklevel.c
index bbcc8e61..5b57d3c6 100644
--- a/libflash/blocklevel.c
+++ b/libflash/blocklevel.c
@@ -53,9 +53,9 @@ static int ecc_protected(struct blocklevel_device *bl, uint64_t pos, uint64_t le
 		}
 
 		/*
-		 * Since we merge regions on inserting we can be sure that a
-		 * partial fit means that the non fitting region won't fit in another ecc
-		 * region
+		 * Even if ranges are merged we can't currently guarantee two
+		 * contiguous regions are sanely ECC protected so a partial fit
+		 * is no good.
 		 */
 		if ((bl->ecc_prot.prot[i].start >= pos && bl->ecc_prot.prot[i].start < pos + len) ||
 		   (bl->ecc_prot.prot[i].start <= pos &&
@@ -696,26 +696,6 @@ static bool insert_bl_prot_range(struct blocklevel_range *ranges, struct bl_prot
 		prot = new_ranges;
 	}
 
-	/* Probably only worth mergeing when we're low on space */
-	if (ranges->n_prot + 1 == ranges->total_prot) {
-		FL_DBG("%s: merging ranges\n", __func__);
-		/* Check to see if we can merge ranges */
-		for (i = 0; i < ranges->n_prot - 1; i++) {
-			if (prot[i].start + prot[i].len == prot[i + 1].start) {
-				int j;
-				FL_DBG("%s: merging 0x%" PRIx64 "..0x%" PRIx64 " with "
-						"0x%" PRIx64 "..0x%" PRIx64 "\n",
-						__func__, prot[i].start, prot[i].start + prot[i].len,
-						prot[i + 1].start, prot[i + 1].start + prot[i + 1].len);
-				prot[i].len += prot[i + 1].len;
-				for (j = i + 1; j < ranges->n_prot - 1; j++)
-					memcpy(&prot[j] , &prot[j + 1], sizeof(range));
-				ranges->n_prot--;
-				i--; /* Maybe the next one can merge too */
-			}
-		}
-	}
-
 	return true;
 }
 
diff --git a/libflash/test/test-blocklevel.c b/libflash/test/test-blocklevel.c
index 42ad146a..a67f5888 100644
--- a/libflash/test/test-blocklevel.c
+++ b/libflash/test/test-blocklevel.c
@@ -289,24 +289,6 @@ int main(void)
 		ERR("Failed to blocklevel_ecc_protect(0x6200, 0x100)\n");
 		return 1;
 	}
-	/*This addition should cause this one to merge the other two together*/
-	if (blocklevel_ecc_protect(bl, 0x6100, 0x100)) {
-		ERR("Failed to blocklevel_ecc_protect(0x6100, 0x100)\n");
-		return 1;
-	}
-	/* Make sure we trigger the merging code */
-	for (i = bl->ecc_prot.n_prot; i < bl->ecc_prot.total_prot; i++)
-		blocklevel_ecc_protect(bl, 0x10000 + i * 0x200, 0x10);
-	/* Check that the region merging works */
-	for (i = 0; i < bl->ecc_prot.n_prot - 1; i++) {
-		if (bl->ecc_prot.prot[i].start + bl->ecc_prot.prot[i].len == bl->ecc_prot.prot[i + 1].start ||
-			  bl->ecc_prot.prot[i + 1].start + bl->ecc_prot.prot[i + 1].len == bl->ecc_prot.prot[i].start) {
-			ERR("Problem with protection range merge code, region starting at 0x%08lx for 0x%08lx appears "
-				"to touch region 0x%lx for 0x%lx\n", bl->ecc_prot.prot[i].start, bl->ecc_prot.prot[i].len,
-				bl->ecc_prot.prot[i + 1].start, bl->ecc_prot.prot[i + 1].len);
-			return 1;
-		}
-	}
 
 	/* Test ECC reading and writing being 100% transparent to the
 	 * caller */
-- 
2.19.1