[{"id":1753848,"web_url":"http://patchwork.ozlabs.org/comment/1753848/","msgid":"<20170822145424.54e57b94@bbrezillon>","list_archive_url":null,"date":"2017-08-22T12:54:24","subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","submitter":{"id":63120,"url":"http://patchwork.ozlabs.org/api/people/63120/","name":"Boris Brezillon","email":"boris.brezillon@free-electrons.com"},"content":"Hi Andrea,\n\nLe Tue, 22 Aug 2017 11:42:52 +0200,\nAndrea Adami a écrit :\n\n> The Sharp SL Series (Zaurus) PXA handhelds have 16/64/128M of NAND flash\n> and share the same layout of the first 7M partition, managed by Sharp FTL.\n> \n> The purpose of this self-contained patch is to add a common parser and\n> remove the hardcoded sizes in the board files (these devices are not yet\n> converted to devicetree).\n> Users will have benefits because the mtdparts= tag will not be necessary\n> anymore and they will be free to repartition the little sized flash.\n> \n> The obsolete bootloader can not pass the partitioning info to modern\n> kernels anymore so it has to be read from flash at known logical addresses.\n> (see http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm )\n> \n> In kernel, under arch/arm/mach-pxa we have already 8 machines:\n> MACH_POODLE, MACH_CORGI, MACH_SHEPERD, MACH_HUSKY, MACH_AKITA, MACH_SPITZ,\n> MACH_BORZOI, MACH_TOSA.\n> Lost after the 2.4 vendor kernel are MACH_BOXER and MACH_TERRIER.\n> \n> Almost every model has different factory partitioning: add to this the\n> units can be repartitioned by users with userspace tools (nandlogical)\n> and installers for popular (back then) linux distributions.\n> \n> The Parameter Area in the first (boot) partition extends from 0x00040000 to\n> 0x0007bfff (176k) and contains two copies of the partition table:\n> ...\n> 0x00060000: Partition Info1 16k\n> 0x00064000: Partition Info2 16k\n> 0x00668000: Model 16k\n> ...\n> \n> The first 7M partition is managed by the Sharp FTL reserving 5% + 1 blocks\n> for wear-leveling: some blocks are remapped and one layer of translation\n> (logical to physical) is necessary.\n> \n> There isn't much documentation about this FTL in the 2.4 sources, just the\n> MTD methods for reading and writing using logical addresses and the block\n> management (wear-leveling, use counter).\n> For the purpose of the MTD parser only the read part of the code was taken.\n> \n> The NAND drivers that can use this parser are sharpsl.c and tmio_nand.c.\n> \n> Signed-off-by: Andrea Adami \n> ---\n\nPlease put a changelog here.\n\n> drivers/mtd/parsers/Kconfig | 7 +\n> drivers/mtd/parsers/Makefile | 1 +\n> drivers/mtd/parsers/sharpslpart.c | 382 ++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 390 insertions(+)\n> create mode 100644 drivers/mtd/parsers/sharpslpart.c\n> \n> diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig\n> index d206b3c..f371022 100644\n> --- a/drivers/mtd/parsers/Kconfig\n> +++ b/drivers/mtd/parsers/Kconfig\n> @@ -6,3 +6,10 @@ config MTD_PARSER_TRX\n> \t may contain up to 3/4 partitions (depending on the version).\n> \t This driver will parse TRX header and report at least two partitions:\n> \t kernel and rootfs.\n\nMissing blank line.\n\n> +config MTD_SHARPSL_PARTS\n> +\ttristate \"Sharp SL Series NAND flash partition parser\"\n> +\tdepends on MTD_NAND_SHARPSL || MTD_NAND_TMIO || COMPILE_TEST\n> +\thelp\n> +\t This provides the read-only FTL logic necessary to read the partition\n> +\t table from the NAND flash of Sharp SL Series (Zaurus) and the MTD\n> +\t partition parser using this code.\n> diff --git a/drivers/mtd/parsers/Makefile b/drivers/mtd/parsers/Makefile\n> index 4d9024e..5b1bcc3 100644\n> --- a/drivers/mtd/parsers/Makefile\n> +++ b/drivers/mtd/parsers/Makefile\n> @@ -1 +1,2 @@\n> obj-$(CONFIG_MTD_PARSER_TRX)\t\t+= parser_trx.o\n> +obj-$(CONFIG_MTD_SHARPSL_PARTS)\t\t+= sharpslpart.o\n> diff --git a/drivers/mtd/parsers/sharpslpart.c b/drivers/mtd/parsers/sharpslpart.c\n> new file mode 100644\n> index 0000000..b2333ae\n> --- /dev/null\n> +++ b/drivers/mtd/parsers/sharpslpart.c\n> @@ -0,0 +1,382 @@\n> +/*\n> + * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL\n> + * for logical addressing, as used on the PXA models of the SHARP SL Series.\n> + *\n> + * Copyright (C) 2017 Andrea Adami \n> + *\n> + * Based on 2.4 sources:\n> + * drivers/mtd/nand/sharp_sl_logical.c\n> + * linux/include/asm-arm/sharp_nand_logical.h\n> + *\n> + * Copyright (C) 2002 SHARP\n> + *\n> + * This program is free software; you can redistribute it and/or modify\n> + * it under the terms of the GNU General Public License as published by\n> + * the Free Software Foundation; either version 2 of the License, or\n> + * (at your option) any later version.\n> + *\n> + * This program is distributed in the hope that it will be useful,\n> + * but WITHOUT ANY WARRANTY; without even the implied warranty of\n> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n> + * GNU General Public License for more details.\n> + *\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +/* oob structure */\n> +#define NAND_NOOB_LOGADDR_00\t\t8\n> +#define NAND_NOOB_LOGADDR_01\t\t9\n> +#define NAND_NOOB_LOGADDR_10\t\t10\n> +#define NAND_NOOB_LOGADDR_11\t\t11\n> +#define NAND_NOOB_LOGADDR_20\t\t12\n> +#define NAND_NOOB_LOGADDR_21\t\t13\n> +\n> +#define BLOCK_IS_RESERVED\t\t0xffff\n> +#define BLOCK_UNMASK\t\t\t0x07fe\n> +#define BLOCK_UNMASK_COMPLEMENT\t\t1\n> +\n> +/* factory defaults */\n> +#define SHARPSL_NAND_PARTS\t\t3\n> +#define SHARPSL_FTL_PART_SIZE\t\t(7 * 1024 * 1024)\n\nNit: s/7 * 1024 * 1024/7 * SZ_1M/\n\n> +#define PARAM_BLOCK_PARTITIONINFO1\t0x00060000\n> +#define PARAM_BLOCK_PARTITIONINFO2\t0x00064000\n\nI'd rename the macro differently. The term block is a bit confusing\nhere since AFAIU the Param Area does not necessarily fit in a single\neraseblock.\n\nHow about:\n\n#define PARAM_AREA_PARTINFO1_LADDR\t0x60000\n#define PARAM_AREA_PARTINFO2_LADDR\t0x64000\n\nor simply\n\n#define SHARPSL_PARTINFO1_LADDR\t\t0x60000\n#define SHARPSL_PARTINFO2_LADDR\t\t0x64000\n\n> +\n> +#define BOOT_MAGIC\t\t\t0x424f4f54\n> +#define FSRO_MAGIC\t\t\t0x4653524f\n> +#define FSRW_MAGIC\t\t\t0x46535257\n> +\n> +/**\n> + * struct sharpsl_ftl - Sharp FTL Logical Table\n> + * @logmax:\t\tnumber of logical blocks\n> + * @log2phy:\t\tthe logical-to-physical table\n> + *\n> + * Stripped down from 2.4 sources for read-only purposes.\n\nNot sure this information is really useful, especially since you don't\nprovide a link to the 2.4 sources (or maybe I missed it).\n\nYou'd better describe what this struct is used for here.\n\n> + */\n> +struct sharpsl_ftl {\n> +\tu_int logmax;\n> +\tu_int *log2phy;\n\nCan you replace u_int by unsigned int and u_char by u8? That goes for\nthe whole file.\n\n> +};\n> +\n> +static int sharpsl_nand_read_oob(struct mtd_info *mtd, loff_t offs, size_t len,\n\nAFAICT, len is always set to mtd->oobsize. You can just drop this\nparameter and set ops->ooblen to mtd->oobsize.\n\n> +\t\t\t\t uint8_t *buf)\n> +{\n> +\tloff_t mask = mtd->writesize - 1;\n\nmask is equivalent to mtd->writesize_mask.\n\n> +\tstruct mtd_oob_ops ops;\n\n\tstruct mtd_oob_ops ops = { };\n\nso that you don't have to initialize all fields if they are set to 0 or\nNULL.\n\nYou could also directly fill this object when declaring it:\n\n\tstruct mtd_oob_ops ops = {\n\t\t.mode = MTD_OPS_PLACE_OOB,\n\t\t.ooblen = mtd->oobsize,\n\t\t.oobbuf = buf,\n\t};\n\n> +\tint ret;\n> +\n> +\tops.mode = MTD_OPS_PLACE_OOB;\n> +\tops.ooboffs = offs & mask;\n\nHm, I'm pretty sure this is wrong. mask is a page mask, not the oob\narea mask. It works because offs is always aligned on a page, but you\ncan directly assign ops.ooboffs to 0 and you should be good. \n\n> +\tops.ooblen = len;\n> +\tops.oobbuf = buf;\n> +\tops.datbuf = NULL;\n> +\n> +\tret = mtd_read_oob(mtd, offs & ~mask, &ops);\n> +\tif (ret != 0 || len != ops.oobretlen)\n> +\t\treturn -1;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/*\n> + * The logical block number assigned to a physical block is stored in the OOB\n> + * of the first page, in 3 16-bit copies with the following layout:\n> + *\n> + * 01234567 89abcdef\n> + * -------- --------\n> + * ECC BB xyxyxy\n> + *\n> + * When reading we check that the first two copies agree.\n> + * In case of error, matching is tried using the following pairs.\n> + * Reserved values 0xffff mean the block is kept for wear leveling.\n> + *\n> + * 01234567 89abcdef\n> + * -------- --------\n> + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9\n> + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11\n> + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13\n> + *\n> + */\n> +static u_int sharpsl_nand_get_logical_num(u_char *oob)\n> +{\n> +\tu16 us;\n> +\tint good0, good1;\n> +\n> +\tif (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] &&\n> +\t oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) {\n> +\t\tgood0 = NAND_NOOB_LOGADDR_00;\n> +\t\tgood1 = NAND_NOOB_LOGADDR_01;\n> +\t} else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] &&\n> +\t\t oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) {\n> +\t\tgood0 = NAND_NOOB_LOGADDR_10;\n> +\t\tgood1 = NAND_NOOB_LOGADDR_11;\n> +\t} else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] &&\n> +\t\t oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) {\n> +\t\tgood0 = NAND_NOOB_LOGADDR_20;\n> +\t\tgood1 = NAND_NOOB_LOGADDR_21;\n> +\t} else {\n> +\t\t/* wrong oob fingerprint, maybe here by mistake? */\n> +\t\treturn UINT_MAX;\n> +\t}\n> +\n> +\tus = oob[good0] | oob[good1] << 8;\n> +\n> +\t/* parity check */\n> +\tif (hweight16(us) & BLOCK_UNMASK_COMPLEMENT)\n> +\t\treturn (UINT_MAX - 1);\n\nWhy not making this function return an int and use regular error codes\nfor the parity and wrong fingerprint errors?\n\n> +\n> +\t/* reserved */\n> +\tif (us == BLOCK_IS_RESERVED)\n> +\t\treturn BLOCK_IS_RESERVED;\n> +\telse\n\nYou can drop the 'else' here.\n\n> +\t\treturn (us & BLOCK_UNMASK) >> 1;\n\nSo, this is a 10bit value, right? Why not doing the >> 1 first so that\nyour BLOCK_UNMASK mask really looks like a valid mask (0x3ff, which can\nalso be defined as GENMASK(9, 0)). \n\n> +}\n> +\n> +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size,\n\ns/sharpsl_nand_init_logical/sharpsl_nand_init_ftl/\n\npartition_size is always set to SHARPSL_FTL_PART_SIZE. Please drop this\nargument.\n\n> +\t\t\t\t struct sharpsl_ftl *ftl)\n> +{\n> +\tu_int block_num, log_num, phymax;\n> +\tloff_t block_adr;\n> +\tu_char *oob;\n> +\tint i, ret;\n> +\n> +\toob = kzalloc(mtd->oobsize, GFP_KERNEL);\n> +\tif (!oob)\n> +\t\treturn -ENOMEM;\n> +\n> +\t/* initialize management structure */\n> +\tphymax = (partition_size / mtd->erasesize);\n\nYou can use mtd_div_by_eb() here.\n\n> +\n> +\t/* FTL reserves 5% of the blocks + 1 spare */\n> +\tftl->logmax = ((phymax * 95) / 100) - 1;\n> +\n> +\tftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int),\n\n\t\t\t\t\t\t sizeof(*ftl->log2phy)\n\n> +\t\t\t\t GFP_KERNEL);\n> +\tif (!ftl->log2phy) {\n> +\t\tret = -ENOMEM;\n> +\t\tgoto exit;\n> +\t}\n> +\n> +\t/* initialize ftl->log2phy */\n> +\tfor (i = 0; i < ftl->logmax; i++)\n> +\t\tftl->log2phy[i] = UINT_MAX;\n> +\n> +\t/* create physical-logical table */\n> +\tfor (block_num = 0; block_num < phymax; block_num++) {\n> +\t\tblock_adr = block_num * mtd->erasesize;\n> +\n> +\t\tif (mtd_block_isbad(mtd, block_adr))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))\n> +\t\t\tcontinue;\n> +\n> +\t\t/* get logical block */\n> +\t\tlog_num = sharpsl_nand_get_logical_num(oob);\n> +\n> +\t\t/* FTL is not used? Exit here if the oob fingerprint is wrong */\n> +\t\tif (log_num == UINT_MAX) {\n> +\t\t\tpr_info(\"sharpslpart: Sharp SL FTL not found.\\n\");\n> +\t\t\tret = -EINVAL;\n> +\t\t\tgoto exit;\n> +\t\t}\n> +\n> +\t\t/* skip out of range and not unique values */\n> +\t\tif (log_num < ftl->logmax) {\n> +\t\t\tif (ftl->log2phy[log_num] == UINT_MAX)\n> +\t\t\t\tftl->log2phy[log_num] = block_num;\n> +\t\t}\n> +\t}\n> +\n> +\tpr_info(\"Sharp SL FTL: %d blocks used (%d logical, %d reserved)\\n\",\n> +\t\tphymax, ftl->logmax,\n> +\t\tphymax - ftl->logmax);\n> +\n> +\tret = 0;\n> +exit:\n> +\tkfree(oob);\n> +\treturn ret;\n> +}\n> +\n> +void sharpsl_nand_cleanup_logical(struct sharpsl_ftl *ftl)\n\ns/sharpsl_nand_cleanup_logical/sharpsl_nand_cleanup_ftl/\n\n> +{\n> +\tkfree(ftl->log2phy);\n> +}\n> +\n> +static int sharpsl_nand_read_laddr(struct mtd_info *mtd,\n> +\t\t\t\t loff_t from,\n> +\t\t\t\t size_t len,\n> +\t\t\t\t u_char *buf,\n> +\t\t\t\t struct sharpsl_ftl *ftl)\n> +{\n> +\tu_int log_num, log_new;\n> +\tu_int block_num;\n> +\tloff_t block_adr;\n> +\tloff_t block_ofs;\n> +\tsize_t retlen;\n> +\tint err;\n> +\n> +\tlog_num = (u32)from / mtd->erasesize;\n\nPlease use mtd_div_by_eb().\n\n> +\tlog_new = ((u32)from + len - 1) / mtd->erasesize;\n\nDitto.\n\nAnd I'm not sure log_new is a good name for this variable. Could be\nlast_log_num or something like that.\n\n> +\n> +\tif (len <= 0 || log_num >= ftl->logmax || log_new > log_num)\n> +\t\treturn -EINVAL;\n> +\n> +\tblock_num = ftl->log2phy[log_num];\n> +\tblock_adr = block_num * mtd->erasesize;\n> +\tblock_ofs = (u32)from % mtd->erasesize;\n\nmtd_mod_by_eb().\n\n> +\n> +\terr = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf);\n> +\t/* Ignore corrected ECC errors */\n> +\tif (mtd_is_bitflip(err))\n> +\t\terr = 0;\n\nPlease add a blank line here\n\n> +\tif (!err && retlen != len)\n> +\t\terr = -EIO;\n\nand here.\n\n> +\tif (err)\n> +\t\tpr_err(\"sharpslpart: error, read failed at %#llx\\n\",\n> +\t\t block_adr + block_ofs);\n> +\n> +\treturn err;\n> +}\n> +\n> +/*\n> + * MTD Partition Parser\n> + *\n> + * Sample values read from SL-C860\n> + *\n> + * # cat /proc/mtd\n> + * dev: size erasesize name\n> + * mtd0: 006d0000 00020000 \"Filesystem\"\n> + * mtd1: 00700000 00004000 \"smf\"\n> + * mtd2: 03500000 00004000 \"root\"\n> + * mtd3: 04400000 00004000 \"home\"\n> + *\n> + * PARTITIONINFO1\n> + * 0x00060000: 00 00 00 00 00 00 70 00 42 4f 4f 54 00 00 00 00 ......p.BOOT....\n> + * 0x00060010: 00 00 70 00 00 00 c0 03 46 53 52 4f 00 00 00 00 ..p.....FSRO....\n> + * 0x00060020: 00 00 c0 03 00 00 00 04 46 53 52 57 00 00 00 00 ........FSRW....\n> + * 0x00060030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................\n> + *\n\nYou can drop this extra empty line.\n\n> + */\n> +struct sharpsl_nand_partinfo {\n> +\t__le32 start;\n> +\t__le32 end;\n> +\t__be32 magic;\n> +\tu32 reserved;\n> +};\n> +\n> +static int sharpsl_nand_read_partinfo(struct mtd_info *master,\n> +\t\t\t\t loff_t from,\n> +\t\t\t\t size_t len,\n> +\t\t\t\t struct sharpsl_nand_partinfo *buf,\n> +\t\t\t\t struct sharpsl_ftl *ftl)\n> +{\n> +\tint ret;\n> +\n> +\tret = sharpsl_nand_read_laddr(master, (u32)from, len, (u_char *)buf,\n\nThe u32 cast is unneeded here, and I'd suggest to change the\nsharpsl_nand_read_laddr() prototype to take a 'void *' instead of a\n'u_char *', so that you can also get rid of this cast. \n\n> +\t\t\t\t ftl);\n> +\tif (ret)\n> +\t\tgoto exit;\n\nWell, you have nothing to cleanup in the exit path, so you can return\ndirectly here.\n\n> +\n> +\t/* check for magics */\n> +\tif (be32_to_cpu(buf[0].magic) != BOOT_MAGIC ||\n> +\t be32_to_cpu(buf[1].magic) != FSRO_MAGIC ||\n> +\t be32_to_cpu(buf[2].magic) != FSRW_MAGIC) {\n> +\t\tpr_err(\"sharpslpart: magic values mismatch\\n\");\n> +\t\tret = -EINVAL;\n> +\t\tgoto exit;\n\nDitto.\n\n> +\t}\n> +\n> +\t/* fixup for hardcoded value 64 MiB (for older models) */\n> +\tbuf[2].end = cpu_to_le32(master->size);\n> +\n> +\t/* extra sanity check */\n> +\tif (le32_to_cpu(buf[0].end) <= le32_to_cpu(buf[0].start) ||\n> +\t le32_to_cpu(buf[1].start) < le32_to_cpu(buf[0].end) ||\n> +\t le32_to_cpu(buf[1].end) <= le32_to_cpu(buf[1].start) ||\n> +\t le32_to_cpu(buf[2].start) < le32_to_cpu(buf[1].end) ||\n> +\t le32_to_cpu(buf[2].end) <= le32_to_cpu(buf[2].start)) {\n> +\t\tpr_err(\"sharpslpart: partition sizes mismatch\\n\");\n> +\t\tret = -EINVAL;\n> +\t\tgoto exit;\n\nDitto.\n\n> +\t}\n> +\n> +\tret = 0;\n> +exit:\n> +\treturn ret;\n> +}\n> +\n> +static int sharpsl_parse_mtd_partitions(struct mtd_info *master,\n> +\t\t\t\t\tconst struct mtd_partition **pparts,\n> +\t\t\t\t\tstruct mtd_part_parser_data *data)\n> +{\n> +\tstruct sharpsl_ftl ftl;\n> +\tstruct sharpsl_nand_partinfo buf[SHARPSL_NAND_PARTS];\n> +\tstruct mtd_partition *sharpsl_nand_parts;\n> +\tint err, ret;\n> +\n> +\t/* init logical mgmt (FTL) */\n> +\terr = sharpsl_nand_init_logical(master, SHARPSL_FTL_PART_SIZE, &ftl);\n> +\tif (err)\n> +\t\treturn err;\n> +\n> +\t/* read and validate first partition table */\n> +\tpr_info(\"sharpslpart: using first partition table\\n\");\n> +\terr = sharpsl_nand_read_partinfo(master,\n> +\t\t\t\t\t PARAM_BLOCK_PARTITIONINFO1,\n> +\t\t\t\t\t sizeof(buf), buf, &ftl);\n> +\tif (err) {\n> +\t\t/* fallback: read second partition table */\n> +\t\tpr_warn(\"sharpslpart: retry using second partition table\\n\");\n> +\t\tret = sharpsl_nand_read_partinfo(master,\n> +\t\t\t\t\t\t PARAM_BLOCK_PARTITIONINFO2,\n> +\t\t\t\t\t\t sizeof(buf), buf, &ftl);\n\nWhy do you need a different variable to store the second\nsharpsl_nand_read_partinfo() return code?\n\n> +\t\tif (ret) {\n> +\t\t\tpr_err(\"sharpslpart: partition tables are invalid\\n\");\n> +\t\t\tsharpsl_nand_cleanup_logical(&ftl);\n> +\t\t\treturn ret;\n> +\t\t}\n> +\t}\n> +\n> +\t/* cleanup logical mgmt (FTL) */\n> +\tsharpsl_nand_cleanup_logical(&ftl);\n\n\nHow about:\n\n\tpr_info(\"sharpslpart: try reading first partition table\\n\");\n\terr = sharpsl_nand_read_partinfo(master,\n\t\t\t\t\t PARAM_BLOCK_PARTITIONINFO1,\n\t\t\t\t\t sizeof(buf), buf, &ftl);\n\tif (err) {\n\t\t/* fallback: read second partition table */\n\t\tpr_warn(\"sharpslpart: first partition table is invalid, retry using second partition table\\n\");\n\t\terr = sharpsl_nand_read_partinfo(master,\n\t\t\t\t\t\t PARAM_BLOCK_PARTITIONINFO2,\n\t\t\t\t\t\t sizeof(buf), buf, &ftl);\n\t}\n\n\t/* cleanup logical mgmt (FTL) */\n\tsharpsl_nand_cleanup_logical(&ftl);\n\n\tif (err) {\n\t\tpr_err(\"sharpslpart: both partition tables are invalid\\n\");\n\t\treturn ret;\n\t}\n\nso that you have a single place where you cleanup the FTL.\n\n> +\n> +\tsharpsl_nand_parts = kzalloc(sizeof(*sharpsl_nand_parts) *\n> +\t\t\t\t SHARPSL_NAND_PARTS, GFP_KERNEL);\n> +\tif (!sharpsl_nand_parts)\n> +\t\treturn -ENOMEM;\n> +\n> +\t/* original names */\n> +\tsharpsl_nand_parts[0].name = \"smf\";\n> +\tsharpsl_nand_parts[0].offset = le32_to_cpu(buf[0].start);\n> +\tsharpsl_nand_parts[0].size = le32_to_cpu(buf[0].end) -\n> +\t\t\t\t le32_to_cpu(buf[0].start);\n> +\n> +\tsharpsl_nand_parts[1].name = \"root\";\n> +\tsharpsl_nand_parts[1].offset = le32_to_cpu(buf[1].start);\n> +\tsharpsl_nand_parts[1].size = le32_to_cpu(buf[1].end) -\n> +\t\t\t\t le32_to_cpu(buf[1].start);\n> +\n> +\tsharpsl_nand_parts[2].name = \"home\";\n> +\tsharpsl_nand_parts[2].offset = le32_to_cpu(buf[2].start);\n> +\tsharpsl_nand_parts[2].size = le32_to_cpu(buf[2].end) -\n> +\t\t\t\t le32_to_cpu(buf[2].start);\n> +\n> +\t*pparts = sharpsl_nand_parts;\n> +\treturn SHARPSL_NAND_PARTS;\n> +}\n> +\n> +static struct mtd_part_parser sharpsl_mtd_parser = {\n> +\t.parse_fn = sharpsl_parse_mtd_partitions,\n> +\t.name = \"sharpslpart\",\n> +};\n> +module_mtd_part_parser(sharpsl_mtd_parser);\n> +\n> +MODULE_LICENSE(\"GPL\");\n> +MODULE_AUTHOR(\"Andrea Adami \");\n> +MODULE_DESCRIPTION(\"MTD partitioning for NAND flash on Sharp SL Series\");","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (mailfrom)\n\tsmtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133;\n\thelo=bombadil.infradead.org;\n\tenvelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"VV3dtsxA\"; \n\tdkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xc9X13VZnz9t24\n\tfor ;\n\tTue, 22 Aug 2017 22:55:29 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dk8i7-0005de-QR; Tue, 22 Aug 2017 12:55:11 +0000","from mail.free-electrons.com ([62.4.15.54])\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dk8hy-0003im-W5\n\tfor linux-mtd@lists.infradead.org; Tue, 22 Aug 2017 12:55:10 +0000","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 1AC352096B; Tue, 22 Aug 2017 14:54:36 +0200 (CEST)","from bbrezillon (91-160-177-164.subs.proxad.net [91.160.177.164])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id B59E020789;\n\tTue, 22 Aug 2017 14:54:25 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:\n\tMessage-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=WJ8JzbkcUqKzcs9aHgkbzGV01jtYpVZ9rzTUuWMh8ic=;\n\tb=VV3dtsxA+DydB8\n\tCSRRa7w77UXXvW2oPhBiw5MD+oscSqmwkrOCSdT2W7oyKiTl/1HOLX6APxT4B+yBzIftazbXjjQkG\n\tihzO1qLEkwcjEL/ou9AuZD1L7L9au0O+QmyZNN/IoNDhV0o/UAlBkpqNpQ3ckftU5pcpBOp8kGKzh\n\tp3jrDbb4AJ1UfPo6NokYJlwtUtBV/cfv+H6fAgUp2nuCkaXofO8Mip1TCzq+t7Se1VEjQgph0Uw+I\n\tVLEhmFwG57YyGCCppcNJWnFePrLMzxsNuMd8PJGRrE2tTyChDANStoYJiunY/lQKf0ycx+svXoqeh\n\tDCEtZpaoQSOBdpH81Neg==;","X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT\n\tshortcircuit=ham autolearn=disabled version=3.4.0","Date":"Tue, 22 Aug 2017 14:54:24 +0200","From":"Boris Brezillon ","To":"Andrea Adami ","Subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","Message-ID":"<20170822145424.54e57b94@bbrezillon>","In-Reply-To":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>","References":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>","X-Mailer":"Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu)","MIME-Version":"1.0","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170822_055503_489528_FB42563D ","X-CRM114-Status":"GOOD ( 42.62 )","X-Spam-Score":"-1.9 (-)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-1.9 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay\n\tdomain\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]","X-BeenThere":"linux-mtd@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"Linux MTD discussion mailing list ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dmitry Eremin-Solenikov ,\n\tRichard Weinberger ,\n\tRobert Jarzmik , linux-kernel@vger.kernel.org,\n\tHaojian Zhuang , \n\tMarek Vasut , linux-mtd@lists.infradead.org,\n\tCyrille Pitchen ,\n\t=?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= ,\n\tBrian Norris ,\n\tDavid Woodhouse ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Sender":"\"linux-mtd\" ","Errors-To":"linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org"}},{"id":1756100,"web_url":"http://patchwork.ozlabs.org/comment/1756100/","msgid":"","list_archive_url":null,"date":"2017-08-24T09:19:56","subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","submitter":{"id":30311,"url":"http://patchwork.ozlabs.org/api/people/30311/","name":"Andrea Adami","email":"andrea.adami@gmail.com"},"content":"Boris,\n\nthanks for the review.\nI have made the required changes apart th elast one: error values to\nbe returned..\nPlease see my comments.\n\nOn Tue, Aug 22, 2017 at 2:54 PM, Boris Brezillon\n wrote:\n> Hi Andrea,\n>\n> Le Tue, 22 Aug 2017 11:42:52 +0200,\n> Andrea Adami a écrit :\n>\n>> The Sharp SL Series (Zaurus) PXA handhelds have 16/64/128M of NAND flash\n>> and share the same layout of the first 7M partition, managed by Sharp FTL.\n>>\n>> The purpose of this self-contained patch is to add a common parser and\n>> remove the hardcoded sizes in the board files (these devices are not yet\n>> converted to devicetree).\n>> Users will have benefits because the mtdparts= tag will not be necessary\n>> anymore and they will be free to repartition the little sized flash.\n>>\n>> The obsolete bootloader can not pass the partitioning info to modern\n>> kernels anymore so it has to be read from flash at known logical addresses.\n>> (see http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm )\n>>\n>> In kernel, under arch/arm/mach-pxa we have already 8 machines:\n>> MACH_POODLE, MACH_CORGI, MACH_SHEPERD, MACH_HUSKY, MACH_AKITA, MACH_SPITZ,\n>> MACH_BORZOI, MACH_TOSA.\n>> Lost after the 2.4 vendor kernel are MACH_BOXER and MACH_TERRIER.\n>>\n>> Almost every model has different factory partitioning: add to this the\n>> units can be repartitioned by users with userspace tools (nandlogical)\n>> and installers for popular (back then) linux distributions.\n>>\n>> The Parameter Area in the first (boot) partition extends from 0x00040000 to\n>> 0x0007bfff (176k) and contains two copies of the partition table:\n>> ...\n>> 0x00060000: Partition Info1 16k\n>> 0x00064000: Partition Info2 16k\n>> 0x00668000: Model 16k\n>> ...\n>>\n>> The first 7M partition is managed by the Sharp FTL reserving 5% + 1 blocks\n>> for wear-leveling: some blocks are remapped and one layer of translation\n>> (logical to physical) is necessary.\n>>\n>> There isn't much documentation about this FTL in the 2.4 sources, just the\n>> MTD methods for reading and writing using logical addresses and the block\n>> management (wear-leveling, use counter).\n>> For the purpose of the MTD parser only the read part of the code was taken.\n>>\n>> The NAND drivers that can use this parser are sharpsl.c and tmio_nand.c.\n>>\n>> Signed-off-by: Andrea Adami \n>> ---\n>\n> Please put a changelog here.\nSure, I'll take the one from the cover-letter.\n\n>\n>> drivers/mtd/parsers/Kconfig | 7 +\n>> drivers/mtd/parsers/Makefile | 1 +\n>> drivers/mtd/parsers/sharpslpart.c | 382 ++++++++++++++++++++++++++++++++++++++\n>> 3 files changed, 390 insertions(+)\n>> create mode 100644 drivers/mtd/parsers/sharpslpart.c\n>>\n>> diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig\n>> index d206b3c..f371022 100644\n>> --- a/drivers/mtd/parsers/Kconfig\n>> +++ b/drivers/mtd/parsers/Kconfig\n>> @@ -6,3 +6,10 @@ config MTD_PARSER_TRX\n>> may contain up to 3/4 partitions (depending on the version).\n>> This driver will parse TRX header and report at least two partitions:\n>> kernel and rootfs.\n>\n> Missing blank line.\nFixed\n\n>\n>> +config MTD_SHARPSL_PARTS\n>> + tristate \"Sharp SL Series NAND flash partition parser\"\n>> + depends on MTD_NAND_SHARPSL || MTD_NAND_TMIO || COMPILE_TEST\n>> + help\n>> + This provides the read-only FTL logic necessary to read the partition\n>> + table from the NAND flash of Sharp SL Series (Zaurus) and the MTD\n>> + partition parser using this code.\n>> diff --git a/drivers/mtd/parsers/Makefile b/drivers/mtd/parsers/Makefile\n>> index 4d9024e..5b1bcc3 100644\n>> --- a/drivers/mtd/parsers/Makefile\n>> +++ b/drivers/mtd/parsers/Makefile\n>> @@ -1 +1,2 @@\n>> obj-$(CONFIG_MTD_PARSER_TRX) += parser_trx.o\n>> +obj-$(CONFIG_MTD_SHARPSL_PARTS) += sharpslpart.o\n>> diff --git a/drivers/mtd/parsers/sharpslpart.c b/drivers/mtd/parsers/sharpslpart.c\n>> new file mode 100644\n>> index 0000000..b2333ae\n>> --- /dev/null\n>> +++ b/drivers/mtd/parsers/sharpslpart.c\n>> @@ -0,0 +1,382 @@\n>> +/*\n>> + * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL\n>> + * for logical addressing, as used on the PXA models of the SHARP SL Series.\n>> + *\n>> + * Copyright (C) 2017 Andrea Adami \n>> + *\n>> + * Based on 2.4 sources:\n>> + * drivers/mtd/nand/sharp_sl_logical.c\n>> + * linux/include/asm-arm/sharp_nand_logical.h\n>> + *\n>> + * Copyright (C) 2002 SHARP\n>> + *\n>> + * This program is free software; you can redistribute it and/or modify\n>> + * it under the terms of the GNU General Public License as published by\n>> + * the Free Software Foundation; either version 2 of the License, or\n>> + * (at your option) any later version.\n>> + *\n>> + * This program is distributed in the hope that it will be useful,\n>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of\n>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n>> + * GNU General Public License for more details.\n>> + *\n>> + */\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +/* oob structure */\n>> +#define NAND_NOOB_LOGADDR_00 8\n>> +#define NAND_NOOB_LOGADDR_01 9\n>> +#define NAND_NOOB_LOGADDR_10 10\n>> +#define NAND_NOOB_LOGADDR_11 11\n>> +#define NAND_NOOB_LOGADDR_20 12\n>> +#define NAND_NOOB_LOGADDR_21 13\n>> +\n>> +#define BLOCK_IS_RESERVED 0xffff\n>> +#define BLOCK_UNMASK 0x07fe\n>> +#define BLOCK_UNMASK_COMPLEMENT 1\n>> +\n>> +/* factory defaults */\n>> +#define SHARPSL_NAND_PARTS 3\n>> +#define SHARPSL_FTL_PART_SIZE (7 * 1024 * 1024)\n>\n> Nit: s/7 * 1024 * 1024/7 * SZ_1M/\nFixed\n\n>\n>> +#define PARAM_BLOCK_PARTITIONINFO1 0x00060000\n>> +#define PARAM_BLOCK_PARTITIONINFO2 0x00064000\n>\n> I'd rename the macro differently. The term block is a bit confusing\n> here since AFAIU the Param Area does not necessarily fit in a single\n> eraseblock.\n>\n> How about:\n>\n> #define PARAM_AREA_PARTINFO1_LADDR 0x60000\n> #define PARAM_AREA_PARTINFO2_LADDR 0x64000\n>\n> or simply\n>\n> #define SHARPSL_PARTINFO1_LADDR 0x60000\n> #define SHARPSL_PARTINFO2_LADDR 0x64000\nUsed these, nicer\n\n>\n>> +\n>> +#define BOOT_MAGIC 0x424f4f54\n>> +#define FSRO_MAGIC 0x4653524f\n>> +#define FSRW_MAGIC 0x46535257\n>> +\n>> +/**\n>> + * struct sharpsl_ftl - Sharp FTL Logical Table\n>> + * @logmax: number of logical blocks\n>> + * @log2phy: the logical-to-physical table\n>> + *\n>> + * Stripped down from 2.4 sources for read-only purposes.\n>\n> Not sure this information is really useful, especially since you don't\n> provide a link to the 2.4 sources (or maybe I missed it).\n\nMaybe here I can add that this FTL was originally tailored for devices\n16KiB erasesize matching the size of the PARAM_BLOCK_ above thus using\ntwo separate blocks for the partition tables. Should I add an\nASCII-art? Pls see\nhttp://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm\nBTW the link was in the cover-letter 0/9, I'll put it here together\nwith the changelog.\n\n>\n> You'd better describe what this struct is used for here.\nSeems self-explanatory..2 fields commented. What would you add here?\n\n>\n>> + */\n>> +struct sharpsl_ftl {\n>> + u_int logmax;\n>> + u_int *log2phy;\n>\n> Can you replace u_int by unsigned int and u_char by u8? That goes for\n> the whole file.\nYes, done\n\n>\n>> +};\n>> +\n>> +static int sharpsl_nand_read_oob(struct mtd_info *mtd, loff_t offs, size_t len,\n>\n> AFAICT, len is always set to mtd->oobsize. You can just drop this\n> parameter and set ops->ooblen to mtd->oobsize.\n>\n>> + uint8_t *buf)\n>> +{\n>> + loff_t mask = mtd->writesize - 1;\n>\n> mask is equivalent to mtd->writesize_mask.\n>\n>> + struct mtd_oob_ops ops;\n>\n> struct mtd_oob_ops ops = { };\n>\n> so that you don't have to initialize all fields if they are set to 0 or\n> NULL.\n>\nOk done, I prefer this syntax, like in the other ftl using read_oob().\n\n> You could also directly fill this object when declaring it:\n>\n> struct mtd_oob_ops ops = {\n> .mode = MTD_OPS_PLACE_OOB,\n> .ooblen = mtd->oobsize,\n> .oobbuf = buf,\n> };\n>\n>> + int ret;\n>> +\n>> + ops.mode = MTD_OPS_PLACE_OOB;\n>> + ops.ooboffs = offs & mask;\n>\n> Hm, I'm pretty sure this is wrong. mask is a page mask, not the oob\n> area mask. It works because offs is always aligned on a page, but you\n> can directly assign ops.ooboffs to 0 and you should be good.\n>\nYou are right, I removed the mask alltogether.\n\n>> + ops.ooblen = len;\n>> + ops.oobbuf = buf;\n>> + ops.datbuf = NULL;\n>> +\n>> + ret = mtd_read_oob(mtd, offs & ~mask, &ops);\n>> + if (ret != 0 || len != ops.oobretlen)\n>> + return -1;\n>> +\n>> + return 0;\n>> +}\n>> +\n>> +/*\n>> + * The logical block number assigned to a physical block is stored in the OOB\n>> + * of the first page, in 3 16-bit copies with the following layout:\n>> + *\n>> + * 01234567 89abcdef\n>> + * -------- --------\n>> + * ECC BB xyxyxy\n>> + *\n>> + * When reading we check that the first two copies agree.\n>> + * In case of error, matching is tried using the following pairs.\n>> + * Reserved values 0xffff mean the block is kept for wear leveling.\n>> + *\n>> + * 01234567 89abcdef\n>> + * -------- --------\n>> + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9\n>> + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11\n>> + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13\n>> + *\n>> + */\n>> +static u_int sharpsl_nand_get_logical_num(u_char *oob)\n>> +{\n>> + u16 us;\n>> + int good0, good1;\n>> +\n>> + if (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] &&\n>> + oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) {\n>> + good0 = NAND_NOOB_LOGADDR_00;\n>> + good1 = NAND_NOOB_LOGADDR_01;\n>> + } else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] &&\n>> + oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) {\n>> + good0 = NAND_NOOB_LOGADDR_10;\n>> + good1 = NAND_NOOB_LOGADDR_11;\n>> + } else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] &&\n>> + oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) {\n>> + good0 = NAND_NOOB_LOGADDR_20;\n>> + good1 = NAND_NOOB_LOGADDR_21;\n>> + } else {\n>> + /* wrong oob fingerprint, maybe here by mistake? */\n>> + return UINT_MAX;\n>> + }\n>> +\n>> + us = oob[good0] | oob[good1] << 8;\n>> +\n>> + /* parity check */\n>> + if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT)\n>> + return (UINT_MAX - 1);\n>\n> Why not making this function return an int and use regular error codes\n> for the parity and wrong fingerprint errors?\n>\nOriginally in case of error it did return the largest values.\nI can return a negative int but then I'd have to check for log_no >0\n\nBesides, what are the 'regular' error codes you'd use here?\nFor the missing FTL we do return -EINVAL later so I'd say this is the\nerror for the wrong oob fingerprint.\n\nAbout the parity error, it does return UINT_MAX -1 so to allow very\nlarge log_num. This value is always bigger than log_max so it is\nskipped in the actual code but the read continues.\nShould we change the philosophy of the old 2.4 code and exit in case\nof parity errors?\n\n>> +\n>> + /* reserved */\n>> + if (us == BLOCK_IS_RESERVED)\n>> + return BLOCK_IS_RESERVED;\n>> + else\n>\n> You can drop the 'else' here.\nDone\n>\n>> + return (us & BLOCK_UNMASK) >> 1;\n>\n> So, this is a 10bit value, right? Why not doing the >> 1 first so that\n> your BLOCK_UNMASK mask really looks like a valid mask (0x3ff, which can\n> also be defined as GENMASK(9, 0)).\nRight, very nice. Done.\nCan I keep BLOCK_UNMASK COMPLEMENT = 1 ?\n\n>\n>> +}\n>> +\n>> +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size,\n>\n> s/sharpsl_nand_init_logical/sharpsl_nand_init_ftl/\nOh yes, renamed\n\n>\n> partition_size is always set to SHARPSL_FTL_PART_SIZE. Please drop this\n> argument.\n>\nOk, done\n\n>> + struct sharpsl_ftl *ftl)\n>> +{\n>> + u_int block_num, log_num, phymax;\n>> + loff_t block_adr;\n>> + u_char *oob;\n>> + int i, ret;\n>> +\n>> + oob = kzalloc(mtd->oobsize, GFP_KERNEL);\n>> + if (!oob)\n>> + return -ENOMEM;\n>> +\n>> + /* initialize management structure */\n>> + phymax = (partition_size / mtd->erasesize);\n>\n> You can use mtd_div_by_eb() here.\n>\nDone for all occurrences\n\n>> +\n>> + /* FTL reserves 5% of the blocks + 1 spare */\n>> + ftl->logmax = ((phymax * 95) / 100) - 1;\n>> +\n>> + ftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int),\n>\n> sizeof(*ftl->log2phy)\nOk, changed.\n\n>\n>> + GFP_KERNEL);\n>> + if (!ftl->log2phy) {\n>> + ret = -ENOMEM;\n>> + goto exit;\n>> + }\n>> +\n>> + /* initialize ftl->log2phy */\n>> + for (i = 0; i < ftl->logmax; i++)\n>> + ftl->log2phy[i] = UINT_MAX;\n>> +\n>> + /* create physical-logical table */\n>> + for (block_num = 0; block_num < phymax; block_num++) {\n>> + block_adr = block_num * mtd->erasesize;\n>> +\n>> + if (mtd_block_isbad(mtd, block_adr))\n>> + continue;\n>> +\n>> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))\n>> + continue;\n>> +\n>> + /* get logical block */\n>> + log_num = sharpsl_nand_get_logical_num(oob);\n>> +\n>> + /* FTL is not used? Exit here if the oob fingerprint is wrong */\n>> + if (log_num == UINT_MAX) {\n>> + pr_info(\"sharpslpart: Sharp SL FTL not found.\\n\");\n>> + ret = -EINVAL;\n>> + goto exit;\n>> + }\n>> +\n>> + /* skip out of range and not unique values */\n>> + if (log_num < ftl->logmax) {\n>> + if (ftl->log2phy[log_num] == UINT_MAX)\n>> + ftl->log2phy[log_num] = block_num;\n>> + }\n>> + }\n>> +\n>> + pr_info(\"Sharp SL FTL: %d blocks used (%d logical, %d reserved)\\n\",\n>> + phymax, ftl->logmax,\n>> + phymax - ftl->logmax);\n>> +\n>> + ret = 0;\n>> +exit:\n>> + kfree(oob);\n>> + return ret;\n>> +}\n>> +\n>> +void sharpsl_nand_cleanup_logical(struct sharpsl_ftl *ftl)\n>\n> s/sharpsl_nand_cleanup_logical/sharpsl_nand_cleanup_ftl/\n>\nYes, renamed as well\n\n>> +{\n>> + kfree(ftl->log2phy);\n>> +}\n>> +\n>> +static int sharpsl_nand_read_laddr(struct mtd_info *mtd,\n>> + loff_t from,\n>> + size_t len,\n>> + u_char *buf,\n>> + struct sharpsl_ftl *ftl)\n>> +{\n>> + u_int log_num, log_new;\n>> + u_int block_num;\n>> + loff_t block_adr;\n>> + loff_t block_ofs;\n>> + size_t retlen;\n>> + int err;\n>> +\n>> + log_num = (u32)from / mtd->erasesize;\n>\n> Please use mtd_div_by_eb().\n>\n>> + log_new = ((u32)from + len - 1) / mtd->erasesize;\n>\n> Ditto.\n>\n> And I'm not sure log_new is a good name for this variable. Could be\n> last_log_num or something like that.\n>\n>> +\n>> + if (len <= 0 || log_num >= ftl->logmax || log_new > log_num)\n>> + return -EINVAL;\n>> +\n>> + block_num = ftl->log2phy[log_num];\n>> + block_adr = block_num * mtd->erasesize;\n>> + block_ofs = (u32)from % mtd->erasesize;\n>\n> mtd_mod_by_eb().\n>\n>> +\n>> + err = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf);\n>> + /* Ignore corrected ECC errors */\n>> + if (mtd_is_bitflip(err))\n>> + err = 0;\n>\n> Please add a blank line here\n>\nOk\n\n>> + if (!err && retlen != len)\n>> + err = -EIO;\n>\n> and here.\n>\nOk (was taken from mtdtest_read() fwiw)\n\n>> + if (err)\n>> + pr_err(\"sharpslpart: error, read failed at %#llx\\n\",\n>> + block_adr + block_ofs);\n>> +\n>> + return err;\n>> +}\n>> +\n>> +/*\n>> + * MTD Partition Parser\n>> + *\n>> + * Sample values read from SL-C860\n>> + *\n>> + * # cat /proc/mtd\n>> + * dev: size erasesize name\n>> + * mtd0: 006d0000 00020000 \"Filesystem\"\n>> + * mtd1: 00700000 00004000 \"smf\"\n>> + * mtd2: 03500000 00004000 \"root\"\n>> + * mtd3: 04400000 00004000 \"home\"\n>> + *\n>> + * PARTITIONINFO1\n>> + * 0x00060000: 00 00 00 00 00 00 70 00 42 4f 4f 54 00 00 00 00 ......p.BOOT....\n>> + * 0x00060010: 00 00 70 00 00 00 c0 03 46 53 52 4f 00 00 00 00 ..p.....FSRO....\n>> + * 0x00060020: 00 00 c0 03 00 00 00 04 46 53 52 57 00 00 00 00 ........FSRW....\n>> + * 0x00060030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................\n>> + *\n>\n> You can drop this extra empty line.\nOk\n\n>\n>> + */\n>> +struct sharpsl_nand_partinfo {\n>> + __le32 start;\n>> + __le32 end;\n>> + __be32 magic;\n>> + u32 reserved;\n>> +};\n>> +\n>> +static int sharpsl_nand_read_partinfo(struct mtd_info *master,\n>> + loff_t from,\n>> + size_t len,\n>> + struct sharpsl_nand_partinfo *buf,\n>> + struct sharpsl_ftl *ftl)\n>> +{\n>> + int ret;\n>> +\n>> + ret = sharpsl_nand_read_laddr(master, (u32)from, len, (u_char *)buf,\n>\n> The u32 cast is unneeded here, and I'd suggest to change the\n> sharpsl_nand_read_laddr() prototype to take a 'void *' instead of a\n> 'u_char *', so that you can also get rid of this cast.\n>\nNice, done\n\n>> + ftl);\n>> + if (ret)\n>> + goto exit;\n>\n> Well, you have nothing to cleanup in the exit path, so you can return\n> directly here.\n>\nAh, I have exaggerated here :) Done all three\n\n>> +\n>> + /* check for magics */\n>> + if (be32_to_cpu(buf[0].magic) != BOOT_MAGIC ||\n>> + be32_to_cpu(buf[1].magic) != FSRO_MAGIC ||\n>> + be32_to_cpu(buf[2].magic) != FSRW_MAGIC) {\n>> + pr_err(\"sharpslpart: magic values mismatch\\n\");\n>> + ret = -EINVAL;\n>> + goto exit;\n>\n> Ditto.\n>\n>> + }\n>> +\n>> + /* fixup for hardcoded value 64 MiB (for older models) */\n>> + buf[2].end = cpu_to_le32(master->size);\n>> +\n>> + /* extra sanity check */\n>> + if (le32_to_cpu(buf[0].end) <= le32_to_cpu(buf[0].start) ||\n>> + le32_to_cpu(buf[1].start) < le32_to_cpu(buf[0].end) ||\n>> + le32_to_cpu(buf[1].end) <= le32_to_cpu(buf[1].start) ||\n>> + le32_to_cpu(buf[2].start) < le32_to_cpu(buf[1].end) ||\n>> + le32_to_cpu(buf[2].end) <= le32_to_cpu(buf[2].start)) {\n>> + pr_err(\"sharpslpart: partition sizes mismatch\\n\");\n>> + ret = -EINVAL;\n>> + goto exit;\n>\n> Ditto.\n>\n>> + }\n>> +\n>> + ret = 0;\n>> +exit:\n>> + return ret;\n>> +}\n>> +\n>> +static int sharpsl_parse_mtd_partitions(struct mtd_info *master,\n>> + const struct mtd_partition **pparts,\n>> + struct mtd_part_parser_data *data)\n>> +{\n>> + struct sharpsl_ftl ftl;\n>> + struct sharpsl_nand_partinfo buf[SHARPSL_NAND_PARTS];\n>> + struct mtd_partition *sharpsl_nand_parts;\n>> + int err, ret;\n>> +\n>> + /* init logical mgmt (FTL) */\n>> + err = sharpsl_nand_init_logical(master, SHARPSL_FTL_PART_SIZE, &ftl);\n>> + if (err)\n>> + return err;\n>> +\n>> + /* read and validate first partition table */\n>> + pr_info(\"sharpslpart: using first partition table\\n\");\n>> + err = sharpsl_nand_read_partinfo(master,\n>> + PARAM_BLOCK_PARTITIONINFO1,\n>> + sizeof(buf), buf, &ftl);\n>> + if (err) {\n>> + /* fallback: read second partition table */\n>> + pr_warn(\"sharpslpart: retry using second partition table\\n\");\n>> + ret = sharpsl_nand_read_partinfo(master,\n>> + PARAM_BLOCK_PARTITIONINFO2,\n>> + sizeof(buf), buf, &ftl);\n>\n> Why do you need a different variable to store the second\n> sharpsl_nand_read_partinfo() return code?\n>\nIt is unneeded, is a copy&paste remnant. Fixed.\n\n>> + if (ret) {\n>> + pr_err(\"sharpslpart: partition tables are invalid\\n\");\n>> + sharpsl_nand_cleanup_logical(&ftl);\n>> + return ret;\n>> + }\n>> + }\n>> +\n>> + /* cleanup logical mgmt (FTL) */\n>> + sharpsl_nand_cleanup_logical(&ftl);\n>\n>\n> How about:\n>\n> pr_info(\"sharpslpart: try reading first partition table\\n\");\n> err = sharpsl_nand_read_partinfo(master,\n> PARAM_BLOCK_PARTITIONINFO1,\n> sizeof(buf), buf, &ftl);\n> if (err) {\n> /* fallback: read second partition table */\n> pr_warn(\"sharpslpart: first partition table is invalid, retry using second partition table\\n\");\n> err = sharpsl_nand_read_partinfo(master,\n> PARAM_BLOCK_PARTITIONINFO2,\n> sizeof(buf), buf, &ftl);\n> }\n>\n> /* cleanup logical mgmt (FTL) */\n> sharpsl_nand_cleanup_logical(&ftl);\n>\n> if (err) {\n> pr_err(\"sharpslpart: both partition tables are invalid\\n\");\n> return ret;\n> }\n>\n> so that you have a single place where you cleanup the FTL.\nVery nice. I was mumbling around it...\nDone\n\n>\n>> +\n>> + sharpsl_nand_parts = kzalloc(sizeof(*sharpsl_nand_parts) *\n>> + SHARPSL_NAND_PARTS, GFP_KERNEL);\n>> + if (!sharpsl_nand_parts)\n>> + return -ENOMEM;\n>> +\n>> + /* original names */\n>> + sharpsl_nand_parts[0].name = \"smf\";\n>> + sharpsl_nand_parts[0].offset = le32_to_cpu(buf[0].start);\n>> + sharpsl_nand_parts[0].size = le32_to_cpu(buf[0].end) -\n>> + le32_to_cpu(buf[0].start);\n>> +\n>> + sharpsl_nand_parts[1].name = \"root\";\n>> + sharpsl_nand_parts[1].offset = le32_to_cpu(buf[1].start);\n>> + sharpsl_nand_parts[1].size = le32_to_cpu(buf[1].end) -\n>> + le32_to_cpu(buf[1].start);\n>> +\n>> + sharpsl_nand_parts[2].name = \"home\";\n>> + sharpsl_nand_parts[2].offset = le32_to_cpu(buf[2].start);\n>> + sharpsl_nand_parts[2].size = le32_to_cpu(buf[2].end) -\n>> + le32_to_cpu(buf[2].start);\n>> +\n>> + *pparts = sharpsl_nand_parts;\n>> + return SHARPSL_NAND_PARTS;\n>> +}\n>> +\n>> +static struct mtd_part_parser sharpsl_mtd_parser = {\n>> + .parse_fn = sharpsl_parse_mtd_partitions,\n>> + .name = \"sharpslpart\",\n>> +};\n>> +module_mtd_part_parser(sharpsl_mtd_parser);\n>> +\n>> +MODULE_LICENSE(\"GPL\");\n>> +MODULE_AUTHOR(\"Andrea Adami \");\n>> +MODULE_DESCRIPTION(\"MTD partitioning for NAND flash on Sharp SL Series\");\n>\nThank you very much. Code is getting better and better.\nAndrea","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (mailfrom)\n\tsmtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133;\n\thelo=bombadil.infradead.org;\n\tenvelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"OPA4gSZi\"; \n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"L4rJyu2a\"; dkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xdJgT07WTz9sPk\n\tfor ;\n\tThu, 24 Aug 2017 19:20:53 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dkoJg-0007ov-5r; Thu, 24 Aug 2017 09:20:44 +0000","from mail-wm0-x229.google.com ([2a00:1450:400c:c09::229])\n\tby bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dkoJH-0006Mx-NW\n\tfor linux-mtd@lists.infradead.org; Thu, 24 Aug 2017 09:20:23 +0000","by mail-wm0-x229.google.com with SMTP id m207so516073wma.1\n\tfor ;\n\tThu, 24 Aug 2017 02:19:59 -0700 (PDT)","by 10.80.226.77 with HTTP; Thu, 24 Aug 2017 02:19:56 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:\n\tReferences:In-Reply-To:MIME-Version:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=xtjOoO5bvN23LXIVqlTnzZS+Z4YOUhKAPKMeBoRKHl4=;\n\tb=OPA4gSZiCJ1soy\n\tSx2fytVuUOk1jcmf9DooWDBOKXmI0KTJdPJxMk9MLYqS5IBxeze4s/1Jh45X4BlKm7QIEm6yES9bq\n\tcHp5jwj//FKfpKFz80th8dCtw/l5CO9im8gD7BrPxoqUvdmuk9rPiFvB5d1kkxrj4tQZpVZ9WOn7m\n\ttVF1fZys+tZuhPL7edUhMsaKhcGuiJFzsbSMUl13bar5S+3WB4Xu2/uXgBEDWsCSwjaLXCMvNW5j8\n\tO7ewz+oMY6MsWH1uMCoKJ4ZAqRQE40X1yOYmEftKQhJPOhc2zuBPTgr0cPwVsQrG9iTBI4T6j5p2G\n\t10FSoF4hkLXtzvXbIzpw==;","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=lNupiCxg+BJkrM7LinE5epDWB6VlKNZ7RilHWITIeg8=;\n\tb=L4rJyu2aYWBy9Zn6osN3i+9jT6ffR3RZa5SChpSEiUwoFQnwpT27mvzkpPHTigcVpp\n\t11MS79fNFYVNrPsGKneXYyCNFIN2EbSh3yUW8xDrK634Y4Io2w9DL2bSzGZSmzIDfkLf\n\teWANYRjLBKM+WPLSpsJ4dWFzZpfOkygBNkTd79Xm7Pm93rB/IFI1Xx8N5KCH2oEyOdB5\n\tfgq2/RCyK0yLlB3NHJE5FXrvWTghaO4uxBUGZincXGtmOUTL1gqJHA1OY9YtYONPWPU9\n\tKBYpeJY7nw8jDvm9hPmYu2tf/2mprSt6ETIkb0B6ZzvrQCvxcxLrRYBF7pVh5J9YJk58\n\tvf7g=="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=lNupiCxg+BJkrM7LinE5epDWB6VlKNZ7RilHWITIeg8=;\n\tb=biMPqryeMdhtbOgCrveQPD0ysvqohOhdmq6BI3ciOd9hcBvaDb2MpPnFETFzLyX1Mh\n\tYye6S0ILtC30+ketof5UYTHbgWEhJ0iuvZctAeYGPgeKf6IhQxEqlHW7LRP3bwuNS0Qn\n\tGLYCWkQl8ZmU18I/rW/sIHQDPnxKUVkasEmzZ8DSQo1VKwaXnZauXlcfglMiqWq0cGAy\n\tBvUNXODZ7vbEqWO9wX4TAz6jJ0pN9D3XaU+C7Xu+jWnR2kr7fH3LXF04GfuGONb1sC3W\n\t6RMOliUIEGjRVcOUVrzeZlXgssbmEu/CmUMtp2PfbAt+ccZUKl6HmO8iinuHtbi5ztHI\n\t0jhw==","X-Gm-Message-State":"AHYfb5hbjce7EMBX2AqXTSc/vZgI0fyxBfe8Z0RhSDbsR+vQG7sXD1tf\n\teHTJVXT0XffsI+tI1mYIOExK3AutyQ==","X-Received":"by 10.80.169.193 with SMTP id n59mr5791045edc.88.1503566397269; \n\tThu, 24 Aug 2017 02:19:57 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170822145424.54e57b94@bbrezillon>","References":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>\n\t<20170822145424.54e57b94@bbrezillon>","From":"Andrea Adami ","Date":"Thu, 24 Aug 2017 11:19:56 +0200","Message-ID":"","Subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","To":"Boris Brezillon ","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170824_022020_126674_228973FB ","X-CRM114-Status":"GOOD ( 43.40 )","X-Spam-Score":"-2.0 (--)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-2.0 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\n\tprovider (andrea.adami[at]gmail.com)\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]\n\t-0.1 DKIM_VALID Message has at least one valid DKIM or DK signature\n\t0.1 DKIM_SIGNED Message has a DKIM or DK signature,\n\tnot necessarily valid\n\t-0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from\n\tauthor's domain","X-BeenThere":"linux-mtd@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"Linux MTD discussion mailing list ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dmitry Eremin-Solenikov ,\n\tRichard Weinberger ,\n\tRobert Jarzmik , linux-kernel@vger.kernel.org,\n\tHaojian Zhuang , \n\tMarek Vasut , linux-mtd@lists.infradead.org,\n\tCyrille Pitchen ,\n\t=?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= ,\n\tBrian Norris ,\n\tDavid Woodhouse ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Sender":"\"linux-mtd\" ","Errors-To":"linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org"}},{"id":1756177,"web_url":"http://patchwork.ozlabs.org/comment/1756177/","msgid":"<20170824120428.46e266c7@bbrezillon>","list_archive_url":null,"date":"2017-08-24T10:04:28","subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","submitter":{"id":63120,"url":"http://patchwork.ozlabs.org/api/people/63120/","name":"Boris Brezillon","email":"boris.brezillon@free-electrons.com"},"content":"On Thu, 24 Aug 2017 11:19:56 +0200\nAndrea Adami wrote:\n\n> >> +/**\n> >> + * struct sharpsl_ftl - Sharp FTL Logical Table\n> >> + * @logmax: number of logical blocks\n> >> + * @log2phy: the logical-to-physical table\n> >> + *\n> >> + * Stripped down from 2.4 sources for read-only purposes. \n> >\n> > Not sure this information is really useful, especially since you don't\n> > provide a link to the 2.4 sources (or maybe I missed it). \n> \n> Maybe here I can add that this FTL was originally tailored for devices\n> 16KiB erasesize matching the size of the PARAM_BLOCK_ above thus using\n> two separate blocks for the partition tables. Should I add an\n> ASCII-art?\n\nYou can add an ASCII-art if you want but that's not mandatory if you\ncan explain it with words :-).\n\n> Pls see\n> http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm\n> BTW the link was in the cover-letter 0/9, I'll put it here together\n> with the changelog.\n\nPlease put it in the code (in a comment). The changelog will disappear\nas soon as I apply the patch.\n\n> \n> >\n> > You'd better describe what this struct is used for here. \n> Seems self-explanatory..2 fields commented. What would you add here?\n\nJust that the struct contains the logical-to-physical translation\ntable used by the SHARPSL FTL. It's just that your initial comment\ndidn't bring any useful information. \n\n> \n> > \n> >> + */\n> >> +struct sharpsl_ftl {\n> >> + u_int logmax;\n> >> + u_int *log2phy; \n> >\n\n[...]\n\n> >> +/*\n> >> + * The logical block number assigned to a physical block is stored in the OOB\n> >> + * of the first page, in 3 16-bit copies with the following layout:\n> >> + *\n> >> + * 01234567 89abcdef\n> >> + * -------- --------\n> >> + * ECC BB xyxyxy\n> >> + *\n> >> + * When reading we check that the first two copies agree.\n> >> + * In case of error, matching is tried using the following pairs.\n> >> + * Reserved values 0xffff mean the block is kept for wear leveling.\n> >> + *\n> >> + * 01234567 89abcdef\n> >> + * -------- --------\n> >> + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9\n> >> + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11\n> >> + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13\n> >> + *\n> >> + */\n> >> +static u_int sharpsl_nand_get_logical_num(u_char *oob)\n> >> +{\n> >> + u16 us;\n> >> + int good0, good1;\n> >> +\n> >> + if (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] &&\n> >> + oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) {\n> >> + good0 = NAND_NOOB_LOGADDR_00;\n> >> + good1 = NAND_NOOB_LOGADDR_01;\n> >> + } else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] &&\n> >> + oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) {\n> >> + good0 = NAND_NOOB_LOGADDR_10;\n> >> + good1 = NAND_NOOB_LOGADDR_11;\n> >> + } else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] &&\n> >> + oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) {\n> >> + good0 = NAND_NOOB_LOGADDR_20;\n> >> + good1 = NAND_NOOB_LOGADDR_21;\n> >> + } else {\n> >> + /* wrong oob fingerprint, maybe here by mistake? */\n> >> + return UINT_MAX;\n> >> + }\n> >> +\n> >> + us = oob[good0] | oob[good1] << 8;\n> >> +\n> >> + /* parity check */\n> >> + if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT)\n> >> + return (UINT_MAX - 1); \n> >\n> > Why not making this function return an int and use regular error codes\n> > for the parity and wrong fingerprint errors?\n> > \n> Originally in case of error it did return the largest values.\n> I can return a negative int but then I'd have to check for log_no >0\n> \n> Besides, what are the 'regular' error codes you'd use here?\n> For the missing FTL we do return -EINVAL later so I'd say this is the\n> error for the wrong oob fingerprint.\n\nEINVAL should be fine for all corruption/mismatch.\n\n> \n> About the parity error, it does return UINT_MAX -1 so to allow very\n> large log_num. This value is always bigger than log_max so it is\n> skipped in the actual code but the read continues.\n\nWell, a parity error is still an error, right?\n\n> Should we change the philosophy of the old 2.4 code and exit in case\n> of parity errors?\n\nHm, you should not exit if the phys -> log information is corrupted, it\njust means you can't consider this physical block is containing a valid\nlogical block, that's all. Pretty much like when there's an oob\nfingerprint mismatch.\n\n> \n> >> +\n> >> + /* reserved */\n> >> + if (us == BLOCK_IS_RESERVED)\n> >> + return BLOCK_IS_RESERVED;\n> >> + else \n> >\n> > You can drop the 'else' here. \n> Done\n> > \n> >> + return (us & BLOCK_UNMASK) >> 1; \n> >\n> > So, this is a 10bit value, right? Why not doing the >> 1 first so that\n> > your BLOCK_UNMASK mask really looks like a valid mask (0x3ff, which can\n> > also be defined as GENMASK(9, 0)). \n> Right, very nice. Done.\n> Can I keep BLOCK_UNMASK COMPLEMENT = 1 ?\n\nYes.\n\n> \n> > \n> >> +}\n> >> +\n> >> +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size, \n> >\n> > s/sharpsl_nand_init_logical/sharpsl_nand_init_ftl/ \n> Oh yes, renamed\n> \n> >\n> > partition_size is always set to SHARPSL_FTL_PART_SIZE. Please drop this\n> > argument.\n> > \n> Ok, done\n> \n> >> + struct sharpsl_ftl *ftl)\n> >> +{\n> >> + u_int block_num, log_num, phymax;\n> >> + loff_t block_adr;\n> >> + u_char *oob;\n> >> + int i, ret;\n> >> +\n> >> + oob = kzalloc(mtd->oobsize, GFP_KERNEL);\n> >> + if (!oob)\n> >> + return -ENOMEM;\n> >> +\n> >> + /* initialize management structure */\n> >> + phymax = (partition_size / mtd->erasesize); \n> >\n> > You can use mtd_div_by_eb() here.\n> > \n> Done for all occurrences\n> \n> >> +\n> >> + /* FTL reserves 5% of the blocks + 1 spare */\n> >> + ftl->logmax = ((phymax * 95) / 100) - 1;\n> >> +\n> >> + ftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int), \n> >\n> > sizeof(*ftl->log2phy) \n> Ok, changed.\n> \n> > \n> >> + GFP_KERNEL);\n> >> + if (!ftl->log2phy) {\n> >> + ret = -ENOMEM;\n> >> + goto exit;\n> >> + }\n> >> +\n> >> + /* initialize ftl->log2phy */\n> >> + for (i = 0; i < ftl->logmax; i++)\n> >> + ftl->log2phy[i] = UINT_MAX;\n> >> +\n> >> + /* create physical-logical table */\n> >> + for (block_num = 0; block_num < phymax; block_num++) {\n> >> + block_adr = block_num * mtd->erasesize;\n> >> +\n> >> + if (mtd_block_isbad(mtd, block_adr))\n> >> + continue;\n> >> +\n> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))\n> >> + continue;\n> >> +\n> >> + /* get logical block */\n> >> + log_num = sharpsl_nand_get_logical_num(oob);\n> >> +\n> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */\n> >> + if (log_num == UINT_MAX) {\n> >> + pr_info(\"sharpslpart: Sharp SL FTL not found.\\n\");\n> >> + ret = -EINVAL;\n> >> + goto exit;\n> >> + }\n\nOkay, I overlooked that part. Why do you exit if there's a fingerprint\nmismatch? Can't you just ignore this physical block and continue\nscanning the remaining ones?","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (mailfrom)\n\tsmtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133;\n\thelo=bombadil.infradead.org;\n\tenvelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"fpwX3L5C\"; \n\tdkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xdKfQ4p4gz9s8J\n\tfor ;\n\tThu, 24 Aug 2017 20:05:02 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dkp0S-0008V1-LM; Thu, 24 Aug 2017 10:04:56 +0000","from mail.free-electrons.com ([62.4.15.54])\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dkp0O-0008Tz-LB\n\tfor linux-mtd@lists.infradead.org; Thu, 24 Aug 2017 10:04:54 +0000","by mail.free-electrons.com (Postfix, from userid 110)\n\tid CB6912093A; Thu, 24 Aug 2017 12:04:28 +0200 (CEST)","from bbrezillon (unknown [91.160.177.164])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id 8315D208F6;\n\tThu, 24 Aug 2017 12:04:28 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:\n\tMessage-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=/AOQgwIooSkwvz7+8HlSs0lbi1Cw+ID9vYS50KzgEWQ=;\n\tb=fpwX3L5CHL9exM\n\t8YLve9z1xgOKW1/6N2l76t66KEzhuk0oeu8HQNLxSmFfojWYSXvbsc/9oabc0Lo8miRld8j9qoAzd\n\t3Qi0m9mG0yzF4KbmhtCC3OGgNBUW5YpotKJYrgqrvOYU38Ori56tjwZ9MmTtn0ipHib0BFR0kGtL1\n\tqMbHD0CaoqE6QZggSOEyqAg5gUAptPAqsXDkFCD+EBabi4muKomoMOVGfv9TzmGGmTGOWRNOHqEX7\n\tY2Uj95qeCFae1I2svAQvBVEb7kcOB/i0+tX6Xhv9HlqxTqoZ7J2m5RilTaOP6vP8e0gTe2JAjrKo6\n\t3xZO0DQq/PRimmcerfDQ==;","X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT\n\tshortcircuit=ham autolearn=disabled version=3.4.0","Date":"Thu, 24 Aug 2017 12:04:28 +0200","From":"Boris Brezillon ","To":"Andrea Adami ","Subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","Message-ID":"<20170824120428.46e266c7@bbrezillon>","In-Reply-To":"","References":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>\n\t<20170822145424.54e57b94@bbrezillon>\n\t","X-Mailer":"Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu)","MIME-Version":"1.0","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170824_030452_987052_35EF59CE ","X-CRM114-Status":"GOOD ( 34.24 )","X-Spam-Score":"-1.9 (-)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-1.9 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay\n\tdomain\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]","X-BeenThere":"linux-mtd@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"Linux MTD discussion mailing list ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dmitry Eremin-Solenikov ,\n\tRichard Weinberger ,\n\tRobert Jarzmik , linux-kernel@vger.kernel.org,\n\tHaojian Zhuang , \n\tMarek Vasut , linux-mtd@lists.infradead.org,\n\tCyrille Pitchen ,\n\t=?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= ,\n\tBrian Norris ,\n\tDavid Woodhouse ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"\"linux-mtd\" ","Errors-To":"linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org"}},{"id":1756193,"web_url":"http://patchwork.ozlabs.org/comment/1756193/","msgid":"","list_archive_url":null,"date":"2017-08-24T10:30:02","subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","submitter":{"id":30311,"url":"http://patchwork.ozlabs.org/api/people/30311/","name":"Andrea Adami","email":"andrea.adami@gmail.com"},"content":"On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon\n wrote:\n> On Thu, 24 Aug 2017 11:19:56 +0200\n> Andrea Adami wrote:\n>\n>> >> +/**\n>> >> + * struct sharpsl_ftl - Sharp FTL Logical Table\n>> >> + * @logmax: number of logical blocks\n>> >> + * @log2phy: the logical-to-physical table\n>> >> + *\n>> >> + * Stripped down from 2.4 sources for read-only purposes.\n>> >\n>> > Not sure this information is really useful, especially since you don't\n>> > provide a link to the 2.4 sources (or maybe I missed it).\n>>\n>> Maybe here I can add that this FTL was originally tailored for devices\n>> 16KiB erasesize matching the size of the PARAM_BLOCK_ above thus using\n>> two separate blocks for the partition tables. Should I add an\n>> ASCII-art?\n>\n> You can add an ASCII-art if you want but that's not mandatory if you\n> can explain it with words :-).\n>\n>> Pls see\n>> http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm\n>> BTW the link was in the cover-letter 0/9, I'll put it here together\n>> with the changelog.\n>\n> Please put it in the code (in a comment). The changelog will disappear\n> as soon as I apply the patch.\n>\n>>\n>> >\n>> > You'd better describe what this struct is used for here.\n>> Seems self-explanatory..2 fields commented. What would you add here?\n>\n> Just that the struct contains the logical-to-physical translation\n> table used by the SHARPSL FTL. It's just that your initial comment\n> didn't bring any useful information.\n>\n>>\n>> >\n>> >> + */\n>> >> +struct sharpsl_ftl {\n>> >> + u_int logmax;\n>> >> + u_int *log2phy;\n>> >\n>\n> [...]\n>\n>> >> +/*\n>> >> + * The logical block number assigned to a physical block is stored in the OOB\n>> >> + * of the first page, in 3 16-bit copies with the following layout:\n>> >> + *\n>> >> + * 01234567 89abcdef\n>> >> + * -------- --------\n>> >> + * ECC BB xyxyxy\n>> >> + *\n>> >> + * When reading we check that the first two copies agree.\n>> >> + * In case of error, matching is tried using the following pairs.\n>> >> + * Reserved values 0xffff mean the block is kept for wear leveling.\n>> >> + *\n>> >> + * 01234567 89abcdef\n>> >> + * -------- --------\n>> >> + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9\n>> >> + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11\n>> >> + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13\n>> >> + *\n>> >> + */\n>> >> +static u_int sharpsl_nand_get_logical_num(u_char *oob)\n>> >> +{\n>> >> + u16 us;\n>> >> + int good0, good1;\n>> >> +\n>> >> + if (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] &&\n>> >> + oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) {\n>> >> + good0 = NAND_NOOB_LOGADDR_00;\n>> >> + good1 = NAND_NOOB_LOGADDR_01;\n>> >> + } else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] &&\n>> >> + oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) {\n>> >> + good0 = NAND_NOOB_LOGADDR_10;\n>> >> + good1 = NAND_NOOB_LOGADDR_11;\n>> >> + } else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] &&\n>> >> + oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) {\n>> >> + good0 = NAND_NOOB_LOGADDR_20;\n>> >> + good1 = NAND_NOOB_LOGADDR_21;\n>> >> + } else {\n>> >> + /* wrong oob fingerprint, maybe here by mistake? */\n>> >> + return UINT_MAX;\n>> >> + }\n>> >> +\n>> >> + us = oob[good0] | oob[good1] << 8;\n>> >> +\n>> >> + /* parity check */\n>> >> + if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT)\n>> >> + return (UINT_MAX - 1);\n>> >\n>> > Why not making this function return an int and use regular error codes\n>> > for the parity and wrong fingerprint errors?\n>> >\n>> Originally in case of error it did return the largest values.\n>> I can return a negative int but then I'd have to check for log_no >0\n>>\n>> Besides, what are the 'regular' error codes you'd use here?\n>> For the missing FTL we do return -EINVAL later so I'd say this is the\n>> error for the wrong oob fingerprint.\n>\n> EINVAL should be fine for all corruption/mismatch.\n>\n>>\n>> About the parity error, it does return UINT_MAX -1 so to allow very\n>> large log_num. This value is always bigger than log_max so it is\n>> skipped in the actual code but the read continues.\n>\n> Well, a parity error is still an error, right?\n>\n>> Should we change the philosophy of the old 2.4 code and exit in case\n>> of parity errors?\n>\n> Hm, you should not exit if the phys -> log information is corrupted, it\n> just means you can't consider this physical block is containing a valid\n> logical block, that's all. Pretty much like when there's an oob\n> fingerprint mismatch.\n\nFor the partition parser purposes, we don't know in advance in which\nphysical block the partition tables are stored.\nMaybe the parity is occurring on 'uninteresting' blocks, so I'll go\nhead to maximize the probabilties to get the tables.\n\n>\n>>\n>> >> +\n>> >> + /* reserved */\n>> >> + if (us == BLOCK_IS_RESERVED)\n>> >> + return BLOCK_IS_RESERVED;\n>> >> + else\n>> >\n>> > You can drop the 'else' here.\n>> Done\n>> >\n>> >> + return (us & BLOCK_UNMASK) >> 1;\n>> >\n>> > So, this is a 10bit value, right? Why not doing the >> 1 first so that\n>> > your BLOCK_UNMASK mask really looks like a valid mask (0x3ff, which can\n>> > also be defined as GENMASK(9, 0)).\n>> Right, very nice. Done.\n>> Can I keep BLOCK_UNMASK COMPLEMENT = 1 ?\n>\n> Yes.\n>\n>>\n>> >\n>> >> +}\n>> >> +\n>> >> +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size,\n>> >\n>> > s/sharpsl_nand_init_logical/sharpsl_nand_init_ftl/\n>> Oh yes, renamed\n>>\n>> >\n>> > partition_size is always set to SHARPSL_FTL_PART_SIZE. Please drop this\n>> > argument.\n>> >\n>> Ok, done\n>>\n>> >> + struct sharpsl_ftl *ftl)\n>> >> +{\n>> >> + u_int block_num, log_num, phymax;\n>> >> + loff_t block_adr;\n>> >> + u_char *oob;\n>> >> + int i, ret;\n>> >> +\n>> >> + oob = kzalloc(mtd->oobsize, GFP_KERNEL);\n>> >> + if (!oob)\n>> >> + return -ENOMEM;\n>> >> +\n>> >> + /* initialize management structure */\n>> >> + phymax = (partition_size / mtd->erasesize);\n>> >\n>> > You can use mtd_div_by_eb() here.\n>> >\n>> Done for all occurrences\n>>\n>> >> +\n>> >> + /* FTL reserves 5% of the blocks + 1 spare */\n>> >> + ftl->logmax = ((phymax * 95) / 100) - 1;\n>> >> +\n>> >> + ftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int),\n>> >\n>> > sizeof(*ftl->log2phy)\n>> Ok, changed.\n>>\n>> >\n>> >> + GFP_KERNEL);\n>> >> + if (!ftl->log2phy) {\n>> >> + ret = -ENOMEM;\n>> >> + goto exit;\n>> >> + }\n>> >> +\n>> >> + /* initialize ftl->log2phy */\n>> >> + for (i = 0; i < ftl->logmax; i++)\n>> >> + ftl->log2phy[i] = UINT_MAX;\n>> >> +\n>> >> + /* create physical-logical table */\n>> >> + for (block_num = 0; block_num < phymax; block_num++) {\n>> >> + block_adr = block_num * mtd->erasesize;\n>> >> +\n>> >> + if (mtd_block_isbad(mtd, block_adr))\n>> >> + continue;\n>> >> +\n>> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))\n>> >> + continue;\n>> >> +\n>> >> + /* get logical block */\n>> >> + log_num = sharpsl_nand_get_logical_num(oob);\n>> >> +\n>> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */\n>> >> + if (log_num == UINT_MAX) {\n>> >> + pr_info(\"sharpslpart: Sharp SL FTL not found.\\n\");\n>> >> + ret = -EINVAL;\n>> >> + goto exit;\n>> >> + }\n>\n> Okay, I overlooked that part. Why do you exit if there's a fingerprint\n> mismatch? Can't you just ignore this physical block and continue\n> scanning the remaining ones?\n\nNorris asked to quit immediately in this case.\nhttps://patchwork.kernel.org/patch/9758361/\n\n\"I wouldn't expect people to want to use this parser, but if we have a\nquick way to say \"this doesn't match, skip me\", then that would be\nhelpful.\"\n\"We don't want to waste too much time scanning for this partition\ntable if possible.\"\n\nNow we are quitting ever before checking for parity erors ...\n\nCheers\nAndrea","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (mailfrom)\n\tsmtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133;\n\thelo=bombadil.infradead.org;\n\tenvelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"SuvAsgn5\"; \n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"af+07YAx\"; dkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xdLD266y2z9sNd\n\tfor ;\n\tThu, 24 Aug 2017 20:30:42 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dkpPC-0004uH-BS; Thu, 24 Aug 2017 10:30:30 +0000","from mail-wm0-x242.google.com ([2a00:1450:400c:c09::242])\n\tby bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dkpP7-0003gJ-2b\n\tfor linux-mtd@lists.infradead.org; Thu, 24 Aug 2017 10:30:27 +0000","by mail-wm0-x242.google.com with SMTP id v186so2281605wmf.4\n\tfor ;\n\tThu, 24 Aug 2017 03:30:04 -0700 (PDT)","by 10.80.226.77 with HTTP; Thu, 24 Aug 2017 03:30:02 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:\n\tReferences:In-Reply-To:MIME-Version:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=U5ukPKmCFNIaQ47wYcMuKczI3dvX0gX6wJOSYzaeC24=;\n\tb=SuvAsgn53ypFXZ\n\txtkBVFZ2Ir2vwDRVUq3Fz1M7KwEEYWFP/GiJKv5Wu3vQzmGyIzomLzr/mL4G2mZ2XJjMk3+83AWl8\n\tRBAYd67qGwXxfrC7+E3pYPo10c1LHl3sP6Vblt1nFMO3bTZZRed6FxhXzPARYlTRZ9WB/EW7syXzG\n\tah/PSA/thTtKBsQ0o+gyR7FndGd4LmZrw1j8lo2/VBa0eBYrFd+dRTnanKNH/5SXi52YRigpCyBBJ\n\tIga2mUe/D8jZkE6zTEOuD6+KW9YGTGugs22QkVH+yEXxuCktMNMKH/6FN8Lv5ymplgIANTx9IfJKn\n\tncn2FclDKEBq1jrcOL7Q==;","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=7mVOvH70as0RNq/qEvvoFxsMhjybQSim93/rhKKzcZc=;\n\tb=af+07YAxlx9g6lqDZZ9Hg5IeYVckFR7Mm5hm00Xyru/5c39KAuNyDg2Rvwq4j5p3bg\n\tBfjNrO4XsnaZgwOAz3UQTXNIG5+ucLABsdO5vdYfC3g7WaOTmO2Md5RrnMdMd6WbfHOD\n\tGNoZP+tvxkuYss2oB2gDdq38I5wYPiPUvonXmp7QqZAz3L7F/MfIquRffNKoROyynkk6\n\t6DSzWa1HUKrubWEaNlaCmIY7klRHJaJLI3R7taYteEEl9SfSXZSEE0mmLnnFKMiWqr+D\n\tetSPEezLEwgamL66BJjJfbrDA7zSDkTJDRmYpNonDzoNdd5X8qrdlWqCrUY6QCjIbQyV\n\t/Ujg=="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=7mVOvH70as0RNq/qEvvoFxsMhjybQSim93/rhKKzcZc=;\n\tb=Q9IN0/KMLBEAPaN1mBW25LKFwcTETIJHc6a13oecFT9Qpf5lpTlAIoe4vfS6iJ0mU4\n\t9quolAnF09XklxEZKqHQKyU1uzyLGTF0d5EkIUTXk6aTcD+SjC8xa74l+tS9tNlNvsVt\n\tliZPV4/O2mre+BHTjh1zphz0yQKESfl2HzAk3OQrFCAQxXWLD7JTmIu3K0OLM8JNJ742\n\tYycuFDBZ8CJg1rq0clqlWx+UEOdKY7oABDMllmUztuO8Hj8nb5zp+AgJAxmE2pyF8EiD\n\tSTbKv1X8KWnJIWAF2mSpuAnAHh/x+xX26qaQBXMVkuzxYxrVKiuR9C+Wnky0o1QnuWc7\n\t86xw==","X-Gm-Message-State":"AHYfb5idKiHU6Z+KfHqReTd+/83Xa3Ypf4dstTB4Gcm1QRW92NbfeDao\n\tcp7FfcP+n8nzs5QRKQAzagbxK8iNpg==","X-Received":"by 10.80.173.227 with SMTP id b32mr5724262edd.81.1503570603245; \n\tThu, 24 Aug 2017 03:30:03 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170824120428.46e266c7@bbrezillon>","References":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>\n\t<20170822145424.54e57b94@bbrezillon>\n\t\n\t<20170824120428.46e266c7@bbrezillon>","From":"Andrea Adami ","Date":"Thu, 24 Aug 2017 12:30:02 +0200","Message-ID":"","Subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","To":"Boris Brezillon ","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170824_033025_426178_C0269F4F ","X-CRM114-Status":"GOOD ( 34.30 )","X-Spam-Score":"-2.0 (--)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-2.0 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at http://www.dnswl.org/,\n\tno\n\ttrust [2a00:1450:400c:c09:0:0:0:242 listed in] [list.dnswl.org]\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\n\tprovider (andrea.adami[at]gmail.com)\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]\n\t-0.1 DKIM_VALID Message has at least one valid DKIM or DK signature\n\t0.1 DKIM_SIGNED Message has a DKIM or DK signature,\n\tnot necessarily valid\n\t-0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from\n\tauthor's domain","X-BeenThere":"linux-mtd@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"Linux MTD discussion mailing list ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dmitry Eremin-Solenikov ,\n\tRichard Weinberger ,\n\tRobert Jarzmik , linux-kernel@vger.kernel.org,\n\tHaojian Zhuang , \n\tMarek Vasut , linux-mtd@lists.infradead.org,\n\tCyrille Pitchen ,\n\t=?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= ,\n\tBrian Norris ,\n\tDavid Woodhouse ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"\"linux-mtd\" ","Errors-To":"linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org"}},{"id":1756260,"web_url":"http://patchwork.ozlabs.org/comment/1756260/","msgid":"<20170824132710.5fdea20c@bbrezillon>","list_archive_url":null,"date":"2017-08-24T11:27:10","subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","submitter":{"id":63120,"url":"http://patchwork.ozlabs.org/api/people/63120/","name":"Boris Brezillon","email":"boris.brezillon@free-electrons.com"},"content":"On Thu, 24 Aug 2017 12:30:02 +0200\nAndrea Adami wrote:\n\n> On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon\n> wrote:\n> > On Thu, 24 Aug 2017 11:19:56 +0200\n> > Andrea Adami wrote:\n> > \n> >> >> +/**\n> >> >> + * struct sharpsl_ftl - Sharp FTL Logical Table\n> >> >> + * @logmax: number of logical blocks\n> >> >> + * @log2phy: the logical-to-physical table\n> >> >> + *\n> >> >> + * Stripped down from 2.4 sources for read-only purposes. \n> >> >\n> >> > Not sure this information is really useful, especially since you don't\n> >> > provide a link to the 2.4 sources (or maybe I missed it). \n> >>\n> >> Maybe here I can add that this FTL was originally tailored for devices\n> >> 16KiB erasesize matching the size of the PARAM_BLOCK_ above thus using\n> >> two separate blocks for the partition tables. Should I add an\n> >> ASCII-art? \n> >\n> > You can add an ASCII-art if you want but that's not mandatory if you\n> > can explain it with words :-).\n> > \n> >> Pls see\n> >> http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm\n> >> BTW the link was in the cover-letter 0/9, I'll put it here together\n> >> with the changelog. \n> >\n> > Please put it in the code (in a comment). The changelog will disappear\n> > as soon as I apply the patch.\n> > \n> >> \n> >> >\n> >> > You'd better describe what this struct is used for here. \n> >> Seems self-explanatory..2 fields commented. What would you add here? \n> >\n> > Just that the struct contains the logical-to-physical translation\n> > table used by the SHARPSL FTL. It's just that your initial comment\n> > didn't bring any useful information.\n> > \n> >> \n> >> > \n> >> >> + */\n> >> >> +struct sharpsl_ftl {\n> >> >> + u_int logmax;\n> >> >> + u_int *log2phy; \n> >> > \n> >\n> > [...]\n> > \n> >> >> +/*\n> >> >> + * The logical block number assigned to a physical block is stored in the OOB\n> >> >> + * of the first page, in 3 16-bit copies with the following layout:\n> >> >> + *\n> >> >> + * 01234567 89abcdef\n> >> >> + * -------- --------\n> >> >> + * ECC BB xyxyxy\n> >> >> + *\n> >> >> + * When reading we check that the first two copies agree.\n> >> >> + * In case of error, matching is tried using the following pairs.\n> >> >> + * Reserved values 0xffff mean the block is kept for wear leveling.\n> >> >> + *\n> >> >> + * 01234567 89abcdef\n> >> >> + * -------- --------\n> >> >> + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9\n> >> >> + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11\n> >> >> + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13\n> >> >> + *\n> >> >> + */\n> >> >> +static u_int sharpsl_nand_get_logical_num(u_char *oob)\n> >> >> +{\n> >> >> + u16 us;\n> >> >> + int good0, good1;\n> >> >> +\n> >> >> + if (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] &&\n> >> >> + oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) {\n> >> >> + good0 = NAND_NOOB_LOGADDR_00;\n> >> >> + good1 = NAND_NOOB_LOGADDR_01;\n> >> >> + } else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] &&\n> >> >> + oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) {\n> >> >> + good0 = NAND_NOOB_LOGADDR_10;\n> >> >> + good1 = NAND_NOOB_LOGADDR_11;\n> >> >> + } else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] &&\n> >> >> + oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) {\n> >> >> + good0 = NAND_NOOB_LOGADDR_20;\n> >> >> + good1 = NAND_NOOB_LOGADDR_21;\n> >> >> + } else {\n> >> >> + /* wrong oob fingerprint, maybe here by mistake? */\n> >> >> + return UINT_MAX;\n> >> >> + }\n> >> >> +\n> >> >> + us = oob[good0] | oob[good1] << 8;\n> >> >> +\n> >> >> + /* parity check */\n> >> >> + if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT)\n> >> >> + return (UINT_MAX - 1); \n> >> >\n> >> > Why not making this function return an int and use regular error codes\n> >> > for the parity and wrong fingerprint errors?\n> >> > \n> >> Originally in case of error it did return the largest values.\n> >> I can return a negative int but then I'd have to check for log_no >0\n> >>\n> >> Besides, what are the 'regular' error codes you'd use here?\n> >> For the missing FTL we do return -EINVAL later so I'd say this is the\n> >> error for the wrong oob fingerprint. \n> >\n> > EINVAL should be fine for all corruption/mismatch.\n> > \n> >>\n> >> About the parity error, it does return UINT_MAX -1 so to allow very\n> >> large log_num. This value is always bigger than log_max so it is\n> >> skipped in the actual code but the read continues. \n> >\n> > Well, a parity error is still an error, right?\n> > \n> >> Should we change the philosophy of the old 2.4 code and exit in case\n> >> of parity errors? \n> >\n> > Hm, you should not exit if the phys -> log information is corrupted, it\n> > just means you can't consider this physical block is containing a valid\n> > logical block, that's all. Pretty much like when there's an oob\n> > fingerprint mismatch. \n> \n> For the partition parser purposes, we don't know in advance in which\n> physical block the partition tables are stored.\n> Maybe the parity is occurring on 'uninteresting' blocks, so I'll go\n> head to maximize the probabilties to get the tables.\n\nYep, that's my point. You need to build the translation table before\ndeciding whether a valid partinfo is present or not.\n\n> \n> > \n> >> \n> >> >> +\n> >> >> + /* reserved */\n> >> >> + if (us == BLOCK_IS_RESERVED)\n> >> >> + return BLOCK_IS_RESERVED;\n> >> >> + else \n> >> >\n> >> > You can drop the 'else' here. \n> >> Done \n> >> > \n> >> >> + return (us & BLOCK_UNMASK) >> 1; \n> >> >\n> >> > So, this is a 10bit value, right? Why not doing the >> 1 first so that\n> >> > your BLOCK_UNMASK mask really looks like a valid mask (0x3ff, which can\n> >> > also be defined as GENMASK(9, 0)). \n> >> Right, very nice. Done.\n> >> Can I keep BLOCK_UNMASK COMPLEMENT = 1 ? \n> >\n> > Yes.\n> > \n> >> \n> >> > \n> >> >> +}\n> >> >> +\n> >> >> +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size, \n> >> >\n> >> > s/sharpsl_nand_init_logical/sharpsl_nand_init_ftl/ \n> >> Oh yes, renamed\n> >> \n> >> >\n> >> > partition_size is always set to SHARPSL_FTL_PART_SIZE. Please drop this\n> >> > argument.\n> >> > \n> >> Ok, done\n> >> \n> >> >> + struct sharpsl_ftl *ftl)\n> >> >> +{\n> >> >> + u_int block_num, log_num, phymax;\n> >> >> + loff_t block_adr;\n> >> >> + u_char *oob;\n> >> >> + int i, ret;\n> >> >> +\n> >> >> + oob = kzalloc(mtd->oobsize, GFP_KERNEL);\n> >> >> + if (!oob)\n> >> >> + return -ENOMEM;\n> >> >> +\n> >> >> + /* initialize management structure */\n> >> >> + phymax = (partition_size / mtd->erasesize); \n> >> >\n> >> > You can use mtd_div_by_eb() here.\n> >> > \n> >> Done for all occurrences\n> >> \n> >> >> +\n> >> >> + /* FTL reserves 5% of the blocks + 1 spare */\n> >> >> + ftl->logmax = ((phymax * 95) / 100) - 1;\n> >> >> +\n> >> >> + ftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int), \n> >> >\n> >> > sizeof(*ftl->log2phy) \n> >> Ok, changed.\n> >> \n> >> > \n> >> >> + GFP_KERNEL);\n> >> >> + if (!ftl->log2phy) {\n> >> >> + ret = -ENOMEM;\n> >> >> + goto exit;\n> >> >> + }\n> >> >> +\n> >> >> + /* initialize ftl->log2phy */\n> >> >> + for (i = 0; i < ftl->logmax; i++)\n> >> >> + ftl->log2phy[i] = UINT_MAX;\n> >> >> +\n> >> >> + /* create physical-logical table */\n> >> >> + for (block_num = 0; block_num < phymax; block_num++) {\n> >> >> + block_adr = block_num * mtd->erasesize;\n> >> >> +\n> >> >> + if (mtd_block_isbad(mtd, block_adr))\n> >> >> + continue;\n> >> >> +\n> >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))\n> >> >> + continue;\n> >> >> +\n> >> >> + /* get logical block */\n> >> >> + log_num = sharpsl_nand_get_logical_num(oob);\n> >> >> +\n> >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */\n> >> >> + if (log_num == UINT_MAX) {\n> >> >> + pr_info(\"sharpslpart: Sharp SL FTL not found.\\n\");\n> >> >> + ret = -EINVAL;\n> >> >> + goto exit;\n> >> >> + } \n> >\n> > Okay, I overlooked that part. Why do you exit if there's a fingerprint\n> > mismatch? Can't you just ignore this physical block and continue\n> > scanning the remaining ones? \n> \n> Norris asked to quit immediately in this case.\n> https://patchwork.kernel.org/patch/9758361/\n> \n> \"I wouldn't expect people to want to use this parser, but if we have a\n> quick way to say \"this doesn't match, skip me\", then that would be\n> helpful.\"\n> \"We don't want to waste too much time scanning for this partition\n> table if possible.\"\n\nActually, you don't save much by exiting on \"bad OOB fingerprint\". If\nyou look at the code you'll see that the only thing you check is\nwhether some oob portions are equal or not, and most of the time the\nOOB area is left untouched by the upper layer, which means all free\nbytes will be left to 0xff, which in turn means the \"is fingerprint\ngood?\" test will pass.\n\n> \n> Now we are quitting ever before checking for parity erors ...\n\nHonestly, I'd recommend not using this parser for anything but SHARPSL\nplatforms, so I don't think we care much about the \"scan all blocks\"\noverhead. Moreover, the sharpsl parser is the last one in the\npart_parsers list, so it should be quite fast if the user specifies a\nvalid mtdparts= on the cmdline or valid partitions in the DT.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (mailfrom)\n\tsmtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133;\n\thelo=bombadil.infradead.org;\n\tenvelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"IMeKE3Oj\"; \n\tdkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xdMTy1RkRz9s8P\n\tfor ;\n\tThu, 24 Aug 2017 21:27:47 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dkqIV-0004QB-7i; Thu, 24 Aug 2017 11:27:39 +0000","from mail.free-electrons.com ([62.4.15.54])\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dkqIP-0004Ax-Ls\n\tfor linux-mtd@lists.infradead.org; Thu, 24 Aug 2017 11:27:37 +0000","by mail.free-electrons.com (Postfix, from userid 110)\n\tid E5F38209A8; Thu, 24 Aug 2017 13:27:10 +0200 (CEST)","from bbrezillon (unknown [91.160.177.164])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id 924412096F;\n\tThu, 24 Aug 2017 13:27:10 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:\n\tMessage-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=C3zbd2AzJlq7RsRclfQnzsTdowXhZQ9anJ3xSNXX6zc=;\n\tb=IMeKE3OjahUmZW\n\tswEXfRN0D+jdO26AN7p2G31muw7RS6L2XD+2Y1XbfEUW7WfRzwuBBFLLMl/GlJsuX1I4MOpzI3KaL\n\tiXHp+ZsMj3gLQgFkyTyDG22PjrmdQb6uPkM7cgl+gUz31uLub/SWLQ3AlxxTCk1w0Sk7vRVRTRU6o\n\t70EcUTy5jh56kQLkC3YOCpQYbtTS2QjfEbPArsbW6z0b3rNj16OL2AAmXBNJemujXiLvPkjGJD7IS\n\tDEv/q93mMkeXQ5myV4UN4D1OsqMoXQCdObnMvDH94Ew+LrnaOC3jjtQrOmypI0eEuLIndszVqD7dj\n\tw9AfytwCCEj0G5o0/yqQ==;","X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT,\n\tURIBL_BLOCKED shortcircuit=ham autolearn=disabled version=3.4.0","Date":"Thu, 24 Aug 2017 13:27:10 +0200","From":"Boris Brezillon ","To":"Andrea Adami ","Subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","Message-ID":"<20170824132710.5fdea20c@bbrezillon>","In-Reply-To":"","References":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>\n\t<20170822145424.54e57b94@bbrezillon>\n\t\n\t<20170824120428.46e266c7@bbrezillon>\n\t","X-Mailer":"Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu)","MIME-Version":"1.0","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170824_042734_088478_62524BD5 ","X-CRM114-Status":"GOOD ( 42.17 )","X-Spam-Score":"-1.9 (-)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-1.9 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay\n\tdomain\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]","X-BeenThere":"linux-mtd@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"Linux MTD discussion mailing list ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dmitry Eremin-Solenikov ,\n\tRichard Weinberger ,\n\tRobert Jarzmik , linux-kernel@vger.kernel.org,\n\tHaojian Zhuang , \n\tMarek Vasut , linux-mtd@lists.infradead.org,\n\tCyrille Pitchen ,\n\t=?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= ,\n\tBrian Norris ,\n\tDavid Woodhouse ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"\"linux-mtd\" ","Errors-To":"linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org"}},{"id":1756846,"web_url":"http://patchwork.ozlabs.org/comment/1756846/","msgid":"<20170825001129.091badb4@bbrezillon>","list_archive_url":null,"date":"2017-08-24T22:11:29","subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","submitter":{"id":63120,"url":"http://patchwork.ozlabs.org/api/people/63120/","name":"Boris Brezillon","email":"boris.brezillon@free-electrons.com"},"content":"On Tue, 22 Aug 2017 11:42:52 +0200\nAndrea Adami wrote:\n\n> The Sharp SL Series (Zaurus) PXA handhelds have 16/64/128M of NAND flash\n> and share the same layout of the first 7M partition, managed by Sharp FTL.\n> \n> The purpose of this self-contained patch is to add a common parser and\n> remove the hardcoded sizes in the board files (these devices are not yet\n> converted to devicetree).\n> Users will have benefits because the mtdparts= tag will not be necessary\n> anymore and they will be free to repartition the little sized flash.\n> \n> The obsolete bootloader can not pass the partitioning info to modern\n> kernels anymore so it has to be read from flash at known logical addresses.\n> (see http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm )\n> \n> In kernel, under arch/arm/mach-pxa we have already 8 machines:\n> MACH_POODLE, MACH_CORGI, MACH_SHEPERD, MACH_HUSKY, MACH_AKITA, MACH_SPITZ,\n> MACH_BORZOI, MACH_TOSA.\n> Lost after the 2.4 vendor kernel are MACH_BOXER and MACH_TERRIER.\n> \n> Almost every model has different factory partitioning: add to this the\n> units can be repartitioned by users with userspace tools (nandlogical)\n> and installers for popular (back then) linux distributions.\n> \n> The Parameter Area in the first (boot) partition extends from 0x00040000 to\n> 0x0007bfff (176k) and contains two copies of the partition table:\n> ...\n> 0x00060000: Partition Info1 16k\n> 0x00064000: Partition Info2 16k\n> 0x00668000: Model 16k\n> ...\n> \n> The first 7M partition is managed by the Sharp FTL reserving 5% + 1 blocks\n> for wear-leveling: some blocks are remapped and one layer of translation\n> (logical to physical) is necessary.\n> \n> There isn't much documentation about this FTL in the 2.4 sources, just the\n> MTD methods for reading and writing using logical addresses and the block\n> management (wear-leveling, use counter).\n> For the purpose of the MTD parser only the read part of the code was taken.\n> \n> The NAND drivers that can use this parser are sharpsl.c and tmio_nand.c.\n> \n> Signed-off-by: Andrea Adami \n> ---\n> drivers/mtd/parsers/Kconfig | 7 +\n> drivers/mtd/parsers/Makefile | 1 +\n> drivers/mtd/parsers/sharpslpart.c | 382 ++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 390 insertions(+)\n> create mode 100644 drivers/mtd/parsers/sharpslpart.c\n> \n> diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig\n> index d206b3c..f371022 100644\n> --- a/drivers/mtd/parsers/Kconfig\n> +++ b/drivers/mtd/parsers/Kconfig\n> @@ -6,3 +6,10 @@ config MTD_PARSER_TRX\n> \t may contain up to 3/4 partitions (depending on the version).\n> \t This driver will parse TRX header and report at least two partitions:\n> \t kernel and rootfs.\n> +config MTD_SHARPSL_PARTS\n> +\ttristate \"Sharp SL Series NAND flash partition parser\"\n> +\tdepends on MTD_NAND_SHARPSL || MTD_NAND_TMIO || COMPILE_TEST\n> +\thelp\n> +\t This provides the read-only FTL logic necessary to read the partition\n> +\t table from the NAND flash of Sharp SL Series (Zaurus) and the MTD\n> +\t partition parser using this code.\n> diff --git a/drivers/mtd/parsers/Makefile b/drivers/mtd/parsers/Makefile\n> index 4d9024e..5b1bcc3 100644\n> --- a/drivers/mtd/parsers/Makefile\n> +++ b/drivers/mtd/parsers/Makefile\n> @@ -1 +1,2 @@\n> obj-$(CONFIG_MTD_PARSER_TRX)\t\t+= parser_trx.o\n> +obj-$(CONFIG_MTD_SHARPSL_PARTS)\t\t+= sharpslpart.o\n> diff --git a/drivers/mtd/parsers/sharpslpart.c b/drivers/mtd/parsers/sharpslpart.c\n> new file mode 100644\n> index 0000000..b2333ae\n> --- /dev/null\n> +++ b/drivers/mtd/parsers/sharpslpart.c\n> @@ -0,0 +1,382 @@\n> +/*\n> + * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL\n> + * for logical addressing, as used on the PXA models of the SHARP SL Series.\n> + *\n> + * Copyright (C) 2017 Andrea Adami \n> + *\n> + * Based on 2.4 sources:\n> + * drivers/mtd/nand/sharp_sl_logical.c\n> + * linux/include/asm-arm/sharp_nand_logical.h\n> + *\n> + * Copyright (C) 2002 SHARP\n> + *\n> + * This program is free software; you can redistribute it and/or modify\n> + * it under the terms of the GNU General Public License as published by\n> + * the Free Software Foundation; either version 2 of the License, or\n> + * (at your option) any later version.\n> + *\n> + * This program is distributed in the hope that it will be useful,\n> + * but WITHOUT ANY WARRANTY; without even the implied warranty of\n> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n> + * GNU General Public License for more details.\n> + *\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +/* oob structure */\n> +#define NAND_NOOB_LOGADDR_00\t\t8\n> +#define NAND_NOOB_LOGADDR_01\t\t9\n> +#define NAND_NOOB_LOGADDR_10\t\t10\n> +#define NAND_NOOB_LOGADDR_11\t\t11\n> +#define NAND_NOOB_LOGADDR_20\t\t12\n> +#define NAND_NOOB_LOGADDR_21\t\t13\n> +\n> +#define BLOCK_IS_RESERVED\t\t0xffff\n> +#define BLOCK_UNMASK\t\t\t0x07fe\n> +#define BLOCK_UNMASK_COMPLEMENT\t\t1\n> +\n> +/* factory defaults */\n> +#define SHARPSL_NAND_PARTS\t\t3\n> +#define SHARPSL_FTL_PART_SIZE\t\t(7 * 1024 * 1024)\n> +#define PARAM_BLOCK_PARTITIONINFO1\t0x00060000\n> +#define PARAM_BLOCK_PARTITIONINFO2\t0x00064000\n> +\n> +#define BOOT_MAGIC\t\t\t0x424f4f54\n> +#define FSRO_MAGIC\t\t\t0x4653524f\n> +#define FSRW_MAGIC\t\t\t0x46535257\n> +\n> +/**\n> + * struct sharpsl_ftl - Sharp FTL Logical Table\n> + * @logmax:\t\tnumber of logical blocks\n> + * @log2phy:\t\tthe logical-to-physical table\n> + *\n> + * Stripped down from 2.4 sources for read-only purposes.\n> + */\n> +struct sharpsl_ftl {\n> +\tu_int logmax;\n> +\tu_int *log2phy;\n> +};\n> +\n> +static int sharpsl_nand_read_oob(struct mtd_info *mtd, loff_t offs, size_t len,\n> +\t\t\t\t uint8_t *buf)\n> +{\n> +\tloff_t mask = mtd->writesize - 1;\n> +\tstruct mtd_oob_ops ops;\n> +\tint ret;\n> +\n> +\tops.mode = MTD_OPS_PLACE_OOB;\n> +\tops.ooboffs = offs & mask;\n> +\tops.ooblen = len;\n> +\tops.oobbuf = buf;\n> +\tops.datbuf = NULL;\n> +\n> +\tret = mtd_read_oob(mtd, offs & ~mask, &ops);\n> +\tif (ret != 0 || len != ops.oobretlen)\n> +\t\treturn -1;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/*\n> + * The logical block number assigned to a physical block is stored in the OOB\n> + * of the first page, in 3 16-bit copies with the following layout:\n> + *\n> + * 01234567 89abcdef\n> + * -------- --------\n> + * ECC BB xyxyxy\n> + *\n> + * When reading we check that the first two copies agree.\n> + * In case of error, matching is tried using the following pairs.\n> + * Reserved values 0xffff mean the block is kept for wear leveling.\n> + *\n> + * 01234567 89abcdef\n> + * -------- --------\n> + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9\n> + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11\n> + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13\n\nI know there's a depends on \"MTD_NAND_SHARPSL || MTD_NAND_TMIO\" in the\nKconfig entry, but one could enable those options just to use the sharpsl\npart parser even if the OOB layout is incompatible with the sharpsl FTL.\n\nI'd recommend that you check that OOB bytes 8 to 15 are actually free\nto be used by the MTD user in sharpsl_parse_mtd_partitions().\n\nCan be done with something like that:\n\nstatic int sharpsl_nand_check_ooblayout(struct mtd_info *mtd)\n{\n\tu8 freebytes = 0;\n\tint section = 0;\n\n\twhile (true) {\n\t\tstruct mtd_oob_region oobfree = { };\n\t\tint ret, i;\n\n\t\tret = mtd_ooblayout_free(mtd, section++, &oobfree);\n\t\tif (ret)\n\t\t\tbreak;\n\n\t\tif (!oobfree.length || oobfree.offset > 15 ||\n\t\t (oobfree.offset + oobfree.length) < 8)\n\t\t\tcontinue;\n\n\t\ti = oobfree.offset >= 8 ? : 8;\n\t\tfor (; i < oobfree.offset + oobfree.length && i < 16; i++)\n\t\t\tfreebytes |= BIT(i - 8);\n\n\t\tif (freebytes == 0xff)\n\t\t\treturn 0;\n\t}\n\n\treturn -ENOTSUPP;\n}\n\n> + *\n> + */\n> +static u_int sharpsl_nand_get_logical_num(u_char *oob)\n> +{\n> +\tu16 us;\n> +\tint good0, good1;\n> +\n> +\tif (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] &&\n> +\t oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) {\n> +\t\tgood0 = NAND_NOOB_LOGADDR_00;\n> +\t\tgood1 = NAND_NOOB_LOGADDR_01;\n> +\t} else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] &&\n> +\t\t oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) {\n> +\t\tgood0 = NAND_NOOB_LOGADDR_10;\n> +\t\tgood1 = NAND_NOOB_LOGADDR_11;\n> +\t} else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] &&\n> +\t\t oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) {\n> +\t\tgood0 = NAND_NOOB_LOGADDR_20;\n> +\t\tgood1 = NAND_NOOB_LOGADDR_21;\n> +\t} else {\n> +\t\t/* wrong oob fingerprint, maybe here by mistake? */\n> +\t\treturn UINT_MAX;\n> +\t}\n> +\n> +\tus = oob[good0] | oob[good1] << 8;\n> +\n> +\t/* parity check */\n> +\tif (hweight16(us) & BLOCK_UNMASK_COMPLEMENT)\n> +\t\treturn (UINT_MAX - 1);\n> +\n> +\t/* reserved */\n> +\tif (us == BLOCK_IS_RESERVED)\n> +\t\treturn BLOCK_IS_RESERVED;\n> +\telse\n> +\t\treturn (us & BLOCK_UNMASK) >> 1;\n> +}\n> +\n> +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size,\n> +\t\t\t\t struct sharpsl_ftl *ftl)\n> +{\n> +\tu_int block_num, log_num, phymax;\n> +\tloff_t block_adr;\n> +\tu_char *oob;\n> +\tint i, ret;\n> +\n> +\toob = kzalloc(mtd->oobsize, GFP_KERNEL);\n> +\tif (!oob)\n> +\t\treturn -ENOMEM;\n> +\n> +\t/* initialize management structure */\n> +\tphymax = (partition_size / mtd->erasesize);\n> +\n> +\t/* FTL reserves 5% of the blocks + 1 spare */\n> +\tftl->logmax = ((phymax * 95) / 100) - 1;\n> +\n> +\tftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int),\n> +\t\t\t\t GFP_KERNEL);\n> +\tif (!ftl->log2phy) {\n> +\t\tret = -ENOMEM;\n> +\t\tgoto exit;\n> +\t}\n> +\n> +\t/* initialize ftl->log2phy */\n> +\tfor (i = 0; i < ftl->logmax; i++)\n> +\t\tftl->log2phy[i] = UINT_MAX;\n> +\n> +\t/* create physical-logical table */\n> +\tfor (block_num = 0; block_num < phymax; block_num++) {\n> +\t\tblock_adr = block_num * mtd->erasesize;\n> +\n> +\t\tif (mtd_block_isbad(mtd, block_adr))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))\n> +\t\t\tcontinue;\n> +\n> +\t\t/* get logical block */\n> +\t\tlog_num = sharpsl_nand_get_logical_num(oob);\n> +\n> +\t\t/* FTL is not used? Exit here if the oob fingerprint is wrong */\n> +\t\tif (log_num == UINT_MAX) {\n> +\t\t\tpr_info(\"sharpslpart: Sharp SL FTL not found.\\n\");\n> +\t\t\tret = -EINVAL;\n> +\t\t\tgoto exit;\n> +\t\t}\n> +\n> +\t\t/* skip out of range and not unique values */\n> +\t\tif (log_num < ftl->logmax) {\n> +\t\t\tif (ftl->log2phy[log_num] == UINT_MAX)\n> +\t\t\t\tftl->log2phy[log_num] = block_num;\n> +\t\t}\n> +\t}\n> +\n> +\tpr_info(\"Sharp SL FTL: %d blocks used (%d logical, %d reserved)\\n\",\n> +\t\tphymax, ftl->logmax,\n> +\t\tphymax - ftl->logmax);\n> +\n> +\tret = 0;\n> +exit:\n> +\tkfree(oob);\n> +\treturn ret;\n> +}\n> +\n> +void sharpsl_nand_cleanup_logical(struct sharpsl_ftl *ftl)\n> +{\n> +\tkfree(ftl->log2phy);\n> +}\n> +\n> +static int sharpsl_nand_read_laddr(struct mtd_info *mtd,\n> +\t\t\t\t loff_t from,\n> +\t\t\t\t size_t len,\n> +\t\t\t\t u_char *buf,\n> +\t\t\t\t struct sharpsl_ftl *ftl)\n> +{\n> +\tu_int log_num, log_new;\n> +\tu_int block_num;\n> +\tloff_t block_adr;\n> +\tloff_t block_ofs;\n> +\tsize_t retlen;\n> +\tint err;\n> +\n> +\tlog_num = (u32)from / mtd->erasesize;\n> +\tlog_new = ((u32)from + len - 1) / mtd->erasesize;\n> +\n> +\tif (len <= 0 || log_num >= ftl->logmax || log_new > log_num)\n> +\t\treturn -EINVAL;\n> +\n> +\tblock_num = ftl->log2phy[log_num];\n> +\tblock_adr = block_num * mtd->erasesize;\n> +\tblock_ofs = (u32)from % mtd->erasesize;\n> +\n> +\terr = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf);\n> +\t/* Ignore corrected ECC errors */\n> +\tif (mtd_is_bitflip(err))\n> +\t\terr = 0;\n> +\tif (!err && retlen != len)\n> +\t\terr = -EIO;\n> +\tif (err)\n> +\t\tpr_err(\"sharpslpart: error, read failed at %#llx\\n\",\n> +\t\t block_adr + block_ofs);\n> +\n> +\treturn err;\n> +}\n> +\n> +/*\n> + * MTD Partition Parser\n> + *\n> + * Sample values read from SL-C860\n> + *\n> + * # cat /proc/mtd\n> + * dev: size erasesize name\n> + * mtd0: 006d0000 00020000 \"Filesystem\"\n> + * mtd1: 00700000 00004000 \"smf\"\n> + * mtd2: 03500000 00004000 \"root\"\n> + * mtd3: 04400000 00004000 \"home\"\n> + *\n> + * PARTITIONINFO1\n> + * 0x00060000: 00 00 00 00 00 00 70 00 42 4f 4f 54 00 00 00 00 ......p.BOOT....\n> + * 0x00060010: 00 00 70 00 00 00 c0 03 46 53 52 4f 00 00 00 00 ..p.....FSRO....\n> + * 0x00060020: 00 00 c0 03 00 00 00 04 46 53 52 57 00 00 00 00 ........FSRW....\n> + * 0x00060030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................\n> + *\n> + */\n> +struct sharpsl_nand_partinfo {\n> +\t__le32 start;\n> +\t__le32 end;\n> +\t__be32 magic;\n> +\tu32 reserved;\n> +};\n> +\n> +static int sharpsl_nand_read_partinfo(struct mtd_info *master,\n> +\t\t\t\t loff_t from,\n> +\t\t\t\t size_t len,\n> +\t\t\t\t struct sharpsl_nand_partinfo *buf,\n> +\t\t\t\t struct sharpsl_ftl *ftl)\n> +{\n> +\tint ret;\n> +\n> +\tret = sharpsl_nand_read_laddr(master, (u32)from, len, (u_char *)buf,\n> +\t\t\t\t ftl);\n> +\tif (ret)\n> +\t\tgoto exit;\n> +\n> +\t/* check for magics */\n> +\tif (be32_to_cpu(buf[0].magic) != BOOT_MAGIC ||\n> +\t be32_to_cpu(buf[1].magic) != FSRO_MAGIC ||\n> +\t be32_to_cpu(buf[2].magic) != FSRW_MAGIC) {\n> +\t\tpr_err(\"sharpslpart: magic values mismatch\\n\");\n> +\t\tret = -EINVAL;\n> +\t\tgoto exit;\n> +\t}\n> +\n> +\t/* fixup for hardcoded value 64 MiB (for older models) */\n> +\tbuf[2].end = cpu_to_le32(master->size);\n> +\n> +\t/* extra sanity check */\n> +\tif (le32_to_cpu(buf[0].end) <= le32_to_cpu(buf[0].start) ||\n> +\t le32_to_cpu(buf[1].start) < le32_to_cpu(buf[0].end) ||\n> +\t le32_to_cpu(buf[1].end) <= le32_to_cpu(buf[1].start) ||\n> +\t le32_to_cpu(buf[2].start) < le32_to_cpu(buf[1].end) ||\n> +\t le32_to_cpu(buf[2].end) <= le32_to_cpu(buf[2].start)) {\n> +\t\tpr_err(\"sharpslpart: partition sizes mismatch\\n\");\n> +\t\tret = -EINVAL;\n> +\t\tgoto exit;\n> +\t}\n> +\n> +\tret = 0;\n> +exit:\n> +\treturn ret;\n> +}\n> +\n> +static int sharpsl_parse_mtd_partitions(struct mtd_info *master,\n> +\t\t\t\t\tconst struct mtd_partition **pparts,\n> +\t\t\t\t\tstruct mtd_part_parser_data *data)\n> +{\n> +\tstruct sharpsl_ftl ftl;\n> +\tstruct sharpsl_nand_partinfo buf[SHARPSL_NAND_PARTS];\n> +\tstruct mtd_partition *sharpsl_nand_parts;\n> +\tint err, ret;\n> +\n> +\t/* init logical mgmt (FTL) */\n> +\terr = sharpsl_nand_init_logical(master, SHARPSL_FTL_PART_SIZE, &ftl);\n> +\tif (err)\n> +\t\treturn err;\n> +\n> +\t/* read and validate first partition table */\n> +\tpr_info(\"sharpslpart: using first partition table\\n\");\n> +\terr = sharpsl_nand_read_partinfo(master,\n> +\t\t\t\t\t PARAM_BLOCK_PARTITIONINFO1,\n> +\t\t\t\t\t sizeof(buf), buf, &ftl);\n> +\tif (err) {\n> +\t\t/* fallback: read second partition table */\n> +\t\tpr_warn(\"sharpslpart: retry using second partition table\\n\");\n> +\t\tret = sharpsl_nand_read_partinfo(master,\n> +\t\t\t\t\t\t PARAM_BLOCK_PARTITIONINFO2,\n> +\t\t\t\t\t\t sizeof(buf), buf, &ftl);\n> +\t\tif (ret) {\n> +\t\t\tpr_err(\"sharpslpart: partition tables are invalid\\n\");\n> +\t\t\tsharpsl_nand_cleanup_logical(&ftl);\n> +\t\t\treturn ret;\n> +\t\t}\n> +\t}\n> +\n> +\t/* cleanup logical mgmt (FTL) */\n> +\tsharpsl_nand_cleanup_logical(&ftl);\n> +\n> +\tsharpsl_nand_parts = kzalloc(sizeof(*sharpsl_nand_parts) *\n> +\t\t\t\t SHARPSL_NAND_PARTS, GFP_KERNEL);\n> +\tif (!sharpsl_nand_parts)\n> +\t\treturn -ENOMEM;\n> +\n> +\t/* original names */\n> +\tsharpsl_nand_parts[0].name = \"smf\";\n> +\tsharpsl_nand_parts[0].offset = le32_to_cpu(buf[0].start);\n> +\tsharpsl_nand_parts[0].size = le32_to_cpu(buf[0].end) -\n> +\t\t\t\t le32_to_cpu(buf[0].start);\n> +\n> +\tsharpsl_nand_parts[1].name = \"root\";\n> +\tsharpsl_nand_parts[1].offset = le32_to_cpu(buf[1].start);\n> +\tsharpsl_nand_parts[1].size = le32_to_cpu(buf[1].end) -\n> +\t\t\t\t le32_to_cpu(buf[1].start);\n> +\n> +\tsharpsl_nand_parts[2].name = \"home\";\n> +\tsharpsl_nand_parts[2].offset = le32_to_cpu(buf[2].start);\n> +\tsharpsl_nand_parts[2].size = le32_to_cpu(buf[2].end) -\n> +\t\t\t\t le32_to_cpu(buf[2].start);\n> +\n> +\t*pparts = sharpsl_nand_parts;\n> +\treturn SHARPSL_NAND_PARTS;\n> +}\n> +\n> +static struct mtd_part_parser sharpsl_mtd_parser = {\n> +\t.parse_fn = sharpsl_parse_mtd_partitions,\n> +\t.name = \"sharpslpart\",\n> +};\n> +module_mtd_part_parser(sharpsl_mtd_parser);\n> +\n> +MODULE_LICENSE(\"GPL\");\n> +MODULE_AUTHOR(\"Andrea Adami \");\n> +MODULE_DESCRIPTION(\"MTD partitioning for NAND flash on Sharp SL Series\");","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (mailfrom)\n\tsmtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133;\n\thelo=bombadil.infradead.org;\n\tenvelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"ERn9is5Z\"; \n\tdkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xddnR0xt3z9t3P\n\tfor ;\n\tFri, 25 Aug 2017 08:12:11 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dl0M4-00043O-Ko; Thu, 24 Aug 2017 22:12:00 +0000","from mail.free-electrons.com ([62.4.15.54])\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dl0Ly-0003YJ-HF\n\tfor linux-mtd@lists.infradead.org; Thu, 24 Aug 2017 22:11:58 +0000","by mail.free-electrons.com (Postfix, from userid 110)\n\tid CB8D520867; Fri, 25 Aug 2017 00:11:31 +0200 (CEST)","from bbrezillon (unknown [91.160.177.164])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id 67DD820860;\n\tFri, 25 Aug 2017 00:11:31 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:\n\tMessage-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=PiyaOOUxbuwqr7o6m9KukVpuVIWieHht6v8D6sWV75M=;\n\tb=ERn9is5ZLIiHZy\n\tDPflKRJPZh13gKywdFa+yuGND7hFpitPgdBq9uszDn5Yh8Jnj5boJ3/Ej7wKIG0Kl4T+/RHWn/CQr\n\toPnb6sGcToWDtes2tpL9r9IcF5axBiShrfDjZkwz4hn2/jS3FMWjKoWffjqaaDTIAjKDAhLCTZO9R\n\tXG2P3Hyo4Pb3ZGtRXRwLqKv+ssuyysg/Gw4GBKO5JRgi/e3mlb3AJ9Kt6FG5hOD3QneVgZusLrrk7\n\tIZlSdTP6hI6IeYl2DeKwMrfAheaj480LM00u1CEGCt4jR3In/rpynmWUa0pYcNgJH/k/DynmPI7Vx\n\tGXEfVh1R/Hv3kBlXk77w==;","X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT\n\tshortcircuit=ham autolearn=disabled version=3.4.0","Date":"Fri, 25 Aug 2017 00:11:29 +0200","From":"Boris Brezillon ","To":"Andrea Adami ","Subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","Message-ID":"<20170825001129.091badb4@bbrezillon>","In-Reply-To":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>","References":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>","X-Mailer":"Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu)","MIME-Version":"1.0","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170824_151154_950248_5C966EEA ","X-CRM114-Status":"GOOD ( 39.70 )","X-Spam-Score":"-1.9 (-)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-1.9 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay\n\tdomain\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]","X-BeenThere":"linux-mtd@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"Linux MTD discussion mailing list ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dmitry Eremin-Solenikov ,\n\tRichard Weinberger ,\n\tRobert Jarzmik , linux-kernel@vger.kernel.org,\n\tHaojian Zhuang , \n\tMarek Vasut , linux-mtd@lists.infradead.org,\n\tCyrille Pitchen ,\n\t=?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= ,\n\tBrian Norris ,\n\tDavid Woodhouse ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"\"linux-mtd\" ","Errors-To":"linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org"}},{"id":1757037,"web_url":"http://patchwork.ozlabs.org/comment/1757037/","msgid":"<20170825045314.GC68252@google.com>","list_archive_url":null,"date":"2017-08-25T04:53:14","subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","submitter":{"id":5014,"url":"http://patchwork.ozlabs.org/api/people/5014/","name":"Brian Norris","email":"computersforpeace@gmail.com"},"content":"On Thu, Aug 24, 2017 at 01:27:10PM +0200, Boris Brezillon wrote:\n> On Thu, 24 Aug 2017 12:30:02 +0200\n> Andrea Adami wrote:\n> \n> > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon\n> > wrote:\n> > > On Thu, 24 Aug 2017 11:19:56 +0200\n> > > Andrea Adami wrote:\n\n...\n\n> > >> >> + /* create physical-logical table */\n> > >> >> + for (block_num = 0; block_num < phymax; block_num++) {\n> > >> >> + block_adr = block_num * mtd->erasesize;\n> > >> >> +\n> > >> >> + if (mtd_block_isbad(mtd, block_adr))\n> > >> >> + continue;\n> > >> >> +\n> > >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))\n> > >> >> + continue;\n> > >> >> +\n> > >> >> + /* get logical block */\n> > >> >> + log_num = sharpsl_nand_get_logical_num(oob);\n> > >> >> +\n> > >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */\n> > >> >> + if (log_num == UINT_MAX) {\n> > >> >> + pr_info(\"sharpslpart: Sharp SL FTL not found.\\n\");\n> > >> >> + ret = -EINVAL;\n> > >> >> + goto exit;\n> > >> >> + } \n> > >\n> > > Okay, I overlooked that part. Why do you exit if there's a fingerprint\n> > > mismatch? Can't you just ignore this physical block and continue\n> > > scanning the remaining ones? \n> > \n> > Norris asked to quit immediately in this case.\n> > https://patchwork.kernel.org/patch/9758361/\n\nI didn't specifically ask for you to quit in *that* case. Quoting myself\nhere, as you did:\n\n> > \"I wouldn't expect people to want to use this parser, but if we have a\n> > quick way to say \"this doesn't match, skip me\", then that would be\n> > helpful.\"\n> > \"We don't want to waste too much time scanning for this partition\n> > table if possible.\"\n\nThat means, is there something (not necessarily writting in the\n\"original code\" that you're massaging) that could be used to reliably\ndetect that this is/isn't a valid \"Sharp FTL\"? I don't think the check\nyou wrote is a good one. Particularly, you *don't* want to just abort\ncompletely because there's one corrupt block. This check is a\nreliability check (so you can possibly ignore old/bad copies and skip\nonto better blocks), not a validity check. It is counter-productive to\nabort here, IIUC.\n\n> Actually, you don't save much by exiting on \"bad OOB fingerprint\". If\n> you look at the code you'll see that the only thing you check is\n> whether some oob portions are equal or not, and most of the time the\n> OOB area is left untouched by the upper layer, which means all free\n> bytes will be left to 0xff, which in turn means the \"is fingerprint\n> good?\" test will pass.\n\nAgreed.\n\nI'd drop this \"abort early\" check and just admit that it's not possible\nto do what I asked.\n\n> > Now we are quitting ever before checking for parity erors ...\n> \n> Honestly, I'd recommend not using this parser for anything but SHARPSL\n> platforms, so I don't think we care much about the \"scan all blocks\"\n> overhead.\n\nSounds about right.\n\n> Moreover, the sharpsl parser is the last one in the\n> part_parsers list, so it should be quite fast if the user specifies a\n> valid mtdparts= on the cmdline or valid partitions in the DT.\n\nBrian\n\nP.S. I alluded to it earlier, but I figured I should write it down\nproperly here sometime, as food for thought; you don't actually need any\nof this parser at all if you're willing to contruct an initramfs that\nwill do the parsing in user space (e.g., some scripting and 'nanddump';\nor link to libmtd) and then add partitions yourself (e.g., with\n'mtdpart add ...', or calling the BLKPG ioctls directly). This would\njust require you have a kernel with CONFIG_MTD_PARTITIONED_MASTER=y.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (mailfrom)\n\tsmtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133;\n\thelo=bombadil.infradead.org;\n\tenvelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"A5zGrIn1\"; \n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"aQSdGVdA\"; dkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xdphx4qbtz9t5h\n\tfor ;\n\tFri, 25 Aug 2017 14:53:53 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dl6co-0007Yy-SP; Fri, 25 Aug 2017 04:53:42 +0000","from mail-pg0-x243.google.com ([2607:f8b0:400e:c05::243])\n\tby bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dl6cl-0007Xr-Bo\n\tfor linux-mtd@lists.infradead.org; Fri, 25 Aug 2017 04:53:41 +0000","by mail-pg0-x243.google.com with SMTP id 83so2253255pgb.3\n\tfor ;\n\tThu, 24 Aug 2017 21:53:18 -0700 (PDT)","from google.com ([2620:0:1000:1600:fc53:4f69:5880:26ca])\n\tby smtp.gmail.com with ESMTPSA id\n\th14sm11721127pgn.34.2017.08.24.21.53.16\n\t(version=TLS1_2 cipher=AES128-SHA bits=128/128);\n\tThu, 24 Aug 2017 21:53:16 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:\n\tMessage-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=w0qhX2bJ12J0Z25N5frEhjc2mDROzAdlnA7euRq0wk8=;\n\tb=A5zGrIn1YFiOj5\n\tmWrWEJApMtlvp/YfXM9UE2hNe6Ki3Mii1UH07up3HS0+0Rmv8BKimzO0tl0T4f9JKygG2DwkJb9LT\n\tEHU3lHoqMASX4QeRCiicgelW2Q4DcAq4K08kb56PCRv4xOI04v576LuNBE3VUTQFuWsgkWpjSmGS8\n\tSWGgHwFr/eWUVmfGA2JciSVwj0LGWeH9DLviSPGk6gvSczOFiz4cCQHdxei8qD4UikPcf2sKLD0ba\n\t45XFkRuVQXJ8ERWSDRjykS6udSDG6L2J98EaYAHwXB+aqWFKdtq89B1s1p4tYz8Z5aaCabH+eTqaV\n\t2lr2XDQi5hpbNi7stgxg==;","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=coMknPVEay71v3ellETOw86hDeoFP+0dKVJc0hV9e/c=;\n\tb=aQSdGVdA4Qfgr2dUrMaeAF6TbMuFHJlErrV09miUJUYM+EhltrIvSkyY6wYfV02zOb\n\tcBE+FU8+nUz+EWhM2JbtnSdIBs7BdnwNBJIPLl6TX4+qJn4jxYynN5nVSdOmHtXu6PfY\n\t1BKwSwSP6XAr20K7Te7Am9G20fVzt7ZoIT4cBXDrsGTeNHdkw+PlYSullN+Ulo0SlvBJ\n\tFWE7sOwZvi+9UZwyH/E3u4xnorXZ7/QgAn+Dtz38NXgFaGrHTS9RlPiCkT/emV2OJeYz\n\tAutghyO9kmoTuMWxS0LpAIxE8wfzsbXOLGCYTKO14v08IGDx65OIfHMdF/7+iYbD85sr\n\tU9iw=="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=coMknPVEay71v3ellETOw86hDeoFP+0dKVJc0hV9e/c=;\n\tb=OgVERX84rMjMYhKA2WlA3EN+qgG05dh6E0kHn9aGgfgnuSHwhOIqcxT0rYOETCQA7p\n\tvl6e+p20Tr7ltFFCMWSjxHUYIOe3K/5Qp5hIfRh4KRa8vMixXczV0L8e+6b6DNl6NmKJ\n\tAelvJvKc9qrFJLr2eR6MjM8L/LoE+Sf1SkoToS8m9X3icvfqyyFKZ+TMG5cFeQkWuXyb\n\tuBlocV/+hkwS1KzI1A6c1ok3BepRhHfxOD3OJtu1JOaZeAqKEtK+q0D9AfQGhiNpmBo3\n\tYQpfn8LoUWE8jBmqwEDJEsqjv3APbvWgQ6Ocv1rhMwN0figih5zVoILUf2nKwJlwDBWt\n\t0s3g==","X-Gm-Message-State":"AHYfb5h65R9xe45CxH2bkt+7ecJUfdafy1v/mhjGfIwRfBtBJ2No1D0S\n\tyf6nTcPPKfeq6A==","X-Received":"by 10.84.234.23 with SMTP id m23mr9337146plk.427.1503636797860; \n\tThu, 24 Aug 2017 21:53:17 -0700 (PDT)","Date":"Thu, 24 Aug 2017 21:53:14 -0700","From":"Brian Norris ","To":"Boris Brezillon ","Subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","Message-ID":"<20170825045314.GC68252@google.com>","References":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>\n\t<20170822145424.54e57b94@bbrezillon>\n\t\n\t<20170824120428.46e266c7@bbrezillon>\n\t\n\t<20170824132710.5fdea20c@bbrezillon>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20170824132710.5fdea20c@bbrezillon>","User-Agent":"Mutt/1.5.21 (2010-09-15)","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170824_215339_437627_65BF7382 ","X-CRM114-Status":"GOOD ( 27.48 )","X-Spam-Score":"-2.0 (--)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-2.0 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at http://www.dnswl.org/,\n\tno\n\ttrust [2607:f8b0:400e:c05:0:0:0:243 listed in] [list.dnswl.org]\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\n\tprovider (computersforpeace[at]gmail.com)\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]\n\t-0.1 DKIM_VALID Message has at least one valid DKIM or DK signature\n\t0.1 DKIM_SIGNED Message has a DKIM or DK signature,\n\tnot necessarily valid\n\t-0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from\n\tauthor's domain","X-BeenThere":"linux-mtd@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"Linux MTD discussion mailing list ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dmitry Eremin-Solenikov ,\n\tRichard Weinberger ,\n\tRobert Jarzmik , linux-kernel@vger.kernel.org,\n\tHaojian Zhuang , \n\tMarek Vasut , linux-mtd@lists.infradead.org,\n\tCyrille Pitchen ,\n\t=?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= ,\n\tAndrea Adami ,\n\tDavid Woodhouse ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"\"linux-mtd\" ","Errors-To":"linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org"}},{"id":1757655,"web_url":"http://patchwork.ozlabs.org/comment/1757655/","msgid":"","list_archive_url":null,"date":"2017-08-25T17:50:25","subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","submitter":{"id":30311,"url":"http://patchwork.ozlabs.org/api/people/30311/","name":"Andrea Adami","email":"andrea.adami@gmail.com"},"content":"On Fri, Aug 25, 2017 at 6:53 AM, Brian Norris\n wrote:\n> On Thu, Aug 24, 2017 at 01:27:10PM +0200, Boris Brezillon wrote:\n>> On Thu, 24 Aug 2017 12:30:02 +0200\n>> Andrea Adami wrote:\n>>\n>> > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon\n>> > wrote:\n>> > > On Thu, 24 Aug 2017 11:19:56 +0200\n>> > > Andrea Adami wrote:\n>\n> ...\n>\n>> > >> >> + /* create physical-logical table */\n>> > >> >> + for (block_num = 0; block_num < phymax; block_num++) {\n>> > >> >> + block_adr = block_num * mtd->erasesize;\n>> > >> >> +\n>> > >> >> + if (mtd_block_isbad(mtd, block_adr))\n>> > >> >> + continue;\n>> > >> >> +\n>> > >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))\n>> > >> >> + continue;\n>> > >> >> +\n>> > >> >> + /* get logical block */\n>> > >> >> + log_num = sharpsl_nand_get_logical_num(oob);\n>> > >> >> +\n>> > >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */\n>> > >> >> + if (log_num == UINT_MAX) {\n>> > >> >> + pr_info(\"sharpslpart: Sharp SL FTL not found.\\n\");\n>> > >> >> + ret = -EINVAL;\n>> > >> >> + goto exit;\n>> > >> >> + }\n>> > >\n>> > > Okay, I overlooked that part. Why do you exit if there's a fingerprint\n>> > > mismatch? Can't you just ignore this physical block and continue\n>> > > scanning the remaining ones?\n>> >\n>> > Norris asked to quit immediately in this case.\n>> > https://patchwork.kernel.org/patch/9758361/\n>\n> I didn't specifically ask for you to quit in *that* case. Quoting myself\n> here, as you did:\n>\n>> > \"I wouldn't expect people to want to use this parser, but if we have a\n>> > quick way to say \"this doesn't match, skip me\", then that would be\n>> > helpful.\"\n>> > \"We don't want to waste too much time scanning for this partition\n>> > table if possible.\"\n>\n> That means, is there something (not necessarily writting in the\n> \"original code\" that you're massaging) that could be used to reliably\n> detect that this is/isn't a valid \"Sharp FTL\"? I don't think the check\n> you wrote is a good one. Particularly, you *don't* want to just abort\n> completely because there's one corrupt block. This check is a\n> reliability check (so you can possibly ignore old/bad copies and skip\n> onto better blocks), not a validity check. It is counter-productive to\n> abort here, IIUC.\n>\n>> Actually, you don't save much by exiting on \"bad OOB fingerprint\". If\n>> you look at the code you'll see that the only thing you check is\n>> whether some oob portions are equal or not, and most of the time the\n>> OOB area is left untouched by the upper layer, which means all free\n>> bytes will be left to 0xff, which in turn means the \"is fingerprint\n>> good?\" test will pass.\n>\n> Agreed.\n>\n> I'd drop this \"abort early\" check and just admit that it's not possible\n> to do what I asked.\n\nOk then, I have misunderstood you. The only time-saving would be to\nskip the creation of the table.\nIn the specific cases, the older devices with erasesize 16KiB have\njust 448 blocks and the routine doesn't really slow down.\nI can imagine it would be a breeze for modern devices.\n\nI will just return -EINVAl and this error, as well as the eventual\nparity-check error on a specific block, will be cut-off by the checks\nand the cycle will move to next block.\n\nSo I understand the sharpsl_nand_check_ooblayout() could be also avoided.\nPlease confirm this, thanks.\n\n>\n>> > Now we are quitting ever before checking for parity erors ...\n>>\n>> Honestly, I'd recommend not using this parser for anything but SHARPSL\n>> platforms, so I don't think we care much about the \"scan all blocks\"\n>> overhead.\n>\n> Sounds about right.\n>\n>> Moreover, the sharpsl parser is the last one in the\n>> part_parsers list, so it should be quite fast if the user specifies a\n>> valid mtdparts= on the cmdline or valid partitions in the DT.\n>\n> Brian\n>\n> P.S. I alluded to it earlier, but I figured I should write it down\n> properly here sometime, as food for thought; you don't actually need any\n> of this parser at all if you're willing to contruct an initramfs that\n> will do the parsing in user space (e.g., some scripting and 'nanddump';\n> or link to libmtd) and then add partitions yourself (e.g., with\n> 'mtdpart add ...', or calling the BLKPG ioctls directly). This would\n> just require you have a kernel with CONFIG_MTD_PARTITIONED_MASTER=y.\n\nBrian, the first problem in this approach is the size: there are only\n1264KiB available for the zImage.\nWe accepted the challenge and wrote linux-kexecboot bootloader, a\nkernel + couple of klibc-static binaries in the initramfs and we\nspecial-cased the Zaurus adding the partition parser in userspace.\n\nNow, as widely discussed before, there are limits to this solution:\n- first, we cannot oblige to flash this kernel+initramfs.\n- second, we need to build one kernel+initramfs for each model\n- third, the machines are really the same from kernel pov, just mtdparts differ\n\nSoon we'll test a single kernel for the pxa25x and for the pxa27x models.\n\nHonestly I did just glimpse at BLKPG ioctl because resizing for mtd\nwas added recently (iirc).\nAs maintainer for the cross-toolchain and of kexecboot I am of the\nopinion that the cleanest solution is the in-kernel partition parser.\n\nThanks for your time reviewing this...is just a partition parser for\nsome of the first commercial linux handhelds.\n\nCheers\nAndrea","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (mailfrom)\n\tsmtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133;\n\thelo=bombadil.infradead.org;\n\tenvelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"UNnal9hG\"; \n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"TJU4vm3d\"; dkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xf7y028bFz9sPk\n\tfor ;\n\tSat, 26 Aug 2017 03:51:20 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dlIkv-0001ws-U2; Fri, 25 Aug 2017 17:50:53 +0000","from mail-wm0-x241.google.com ([2a00:1450:400c:c09::241])\n\tby bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dlIkr-0001ZP-2r\n\tfor linux-mtd@lists.infradead.org; Fri, 25 Aug 2017 17:50:51 +0000","by mail-wm0-x241.google.com with SMTP id i76so518329wme.1\n\tfor ;\n\tFri, 25 Aug 2017 10:50:27 -0700 (PDT)","by 10.80.154.1 with HTTP; Fri, 25 Aug 2017 10:50:25 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:\n\tReferences:In-Reply-To:MIME-Version:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=ef2eVMWqiMq9+aUj8Za31gVoBs5UHwHv7OR6Mbp97qQ=;\n\tb=UNnal9hGbPj8wm\n\t7CYtubMMPJLbLeRssyM5Zj83wyq0q7upCnN60OcI0k+S7D4w1xwUrSWO0gyt8VmAfSLs2tFzbCtiw\n\thmGPWNPNTMMvbBJtbtoA/jPSWiedZOsqDWyPV9RAGkQE2M3APMTDQt4TLV8Hn+o4jaQ7EWdL3A3MJ\n\totPxzrg9+SFIms2cvADd4c8a8imrNTFaHu0IJRzT6EzVdKfMWlcoO7MHcFnJU5mnU0q3opVxnHN90\n\tCLJb70/PT8cSa2ZCBlJ4S+NsKSJZ3bNay4tYrjicc/PDVMynpukZ/4I47AsPvfYsWDncsQ+1XrHlA\n\tlLL0ilan+NJjW0c5JtZQ==;","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=qEGwF3KzaBHUTBT+sizXNRSFX8EZm7SFS1mcRUTFPPQ=;\n\tb=TJU4vm3dY+kdGm0vbhgeOieuSA1ItXcZzJCwdC4CsL6KtOjFnQ+rY4/tvbXj1A8+U6\n\tLnDlysI5GfVxAaA37GJvY3OCePVq5TFZpeSpjA/fNrpavlGLWdVrUJfFPqO/coClDd0C\n\tQKXfn7hbhJn9jcBE9+2aKgaFFKorRmhzg5cVjCGDN06nPUJqV8Hagu5prHCSGVff0MFP\n\t/dtVjIJdScX7ga9Bd0oRNvKNrzdDD360aQtGEf8yVnfZTSdGhqQVg+o/VaM1z+5TB8/V\n\tyZUk6nhuvvE53xUOcUkd4MRY2rxjnS6eg/fj5/jwE9OYcEdUjGpBd7nUK6mj3PqsF2Z1\n\tmMdw=="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=qEGwF3KzaBHUTBT+sizXNRSFX8EZm7SFS1mcRUTFPPQ=;\n\tb=JXMd7i7bc01x+WgQADCJL2yYsyghNmLclx3JQrl9rinWJPggcidpWi+GHJxUASJRn9\n\t4l7nWquJW1JcL54/JdQtZ3jHWxVGXy/a4jE+u9L71VEzA3YGq01GfhvSQwMIu0xSe0+r\n\tWYHEce3v//rS+qMRi4eDSsy9kQJiYGia8xyELQ3Zuq3+ePROKPagze+vuYhKwwNyAaCc\n\ttOVIh3ZWPLdM7ERoUZp/PLP1fmHSa1HUAyQhAaD3SMc+inGJkIrrPzI63ifadFexaRjG\n\t8BsW8x6Fvnw3ppQU3C4E2sAqR4+U9b9CvSuoFvNRB88KpQKFeNmrIZl+UHUEGmxEVLgL\n\t9rug==","X-Gm-Message-State":"AHYfb5isn41vSfGvj9323rHL/oBbxy7nsz08Ii9MT4ANR/NV05rFypJL\n\tIGjcv25Dz1UeyvFeMNGoxrxCVoE7Xw==","X-Received":"by 10.80.215.138 with SMTP id w10mr10561076edi.240.1503683426485;\n\tFri, 25 Aug 2017 10:50:26 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170825045314.GC68252@google.com>","References":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>\n\t<20170822145424.54e57b94@bbrezillon>\n\t\n\t<20170824120428.46e266c7@bbrezillon>\n\t\n\t<20170824132710.5fdea20c@bbrezillon>\n\t<20170825045314.GC68252@google.com>","From":"Andrea Adami ","Date":"Fri, 25 Aug 2017 19:50:25 +0200","Message-ID":"","Subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","To":"Brian Norris ","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170825_105049_359447_C0AADC4C ","X-CRM114-Status":"GOOD ( 31.92 )","X-Spam-Score":"-2.0 (--)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-2.0 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at http://www.dnswl.org/,\n\tno\n\ttrust [2a00:1450:400c:c09:0:0:0:241 listed in] [list.dnswl.org]\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\n\tprovider (andrea.adami[at]gmail.com)\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]\n\t-0.1 DKIM_VALID Message has at least one valid DKIM or DK signature\n\t0.1 DKIM_SIGNED Message has a DKIM or DK signature,\n\tnot necessarily valid\n\t-0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from\n\tauthor's domain","X-BeenThere":"linux-mtd@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"Linux MTD discussion mailing list ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Boris Brezillon ,\n\tDmitry Eremin-Solenikov ,\n\tRichard Weinberger ,\n\tRobert Jarzmik , linux-kernel@vger.kernel.org,\n\tHaojian Zhuang , \n\tMarek Vasut , linux-mtd@lists.infradead.org,\n\tCyrille Pitchen ,\n\t=?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= ,\n\tDavid Woodhouse ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"\"linux-mtd\" ","Errors-To":"linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org"}},{"id":1757771,"web_url":"http://patchwork.ozlabs.org/comment/1757771/","msgid":"<20170825234816.2d6d31b4@bbrezillon>","list_archive_url":null,"date":"2017-08-25T21:48:16","subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","submitter":{"id":63120,"url":"http://patchwork.ozlabs.org/api/people/63120/","name":"Boris Brezillon","email":"boris.brezillon@free-electrons.com"},"content":"On Fri, 25 Aug 2017 19:50:25 +0200\nAndrea Adami wrote:\n\n> On Fri, Aug 25, 2017 at 6:53 AM, Brian Norris\n> wrote:\n> > On Thu, Aug 24, 2017 at 01:27:10PM +0200, Boris Brezillon wrote: \n> >> On Thu, 24 Aug 2017 12:30:02 +0200\n> >> Andrea Adami wrote:\n> >> \n> >> > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon\n> >> > wrote: \n> >> > > On Thu, 24 Aug 2017 11:19:56 +0200\n> >> > > Andrea Adami wrote: \n> >\n> > ...\n> > \n> >> > >> >> + /* create physical-logical table */\n> >> > >> >> + for (block_num = 0; block_num < phymax; block_num++) {\n> >> > >> >> + block_adr = block_num * mtd->erasesize;\n> >> > >> >> +\n> >> > >> >> + if (mtd_block_isbad(mtd, block_adr))\n> >> > >> >> + continue;\n> >> > >> >> +\n> >> > >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))\n> >> > >> >> + continue;\n> >> > >> >> +\n> >> > >> >> + /* get logical block */\n> >> > >> >> + log_num = sharpsl_nand_get_logical_num(oob);\n> >> > >> >> +\n> >> > >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */\n> >> > >> >> + if (log_num == UINT_MAX) {\n> >> > >> >> + pr_info(\"sharpslpart: Sharp SL FTL not found.\\n\");\n> >> > >> >> + ret = -EINVAL;\n> >> > >> >> + goto exit;\n> >> > >> >> + } \n> >> > >\n> >> > > Okay, I overlooked that part. Why do you exit if there's a fingerprint\n> >> > > mismatch? Can't you just ignore this physical block and continue\n> >> > > scanning the remaining ones? \n> >> >\n> >> > Norris asked to quit immediately in this case.\n> >> > https://patchwork.kernel.org/patch/9758361/ \n> >\n> > I didn't specifically ask for you to quit in *that* case. Quoting myself\n> > here, as you did:\n> > \n> >> > \"I wouldn't expect people to want to use this parser, but if we have a\n> >> > quick way to say \"this doesn't match, skip me\", then that would be\n> >> > helpful.\"\n> >> > \"We don't want to waste too much time scanning for this partition\n> >> > table if possible.\" \n> >\n> > That means, is there something (not necessarily writting in the\n> > \"original code\" that you're massaging) that could be used to reliably\n> > detect that this is/isn't a valid \"Sharp FTL\"? I don't think the check\n> > you wrote is a good one. Particularly, you *don't* want to just abort\n> > completely because there's one corrupt block. This check is a\n> > reliability check (so you can possibly ignore old/bad copies and skip\n> > onto better blocks), not a validity check. It is counter-productive to\n> > abort here, IIUC.\n> > \n> >> Actually, you don't save much by exiting on \"bad OOB fingerprint\". If\n> >> you look at the code you'll see that the only thing you check is\n> >> whether some oob portions are equal or not, and most of the time the\n> >> OOB area is left untouched by the upper layer, which means all free\n> >> bytes will be left to 0xff, which in turn means the \"is fingerprint\n> >> good?\" test will pass. \n> >\n> > Agreed.\n> >\n> > I'd drop this \"abort early\" check and just admit that it's not possible\n> > to do what I asked. \n> \n> Ok then, I have misunderstood you. The only time-saving would be to\n> skip the creation of the table.\n> In the specific cases, the older devices with erasesize 16KiB have\n> just 448 blocks and the routine doesn't really slow down.\n> I can imagine it would be a breeze for modern devices.\n> \n> I will just return -EINVAl and this error, as well as the eventual\n> parity-check error on a specific block, will be cut-off by the checks\n> and the cycle will move to next block.\n> \n> So I understand the sharpsl_nand_check_ooblayout() could be also avoided.\n> Please confirm this, thanks.\n\nNo, that check is still needed to bail out if someone tries to use\nthis parser with an incompatible NAND controller/ECC engine. Though it\nshould only be called once from your sharpsl_parse_mtd_partitions()\nfunction (one of the first thing you should do actually).\n\n> \n> > \n> >> > Now we are quitting ever before checking for parity erors ... \n> >>\n> >> Honestly, I'd recommend not using this parser for anything but SHARPSL\n> >> platforms, so I don't think we care much about the \"scan all blocks\"\n> >> overhead. \n> >\n> > Sounds about right.\n> > \n> >> Moreover, the sharpsl parser is the last one in the\n> >> part_parsers list, so it should be quite fast if the user specifies a\n> >> valid mtdparts= on the cmdline or valid partitions in the DT. \n> >\n> > Brian\n> >\n> > P.S. I alluded to it earlier, but I figured I should write it down\n> > properly here sometime, as food for thought; you don't actually need any\n> > of this parser at all if you're willing to contruct an initramfs that\n> > will do the parsing in user space (e.g., some scripting and 'nanddump';\n> > or link to libmtd) and then add partitions yourself (e.g., with\n> > 'mtdpart add ...', or calling the BLKPG ioctls directly). This would\n> > just require you have a kernel with CONFIG_MTD_PARTITIONED_MASTER=y. \n> \n> Brian, the first problem in this approach is the size: there are only\n> 1264KiB available for the zImage.\n> We accepted the challenge and wrote linux-kexecboot bootloader, a\n> kernel + couple of klibc-static binaries in the initramfs and we\n> special-cased the Zaurus adding the partition parser in userspace.\n> \n> Now, as widely discussed before, there are limits to this solution:\n> - first, we cannot oblige to flash this kernel+initramfs.\n\nJust like you cannot oblige people to update to a recent/mainline\nkernel.\n\n> - second, we need to build one kernel+initramfs for each model\n\nWhy is that? If you have a userspace implementation of your part-parser\nonly one initramfs is needed, or am I missing something?\n\n> - third, the machines are really the same from kernel pov, just mtdparts differ\n\nI don't get that one, sorry. If you have a userspace tool in your\ninitramfs that creates the partitions from the partinfo definitions\nusing BLKPG ioctls you'll still be able to keep one kernel and various\npartitioning.\n\n> \n> Soon we'll test a single kernel for the pxa25x and for the pxa27x models.\n\nStill don't see the link with the suggestion made by Brian.\n\n> \n> Honestly I did just glimpse at BLKPG ioctl because resizing for mtd\n> was added recently (iirc).\n> As maintainer for the cross-toolchain and of kexecboot I am of the\n> opinion that the cleanest solution is the in-kernel partition parser.\n\nWe already had this discussion. As a maintainer of the MTD subsystem\nI'm concerned about adding support for an old FTL and part parser that\nhas never been mainlined before and more importantly which you don't\nseem to understand.\nThe question is, who is responsible for the code if something does not\nwork. As I said many times, I'm not against adding new FTLs or\npart-parsers in the kernel, just find it worrisome that you failed to\nanswer to some question about how it's supposed to work.\n\n> \n> Thanks for your time reviewing this...is just a partition parser for\n> some of the first commercial linux handhelds.\n\nCome on, not this argument again.\n\nOn a final note, I'd like to say I spend time reviewing the code, and\nit looks good overall, but my concern about the FTL design still stands\nand I keep thinking that it's not such a good idea to support it in\nmainline just because some old devices used to use this FTL in their\nproprietary firmware. If people want to update their kernels why not\nforcing them to use an initramfs that hides all the ugly things in some\nspecific userspace tools?","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (mailfrom)\n\tsmtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133;\n\thelo=bombadil.infradead.org;\n\tenvelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"qICCj7++\"; \n\tdkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xfFDN4Qf5z9s7M\n\tfor ;\n\tSat, 26 Aug 2017 07:49:08 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dlMTK-0005rt-Sv; Fri, 25 Aug 2017 21:48:58 +0000","from mail.free-electrons.com ([62.4.15.54])\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dlMTG-0005hJ-Vb\n\tfor linux-mtd@lists.infradead.org; Fri, 25 Aug 2017 21:48:57 +0000","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 21DB120918; Fri, 25 Aug 2017 23:48:28 +0200 (CEST)","from bbrezillon (unknown [91.160.177.164])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id C9F34207ED;\n\tFri, 25 Aug 2017 23:48:17 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:\n\tMessage-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=dAbbrrUe3Ns0QEhqItrlszbCug72Y7jV4Vo9GHBibaE=;\n\tb=qICCj7+++DSu1d\n\tgBhnPfnDuIfbNpEdQdx0WmTKDsCecyZCEsHPpuLej3q4+GnA86jgdt2btw035yV18P05siJ7bhCCz\n\tXwL0xzqtoTsaNyYHVliZcuD09DdfWWv+zWiXgAq+xHoZPaYJQIhwvEKUGszSIhf7Tx8sR0xfdfdmz\n\tc3gqYi/dYAzQP0ktMXjWwtX6BKldEOFrBrhulWnRKOfyUIJjWqf0/HVGVjEVKJMfa60OPI2oUVPCY\n\t32o1tJ96EqdAWhuG+myEEv8x22+EzZnIZI4uxOZpDESK2UdWaHV2Hdv//xqxguYnPD6YrIW4Qwk7b\n\tIqmc6Wt3YJ7HPIsFz0ag==;","X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT,\n\tURIBL_BLOCKED shortcircuit=ham autolearn=disabled version=3.4.0","Date":"Fri, 25 Aug 2017 23:48:16 +0200","From":"Boris Brezillon ","To":"Andrea Adami ","Subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","Message-ID":"<20170825234816.2d6d31b4@bbrezillon>","In-Reply-To":"","References":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>\n\t<20170822145424.54e57b94@bbrezillon>\n\t\n\t<20170824120428.46e266c7@bbrezillon>\n\t\n\t<20170824132710.5fdea20c@bbrezillon>\n\t<20170825045314.GC68252@google.com>\n\t","X-Mailer":"Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu)","MIME-Version":"1.0","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170825_144855_318758_C9D83F6B ","X-CRM114-Status":"GOOD ( 46.93 )","X-Spam-Score":"-1.9 (-)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-1.9 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay\n\tdomain\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]","X-BeenThere":"linux-mtd@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"Linux MTD discussion mailing list ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dmitry Eremin-Solenikov ,\n\tRichard Weinberger ,\n\tRobert Jarzmik , linux-kernel@vger.kernel.org,\n\tHaojian Zhuang , \n\tMarek Vasut , linux-mtd@lists.infradead.org,\n\tCyrille Pitchen ,\n\t=?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= ,\n\tBrian Norris ,\n\tDavid Woodhouse ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"\"linux-mtd\" ","Errors-To":"linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org"}},{"id":1757799,"web_url":"http://patchwork.ozlabs.org/comment/1757799/","msgid":"","list_archive_url":null,"date":"2017-08-25T22:09:49","subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","submitter":{"id":30311,"url":"http://patchwork.ozlabs.org/api/people/30311/","name":"Andrea Adami","email":"andrea.adami@gmail.com"},"content":"On Fri, Aug 25, 2017 at 11:48 PM, Boris Brezillon\n wrote:\n> On Fri, 25 Aug 2017 19:50:25 +0200\n> Andrea Adami wrote:\n>\n>> On Fri, Aug 25, 2017 at 6:53 AM, Brian Norris\n>> wrote:\n>> > On Thu, Aug 24, 2017 at 01:27:10PM +0200, Boris Brezillon wrote:\n>> >> On Thu, 24 Aug 2017 12:30:02 +0200\n>> >> Andrea Adami wrote:\n>> >>\n>> >> > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon\n>> >> > wrote:\n>> >> > > On Thu, 24 Aug 2017 11:19:56 +0200\n>> >> > > Andrea Adami wrote:\n>> >\n>> > ...\n>> >\n>> >> > >> >> + /* create physical-logical table */\n>> >> > >> >> + for (block_num = 0; block_num < phymax; block_num++) {\n>> >> > >> >> + block_adr = block_num * mtd->erasesize;\n>> >> > >> >> +\n>> >> > >> >> + if (mtd_block_isbad(mtd, block_adr))\n>> >> > >> >> + continue;\n>> >> > >> >> +\n>> >> > >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))\n>> >> > >> >> + continue;\n>> >> > >> >> +\n>> >> > >> >> + /* get logical block */\n>> >> > >> >> + log_num = sharpsl_nand_get_logical_num(oob);\n>> >> > >> >> +\n>> >> > >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */\n>> >> > >> >> + if (log_num == UINT_MAX) {\n>> >> > >> >> + pr_info(\"sharpslpart: Sharp SL FTL not found.\\n\");\n>> >> > >> >> + ret = -EINVAL;\n>> >> > >> >> + goto exit;\n>> >> > >> >> + }\n>> >> > >\n>> >> > > Okay, I overlooked that part. Why do you exit if there's a fingerprint\n>> >> > > mismatch? Can't you just ignore this physical block and continue\n>> >> > > scanning the remaining ones?\n>> >> >\n>> >> > Norris asked to quit immediately in this case.\n>> >> > https://patchwork.kernel.org/patch/9758361/\n>> >\n>> > I didn't specifically ask for you to quit in *that* case. Quoting myself\n>> > here, as you did:\n>> >\n>> >> > \"I wouldn't expect people to want to use this parser, but if we have a\n>> >> > quick way to say \"this doesn't match, skip me\", then that would be\n>> >> > helpful.\"\n>> >> > \"We don't want to waste too much time scanning for this partition\n>> >> > table if possible.\"\n>> >\n>> > That means, is there something (not necessarily writting in the\n>> > \"original code\" that you're massaging) that could be used to reliably\n>> > detect that this is/isn't a valid \"Sharp FTL\"? I don't think the check\n>> > you wrote is a good one. Particularly, you *don't* want to just abort\n>> > completely because there's one corrupt block. This check is a\n>> > reliability check (so you can possibly ignore old/bad copies and skip\n>> > onto better blocks), not a validity check. It is counter-productive to\n>> > abort here, IIUC.\n>> >\n>> >> Actually, you don't save much by exiting on \"bad OOB fingerprint\". If\n>> >> you look at the code you'll see that the only thing you check is\n>> >> whether some oob portions are equal or not, and most of the time the\n>> >> OOB area is left untouched by the upper layer, which means all free\n>> >> bytes will be left to 0xff, which in turn means the \"is fingerprint\n>> >> good?\" test will pass.\n>> >\n>> > Agreed.\n>> >\n>> > I'd drop this \"abort early\" check and just admit that it's not possible\n>> > to do what I asked.\n>>\n>> Ok then, I have misunderstood you. The only time-saving would be to\n>> skip the creation of the table.\n>> In the specific cases, the older devices with erasesize 16KiB have\n>> just 448 blocks and the routine doesn't really slow down.\n>> I can imagine it would be a breeze for modern devices.\n>>\n>> I will just return -EINVAl and this error, as well as the eventual\n>> parity-check error on a specific block, will be cut-off by the checks\n>> and the cycle will move to next block.\n>>\n>> So I understand the sharpsl_nand_check_ooblayout() could be also avoided.\n>> Please confirm this, thanks.\n>\n> No, that check is still needed to bail out if someone tries to use\n> this parser with an incompatible NAND controller/ECC engine. Though it\n> should only be called once from your sharpsl_parse_mtd_partitions()\n> function (one of the first thing you should do actually).\n>\n>>\n>> >\n>> >> > Now we are quitting ever before checking for parity erors ...\n>> >>\n>> >> Honestly, I'd recommend not using this parser for anything but SHARPSL\n>> >> platforms, so I don't think we care much about the \"scan all blocks\"\n>> >> overhead.\n>> >\n>> > Sounds about right.\n>> >\n>> >> Moreover, the sharpsl parser is the last one in the\n>> >> part_parsers list, so it should be quite fast if the user specifies a\n>> >> valid mtdparts= on the cmdline or valid partitions in the DT.\n>> >\n>> > Brian\n>> >\n>> > P.S. I alluded to it earlier, but I figured I should write it down\n>> > properly here sometime, as food for thought; you don't actually need any\n>> > of this parser at all if you're willing to contruct an initramfs that\n>> > will do the parsing in user space (e.g., some scripting and 'nanddump';\n>> > or link to libmtd) and then add partitions yourself (e.g., with\n>> > 'mtdpart add ...', or calling the BLKPG ioctls directly). This would\n>> > just require you have a kernel with CONFIG_MTD_PARTITIONED_MASTER=y.\n>>\n>> Brian, the first problem in this approach is the size: there are only\n>> 1264KiB available for the zImage.\n>> We accepted the challenge and wrote linux-kexecboot bootloader, a\n>> kernel + couple of klibc-static binaries in the initramfs and we\n>> special-cased the Zaurus adding the partition parser in userspace.\n>>\n>> Now, as widely discussed before, there are limits to this solution:\n>> - first, we cannot oblige to flash this kernel+initramfs.\n>\n> Just like you cannot oblige people to update to a recent/mainline\n> kernel.\n>\n>> - second, we need to build one kernel+initramfs for each model\n>\n> Why is that? If you have a userspace implementation of your part-parser\n> only one initramfs is needed, or am I missing something?\n>\n>> - third, the machines are really the same from kernel pov, just mtdparts differ\n>\n> I don't get that one, sorry. If you have a userspace tool in your\n> initramfs that creates the partitions from the partinfo definitions\n> using BLKPG ioctls you'll still be able to keep one kernel and various\n> partitioning.\n>\n>>\n>> Soon we'll test a single kernel for the pxa25x and for the pxa27x models.\n>\n> Still don't see the link with the suggestion made by Brian.\n>\n>>\n>> Honestly I did just glimpse at BLKPG ioctl because resizing for mtd\n>> was added recently (iirc).\n>> As maintainer for the cross-toolchain and of kexecboot I am of the\n>> opinion that the cleanest solution is the in-kernel partition parser.\n>\n> We already had this discussion. As a maintainer of the MTD subsystem\n> I'm concerned about adding support for an old FTL and part parser that\n> has never been mainlined before and more importantly which you don't\n> seem to understand.\n> The question is, who is responsible for the code if something does not\n> work. As I said many times, I'm not against adding new FTLs or\n> part-parsers in the kernel, just find it worrisome that you failed to\n> answer to some question about how it's supposed to work.\n>\n>>\n>> Thanks for your time reviewing this...is just a partition parser for\n>> some of the first commercial linux handhelds.\n>\n> Come on, not this argument again.\n>\n> On a final note, I'd like to say I spend time reviewing the code, and\n> it looks good overall, but my concern about the FTL design still stands\n> and I keep thinking that it's not such a good idea to support it in\n> mainline just because some old devices used to use this FTL in their\n> proprietary firmware. If people want to update their kernels why not\n> forcing them to use an initramfs that hides all the ugly things in some\n> specific userspace tools?\n>\n\nBoris,\n\nI am really sorry but I won't answer again in detail about the issues\nwith repartitioning and the fact that I have no control over other\ndistributions, etc. etc.\n\nI don't understand where is your problem now, after 6 reviews.\nI'll be happy to send v7 with the added oob check and I'll have to add\nyou to the authors now :)\n\nMy point of view is:\n1- Does Zaurus devices exist under arm/mach-pxa YES\n2- Are the devices maintained YES\n3- Is a new module/functionality provided YES\n\nWhat is wrong in this?\n\nThanks\n\nAndrea","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (mailfrom)\n\tsmtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133;\n\thelo=bombadil.infradead.org;\n\tenvelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"lswNDocX\"; \n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"vQpwZm7r\"; dkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xfFj03pzyz9sNv\n\tfor ;\n\tSat, 26 Aug 2017 08:10:28 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dlMnx-0002m6-8G; Fri, 25 Aug 2017 22:10:17 +0000","from mail-wm0-x241.google.com ([2a00:1450:400c:c09::241])\n\tby bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dlMns-0001SO-Hc\n\tfor linux-mtd@lists.infradead.org; Fri, 25 Aug 2017 22:10:15 +0000","by mail-wm0-x241.google.com with SMTP id z132so1046648wmg.5\n\tfor ;\n\tFri, 25 Aug 2017 15:09:51 -0700 (PDT)","by 10.80.154.1 with HTTP; Fri, 25 Aug 2017 15:09:49 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:\n\tReferences:In-Reply-To:MIME-Version:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=Mx57sYWFrcurIHdlOrp2pFMd96usAsGFnYU7jAAQYJI=;\n\tb=lswNDocXU2auK2\n\tVPssezS7OqJrmNUYAuGTC0mEWPA4EYdG6hx5o2h/VGkmN66ZbD7TxymNzIYp2/NOoWco+HsyJGVFy\n\tU+NfKLfiw/6A7labBAqPN6Im9ioVoWgtXW2DjtLQUY7XTnXg3+tIbEYx0PBuwevF7TNBkffpvC6oa\n\tt/V609Kke7B4E0ERsYVVjP9Z2ue7Kxmrg5GF/ab5uQWJFqSueYDXWbhENBD7x8rtuk6plWEu1Tv19\n\t04cdDOvRdKcnesADcU7jDu5Tzd6JyE5zMy6h30OewPYDt/dqmnyiGuPWXB1VFg3+Eok0qLZvpPKSu\n\t1xDqjaVu4ho9xOpfpGrA==;","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=mqTuJpVG3ghPoPmJTBmbtaXRnpAHYRH1UXnfigPfAbw=;\n\tb=vQpwZm7rxPuj2b0O0AN1zPnjm+3ro9VnyyiiKwzsiQbYslJp1kmNotr+cPFpmxzPyL\n\tNqOou76Gy0v6sXqOL8XTiJ3wjT30LxcShvjxngGKXDgqjctWsxZ/JKPXJn9UvzLOSSU1\n\ttMd8PXVrpDyUf/S7zF7uh5cy7tx/GxeHNbuRMoKYTHBTOmvm3Gl5f6TWS9HuZM/GZ2nr\n\t4FbmkH/Ilk6/SvbMNNISwHiqsACttvvTn5kWGWmWd1ChLazmOy/oVLrABMIugO+By2sV\n\tQhzZ92FR1dzI/CE5RLrjIwVFnM/qgh9gAI7Ad5LWyEU8BshCpDd2ZRw1Paue4WHRkXHm\n\tjcLg=="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=mqTuJpVG3ghPoPmJTBmbtaXRnpAHYRH1UXnfigPfAbw=;\n\tb=mZk3kMyQgzhUy3FbCodDEYAP8dMGEdM7AzNt+8CBsC97c+2iF7vAN5DTqyblbUHl7r\n\t0RXwOKamIMqhqyOFuvGoJKhHjTuUde238/8AoZwdekfmzsKmRFNX5GUeNz1aRdl4I0JT\n\tPIRBBF6xxdRCdrcOK4JkIx6U/v3WYGy2UkzUt0bYGuiQJMDIaap/fadCzNx/Woov2orE\n\tYOU7TAVZjedJ/RXnnOEgCbSaJeyRdpy9HM30ky2Kq4UH7TJGd6CqjYYn/LzhiJbv4geQ\n\t6TEaF02+aWEdi/bthnh5wcfWZSJZjVmDDcdYqAaGvmOhfJ2O52AzLMCsYDfhAm6U/ssz\n\tEpMg==","X-Gm-Message-State":"AHYfb5g9PZs7YgBEFzSeo6VsaEsWXpU3LT+Uen5H1UcbKceqbIl5fh7V\n\tIOlnq3xH+dXLO+Fk7l+di3iAaJSbvw==","X-Received":"by 10.80.169.193 with SMTP id n59mr10929858edc.88.1503698990205; \n\tFri, 25 Aug 2017 15:09:50 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170825234816.2d6d31b4@bbrezillon>","References":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>\n\t<20170822145424.54e57b94@bbrezillon>\n\t\n\t<20170824120428.46e266c7@bbrezillon>\n\t\n\t<20170824132710.5fdea20c@bbrezillon>\n\t<20170825045314.GC68252@google.com>\n\t\n\t<20170825234816.2d6d31b4@bbrezillon>","From":"Andrea Adami ","Date":"Sat, 26 Aug 2017 00:09:49 +0200","Message-ID":"","Subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","To":"Boris Brezillon ","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170825_151012_932020_BEA0D842 ","X-CRM114-Status":"GOOD ( 43.78 )","X-Spam-Score":"-2.0 (--)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-2.0 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\n\tprovider (andrea.adami[at]gmail.com)\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]\n\t-0.1 DKIM_VALID Message has at least one valid DKIM or DK signature\n\t0.1 DKIM_SIGNED Message has a DKIM or DK signature,\n\tnot necessarily valid\n\t-0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from\n\tauthor's domain","X-BeenThere":"linux-mtd@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"Linux MTD discussion mailing list ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dmitry Eremin-Solenikov ,\n\tRichard Weinberger ,\n\tRobert Jarzmik , linux-kernel@vger.kernel.org,\n\tHaojian Zhuang , \n\tMarek Vasut , linux-mtd@lists.infradead.org,\n\tCyrille Pitchen ,\n\t=?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= ,\n\tBrian Norris ,\n\tDavid Woodhouse ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"\"linux-mtd\" ","Errors-To":"linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org"}},{"id":1757922,"web_url":"http://patchwork.ozlabs.org/comment/1757922/","msgid":"<20170826075938.5a9b861d@bbrezillon>","list_archive_url":null,"date":"2017-08-26T05:59:38","subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","submitter":{"id":63120,"url":"http://patchwork.ozlabs.org/api/people/63120/","name":"Boris Brezillon","email":"boris.brezillon@free-electrons.com"},"content":"On Fri, 25 Aug 2017 00:11:29 +0200\nBoris Brezillon wrote:\n\n\n> > +/*\n> > + * The logical block number assigned to a physical block is stored in the OOB\n> > + * of the first page, in 3 16-bit copies with the following layout:\n> > + *\n> > + * 01234567 89abcdef\n> > + * -------- --------\n> > + * ECC BB xyxyxy\n> > + *\n> > + * When reading we check that the first two copies agree.\n> > + * In case of error, matching is tried using the following pairs.\n> > + * Reserved values 0xffff mean the block is kept for wear leveling.\n> > + *\n> > + * 01234567 89abcdef\n> > + * -------- --------\n> > + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9\n> > + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11\n> > + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13 \n> \n> I know there's a depends on \"MTD_NAND_SHARPSL || MTD_NAND_TMIO\" in the\n> Kconfig entry, but one could enable those options just to use the sharpsl\n> part parser even if the OOB layout is incompatible with the sharpsl FTL.\n> \n> I'd recommend that you check that OOB bytes 8 to 15 are actually free\n> to be used by the MTD user in sharpsl_parse_mtd_partitions().\n> \n> Can be done with something like that:\n> \n> static int sharpsl_nand_check_ooblayout(struct mtd_info *mtd)\n> {\n> \tu8 freebytes = 0;\n> \tint section = 0;\n> \n> \twhile (true) {\n> \t\tstruct mtd_oob_region oobfree = { };\n> \t\tint ret, i;\n> \n> \t\tret = mtd_ooblayout_free(mtd, section++, &oobfree);\n> \t\tif (ret)\n> \t\t\tbreak;\n> \n> \t\tif (!oobfree.length || oobfree.offset > 15 ||\n> \t\t (oobfree.offset + oobfree.length) < 8)\n> \t\t\tcontinue;\n> \n> \t\ti = oobfree.offset >= 8 ? : 8;\n\nAs you reported on IRC there's an mistake here, it should be:\n\n\t\ti = oobfree.offset >= 8 ? oobfree.offset : 8\n\n> \t\tfor (; i < oobfree.offset + oobfree.length && i < 16; i++)\n> \t\t\tfreebytes |= BIT(i - 8);\n> \n> \t\tif (freebytes == 0xff)\n> \t\t\treturn 0;\n> \t}\n> \n> \treturn -ENOTSUPP;\n> }","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (mailfrom)\n\tsmtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133;\n\thelo=bombadil.infradead.org;\n\tenvelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"BFnAmdPv\"; \n\tdkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xfS7P4t2Lz9t45\n\tfor ;\n\tSat, 26 Aug 2017 16:00:33 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dlU8p-0005BK-OX; Sat, 26 Aug 2017 06:00:19 +0000","from mail.free-electrons.com ([62.4.15.54])\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dlU8j-0003yN-1W\n\tfor linux-mtd@lists.infradead.org; Sat, 26 Aug 2017 06:00:17 +0000","by mail.free-electrons.com (Postfix, from userid 110)\n\tid D4BA9209E3; Sat, 26 Aug 2017 07:59:48 +0200 (CEST)","from bbrezillon (91-160-177-164.subs.proxad.net [91.160.177.164])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id 8DCFD20986;\n\tSat, 26 Aug 2017 07:59:38 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:\n\tMessage-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=u4i0kH5Iw2PBhHGmk/NlIDqQQLyKohdhjCsDYOuuVMg=;\n\tb=BFnAmdPvSEQn4d\n\tW7tFnAygiKeyjSlT1S9dBO0aveZsXC2EWK/VcjGJZ0yWmWGBaZYZoO9c9PGM/HIwEHkMO+m0QViNn\n\tUByz27COonPrhyQBuLu84D8Y5FFTK9z0aCaO7iY+HpPX0hgsCH75/rPzmhPpuJQyJLyjE1s+xZ6Ks\n\t5b/KSFeZeNbk2FsOmImXAgea1XqyChI8HgT8T7S90SvpuAKauPvsSg5j7XKLD/R5U3tIxOaZfleTO\n\ttAEvyM1sqy0hshcxioWMAYlYhFwhPu+2qBat+ZOEqS9UwX84uvhK7cC/n1SOCwzfpHksyLDbRZhU8\n\t7SZaL2dylWCZeJwGd+ZA==;","X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT\n\tshortcircuit=ham autolearn=disabled version=3.4.0","Date":"Sat, 26 Aug 2017 07:59:38 +0200","From":"Boris Brezillon ","To":"Andrea Adami ","Subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","Message-ID":"<20170826075938.5a9b861d@bbrezillon>","In-Reply-To":"<20170825001129.091badb4@bbrezillon>","References":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>\n\t<20170825001129.091badb4@bbrezillon>","X-Mailer":"Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu)","MIME-Version":"1.0","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170825_230013_394243_0A01AD50 ","X-CRM114-Status":"GOOD ( 16.93 )","X-Spam-Score":"-1.9 (-)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-1.9 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay\n\tdomain\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]","X-BeenThere":"linux-mtd@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"Linux MTD discussion mailing list ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dmitry Eremin-Solenikov ,\n\tRichard Weinberger , linux-kernel@vger.kernel.org,\n\tHaojian Zhuang ,\n\tMarek Vasut , linux-mtd@lists.infradead.org,\n\tCyrille Pitchen , \n\t=?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= ,\n\tBrian Norris ,\n\tRobert Jarzmik ,\n\tDavid Woodhouse ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"\"linux-mtd\" ","Errors-To":"linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org"}},{"id":1757934,"web_url":"http://patchwork.ozlabs.org/comment/1757934/","msgid":"<20170826085827.77fba4c2@bbrezillon>","list_archive_url":null,"date":"2017-08-26T06:58:27","subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","submitter":{"id":63120,"url":"http://patchwork.ozlabs.org/api/people/63120/","name":"Boris Brezillon","email":"boris.brezillon@free-electrons.com"},"content":"On Sat, 26 Aug 2017 00:09:49 +0200\nAndrea Adami wrote:\n\n> On Fri, Aug 25, 2017 at 11:48 PM, Boris Brezillon\n> wrote:\n> > On Fri, 25 Aug 2017 19:50:25 +0200\n> > Andrea Adami wrote:\n> > \n> >> On Fri, Aug 25, 2017 at 6:53 AM, Brian Norris\n> >> wrote: \n> >> > On Thu, Aug 24, 2017 at 01:27:10PM +0200, Boris Brezillon wrote: \n> >> >> On Thu, 24 Aug 2017 12:30:02 +0200\n> >> >> Andrea Adami wrote:\n> >> >> \n> >> >> > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon\n> >> >> > wrote: \n> >> >> > > On Thu, 24 Aug 2017 11:19:56 +0200\n> >> >> > > Andrea Adami wrote: \n> >> >\n> >> > ...\n> >> > \n> >> >> > >> >> + /* create physical-logical table */\n> >> >> > >> >> + for (block_num = 0; block_num < phymax; block_num++) {\n> >> >> > >> >> + block_adr = block_num * mtd->erasesize;\n> >> >> > >> >> +\n> >> >> > >> >> + if (mtd_block_isbad(mtd, block_adr))\n> >> >> > >> >> + continue;\n> >> >> > >> >> +\n> >> >> > >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))\n> >> >> > >> >> + continue;\n> >> >> > >> >> +\n> >> >> > >> >> + /* get logical block */\n> >> >> > >> >> + log_num = sharpsl_nand_get_logical_num(oob);\n> >> >> > >> >> +\n> >> >> > >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */\n> >> >> > >> >> + if (log_num == UINT_MAX) {\n> >> >> > >> >> + pr_info(\"sharpslpart: Sharp SL FTL not found.\\n\");\n> >> >> > >> >> + ret = -EINVAL;\n> >> >> > >> >> + goto exit;\n> >> >> > >> >> + } \n> >> >> > >\n> >> >> > > Okay, I overlooked that part. Why do you exit if there's a fingerprint\n> >> >> > > mismatch? Can't you just ignore this physical block and continue\n> >> >> > > scanning the remaining ones? \n> >> >> >\n> >> >> > Norris asked to quit immediately in this case.\n> >> >> > https://patchwork.kernel.org/patch/9758361/ \n> >> >\n> >> > I didn't specifically ask for you to quit in *that* case. Quoting myself\n> >> > here, as you did:\n> >> > \n> >> >> > \"I wouldn't expect people to want to use this parser, but if we have a\n> >> >> > quick way to say \"this doesn't match, skip me\", then that would be\n> >> >> > helpful.\"\n> >> >> > \"We don't want to waste too much time scanning for this partition\n> >> >> > table if possible.\" \n> >> >\n> >> > That means, is there something (not necessarily writting in the\n> >> > \"original code\" that you're massaging) that could be used to reliably\n> >> > detect that this is/isn't a valid \"Sharp FTL\"? I don't think the check\n> >> > you wrote is a good one. Particularly, you *don't* want to just abort\n> >> > completely because there's one corrupt block. This check is a\n> >> > reliability check (so you can possibly ignore old/bad copies and skip\n> >> > onto better blocks), not a validity check. It is counter-productive to\n> >> > abort here, IIUC.\n> >> > \n> >> >> Actually, you don't save much by exiting on \"bad OOB fingerprint\". If\n> >> >> you look at the code you'll see that the only thing you check is\n> >> >> whether some oob portions are equal or not, and most of the time the\n> >> >> OOB area is left untouched by the upper layer, which means all free\n> >> >> bytes will be left to 0xff, which in turn means the \"is fingerprint\n> >> >> good?\" test will pass. \n> >> >\n> >> > Agreed.\n> >> >\n> >> > I'd drop this \"abort early\" check and just admit that it's not possible\n> >> > to do what I asked. \n> >>\n> >> Ok then, I have misunderstood you. The only time-saving would be to\n> >> skip the creation of the table.\n> >> In the specific cases, the older devices with erasesize 16KiB have\n> >> just 448 blocks and the routine doesn't really slow down.\n> >> I can imagine it would be a breeze for modern devices.\n> >>\n> >> I will just return -EINVAl and this error, as well as the eventual\n> >> parity-check error on a specific block, will be cut-off by the checks\n> >> and the cycle will move to next block.\n> >>\n> >> So I understand the sharpsl_nand_check_ooblayout() could be also avoided.\n> >> Please confirm this, thanks. \n> >\n> > No, that check is still needed to bail out if someone tries to use\n> > this parser with an incompatible NAND controller/ECC engine. Though it\n> > should only be called once from your sharpsl_parse_mtd_partitions()\n> > function (one of the first thing you should do actually).\n> > \n> >> \n> >> > \n> >> >> > Now we are quitting ever before checking for parity erors ... \n> >> >>\n> >> >> Honestly, I'd recommend not using this parser for anything but SHARPSL\n> >> >> platforms, so I don't think we care much about the \"scan all blocks\"\n> >> >> overhead. \n> >> >\n> >> > Sounds about right.\n> >> > \n> >> >> Moreover, the sharpsl parser is the last one in the\n> >> >> part_parsers list, so it should be quite fast if the user specifies a\n> >> >> valid mtdparts= on the cmdline or valid partitions in the DT. \n> >> >\n> >> > Brian\n> >> >\n> >> > P.S. I alluded to it earlier, but I figured I should write it down\n> >> > properly here sometime, as food for thought; you don't actually need any\n> >> > of this parser at all if you're willing to contruct an initramfs that\n> >> > will do the parsing in user space (e.g., some scripting and 'nanddump';\n> >> > or link to libmtd) and then add partitions yourself (e.g., with\n> >> > 'mtdpart add ...', or calling the BLKPG ioctls directly). This would\n> >> > just require you have a kernel with CONFIG_MTD_PARTITIONED_MASTER=y. \n> >>\n> >> Brian, the first problem in this approach is the size: there are only\n> >> 1264KiB available for the zImage.\n> >> We accepted the challenge and wrote linux-kexecboot bootloader, a\n> >> kernel + couple of klibc-static binaries in the initramfs and we\n> >> special-cased the Zaurus adding the partition parser in userspace.\n> >>\n> >> Now, as widely discussed before, there are limits to this solution:\n> >> - first, we cannot oblige to flash this kernel+initramfs. \n> >\n> > Just like you cannot oblige people to update to a recent/mainline\n> > kernel.\n> > \n> >> - second, we need to build one kernel+initramfs for each model \n> >\n> > Why is that? If you have a userspace implementation of your part-parser\n> > only one initramfs is needed, or am I missing something?\n> > \n> >> - third, the machines are really the same from kernel pov, just mtdparts differ \n> >\n> > I don't get that one, sorry. If you have a userspace tool in your\n> > initramfs that creates the partitions from the partinfo definitions\n> > using BLKPG ioctls you'll still be able to keep one kernel and various\n> > partitioning.\n> > \n> >>\n> >> Soon we'll test a single kernel for the pxa25x and for the pxa27x models. \n> >\n> > Still don't see the link with the suggestion made by Brian.\n> > \n> >>\n> >> Honestly I did just glimpse at BLKPG ioctl because resizing for mtd\n> >> was added recently (iirc).\n> >> As maintainer for the cross-toolchain and of kexecboot I am of the\n> >> opinion that the cleanest solution is the in-kernel partition parser. \n> >\n> > We already had this discussion. As a maintainer of the MTD subsystem\n> > I'm concerned about adding support for an old FTL and part parser that\n> > has never been mainlined before and more importantly which you don't\n> > seem to understand.\n> > The question is, who is responsible for the code if something does not\n> > work. As I said many times, I'm not against adding new FTLs or\n> > part-parsers in the kernel, just find it worrisome that you failed to\n> > answer to some question about how it's supposed to work.\n> > \n> >>\n> >> Thanks for your time reviewing this...is just a partition parser for\n> >> some of the first commercial linux handhelds. \n> >\n> > Come on, not this argument again.\n> >\n> > On a final note, I'd like to say I spend time reviewing the code, and\n> > it looks good overall, but my concern about the FTL design still stands\n> > and I keep thinking that it's not such a good idea to support it in\n> > mainline just because some old devices used to use this FTL in their\n> > proprietary firmware. If people want to update their kernels why not\n> > forcing them to use an initramfs that hides all the ugly things in some\n> > specific userspace tools?\n> > \n> \n> Boris,\n> \n> I am really sorry but I won't answer again in detail about the issues\n> with repartitioning and the fact that I have no control over other\n> distributions, etc. etc.\n\nAnd do you have control on what those distributions enable in there\nkernel? Will they really enable this SHARPSL part parser?\n\n> \n> I don't understand where is your problem now, after 6 reviews.\n\nMy concerns didn't vanish, and I still don't buy your different\narguments which are either non-technical or are not related to the\nsuggestions we do. In this regards, this thread is a good example, when\nBrian suggests to use a userspace program to parse partinfo and create\npartitions with the BLKPG ioctl, you answer that you'll require one\ninitramfs per board, which AFAICT is not true.\n\n> I'll be happy to send v7 with the added oob check and I'll have to add\n> you to the authors now :)\n\nHell no! Send a v7 if you want but don't flag me as one of the author.\n\n> \n> My point of view is:\n> 1- Does Zaurus devices exist under arm/mach-pxa YES\n> 2- Are the devices maintained YES\n> 3- Is a new module/functionality provided YES\n> \n> What is wrong in this?\n\nWhat is wrong?! Maybe the fact that you're trying to add support for\nsomething that sharp didn't even dare discussing with the community\nwhen they designed it. Also, no one seems to know enough about this FTL\nto explain some of the choices they made. And last, it seems this FTL\nwas designed with old NANDs in mind (those 16K eraseblock NANDs) and\nthen hacked to support bigger chips, which is not a good sign.\n\nNow, I'd like to stop this discussion here because we clearly disagree\nand I don't think one can convince the other.\nAs I said from the beginning, if other MTD maintainers are fine with\nthis parser I won't block its inclusion. I've actually done more than I\ninitially planned to do: I reviewed the code, but don't expect me to\nagree with what you're doing, because I won't.\n\nRegards,\n\nBoris","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org; spf=none (mailfrom)\n\tsmtp.mailfrom=lists.infradead.org (client-ip=65.50.211.133;\n\thelo=bombadil.infradead.org;\n\tenvelope-from=linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"gZ7Omb8b\"; \n\tdkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xfTR10gtgz9t4R\n\tfor ;\n\tSat, 26 Aug 2017 16:59:09 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dlV3f-0006QT-K4; Sat, 26 Aug 2017 06:59:03 +0000","from mail.free-electrons.com ([62.4.15.54])\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dlV3b-0006Iw-9D\n\tfor linux-mtd@lists.infradead.org; Sat, 26 Aug 2017 06:59:01 +0000","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 7880F20909; Sat, 26 Aug 2017 08:58:37 +0200 (CEST)","from bbrezillon (unknown [91.160.177.164])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id 2669A208FC;\n\tSat, 26 Aug 2017 08:58:27 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:\n\tMessage-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=vEq4HQDfOQbWgUXLQVNGAhYnEspVo0NrZjLY9G+lrFk=;\n\tb=gZ7Omb8b9dzCSL\n\tRbAsaq0P0rihUj+clhOOd7Ptm9NQRib9KenjPHuKfyGBkkF0Cp7reoIrVurtomzEEi9lQGE3JrM19\n\tdtF0cvgjmcInHOjMaReQ6XU9JFVP41etKMXuw38U7yYvikNRSfZYrSb5Bw0AD825arIGvj6A02NZi\n\typY/XIzHGbrGmWfWZ71R+A6Ebd9nuC7cEBIHtGP7Pq2BiMA6IWry7uz9R5UeqO2WmAtBUHa4PdX7h\n\tmSGXxmSayw7+eOnD0U2KlBfPsuMG+RJEppJg+3lxjATNyDdWJx/qHdsu1dcYuvvAlKuubjt9EFTcC\n\tHu1IU1LxiUDpA1gRxIfw==;","X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT\n\tshortcircuit=ham autolearn=disabled version=3.4.0","Date":"Sat, 26 Aug 2017 08:58:27 +0200","From":"Boris Brezillon ","To":"Andrea Adami ","Subject":"Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser","Message-ID":"<20170826085827.77fba4c2@bbrezillon>","In-Reply-To":"","References":"<1503394972-12541-1-git-send-email-andrea.adami@gmail.com>\n\t<20170822145424.54e57b94@bbrezillon>\n\t\n\t<20170824120428.46e266c7@bbrezillon>\n\t\n\t<20170824132710.5fdea20c@bbrezillon>\n\t<20170825045314.GC68252@google.com>\n\t\n\t<20170825234816.2d6d31b4@bbrezillon>\n\t","X-Mailer":"Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu)","MIME-Version":"1.0","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170825_235859_625811_D18D7D9B ","X-CRM114-Status":"GOOD ( 52.84 )","X-Spam-Score":"-1.9 (-)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-1.9 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay\n\tdomain\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]","X-BeenThere":"linux-mtd@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"Linux MTD discussion mailing list ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Dmitry Eremin-Solenikov ,\n\tRichard Weinberger , linux-kernel@vger.kernel.org,\n\tHaojian Zhuang ,\n\tMarek Vasut , linux-mtd@lists.infradead.org,\n\tCyrille Pitchen , \n\t=?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= ,\n\tBrian Norris ,\n\tRobert Jarzmik ,\n\tDavid Woodhouse ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"\"linux-mtd\" ","Errors-To":"linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org"}}]