On 03/16/2016 03:40 PM, Eric Nelson wrote:
Signed-off-by: Eric Nelson <e...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to