[Skiboot] [PATCH v2] libflash/blocklevel: Allow double ecc protecting a region

Cyril Bur cyril.bur at au1.ibm.com
Tue Jul 26 11:45:56 AEST 2016


On Fri, 15 Jul 2016 15:34:04 +1000
Cyril Bur <cyril.bur at au1.ibm.com> wrote:

Please disregard, I'll resend it as part of a series to get all of the
blocklevel for skiboot working.

> Currently the policy for calling ECC protecting a range at the
> blocklevel layer is that the requested region be completely
> unprotected otherwise the call will return an error. It turns out that
> duplicate calls to ffs_init() with true as the last parameter (for the
> same blocklevel structure) will cause duplicate attempts to
> ecc_protect() ranges.
> 
> Change the policy within blocklevel to allow duplicate protecting.
> In fact the new policy almost guarantees no failure (baring something
> odd like malloc() failing). It will detect that the range is currently
> already fully protected and do nothing, detect that part of the range
> is or is not and extend the existing range or detect that a range fits
> perfectly between two ranges in which case it will merge the ranges.
> 
> Also adjust tests to match the new policy.
> 
> Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> ---
> V2: Significant rewrite, v1 was full of holes. This one should be
> better
>     Fixup the tests
> 
> 
>  libflash/blocklevel.c           | 114 ++++++++++++++++++++++++++--------------
>  libflash/libffs.c               |   2 +-
>  libflash/test/test-blocklevel.c |  54 ++++++++++++++-----
>  3 files changed, 119 insertions(+), 51 deletions(-)
> 
> diff --git a/libflash/blocklevel.c b/libflash/blocklevel.c
> index 9591194..cbb9937 100644
> --- a/libflash/blocklevel.c
> +++ b/libflash/blocklevel.c
> @@ -327,53 +327,95 @@ out_free:
>  	return rc;
>  }
>  
> -static int insert_bl_prot_range(struct blocklevel_range *ranges, struct bl_prot_range range)
> +static bool insert_bl_prot_range(struct blocklevel_range *ranges, struct bl_prot_range range)
>  {
> -	struct bl_prot_range *new_ranges;
> -	struct bl_prot_range *old_ranges = ranges->prot;
> -	int i, count = ranges->n_prot;
> +	int i;
> +	uint32_t pos, len;
> +	struct bl_prot_range *prot = ranges->prot;
>  
> -	/* Try to merge into an existing range */
> -	for (i = 0; i < count; i++) {
> -		if (!(range.start + range.len == old_ranges[i].start ||
> -			  old_ranges[i].start + old_ranges[i].len == range.start))
> -			continue;
> +	pos = range.start;
> +	len = range.len;
>  
> -		if (range.start + range.len == old_ranges[i].start)
> -			old_ranges[i].start = range.start;
> +	if (len == 0)
> +		return true;
>  
> -		old_ranges[i].len += range.len;
> +	/* Check for overflow */
> +	if (pos + len < len)
> +		return false;
>  
> -		/*
> -		 * Check the inserted range isn't wedged between two ranges, if it
> -		 * is, merge as well
> -		 */
> -		i++;
> -		if (i < count && range.start + range.len == old_ranges[i].start) {
> -			old_ranges[i - 1].len += old_ranges[i].len;
> +	for (i = 0; i < ranges->n_prot && len > 0; i++) {
> +		if (prot[i].start <= pos && prot[i].start + prot[i].len >= pos + len) {
> +			len = 0;
> +			break; /* Might as well, the next two conditions can't be true */
> +		}
>  
> -			for (; i + 1 < count; i++)
> -				old_ranges[i] = old_ranges[i + 1];
> -			ranges->n_prot--;
> +		/* Can easily extend this down just by adjusting start */
> +		if (pos <= prot[i].start && pos + len >= prot[i].start) {
> +			prot[i].len += prot[i].start - pos;
> +			prot[i].start = pos;
> +			pos += prot[i].len;
> +			if (prot[i].len >= len)
> +				len = 0;
> +			else
> +				len -= prot[i].len;
>  		}
>  
> -		return 0;
> +		/*
> +		 * Jump over this range but the new range might be so big that
> +		 * theres a chunk after
> +		 */
> +		if (pos >= prot[i].start && pos < prot[i].start + prot[i].len) {
> +			if (prot[i].start + prot[i].len - pos > len) {
> +				len -= prot[i].start + prot[i].len - pos;
> +				pos = prot[i].start + prot[i].len;
> +			} else {
> +				len = 0;
> +			}
> +		}
> +		/*
> +		 * This condition will be true if the range is smaller than
> +		 * the current range, therefore it should go here!
> +		 */
> +		if (pos < prot[i].start && pos + len <= prot[i].start)
> +			break;
>  	}
>  
> -	if (ranges->n_prot == ranges->total_prot) {
> -		new_ranges = realloc(ranges->prot, sizeof(range) * ((ranges->n_prot) + PROT_REALLOC_NUM));
> -		if (new_ranges)
> +	if (len) {
> +		int insert_pos = i;
> +		struct bl_prot_range *new_ranges = ranges->prot;
> +		if (ranges->n_prot == ranges->total_prot) {
> +			new_ranges = realloc(ranges->prot,
> +					sizeof(range) * ((ranges->n_prot) + PROT_REALLOC_NUM));
> +			if (!new_ranges)
> +				return false;
>  			ranges->total_prot += PROT_REALLOC_NUM;
> -	} else {
> -		new_ranges = old_ranges;
> -	}
> -	if (new_ranges) {
> -		memcpy(new_ranges + ranges->n_prot, &range, sizeof(range));
> +		}
> +		if (insert_pos != ranges->n_prot)
> +			for (i = ranges->n_prot; i > insert_pos; i--)
> +				memcpy(&new_ranges[i], &new_ranges[i - 1], sizeof(range));
> +		range.start = pos;
> +		range.len = len;
> +		memcpy(&new_ranges[insert_pos], &range, sizeof(range));
>  		ranges->prot = new_ranges;
>  		ranges->n_prot++;
>  	}
>  
> -	return !new_ranges;
> +	/* Probably only worth mergeing when we're low on space */
> +	if (ranges->n_prot + 1 == ranges->total_prot) {
> +		/* 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;
> +				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;
>  }
>  
>  int blocklevel_ecc_protect(struct blocklevel_device *bl, uint32_t start, uint32_t len)
> @@ -385,11 +427,7 @@ int blocklevel_ecc_protect(struct blocklevel_device *bl, uint32_t start, uint32_
>  	 */
>  	struct bl_prot_range range = { .start = start, .len = len };
>  
> -	/*
> -	 * Refuse to add regions that are already protected or are partially
> -	 * protected
> -	 */
> -	if (len < BYTES_PER_ECC || ecc_protected(bl, start, len))
> +	if (len < BYTES_PER_ECC)
>  		return -1;
> -	return insert_bl_prot_range(&bl->ecc_prot, range);
> +	return !insert_bl_prot_range(&bl->ecc_prot, range);
>  }
> diff --git a/libflash/libffs.c b/libflash/libffs.c
> index 8134962..97b48c4 100644
> --- a/libflash/libffs.c
> +++ b/libflash/libffs.c
> @@ -194,7 +194,7 @@ int ffs_init(uint32_t offset, uint32_t max_size, struct blocklevel_device *bl,
>  			if (ecc) {
>  				rc = blocklevel_ecc_protect(bl, start, total_size);
>  				if (rc) {
> -					FL_ERR("Failed to blocklevel_ecc_protect(0x%08x, 0x%08x)\n",
> +					FL_ERR("FFS: Failed to blocklevel_ecc_protect(0x%08x, 0x%08x)\n",
>  					       start, total_size);
>  					goto out;
>  				}
> diff --git a/libflash/test/test-blocklevel.c b/libflash/test/test-blocklevel.c
> index ef5d9b5..9630939 100644
> --- a/libflash/test/test-blocklevel.c
> +++ b/libflash/test/test-blocklevel.c
> @@ -44,6 +44,10 @@ int main(void)
>  		ERR("Failed to blocklevel_ecc_protect(0x3000, 0x1000)\n");
>  		return 1;
>  	}
> +	if (blocklevel_ecc_protect(bl, 0x2f00, 0x1100)) {
> +		ERR("Failed to blocklevel_ecc_protect(0x2f00, 0x1100)\n");
> +		return 1;
> +	}
>  
>  	/* Zero length protection */
>  	if (!blocklevel_ecc_protect(bl, 0x4000, 0)) {
> @@ -58,14 +62,20 @@ int main(void)
>  	}
>  
>  	/* Deal with overlapping protections */
> -	if (!blocklevel_ecc_protect(bl, 0x100, 0x1000)) {
> -		ERR("Shouldn't have succeeded blocklevel_ecc_protect(0x100, 0x1000)\n");
> +	if (blocklevel_ecc_protect(bl, 0x100, 0x1000)) {
> +		ERR("Failed to protect overlaping region blocklevel_ecc_protect(0x100, 0x1000)\n");
> +		return 1;
> +	}
> +
> +	/* Deal with overflow */
> +	if (!blocklevel_ecc_protect(bl, 1, 0xFFFFFFFF)) {
> +		ERR("Added an 'overflow' protection blocklevel_ecc_protect(1, 0xFFFFFFFF)\n");
>  		return 1;
>  	}
>  
> -	/* Deal with protections greater than max size */
> -	if (!blocklevel_ecc_protect(bl, 0, 0xFFFFFFFF)) {
> -		ERR("Failed to blocklevel_ecc_protect(0, 0xFFFFFFFF)\n");
> +	/* Protect everything */
> +	if (blocklevel_ecc_protect(bl, 0, 0xFFFFFFFF)) {
> +		ERR("Couldn't protect everything blocklevel_ecc_protect(0, 0xFFFFFFFF)\n");
>  		return 1;
>  	}
>  
> @@ -84,17 +94,30 @@ int main(void)
>  		return 1;
>  	}
>  
> -	if (ecc_protected(bl, 0x1000, 0) != 0) {
> +	/* Clear the protections */
> +	bl->ecc_prot.n_prot = 0;
> +	/* Reprotect */
> +	if (blocklevel_ecc_protect(bl, 0x3000, 0x1000)) {
> +		ERR("Failed to blocklevel_ecc_protect(0x3000, 0x1000)\n");
> +		return 1;
> +	}
> +	/* Deal with overlapping protections */
> +	if (blocklevel_ecc_protect(bl, 0x100, 0x1000)) {
> +		ERR("Failed to protect overlaping region blocklevel_ecc_protect(0x100, 0x1000)\n");
> +		return 1;
> +	}
> +
> +	if (ecc_protected(bl, 0x1000, 0) != 1) {
>  		ERR("Invalid result for ecc_protected(0x1000, 0)\n");
>  		return 1;
>  	}
>  
> -	if (ecc_protected(bl, 0x1000, 0x1000) != 0) {
> +	if (ecc_protected(bl, 0x1000, 0x1000) != -1) {
>  		ERR("Invalid result for ecc_protected(0x1000, 0x1000)\n");
>  		return 1;
>  	}
>  
> -	if (ecc_protected(bl, 0x1000, 0x100) != 0) {
> +	if (ecc_protected(bl, 0x1000, 0x100) != 1) {
>  		ERR("Invalid result for ecc_protected(0x1000, 0x100)\n");
>  		return 1;
>  	}
> @@ -104,7 +127,7 @@ int main(void)
>  		return 1;
>  	}
>  
> -	if (ecc_protected(bl, 0x4000, 1) != 1) {
> +	if (ecc_protected(bl, 0x4000, 1) != 0) {
>  		ERR("Invalid result for ecc_protected(0x4000, 1)\n");
>  		return 1;
>  	}
> @@ -136,6 +159,11 @@ int main(void)
>  		return 1;
>  	}
>  
> +	if (blocklevel_ecc_protect(bl, 0x4f00, 0x100)) {
> +		ERR("Failed to blocklevel_ecc_protected(0x4900, 0x100)\n");
> +		return 1;
> +	}
> +
>  	if (blocklevel_ecc_protect(bl, 0x4900, 0x100)) {
>  		ERR("Failed to blocklevel_ecc_protected(0x4900, 0x100)\n");
>  		return 1;
> @@ -146,8 +174,8 @@ int main(void)
>  		return 1;
>  	}
>  
> -	if (!blocklevel_ecc_protect(bl, 0x5290, 0x10)) {
> -		ERR("Shouldn't have been able to blocklevel_ecc_protect(0x5290, 0x10)\n");
> +	if (blocklevel_ecc_protect(bl, 0x5290, 0x10)) {
> +		ERR("Failed to blocklevel_ecc_protect(0x5290, 0x10)\n");
>  		return 1;
>  	}
>  
> @@ -166,7 +194,9 @@ int main(void)
>  		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 ||



More information about the Skiboot mailing list