Re: [PATCH v4 5/5] mtd: nand: Improve bitflip detection for on-die ECC scheme.

[Brian Norris <computersforpeace@gmail.com>]

On Tue, Apr 01, 2014 at 09:51:13AM -0600, David Mosberger wrote:
> On Tue, Apr 1, 2014 at 12:29 AM, Gupta, Pekon <pekon@ti.com> wrote:
> >>+/*
> >>+ * Return the number of bits that differ between buffers SRC1 and
> >>+ * SRC2, both of which are LEN bytes long.
> >>+ *
> >>+ * This code could be optimized for, but it only gets called on pages
> >>+ * with bitflips and compared to the cost of migrating an eraseblock,
> >>+ * the execution time here is trivial...
> >>+ */
> >>+static int
> >>+bitdiff(const void *s1, const void *s2, size_t len)
> >>+{
> >>+      const uint8_t *src1 = s1, *src2 = s2;
> >>+      int count = 0, i;
> >>+
> >>+      for (i = 0; i < len; ++i)
> >>+              count += hweight8(*src1++ ^ *src2++);
> >>+      return count;
> >
> > hweight8() might not be good option, how about using hweight32 ?
> 
> Why don't you read the comment above.  Yes, you can add more code
> and make it faster and it will not matter one bit, so I chose to go with
> the shortest code possible.  Sue me.

I'm guilty of making the same comment, and it really isn't warranted in
some cases where simplicity should win. I'm fine taking this piece
as-is, I suppose, since it likely experiences a low bit-error rate. But
I would caution that bit errors are becoming increasingly common, so
your "trivial" comment may not age well.

> >>+                      flips += hweight8(chkoob[*eccpos] ^ rawoob[*eccpos]);
> >>+                      ++eccpos;
> >>+              }
> >>+              if (flips > 0)
> >>+                      mtd->ecc_stats.corrected += flips;
> >>+              max_bitflips = max_t(int, max_bitflips, flips);
> >>+              chkbuf += chip->ecc.size;
> >>+              rawbuf += chip->ecc.size;
> >>+      }

Brian