[flashrom] [PATCH] Simplify chunking logic

[Carl-Daniel Hailfinger]

On 11.07.2010 18:42, Michael Karcher wrote:
> Read function tested for chunk size 62 and with start offset 3, works
> as expected.
>
> Signed-off-by: Michael Karcher <flashrom at mkarcher.dialup.fu-berlin.de>
> ---
>  spi25.c |   80 +++++++++++++++++++++++++-------------------------------------
>  1 files changed, 32 insertions(+), 48 deletions(-)
>
> diff --git a/spi25.c b/spi25.c
> index 51be397..260dd1c 100644
> --- a/spi25.c
> +++ b/spi25.c
> @@ -879,35 +879,27 @@ int spi_nbyte_read(int address, uint8_t *bytes, int len)
>  int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize)
>  {
>  	int rc = 0;
> -	int i, j, starthere, lenhere;
> +	int ofs, startofs, endofs;	/* offsets relative to page begin */
>  	int page_size = flash->page_size;
> -	int toread;
> -
> -	/* Warning: This loop has a very unusual condition and body.
> -	 * The loop needs to go through each page with at least one affected
> -	 * byte. The lowest page number is (start / page_size) since that
> -	 * division rounds down. The highest page number we want is the page
> -	 * where the last byte of the range lives. That last byte has the
> -	 * address (start + len - 1), thus the highest page number is
> -	 * (start + len - 1) / page_size. Since we want to include that last
> -	 * page as well, the loop condition uses <=.
> -	 */
> -	for (i = start / page_size; i <= (start + len - 1) / page_size; i++) {
> -		/* Byte position of the first byte in the range in this page. */
> -		/* starthere is an offset to the base address of the chip. */
> -		starthere = max(start, i * page_size);
> -		/* Length of bytes in the range in this page. */
> -		lenhere = min(start + len, (i + 1) * page_size) - starthere;
> -		for (j = 0; j < lenhere; j += chunksize) {
> -			toread = min(chunksize, lenhere - j);
> -			rc = spi_nbyte_read(starthere + j, buf + starthere - start + j, toread);
> +	int pageaddr, toread;
> +
>   

/* startofs is an offset to current page address. */


> +	startofs = start % page_size;
> +	/* iterate over all affected pages */
> +	for (pageaddr = start - startofs; pageaddr < start + len;
> +             pageaddr += page_size) {
>   

Please use tabs here, at least for the first 8-space blocks.


> +     		endofs = min(page_size, start + len - pageaddr);
>   

endofs is a misnomer. The correct name would be one_after_endofs, but
that name is ugly. What about

/* Number of bytes to write in this page. */
lenhere = min(page_size, start + len - pageaddr) - startofs;

and using (startofs + lenhere) in place of endofs everywhere?


> +     		/*  iterate over all chunks in the current page */
> +		for (ofs = startofs; ofs < endofs; ofs += chunksize) {
> +			toread = min(chunksize, endofs - ofs);
> +			rc = spi_nbyte_read(pageaddr + ofs, buf, toread);
> +			buf += toread;
>  			if (rc)
> -				break;
> +				goto leave_page_loop;
>   

return rc;


>  		}
> -		if (rc)
> -			break;
> +		startofs = 0;	/* all further pages processed from start */
>  	}
>  
> +leave_page_loop:
>   

Not needed if you return rc immediately in the error case.


>  	return rc;
>  }
>   

Here is my variant of your code. Feel free to ignore it completely, I
just wanted to send it for reference.

int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize)
{
        int rc = 0;
        int j, lenhere;
        int page_size = flash->page_size;
        int pageaddr, toread, startofs;

        /* This loop needs to go through each page with at least one affected
         * byte.
         */
        startofs = start % page_size;
        for (pageaddr = start - startofs; pageaddr < start + len; pageaddr += page_size) {
                /* startofs is an offset to current page address. */
                /* Number of bytes to write in this page. */
                lenhere = min(page_size, start + len - pageaddr) - startofs;
                for (j = startofs; j < startofs + lenhere; j += chunksize) {
                        toread = min(chunksize, startofs + lenhere - j);
                        rc = spi_nbyte_read(pageaddr + j, buf, toread);
                        if (rc)
                                return rc;
                        buf += toread;
                }
                startofs = 0;
        }

        return rc;
}


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/