From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Thu, 17 Mar 2016 15:16:58 -0600
Subject: [U-Boot] [RFC PATCH 1/2] add block device cache
In-Reply-To: <1458164424-15363-2-git-send-email-eric@nelint.com>
References: <56E9A92F.5000205@nelint.com>
 <1458164424-15363-1-git-send-email-eric@nelint.com>
 <1458164424-15363-2-git-send-email-eric@nelint.com>
Message-ID: <56EB1ECA.3030204@wwwdotorg.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 03/16/2016 03:40 PM, Eric Nelson wrote:
> Signed-off-by: Eric Nelson <eric@nelint.com>

A patch description would be useful here; the cover letter wouldn't be 
checked in.

> diff --git a/drivers/block/cache_block.c b/drivers/block/cache_block.c

> +static int cache_iftype = -1;
> +static int cache_devnum = -1;
> +static lbaint_t cache_start = -1;
> +static lbaint_t cache_blkcnt = -1;
 > +static unsigned long cache_blksz;
 > +static void *cache;

Since variable "cache" is in BSS and hence is 0, which is included in 
all checks below, none of the other values need to be initialized.

> +int cache_block_read(int iftype, int dev,
> +		     lbaint_t start, lbaint_t blkcnt,
> +		     unsigned long blksz, void *buffer)
> +{
> +	if ((iftype == cache_iftype) &&
> +	    (dev == cache_devnum) &&
> +	    (start == cache_start) &&
> +	    (blkcnt <= cache_blkcnt) &&
> +	    (blksz == cache_blksz) &&
> +	    (cache != 0)) {

I'd suggest putting (cache != 0) first, since that check indicates 
whether any of the others are relevant.

> +		memcpy(buffer, cache, blksz*blkcnt);

Nit: space around the * operator. Same comment everywhere.

It's probably hard to fix this given the simple API and lack of MMU 
page-tables to remap on a per-page basis, but one deficiency in this 
(deliberately) simple implementation is that if someone caches blocks 
0..7 then later tries to read 2..10 (or even 2..3) they won't get a 
cache hit. While partial hits (the 2..10 case) are hard to deal with, 
perhaps it's useful to make subset cases (2..3 with 0..7 cached) work?

> +void cache_block_fill(int iftype, int dev,
> +		      lbaint_t start, lbaint_t blkcnt,
> +		      unsigned long blksz, void const *buffer)

> +	bytes = blksz*blkcnt;
> +	if (cache != 0) {
> +		if (bytes != (cache_blksz*cache_blkcnt)) {
> +			free(cache);
> +			cache = malloc(blksz*blkcnt);
> +			if (!cache)
> +				return;
> +		} /* change in size */
> +	} else {
> +		cache = malloc(blksz*blkcnt);
> +		if (!cache)
> +			return;
> +	}

Is it better to put the if (!cache) test after the whole if/else block 
so it's unified?

> +void cache_block_invalidate(int iftype, int dev)
> +{
> +	cache_iftype = -1;

That isn't actually necessary, since assigning 0 to cache below will 
prevent anything from using the cache.

> +	if (cache) {
> +		free(cache);
> +		cache = 0;
> +	}
> +}

> diff --git a/include/part.h b/include/part.h

> +#ifdef CONFIG_BLOCK_CACHE
> +/**
> + * cache_block_read() - attempt to read a set of blocks from cache
> + *
> + * @param iftype - IF_TYPE_x for type of device
> + * @param dev - device index of particular type
> + * @param start - starting block number
> + * @param blkcnt - number of blocks to read
> + * @param blksz - size in bytes of each block
> + * @param buf - buffer to contain cached data

s/contain/receive/?

> + *
> + * @return - '1' if block returned from cache, '0' otherwise.
> + */
> +int cache_block_read
> +	(int iftype, int dev,
> +	 lbaint_t start, lbaint_t blkcnt,
> +	 unsigned long blksz, void *buffer);

Nit: ( and probably the first n parameters should be on the same line as 
the function name.