From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752162AbcFKWaI (ORCPT ); Sat, 11 Jun 2016 18:30:08 -0400 Received: from ns.sciencehorizons.net ([71.41.210.147]:26757 "HELO ns.sciencehorizons.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751849AbcFKWaH (ORCPT ); Sat, 11 Jun 2016 18:30:07 -0400 Date: 11 Jun 2016 18:30:04 -0400 Message-ID: <20160611223004.20916.qmail@ns.sciencehorizons.net> From: "George Spelvin" To: boris.brezillon@free-electrons.com Subject: Re: [PATCH 2/4] mtd: nand: implement two pairing scheme Cc: linux@sciencehorizons.net, linux-kernel@vger.kernel.org, richard@nod.at Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org I was just browsing LKML history and wanted to understand this concept, but while reading I think I spotted an error. +static void nand_pairing_dist3_get_info(struct mtd_info *mtd, int page, + struct mtd_pairing_info *info) +{ + int lastpage = (mtd->erasesize / mtd->writesize) - 1; + int dist = 3; + + if (page == lastpage) + dist = 2; + + if (!page || (page & 1)) { + info->group = 0; + info->pair = (page + 1) / 2; + } else { + info->group = 1; + info->pair = (page + 1 - dist) / 2; + } +} As I understand this, the general rule is that odd pages i are paired with even pages i+3, and group = !(i & 1). If there are N pages total (numbered 0..N-1), this leaves four exceptions to deal with: -3 would be apired with 0 -1 would be paired with 2 N-3 would be paired with N N-1 would be paired with N+2 This is solved by pairing 0 with 2, and N-1 with N-3. In terms of group addresses, there are only two exceptions: * 0 is pair 0, group 0 (not pair -1, group 1) * N-1 is pair N/2/-1, group 1 (not pair N/2, group 0) You have the first exception correct, but not the second; the condition detects it, but the change to "dist" doesn't matter since the first half of the second if() will be taken. I think the easiest way to get this right is the following: page = page + (page != 0) + (page == lastpage); info->group = page & 1; if (page & 1) page -= dist; info->pair = page >> 1; Rather than make up too many rules, this just maps 0 -> -1 and N-1 -> N, then applies the general rule. It also applies an offset of +1, to avoid negative numbers and the problems of signed divides. Also, a very minor note: divides are expensive. A cheaper way to evaluate the "page == lastpage" condition is if ((page + 1) * mtd->writesize == mtd->erasesize) (or you could add an mtd->write_per_erase field). Applying all of this, the corrected code looks like the following: (Note the addition of "const" and "__pure" annotations, which should get copied to the "struct mtd_pairing_scheme" declaration.) Signed-off-by: George Spelvin /* * In distance-3 pairing, odd pages i are group 0, and are paired * with even pages i+3. The exceptions are the first page (page 0) * and last page (page N-1) in an erase group. These pair as if * they were pages -1 and N, respectively. */ static void nand_pairing_dist3_get_info(const struct mtd_info *mtd, int page, struct mtd_pairing_info *info) { page += (page != 0) + ((page + 1) * mtd->writesize == mtd->erasesize); info->group = page & 1; if (page & 1) page -= 3; info->pair = page >> 1; } static int __pure nand_pairing_dist3_get_wunit(const struct mtd_info *mtd, const struct mtd_pairing_info *info) { int page = 2 * info->pair + 3 * info->group; page -= (page != 0) + (page * mtd->writesize > mtd->erasesize); return page; } const struct mtd_pairing_scheme dist3_pairing_scheme = { .ngroups = 2, .get_info = nand_pairing_dist3_get_info, .get_wunit = nand_pairing_dist3_get_wunit, }; EXPORT_SYMBOL_GPL(dist3_pairing_scheme); /* * Distance-6 pairing works like distance-3 pairing, except that pages * are taken two at a time. The lsbit of the page number is chopped off * and later re-added as the lsbit of the pair number. */ static void nand_pairing_dist6_get_info(const struct mtd_info *mtd, int page, struct mtd_pairing_info *info) { bool lsbit = page & 1; page >>= 1; page += (page != 0) + ((page+1) * 2*mtd->writesize == mtd->erasesize); info->group = page & 1; if (page & 1) page -= 3; info->pair = page | lsbit; } static int __pure nand_pairing_dist6_get_wunit(const struct mtd_info *mtd, const struct mtd_pairing_info *info) { int page = (info->pair & ~1) + 3 * info->group; page -= (page != 0) + (page * 2 * mtd->writesize > mtd->erasesize); return 2 * page + (info->pair & 1); } const struct mtd_pairing_scheme dist6_pairing_scheme = { .ngroups = 2, .get_info = nand_pairing_dist6_get_info, .get_wunit = nand_pairing_dist6_get_wunit, }; EXPORT_SYMBOL_GPL(dist6_pairing_scheme);