From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from nf-out-0910.google.com ([64.233.182.185]) by bombadil.infradead.org with esmtp (Exim 4.68 #1 (Red Hat Linux)) id 1KVByQ-0002EF-TX for linux-mtd@lists.infradead.org; Mon, 18 Aug 2008 21:09:13 +0000 Received: by nf-out-0910.google.com with SMTP id d21so2031997nfb.22 for ; Mon, 18 Aug 2008 14:09:00 -0700 (PDT) Message-ID: Date: Mon, 18 Aug 2008 23:09:00 +0200 From: "Frans Meulenbroeks" To: "Troy Kisky" Subject: Re: [RESUBMIT] [PATCH] [MTD] NAND nand_ecc.c: rewrite for improved performance In-Reply-To: <48A9AF73.2040105@boundarydevices.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <1218793271.3184.77.camel@pmac.infradead.org> <1218795140.3184.84.camel@pmac.infradead.org> <48A5D154.2000409@boundarydevices.com> <48A8937D.1010007@boundarydevices.com> <48A9AF73.2040105@boundarydevices.com> Cc: linux-mtd@lists.infradead.org, David Woodhouse List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 2008/8/18 Troy Kisky : > Frans Meulenbroeks wrote: >> Yes, the NSLU2 had a filesystem that was created before the patch was applied. >> But actually I think the filesystem is irrelevant. >> I verified the proper operation by comparing the values generated by >> the original code with the values generated by my code over a set of >> input blocks. >> Guess there is no endianness dependency and that if the data is big >> endian the ecc is too. > > Does that make logical sense to you? The correction routine > accesses the data as a byte and flips a bit. If it accessed it as > an uint32 and flipped the bit, then I can see that there would be > no endianness dependency. I'm not suggesting you do that, as it would be > incompatible with current ecc, just explaining my logic. I'd very > much appreciate an explanation of why I'm wrong. I would expect > big endian ecc to have 4 bits differences whenever the entire > block parity is odd. These would be the bits that select the byte > within the uint32. Troy, did a further investigation. Your explanation is correct. My test program had a flaw causing this case to be undetected. Indeed in case of odd parity the 4 bits selecting the byte are flipped on big endian systems. (little endian is ok). Still looking at what the best way to fix it. In the code you posted before you used __cpu_to_le64s. Not sure why you are using the 64 variant. As it is an uint32_t, I would expect __cpu_to_le32s to suffice. Then again I am not too eager to use that function as it generates some overhead. I'd rather use the builtin gcc macro __BIG_ENDIAN__ (in that case I can just use an #ifdef to distinguish the two cases and in case of BE no byte swapping is needed. What is your opinion on this? Frans. PS: wrt the 11 bits check for the other message. Can't really envision why this fails, but maybe it is just too late. If you have an ecc and a faulty 256 byte data block that would be erroneously accepted by my code and that would be rightfully rejected by the original code, I'll be more than happy to change it. Performancewise the difference is very small and it is a rare situation anyway. The original test is definitely more rigid than just the nr of bits test.