Switched to separate-tag encoding of forward-looking CRCs

Previously forward-looking CRCs was just two new CRC types, one for
commits with forward-looking CRCs, one without. These both contained the
CRC needed to complete the current commit (note that the commit CRC
must come last!).

         [--   32   --|--   32   --|--   32   --|--   32   --]
with:    [  crc3 tag  | nprog size |  nprog crc | commit crc ]
without: [  crc2 tag  | commit crc ]

This meant there had to be several checks for the two possible structure
sizes, messying up the implementation.

         [--   32   --|--   32   --|--   32   --|--   32   --|--   32   --]
with:    [nprogcrc tag| nprog size |  nprog crc | commit tag | commit crc ]
without: [ commit tag | commit crc ]

But we already have a mechanism for storing optional metadata! The
different metadata tags! So why not use a separate tage for the
forward-looking CRC, separate from the commit CRC?

I wasn't sure this would actually help that much, there are still
necessary conditions for wether or not a forward-looking CRC is there,
but in the end it simplified the code quite nicely, and resulted in a ~200 byte
code-cost saving.

lfs.c

@@ -330,6 +330,10 @@ static inline uint16_t lfs_tag_type1(lfs_tag_t tag) {
     return (tag & 0x70000000) >> 20;
 }
 
+static inline uint16_t lfs_tag_type2(lfs_tag_t tag) {
+    return (tag & 0x78000000) >> 20;
+}
+
 static inline uint16_t lfs_tag_type3(lfs_tag_t tag) {
     return (tag & 0x7ff00000) >> 20;
 }
@@ -916,6 +920,9 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
         bool tempsplit = false;
         lfs_stag_t tempbesttag = besttag;
 
+        bool hasestate = false;
+        struct lfs_estate estate;
+
         dir->rev = lfs_tole32(dir->rev);
         uint32_t crc = lfs_crc(0xffffffff, &dir->rev, sizeof(dir->rev));
         dir->rev = lfs_fromle32(dir->rev);
@@ -947,32 +954,12 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
 
             ptag = tag;
 
-            if (lfs_tag_type1(tag) == LFS_TYPE_CRC) {
+            if (lfs_tag_type2(tag) == LFS_TYPE_CRC) {
-                lfs_off_t noff = off + sizeof(tag);
-                struct lfs_estate estate;
-
-                if (lfs_tag_chunk(tag) == 3) {
-                    err = lfs_bd_read(lfs,
-                            NULL, &lfs->rcache, lfs->cfg->block_size,
-                            dir->pair[0], noff, &estate, sizeof(estate));
-                    if (err) {
-                        if (err == LFS_ERR_CORRUPT) {
-                            dir->erased = false;
-                            break;
-                        }
-                        return err;
-                    }
-
-                    crc = lfs_crc(crc, &estate, sizeof(estate));
-                    lfs_estate_fromle32(&estate);
-                    noff += sizeof(estate);
-                }
-
                 // check the crc attr
                 uint32_t dcrc;
                 err = lfs_bd_read(lfs,
                         NULL, &lfs->rcache, lfs->cfg->block_size,
-                        dir->pair[0], noff, &dcrc, sizeof(dcrc));
+                        dir->pair[0], off+sizeof(tag), &dcrc, sizeof(dcrc));
                 if (err) {
                     if (err == LFS_ERR_CORRUPT) {
                         dir->erased = false;
@@ -1002,10 +989,7 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
                 dir->tail[1] = temptail[1];
                 dir->split = tempsplit;
 
-                // reset crc
+                if (hasestate) {
-                crc = 0xffffffff;
-
-                if (lfs_tag_chunk(tag) == 3) {
                     // check if the next page is erased
                     //
                     // this may look inefficient, but since cache_size is
@@ -1041,15 +1025,19 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
                         dir->erased = true;
                         break;
                     }
-                } else if (lfs_tag_chunk(tag) == 2) {
+                } else if (lfs_tag_chunk(tag) < 2) {
-                    // end of block commit
-                    dir->erased = false;
-                    break;
-                } else {
                     // for backwards compatibility we fall through here on
                     // unrecognized tags, leaving it up to the CRC to reject
                     // bad commits
+                } else {
+                    // end of block commit
+                    dir->erased = false;
+                    break;
                 }
+
+                // reset crc
+                crc = 0xffffffff;
+                hasestate = false;
             } else {
                 // crc the entry first, hopefully leaving it in the cache
                 err = lfs_bd_crc(lfs,
@@ -1085,7 +1073,8 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
 
                     err = lfs_bd_read(lfs,
                             NULL, &lfs->rcache, lfs->cfg->block_size,
-                            dir->pair[0], off+sizeof(tag), &temptail, 8);
+                            dir->pair[0], off+sizeof(tag),
+                            &temptail, sizeof(temptail));
                     if (err) {
                         if (err == LFS_ERR_CORRUPT) {
                             dir->erased = false;
@@ -1093,6 +1082,19 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
                         }
                     }
                     lfs_pair_fromle32(temptail);
+                } else if (lfs_tag_type1(tag) == LFS_TYPE_NPROGCRC) {
+                    err = lfs_bd_read(lfs,
+                            NULL, &lfs->rcache, lfs->cfg->block_size,
+                            dir->pair[0], off+sizeof(tag),
+                            &estate, sizeof(estate));
+                    if (err) {
+                        if (err == LFS_ERR_CORRUPT) {
+                            dir->erased = false;
+                            break;
+                        }
+                    }
+                    lfs_estate_fromle32(&estate);
+                    hasestate = true;
                 }
 
                 // found a match for our fetcher?
@@ -1434,13 +1436,9 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
     // - 4-word crc with estate to check following prog (middle of block)
     // - 2-word crc with no following prog (end of block)
     const lfs_off_t end = lfs_alignup(
-            lfs_min(commit->off + 4*sizeof(uint32_t), lfs->cfg->block_size),
+            lfs_min(commit->off + 5*sizeof(uint32_t), lfs->cfg->block_size),
             lfs->cfg->prog_size);
 
-    // clamp erase size to tag size, this gives us the full tag as potential
-    // to intentionally invalidate erase CRCs
-    const lfs_size_t esize = lfs_max(lfs->cfg->prog_size, sizeof(lfs_tag_t));
-
     lfs_off_t off1 = 0;
     uint32_t crc1 = 0;
 
@@ -1454,23 +1452,12 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
             noff = lfs_min(noff, end - 2*sizeof(uint32_t));
         }
 
-        // build crc tag
+        // clamp erase size to tag size, this gives us the full tag as potential
+        // to intentionally invalidate erase CRCs
+        const lfs_size_t esize = lfs_max(
+                lfs->cfg->prog_size, sizeof(lfs_tag_t));
         lfs_tag_t etag = 0;
-        lfs_tag_t tag = LFS_MKTAG(LFS_TYPE_CRC + 2, 0x3ff, noff - off);
+        // space for estate? also only emit on last commit in padding commits
-        lfs_size_t size = 2*sizeof(uint32_t);
-        struct {
-            lfs_tag_t tag;
-            union {
-                struct {
-                    uint32_t crc;
-                } crc2;
-                struct {
-                    struct lfs_estate estate;
-                    uint32_t crc;
-                } crc3;
-            } u;
-        } data;
-
         if (noff <= lfs->cfg->block_size - esize) {
             // first we get a tag-worth of bits, this is so we can
             // tweak our current tag to force future writes to be
@@ -1491,36 +1478,43 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
                 return err;
             }
 
-            data.u.crc3.estate.size = esize;
+            struct lfs_estate estate = {.size = esize, .crc = ecrc};
-            data.u.crc3.estate.crc = ecrc;
+            lfs_estate_tole32(&estate);
-            lfs_estate_tole32(&data.u.crc3.estate);
+            err = lfs_dir_commitattr(lfs, commit,
-
+                    LFS_MKTAG(LFS_TYPE_NPROGCRC, 0x3ff, sizeof(estate)),
-            // indicate we include estate
+                    &estate);
-            tag |= LFS_MKTAG(1, 0, 0);
+            if (err) {
-            size += sizeof(struct lfs_estate);
+                return err;
+            }
         }
 
-        data.tag = lfs_tobe32(tag ^ commit->ptag);
+        // build crc state
-        commit->crc = lfs_crc(commit->crc, &data, size-sizeof(uint32_t));
+        off = commit->off + sizeof(lfs_tag_t);
-        ((uint32_t*)&data)[size/sizeof(uint32_t) - 1] = lfs_tole32(commit->crc);
+        struct {
+            lfs_tag_t tag;
+            uint32_t crc;
+        } cstate;
+        lfs_tag_t ctag = LFS_MKTAG(LFS_TYPE_COMMITCRC, 0x3ff, noff-off);
+        cstate.tag = lfs_tobe32(ctag ^ commit->ptag);
+        commit->crc = lfs_crc(commit->crc, &cstate.tag, sizeof(cstate.tag));
+        cstate.crc = lfs_tole32(commit->crc);
 
         int err = lfs_bd_prog(lfs,
                 &lfs->pcache, &lfs->rcache, false,
-                commit->block, commit->off, &data, size);
+                commit->block, commit->off, &cstate, sizeof(cstate));
         if (err) {
             return err;
         }
 
         // keep track of non-padding checksum to verify
         if (off1 == 0) {
-            //off1 = commit->off + sizeof(uint32_t);
+            off1 = off;
-            off1 = commit->off + size-sizeof(uint32_t);
             crc1 = commit->crc;
         }
 
-        commit->off += sizeof(tag)+lfs_tag_size(tag);
+        commit->off = noff;
         // perturb valid bit?
-        commit->ptag = tag | (0x80000000 & ~lfs_frombe32(etag));
+        commit->ptag = ctag | (0x80000000 & ~lfs_frombe32(etag));
         // reset crc for next commit
         commit->crc = 0xffffffff;
     }

lfs.h

@@ -113,6 +113,8 @@ enum lfs_type {
     LFS_TYPE_SOFTTAIL       = 0x600,
     LFS_TYPE_HARDTAIL       = 0x601,
     LFS_TYPE_MOVESTATE      = 0x7ff,
+    LFS_TYPE_COMMITCRC      = 0x502,
+    LFS_TYPE_NPROGCRC       = 0x5ff,
 
     // internal chip sources
     LFS_FROM_NOOP           = 0x000,

scripts/readmdir.py

@@ -24,6 +24,7 @@ TAG_TYPES = {
     'gstate':       (0x700, 0x700),
     'movestate':    (0x7ff, 0x7ff),
     'crc':          (0x700, 0x500),
+    'nprogcrc':     (0x7ff, 0x5ff),
 }
 
 class Tag:
@@ -125,9 +126,10 @@ class Tag:
         return ntag
 
     def typerepr(self):
-        if self.is_('crc') and getattr(self, 'crc', 0xffffffff) != 0xffffffff:
+        if (self.is_('crc') and not self.is_('nprogcrc') and
+                getattr(self, 'crc', 0xffffffff) != 0xffffffff):
             crc_status = ' (bad)'
-        elif self.is_('crc') and getattr(self, 'erased', False):
+        elif self.is_('nprogcrc') and getattr(self, 'erased', False):
             crc_status = ' (era)'
         else:
             crc_status = ''
@@ -186,6 +188,8 @@ class MetadataPair:
 
         self.rev, = struct.unpack('<I', block[0:4])
         crc = binascii.crc32(block[0:4])
+        etag = None
+        estate = None
 
         # parse tags
         corrupt = False
@@ -199,9 +203,7 @@ class MetadataPair:
             tag = Tag((int(tag) ^ ntag) & 0x7fffffff)
             tag.off = off + 4
             tag.data = block[off+4:off+tag.dsize]
-            if tag.is_('crc 0x3'):
+            if tag.is_('crc') and not tag.is_('nprogcrc'):
-                crc = binascii.crc32(block[off:off+4*4], crc)
-            elif tag.is_('crc'):
                 crc = binascii.crc32(block[off:off+2*4], crc)
             else:
                 crc = binascii.crc32(block[off:off+tag.dsize], crc)
@@ -210,25 +212,29 @@ class MetadataPair:
 
             self.all_.append(tag)
 
-            if tag.is_('crc'):
+            if tag.is_('nprogcrc') and len(tag.data) == 8:
+                etag = tag
+                estate = struct.unpack('<II', tag.data)
+            elif tag.is_('crc'):
                 # is valid commit?
                 if crc != 0xffffffff:
                     corrupt = True
                 if not corrupt:
                     self.log = self.all_.copy()
-
                     # end of commit?
-                    if tag.is_('crc 0x3'):
+                    if estate:
-                        esize, ecrc = struct.unpack('<II', tag.data[:8])
+                        esize, ecrc = estate
                         dcrc = 0xffffffff ^ binascii.crc32(block[off:off+esize])
                         if ecrc == dcrc:
-                            tag.erased = True
+                            etag.erased = True
                             corrupt = True
-                    elif tag.is_('crc 0x2'):
+                    elif not (tag.is_('crc 0x0') or tag.is_('crc 0x1')):
                         corrupt = True
 
                 # reset tag parsing
                 crc = 0
+                etag = None
+                estate = None
 
         # find active ids
         self.ids = list(it.takewhile(
@@ -305,7 +311,7 @@ class MetadataPair:
         f.write('\n')
 
         for tag in tags:
-            f.write("%08x: %08x  %-13s %4s %4s" % (
+            f.write("%08x: %08x  %-14s %3s %4s" % (
                 tag.off, tag,
                 tag.typerepr(), tag.idrepr(), tag.sizerepr()))
             if truncate: