| Message ID | 1503394972-12541-1-git-send-email-andrea.adami@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Andrea, Le Tue, 22 Aug 2017 11:42:52 +0200, Andrea Adami <andrea.adami@gmail.com> a écrit : > The Sharp SL Series (Zaurus) PXA handhelds have 16/64/128M of NAND flash > and share the same layout of the first 7M partition, managed by Sharp FTL. > > The purpose of this self-contained patch is to add a common parser and > remove the hardcoded sizes in the board files (these devices are not yet > converted to devicetree). > Users will have benefits because the mtdparts= tag will not be necessary > anymore and they will be free to repartition the little sized flash. > > The obsolete bootloader can not pass the partitioning info to modern > kernels anymore so it has to be read from flash at known logical addresses. > (see http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm ) > > In kernel, under arch/arm/mach-pxa we have already 8 machines: > MACH_POODLE, MACH_CORGI, MACH_SHEPERD, MACH_HUSKY, MACH_AKITA, MACH_SPITZ, > MACH_BORZOI, MACH_TOSA. > Lost after the 2.4 vendor kernel are MACH_BOXER and MACH_TERRIER. > > Almost every model has different factory partitioning: add to this the > units can be repartitioned by users with userspace tools (nandlogical) > and installers for popular (back then) linux distributions. > > The Parameter Area in the first (boot) partition extends from 0x00040000 to > 0x0007bfff (176k) and contains two copies of the partition table: > ... > 0x00060000: Partition Info1 16k > 0x00064000: Partition Info2 16k > 0x00668000: Model 16k > ... > > The first 7M partition is managed by the Sharp FTL reserving 5% + 1 blocks > for wear-leveling: some blocks are remapped and one layer of translation > (logical to physical) is necessary. > > There isn't much documentation about this FTL in the 2.4 sources, just the > MTD methods for reading and writing using logical addresses and the block > management (wear-leveling, use counter). > For the purpose of the MTD parser only the read part of the code was taken. > > The NAND drivers that can use this parser are sharpsl.c and tmio_nand.c. > > Signed-off-by: Andrea Adami <andrea.adami@gmail.com> > --- Please put a changelog here. > drivers/mtd/parsers/Kconfig | 7 + > drivers/mtd/parsers/Makefile | 1 + > drivers/mtd/parsers/sharpslpart.c | 382 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 390 insertions(+) > create mode 100644 drivers/mtd/parsers/sharpslpart.c > > diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig > index d206b3c..f371022 100644 > --- a/drivers/mtd/parsers/Kconfig > +++ b/drivers/mtd/parsers/Kconfig > @@ -6,3 +6,10 @@ config MTD_PARSER_TRX > may contain up to 3/4 partitions (depending on the version). > This driver will parse TRX header and report at least two partitions: > kernel and rootfs. Missing blank line. > +config MTD_SHARPSL_PARTS > + tristate "Sharp SL Series NAND flash partition parser" > + depends on MTD_NAND_SHARPSL || MTD_NAND_TMIO || COMPILE_TEST > + help > + This provides the read-only FTL logic necessary to read the partition > + table from the NAND flash of Sharp SL Series (Zaurus) and the MTD > + partition parser using this code. > diff --git a/drivers/mtd/parsers/Makefile b/drivers/mtd/parsers/Makefile > index 4d9024e..5b1bcc3 100644 > --- a/drivers/mtd/parsers/Makefile > +++ b/drivers/mtd/parsers/Makefile > @@ -1 +1,2 @@ > obj-$(CONFIG_MTD_PARSER_TRX) += parser_trx.o > +obj-$(CONFIG_MTD_SHARPSL_PARTS) += sharpslpart.o > diff --git a/drivers/mtd/parsers/sharpslpart.c b/drivers/mtd/parsers/sharpslpart.c > new file mode 100644 > index 0000000..b2333ae > --- /dev/null > +++ b/drivers/mtd/parsers/sharpslpart.c > @@ -0,0 +1,382 @@ > +/* > + * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL > + * for logical addressing, as used on the PXA models of the SHARP SL Series. > + * > + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com> > + * > + * Based on 2.4 sources: > + * drivers/mtd/nand/sharp_sl_logical.c > + * linux/include/asm-arm/sharp_nand_logical.h > + * > + * Copyright (C) 2002 SHARP > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/module.h> > +#include <linux/types.h> > +#include <linux/mtd/mtd.h> > +#include <linux/mtd/partitions.h> > + > +/* oob structure */ > +#define NAND_NOOB_LOGADDR_00 8 > +#define NAND_NOOB_LOGADDR_01 9 > +#define NAND_NOOB_LOGADDR_10 10 > +#define NAND_NOOB_LOGADDR_11 11 > +#define NAND_NOOB_LOGADDR_20 12 > +#define NAND_NOOB_LOGADDR_21 13 > + > +#define BLOCK_IS_RESERVED 0xffff > +#define BLOCK_UNMASK 0x07fe > +#define BLOCK_UNMASK_COMPLEMENT 1 > + > +/* factory defaults */ > +#define SHARPSL_NAND_PARTS 3 > +#define SHARPSL_FTL_PART_SIZE (7 * 1024 * 1024) Nit: s/7 * 1024 * 1024/7 * SZ_1M/ > +#define PARAM_BLOCK_PARTITIONINFO1 0x00060000 > +#define PARAM_BLOCK_PARTITIONINFO2 0x00064000 I'd rename the macro differently. The term block is a bit confusing here since AFAIU the Param Area does not necessarily fit in a single eraseblock. How about: #define PARAM_AREA_PARTINFO1_LADDR 0x60000 #define PARAM_AREA_PARTINFO2_LADDR 0x64000 or simply #define SHARPSL_PARTINFO1_LADDR 0x60000 #define SHARPSL_PARTINFO2_LADDR 0x64000 > + > +#define BOOT_MAGIC 0x424f4f54 > +#define FSRO_MAGIC 0x4653524f > +#define FSRW_MAGIC 0x46535257 > + > +/** > + * struct sharpsl_ftl - Sharp FTL Logical Table > + * @logmax: number of logical blocks > + * @log2phy: the logical-to-physical table > + * > + * Stripped down from 2.4 sources for read-only purposes. Not sure this information is really useful, especially since you don't provide a link to the 2.4 sources (or maybe I missed it). You'd better describe what this struct is used for here. > + */ > +struct sharpsl_ftl { > + u_int logmax; > + u_int *log2phy; Can you replace u_int by unsigned int and u_char by u8? That goes for the whole file. > +}; > + > +static int sharpsl_nand_read_oob(struct mtd_info *mtd, loff_t offs, size_t len, AFAICT, len is always set to mtd->oobsize. You can just drop this parameter and set ops->ooblen to mtd->oobsize. > + uint8_t *buf) > +{ > + loff_t mask = mtd->writesize - 1; mask is equivalent to mtd->writesize_mask. > + struct mtd_oob_ops ops; struct mtd_oob_ops ops = { }; so that you don't have to initialize all fields if they are set to 0 or NULL. You could also directly fill this object when declaring it: struct mtd_oob_ops ops = { .mode = MTD_OPS_PLACE_OOB, .ooblen = mtd->oobsize, .oobbuf = buf, }; > + int ret; > + > + ops.mode = MTD_OPS_PLACE_OOB; > + ops.ooboffs = offs & mask; Hm, I'm pretty sure this is wrong. mask is a page mask, not the oob area mask. It works because offs is always aligned on a page, but you can directly assign ops.ooboffs to 0 and you should be good. > + ops.ooblen = len; > + ops.oobbuf = buf; > + ops.datbuf = NULL; > + > + ret = mtd_read_oob(mtd, offs & ~mask, &ops); > + if (ret != 0 || len != ops.oobretlen) > + return -1; > + > + return 0; > +} > + > +/* > + * The logical block number assigned to a physical block is stored in the OOB > + * of the first page, in 3 16-bit copies with the following layout: > + * > + * 01234567 89abcdef > + * -------- -------- > + * ECC BB xyxyxy > + * > + * When reading we check that the first two copies agree. > + * In case of error, matching is tried using the following pairs. > + * Reserved values 0xffff mean the block is kept for wear leveling. > + * > + * 01234567 89abcdef > + * -------- -------- > + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9 > + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11 > + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13 > + * > + */ > +static u_int sharpsl_nand_get_logical_num(u_char *oob) > +{ > + u16 us; > + int good0, good1; > + > + if (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] && > + oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) { > + good0 = NAND_NOOB_LOGADDR_00; > + good1 = NAND_NOOB_LOGADDR_01; > + } else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] && > + oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) { > + good0 = NAND_NOOB_LOGADDR_10; > + good1 = NAND_NOOB_LOGADDR_11; > + } else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] && > + oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) { > + good0 = NAND_NOOB_LOGADDR_20; > + good1 = NAND_NOOB_LOGADDR_21; > + } else { > + /* wrong oob fingerprint, maybe here by mistake? */ > + return UINT_MAX; > + } > + > + us = oob[good0] | oob[good1] << 8; > + > + /* parity check */ > + if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT) > + return (UINT_MAX - 1); Why not making this function return an int and use regular error codes for the parity and wrong fingerprint errors? > + > + /* reserved */ > + if (us == BLOCK_IS_RESERVED) > + return BLOCK_IS_RESERVED; > + else You can drop the 'else' here. > + return (us & BLOCK_UNMASK) >> 1; So, this is a 10bit value, right? Why not doing the >> 1 first so that your BLOCK_UNMASK mask really looks like a valid mask (0x3ff, which can also be defined as GENMASK(9, 0)). > +} > + > +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size, s/sharpsl_nand_init_logical/sharpsl_nand_init_ftl/ partition_size is always set to SHARPSL_FTL_PART_SIZE. Please drop this argument. > + struct sharpsl_ftl *ftl) > +{ > + u_int block_num, log_num, phymax; > + loff_t block_adr; > + u_char *oob; > + int i, ret; > + > + oob = kzalloc(mtd->oobsize, GFP_KERNEL); > + if (!oob) > + return -ENOMEM; > + > + /* initialize management structure */ > + phymax = (partition_size / mtd->erasesize); You can use mtd_div_by_eb() here. > + > + /* FTL reserves 5% of the blocks + 1 spare */ > + ftl->logmax = ((phymax * 95) / 100) - 1; > + > + ftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int), sizeof(*ftl->log2phy) > + GFP_KERNEL); > + if (!ftl->log2phy) { > + ret = -ENOMEM; > + goto exit; > + } > + > + /* initialize ftl->log2phy */ > + for (i = 0; i < ftl->logmax; i++) > + ftl->log2phy[i] = UINT_MAX; > + > + /* create physical-logical table */ > + for (block_num = 0; block_num < phymax; block_num++) { > + block_adr = block_num * mtd->erasesize; > + > + if (mtd_block_isbad(mtd, block_adr)) > + continue; > + > + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) > + continue; > + > + /* get logical block */ > + log_num = sharpsl_nand_get_logical_num(oob); > + > + /* FTL is not used? Exit here if the oob fingerprint is wrong */ > + if (log_num == UINT_MAX) { > + pr_info("sharpslpart: Sharp SL FTL not found.\n"); > + ret = -EINVAL; > + goto exit; > + } > + > + /* skip out of range and not unique values */ > + if (log_num < ftl->logmax) { > + if (ftl->log2phy[log_num] == UINT_MAX) > + ftl->log2phy[log_num] = block_num; > + } > + } > + > + pr_info("Sharp SL FTL: %d blocks used (%d logical, %d reserved)\n", > + phymax, ftl->logmax, > + phymax - ftl->logmax); > + > + ret = 0; > +exit: > + kfree(oob); > + return ret; > +} > + > +void sharpsl_nand_cleanup_logical(struct sharpsl_ftl *ftl) s/sharpsl_nand_cleanup_logical/sharpsl_nand_cleanup_ftl/ > +{ > + kfree(ftl->log2phy); > +} > + > +static int sharpsl_nand_read_laddr(struct mtd_info *mtd, > + loff_t from, > + size_t len, > + u_char *buf, > + struct sharpsl_ftl *ftl) > +{ > + u_int log_num, log_new; > + u_int block_num; > + loff_t block_adr; > + loff_t block_ofs; > + size_t retlen; > + int err; > + > + log_num = (u32)from / mtd->erasesize; Please use mtd_div_by_eb(). > + log_new = ((u32)from + len - 1) / mtd->erasesize; Ditto. And I'm not sure log_new is a good name for this variable. Could be last_log_num or something like that. > + > + if (len <= 0 || log_num >= ftl->logmax || log_new > log_num) > + return -EINVAL; > + > + block_num = ftl->log2phy[log_num]; > + block_adr = block_num * mtd->erasesize; > + block_ofs = (u32)from % mtd->erasesize; mtd_mod_by_eb(). > + > + err = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf); > + /* Ignore corrected ECC errors */ > + if (mtd_is_bitflip(err)) > + err = 0; Please add a blank line here > + if (!err && retlen != len) > + err = -EIO; and here. > + if (err) > + pr_err("sharpslpart: error, read failed at %#llx\n", > + block_adr + block_ofs); > + > + return err; > +} > + > +/* > + * MTD Partition Parser > + * > + * Sample values read from SL-C860 > + * > + * # cat /proc/mtd > + * dev: size erasesize name > + * mtd0: 006d0000 00020000 "Filesystem" > + * mtd1: 00700000 00004000 "smf" > + * mtd2: 03500000 00004000 "root" > + * mtd3: 04400000 00004000 "home" > + * > + * PARTITIONINFO1 > + * 0x00060000: 00 00 00 00 00 00 70 00 42 4f 4f 54 00 00 00 00 ......p.BOOT.... > + * 0x00060010: 00 00 70 00 00 00 c0 03 46 53 52 4f 00 00 00 00 ..p.....FSRO.... > + * 0x00060020: 00 00 c0 03 00 00 00 04 46 53 52 57 00 00 00 00 ........FSRW.... > + * 0x00060030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................ > + * You can drop this extra empty line. > + */ > +struct sharpsl_nand_partinfo { > + __le32 start; > + __le32 end; > + __be32 magic; > + u32 reserved; > +}; > + > +static int sharpsl_nand_read_partinfo(struct mtd_info *master, > + loff_t from, > + size_t len, > + struct sharpsl_nand_partinfo *buf, > + struct sharpsl_ftl *ftl) > +{ > + int ret; > + > + ret = sharpsl_nand_read_laddr(master, (u32)from, len, (u_char *)buf, The u32 cast is unneeded here, and I'd suggest to change the sharpsl_nand_read_laddr() prototype to take a 'void *' instead of a 'u_char *', so that you can also get rid of this cast. > + ftl); > + if (ret) > + goto exit; Well, you have nothing to cleanup in the exit path, so you can return directly here. > + > + /* check for magics */ > + if (be32_to_cpu(buf[0].magic) != BOOT_MAGIC || > + be32_to_cpu(buf[1].magic) != FSRO_MAGIC || > + be32_to_cpu(buf[2].magic) != FSRW_MAGIC) { > + pr_err("sharpslpart: magic values mismatch\n"); > + ret = -EINVAL; > + goto exit; Ditto. > + } > + > + /* fixup for hardcoded value 64 MiB (for older models) */ > + buf[2].end = cpu_to_le32(master->size); > + > + /* extra sanity check */ > + if (le32_to_cpu(buf[0].end) <= le32_to_cpu(buf[0].start) || > + le32_to_cpu(buf[1].start) < le32_to_cpu(buf[0].end) || > + le32_to_cpu(buf[1].end) <= le32_to_cpu(buf[1].start) || > + le32_to_cpu(buf[2].start) < le32_to_cpu(buf[1].end) || > + le32_to_cpu(buf[2].end) <= le32_to_cpu(buf[2].start)) { > + pr_err("sharpslpart: partition sizes mismatch\n"); > + ret = -EINVAL; > + goto exit; Ditto. > + } > + > + ret = 0; > +exit: > + return ret; > +} > + > +static int sharpsl_parse_mtd_partitions(struct mtd_info *master, > + const struct mtd_partition **pparts, > + struct mtd_part_parser_data *data) > +{ > + struct sharpsl_ftl ftl; > + struct sharpsl_nand_partinfo buf[SHARPSL_NAND_PARTS]; > + struct mtd_partition *sharpsl_nand_parts; > + int err, ret; > + > + /* init logical mgmt (FTL) */ > + err = sharpsl_nand_init_logical(master, SHARPSL_FTL_PART_SIZE, &ftl); > + if (err) > + return err; > + > + /* read and validate first partition table */ > + pr_info("sharpslpart: using first partition table\n"); > + err = sharpsl_nand_read_partinfo(master, > + PARAM_BLOCK_PARTITIONINFO1, > + sizeof(buf), buf, &ftl); > + if (err) { > + /* fallback: read second partition table */ > + pr_warn("sharpslpart: retry using second partition table\n"); > + ret = sharpsl_nand_read_partinfo(master, > + PARAM_BLOCK_PARTITIONINFO2, > + sizeof(buf), buf, &ftl); Why do you need a different variable to store the second sharpsl_nand_read_partinfo() return code? > + if (ret) { > + pr_err("sharpslpart: partition tables are invalid\n"); > + sharpsl_nand_cleanup_logical(&ftl); > + return ret; > + } > + } > + > + /* cleanup logical mgmt (FTL) */ > + sharpsl_nand_cleanup_logical(&ftl); How about: pr_info("sharpslpart: try reading first partition table\n"); err = sharpsl_nand_read_partinfo(master, PARAM_BLOCK_PARTITIONINFO1, sizeof(buf), buf, &ftl); if (err) { /* fallback: read second partition table */ pr_warn("sharpslpart: first partition table is invalid, retry using second partition table\n"); err = sharpsl_nand_read_partinfo(master, PARAM_BLOCK_PARTITIONINFO2, sizeof(buf), buf, &ftl); } /* cleanup logical mgmt (FTL) */ sharpsl_nand_cleanup_logical(&ftl); if (err) { pr_err("sharpslpart: both partition tables are invalid\n"); return ret; } so that you have a single place where you cleanup the FTL. > + > + sharpsl_nand_parts = kzalloc(sizeof(*sharpsl_nand_parts) * > + SHARPSL_NAND_PARTS, GFP_KERNEL); > + if (!sharpsl_nand_parts) > + return -ENOMEM; > + > + /* original names */ > + sharpsl_nand_parts[0].name = "smf"; > + sharpsl_nand_parts[0].offset = le32_to_cpu(buf[0].start); > + sharpsl_nand_parts[0].size = le32_to_cpu(buf[0].end) - > + le32_to_cpu(buf[0].start); > + > + sharpsl_nand_parts[1].name = "root"; > + sharpsl_nand_parts[1].offset = le32_to_cpu(buf[1].start); > + sharpsl_nand_parts[1].size = le32_to_cpu(buf[1].end) - > + le32_to_cpu(buf[1].start); > + > + sharpsl_nand_parts[2].name = "home"; > + sharpsl_nand_parts[2].offset = le32_to_cpu(buf[2].start); > + sharpsl_nand_parts[2].size = le32_to_cpu(buf[2].end) - > + le32_to_cpu(buf[2].start); > + > + *pparts = sharpsl_nand_parts; > + return SHARPSL_NAND_PARTS; > +} > + > +static struct mtd_part_parser sharpsl_mtd_parser = { > + .parse_fn = sharpsl_parse_mtd_partitions, > + .name = "sharpslpart", > +}; > +module_mtd_part_parser(sharpsl_mtd_parser); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Andrea Adami <andrea.adami@gmail.com>"); > +MODULE_DESCRIPTION("MTD partitioning for NAND flash on Sharp SL Series");
Boris, thanks for the review. I have made the required changes apart th elast one: error values to be returned.. Please see my comments. On Tue, Aug 22, 2017 at 2:54 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi Andrea, > > Le Tue, 22 Aug 2017 11:42:52 +0200, > Andrea Adami <andrea.adami@gmail.com> a écrit : > >> The Sharp SL Series (Zaurus) PXA handhelds have 16/64/128M of NAND flash >> and share the same layout of the first 7M partition, managed by Sharp FTL. >> >> The purpose of this self-contained patch is to add a common parser and >> remove the hardcoded sizes in the board files (these devices are not yet >> converted to devicetree). >> Users will have benefits because the mtdparts= tag will not be necessary >> anymore and they will be free to repartition the little sized flash. >> >> The obsolete bootloader can not pass the partitioning info to modern >> kernels anymore so it has to be read from flash at known logical addresses. >> (see http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm ) >> >> In kernel, under arch/arm/mach-pxa we have already 8 machines: >> MACH_POODLE, MACH_CORGI, MACH_SHEPERD, MACH_HUSKY, MACH_AKITA, MACH_SPITZ, >> MACH_BORZOI, MACH_TOSA. >> Lost after the 2.4 vendor kernel are MACH_BOXER and MACH_TERRIER. >> >> Almost every model has different factory partitioning: add to this the >> units can be repartitioned by users with userspace tools (nandlogical) >> and installers for popular (back then) linux distributions. >> >> The Parameter Area in the first (boot) partition extends from 0x00040000 to >> 0x0007bfff (176k) and contains two copies of the partition table: >> ... >> 0x00060000: Partition Info1 16k >> 0x00064000: Partition Info2 16k >> 0x00668000: Model 16k >> ... >> >> The first 7M partition is managed by the Sharp FTL reserving 5% + 1 blocks >> for wear-leveling: some blocks are remapped and one layer of translation >> (logical to physical) is necessary. >> >> There isn't much documentation about this FTL in the 2.4 sources, just the >> MTD methods for reading and writing using logical addresses and the block >> management (wear-leveling, use counter). >> For the purpose of the MTD parser only the read part of the code was taken. >> >> The NAND drivers that can use this parser are sharpsl.c and tmio_nand.c. >> >> Signed-off-by: Andrea Adami <andrea.adami@gmail.com> >> --- > > Please put a changelog here. Sure, I'll take the one from the cover-letter. > >> drivers/mtd/parsers/Kconfig | 7 + >> drivers/mtd/parsers/Makefile | 1 + >> drivers/mtd/parsers/sharpslpart.c | 382 ++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 390 insertions(+) >> create mode 100644 drivers/mtd/parsers/sharpslpart.c >> >> diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig >> index d206b3c..f371022 100644 >> --- a/drivers/mtd/parsers/Kconfig >> +++ b/drivers/mtd/parsers/Kconfig >> @@ -6,3 +6,10 @@ config MTD_PARSER_TRX >> may contain up to 3/4 partitions (depending on the version). >> This driver will parse TRX header and report at least two partitions: >> kernel and rootfs. > > Missing blank line. Fixed > >> +config MTD_SHARPSL_PARTS >> + tristate "Sharp SL Series NAND flash partition parser" >> + depends on MTD_NAND_SHARPSL || MTD_NAND_TMIO || COMPILE_TEST >> + help >> + This provides the read-only FTL logic necessary to read the partition >> + table from the NAND flash of Sharp SL Series (Zaurus) and the MTD >> + partition parser using this code. >> diff --git a/drivers/mtd/parsers/Makefile b/drivers/mtd/parsers/Makefile >> index 4d9024e..5b1bcc3 100644 >> --- a/drivers/mtd/parsers/Makefile >> +++ b/drivers/mtd/parsers/Makefile >> @@ -1 +1,2 @@ >> obj-$(CONFIG_MTD_PARSER_TRX) += parser_trx.o >> +obj-$(CONFIG_MTD_SHARPSL_PARTS) += sharpslpart.o >> diff --git a/drivers/mtd/parsers/sharpslpart.c b/drivers/mtd/parsers/sharpslpart.c >> new file mode 100644 >> index 0000000..b2333ae >> --- /dev/null >> +++ b/drivers/mtd/parsers/sharpslpart.c >> @@ -0,0 +1,382 @@ >> +/* >> + * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL >> + * for logical addressing, as used on the PXA models of the SHARP SL Series. >> + * >> + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com> >> + * >> + * Based on 2.4 sources: >> + * drivers/mtd/nand/sharp_sl_logical.c >> + * linux/include/asm-arm/sharp_nand_logical.h >> + * >> + * Copyright (C) 2002 SHARP >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/slab.h> >> +#include <linux/module.h> >> +#include <linux/types.h> >> +#include <linux/mtd/mtd.h> >> +#include <linux/mtd/partitions.h> >> + >> +/* oob structure */ >> +#define NAND_NOOB_LOGADDR_00 8 >> +#define NAND_NOOB_LOGADDR_01 9 >> +#define NAND_NOOB_LOGADDR_10 10 >> +#define NAND_NOOB_LOGADDR_11 11 >> +#define NAND_NOOB_LOGADDR_20 12 >> +#define NAND_NOOB_LOGADDR_21 13 >> + >> +#define BLOCK_IS_RESERVED 0xffff >> +#define BLOCK_UNMASK 0x07fe >> +#define BLOCK_UNMASK_COMPLEMENT 1 >> + >> +/* factory defaults */ >> +#define SHARPSL_NAND_PARTS 3 >> +#define SHARPSL_FTL_PART_SIZE (7 * 1024 * 1024) > > Nit: s/7 * 1024 * 1024/7 * SZ_1M/ Fixed > >> +#define PARAM_BLOCK_PARTITIONINFO1 0x00060000 >> +#define PARAM_BLOCK_PARTITIONINFO2 0x00064000 > > I'd rename the macro differently. The term block is a bit confusing > here since AFAIU the Param Area does not necessarily fit in a single > eraseblock. > > How about: > > #define PARAM_AREA_PARTINFO1_LADDR 0x60000 > #define PARAM_AREA_PARTINFO2_LADDR 0x64000 > > or simply > > #define SHARPSL_PARTINFO1_LADDR 0x60000 > #define SHARPSL_PARTINFO2_LADDR 0x64000 Used these, nicer > >> + >> +#define BOOT_MAGIC 0x424f4f54 >> +#define FSRO_MAGIC 0x4653524f >> +#define FSRW_MAGIC 0x46535257 >> + >> +/** >> + * struct sharpsl_ftl - Sharp FTL Logical Table >> + * @logmax: number of logical blocks >> + * @log2phy: the logical-to-physical table >> + * >> + * Stripped down from 2.4 sources for read-only purposes. > > Not sure this information is really useful, especially since you don't > provide a link to the 2.4 sources (or maybe I missed it). Maybe here I can add that this FTL was originally tailored for devices 16KiB erasesize matching the size of the PARAM_BLOCK_ above thus using two separate blocks for the partition tables. Should I add an ASCII-art? Pls see http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm BTW the link was in the cover-letter 0/9, I'll put it here together with the changelog. > > You'd better describe what this struct is used for here. Seems self-explanatory..2 fields commented. What would you add here? > >> + */ >> +struct sharpsl_ftl { >> + u_int logmax; >> + u_int *log2phy; > > Can you replace u_int by unsigned int and u_char by u8? That goes for > the whole file. Yes, done > >> +}; >> + >> +static int sharpsl_nand_read_oob(struct mtd_info *mtd, loff_t offs, size_t len, > > AFAICT, len is always set to mtd->oobsize. You can just drop this > parameter and set ops->ooblen to mtd->oobsize. > >> + uint8_t *buf) >> +{ >> + loff_t mask = mtd->writesize - 1; > > mask is equivalent to mtd->writesize_mask. > >> + struct mtd_oob_ops ops; > > struct mtd_oob_ops ops = { }; > > so that you don't have to initialize all fields if they are set to 0 or > NULL. > Ok done, I prefer this syntax, like in the other ftl using read_oob(). > You could also directly fill this object when declaring it: > > struct mtd_oob_ops ops = { > .mode = MTD_OPS_PLACE_OOB, > .ooblen = mtd->oobsize, > .oobbuf = buf, > }; > >> + int ret; >> + >> + ops.mode = MTD_OPS_PLACE_OOB; >> + ops.ooboffs = offs & mask; > > Hm, I'm pretty sure this is wrong. mask is a page mask, not the oob > area mask. It works because offs is always aligned on a page, but you > can directly assign ops.ooboffs to 0 and you should be good. > You are right, I removed the mask alltogether. >> + ops.ooblen = len; >> + ops.oobbuf = buf; >> + ops.datbuf = NULL; >> + >> + ret = mtd_read_oob(mtd, offs & ~mask, &ops); >> + if (ret != 0 || len != ops.oobretlen) >> + return -1; >> + >> + return 0; >> +} >> + >> +/* >> + * The logical block number assigned to a physical block is stored in the OOB >> + * of the first page, in 3 16-bit copies with the following layout: >> + * >> + * 01234567 89abcdef >> + * -------- -------- >> + * ECC BB xyxyxy >> + * >> + * When reading we check that the first two copies agree. >> + * In case of error, matching is tried using the following pairs. >> + * Reserved values 0xffff mean the block is kept for wear leveling. >> + * >> + * 01234567 89abcdef >> + * -------- -------- >> + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9 >> + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11 >> + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13 >> + * >> + */ >> +static u_int sharpsl_nand_get_logical_num(u_char *oob) >> +{ >> + u16 us; >> + int good0, good1; >> + >> + if (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] && >> + oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) { >> + good0 = NAND_NOOB_LOGADDR_00; >> + good1 = NAND_NOOB_LOGADDR_01; >> + } else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] && >> + oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) { >> + good0 = NAND_NOOB_LOGADDR_10; >> + good1 = NAND_NOOB_LOGADDR_11; >> + } else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] && >> + oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) { >> + good0 = NAND_NOOB_LOGADDR_20; >> + good1 = NAND_NOOB_LOGADDR_21; >> + } else { >> + /* wrong oob fingerprint, maybe here by mistake? */ >> + return UINT_MAX; >> + } >> + >> + us = oob[good0] | oob[good1] << 8; >> + >> + /* parity check */ >> + if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT) >> + return (UINT_MAX - 1); > > Why not making this function return an int and use regular error codes > for the parity and wrong fingerprint errors? > Originally in case of error it did return the largest values. I can return a negative int but then I'd have to check for log_no >0 Besides, what are the 'regular' error codes you'd use here? For the missing FTL we do return -EINVAL later so I'd say this is the error for the wrong oob fingerprint. About the parity error, it does return UINT_MAX -1 so to allow very large log_num. This value is always bigger than log_max so it is skipped in the actual code but the read continues. Should we change the philosophy of the old 2.4 code and exit in case of parity errors? >> + >> + /* reserved */ >> + if (us == BLOCK_IS_RESERVED) >> + return BLOCK_IS_RESERVED; >> + else > > You can drop the 'else' here. Done > >> + return (us & BLOCK_UNMASK) >> 1; > > So, this is a 10bit value, right? Why not doing the >> 1 first so that > your BLOCK_UNMASK mask really looks like a valid mask (0x3ff, which can > also be defined as GENMASK(9, 0)). Right, very nice. Done. Can I keep BLOCK_UNMASK COMPLEMENT = 1 ? > >> +} >> + >> +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size, > > s/sharpsl_nand_init_logical/sharpsl_nand_init_ftl/ Oh yes, renamed > > partition_size is always set to SHARPSL_FTL_PART_SIZE. Please drop this > argument. > Ok, done >> + struct sharpsl_ftl *ftl) >> +{ >> + u_int block_num, log_num, phymax; >> + loff_t block_adr; >> + u_char *oob; >> + int i, ret; >> + >> + oob = kzalloc(mtd->oobsize, GFP_KERNEL); >> + if (!oob) >> + return -ENOMEM; >> + >> + /* initialize management structure */ >> + phymax = (partition_size / mtd->erasesize); > > You can use mtd_div_by_eb() here. > Done for all occurrences >> + >> + /* FTL reserves 5% of the blocks + 1 spare */ >> + ftl->logmax = ((phymax * 95) / 100) - 1; >> + >> + ftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int), > > sizeof(*ftl->log2phy) Ok, changed. > >> + GFP_KERNEL); >> + if (!ftl->log2phy) { >> + ret = -ENOMEM; >> + goto exit; >> + } >> + >> + /* initialize ftl->log2phy */ >> + for (i = 0; i < ftl->logmax; i++) >> + ftl->log2phy[i] = UINT_MAX; >> + >> + /* create physical-logical table */ >> + for (block_num = 0; block_num < phymax; block_num++) { >> + block_adr = block_num * mtd->erasesize; >> + >> + if (mtd_block_isbad(mtd, block_adr)) >> + continue; >> + >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) >> + continue; >> + >> + /* get logical block */ >> + log_num = sharpsl_nand_get_logical_num(oob); >> + >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */ >> + if (log_num == UINT_MAX) { >> + pr_info("sharpslpart: Sharp SL FTL not found.\n"); >> + ret = -EINVAL; >> + goto exit; >> + } >> + >> + /* skip out of range and not unique values */ >> + if (log_num < ftl->logmax) { >> + if (ftl->log2phy[log_num] == UINT_MAX) >> + ftl->log2phy[log_num] = block_num; >> + } >> + } >> + >> + pr_info("Sharp SL FTL: %d blocks used (%d logical, %d reserved)\n", >> + phymax, ftl->logmax, >> + phymax - ftl->logmax); >> + >> + ret = 0; >> +exit: >> + kfree(oob); >> + return ret; >> +} >> + >> +void sharpsl_nand_cleanup_logical(struct sharpsl_ftl *ftl) > > s/sharpsl_nand_cleanup_logical/sharpsl_nand_cleanup_ftl/ > Yes, renamed as well >> +{ >> + kfree(ftl->log2phy); >> +} >> + >> +static int sharpsl_nand_read_laddr(struct mtd_info *mtd, >> + loff_t from, >> + size_t len, >> + u_char *buf, >> + struct sharpsl_ftl *ftl) >> +{ >> + u_int log_num, log_new; >> + u_int block_num; >> + loff_t block_adr; >> + loff_t block_ofs; >> + size_t retlen; >> + int err; >> + >> + log_num = (u32)from / mtd->erasesize; > > Please use mtd_div_by_eb(). > >> + log_new = ((u32)from + len - 1) / mtd->erasesize; > > Ditto. > > And I'm not sure log_new is a good name for this variable. Could be > last_log_num or something like that. > >> + >> + if (len <= 0 || log_num >= ftl->logmax || log_new > log_num) >> + return -EINVAL; >> + >> + block_num = ftl->log2phy[log_num]; >> + block_adr = block_num * mtd->erasesize; >> + block_ofs = (u32)from % mtd->erasesize; > > mtd_mod_by_eb(). > >> + >> + err = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf); >> + /* Ignore corrected ECC errors */ >> + if (mtd_is_bitflip(err)) >> + err = 0; > > Please add a blank line here > Ok >> + if (!err && retlen != len) >> + err = -EIO; > > and here. > Ok (was taken from mtdtest_read() fwiw) >> + if (err) >> + pr_err("sharpslpart: error, read failed at %#llx\n", >> + block_adr + block_ofs); >> + >> + return err; >> +} >> + >> +/* >> + * MTD Partition Parser >> + * >> + * Sample values read from SL-C860 >> + * >> + * # cat /proc/mtd >> + * dev: size erasesize name >> + * mtd0: 006d0000 00020000 "Filesystem" >> + * mtd1: 00700000 00004000 "smf" >> + * mtd2: 03500000 00004000 "root" >> + * mtd3: 04400000 00004000 "home" >> + * >> + * PARTITIONINFO1 >> + * 0x00060000: 00 00 00 00 00 00 70 00 42 4f 4f 54 00 00 00 00 ......p.BOOT.... >> + * 0x00060010: 00 00 70 00 00 00 c0 03 46 53 52 4f 00 00 00 00 ..p.....FSRO.... >> + * 0x00060020: 00 00 c0 03 00 00 00 04 46 53 52 57 00 00 00 00 ........FSRW.... >> + * 0x00060030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................ >> + * > > You can drop this extra empty line. Ok > >> + */ >> +struct sharpsl_nand_partinfo { >> + __le32 start; >> + __le32 end; >> + __be32 magic; >> + u32 reserved; >> +}; >> + >> +static int sharpsl_nand_read_partinfo(struct mtd_info *master, >> + loff_t from, >> + size_t len, >> + struct sharpsl_nand_partinfo *buf, >> + struct sharpsl_ftl *ftl) >> +{ >> + int ret; >> + >> + ret = sharpsl_nand_read_laddr(master, (u32)from, len, (u_char *)buf, > > The u32 cast is unneeded here, and I'd suggest to change the > sharpsl_nand_read_laddr() prototype to take a 'void *' instead of a > 'u_char *', so that you can also get rid of this cast. > Nice, done >> + ftl); >> + if (ret) >> + goto exit; > > Well, you have nothing to cleanup in the exit path, so you can return > directly here. > Ah, I have exaggerated here :) Done all three >> + >> + /* check for magics */ >> + if (be32_to_cpu(buf[0].magic) != BOOT_MAGIC || >> + be32_to_cpu(buf[1].magic) != FSRO_MAGIC || >> + be32_to_cpu(buf[2].magic) != FSRW_MAGIC) { >> + pr_err("sharpslpart: magic values mismatch\n"); >> + ret = -EINVAL; >> + goto exit; > > Ditto. > >> + } >> + >> + /* fixup for hardcoded value 64 MiB (for older models) */ >> + buf[2].end = cpu_to_le32(master->size); >> + >> + /* extra sanity check */ >> + if (le32_to_cpu(buf[0].end) <= le32_to_cpu(buf[0].start) || >> + le32_to_cpu(buf[1].start) < le32_to_cpu(buf[0].end) || >> + le32_to_cpu(buf[1].end) <= le32_to_cpu(buf[1].start) || >> + le32_to_cpu(buf[2].start) < le32_to_cpu(buf[1].end) || >> + le32_to_cpu(buf[2].end) <= le32_to_cpu(buf[2].start)) { >> + pr_err("sharpslpart: partition sizes mismatch\n"); >> + ret = -EINVAL; >> + goto exit; > > Ditto. > >> + } >> + >> + ret = 0; >> +exit: >> + return ret; >> +} >> + >> +static int sharpsl_parse_mtd_partitions(struct mtd_info *master, >> + const struct mtd_partition **pparts, >> + struct mtd_part_parser_data *data) >> +{ >> + struct sharpsl_ftl ftl; >> + struct sharpsl_nand_partinfo buf[SHARPSL_NAND_PARTS]; >> + struct mtd_partition *sharpsl_nand_parts; >> + int err, ret; >> + >> + /* init logical mgmt (FTL) */ >> + err = sharpsl_nand_init_logical(master, SHARPSL_FTL_PART_SIZE, &ftl); >> + if (err) >> + return err; >> + >> + /* read and validate first partition table */ >> + pr_info("sharpslpart: using first partition table\n"); >> + err = sharpsl_nand_read_partinfo(master, >> + PARAM_BLOCK_PARTITIONINFO1, >> + sizeof(buf), buf, &ftl); >> + if (err) { >> + /* fallback: read second partition table */ >> + pr_warn("sharpslpart: retry using second partition table\n"); >> + ret = sharpsl_nand_read_partinfo(master, >> + PARAM_BLOCK_PARTITIONINFO2, >> + sizeof(buf), buf, &ftl); > > Why do you need a different variable to store the second > sharpsl_nand_read_partinfo() return code? > It is unneeded, is a copy&paste remnant. Fixed. >> + if (ret) { >> + pr_err("sharpslpart: partition tables are invalid\n"); >> + sharpsl_nand_cleanup_logical(&ftl); >> + return ret; >> + } >> + } >> + >> + /* cleanup logical mgmt (FTL) */ >> + sharpsl_nand_cleanup_logical(&ftl); > > > How about: > > pr_info("sharpslpart: try reading first partition table\n"); > err = sharpsl_nand_read_partinfo(master, > PARAM_BLOCK_PARTITIONINFO1, > sizeof(buf), buf, &ftl); > if (err) { > /* fallback: read second partition table */ > pr_warn("sharpslpart: first partition table is invalid, retry using second partition table\n"); > err = sharpsl_nand_read_partinfo(master, > PARAM_BLOCK_PARTITIONINFO2, > sizeof(buf), buf, &ftl); > } > > /* cleanup logical mgmt (FTL) */ > sharpsl_nand_cleanup_logical(&ftl); > > if (err) { > pr_err("sharpslpart: both partition tables are invalid\n"); > return ret; > } > > so that you have a single place where you cleanup the FTL. Very nice. I was mumbling around it... Done > >> + >> + sharpsl_nand_parts = kzalloc(sizeof(*sharpsl_nand_parts) * >> + SHARPSL_NAND_PARTS, GFP_KERNEL); >> + if (!sharpsl_nand_parts) >> + return -ENOMEM; >> + >> + /* original names */ >> + sharpsl_nand_parts[0].name = "smf"; >> + sharpsl_nand_parts[0].offset = le32_to_cpu(buf[0].start); >> + sharpsl_nand_parts[0].size = le32_to_cpu(buf[0].end) - >> + le32_to_cpu(buf[0].start); >> + >> + sharpsl_nand_parts[1].name = "root"; >> + sharpsl_nand_parts[1].offset = le32_to_cpu(buf[1].start); >> + sharpsl_nand_parts[1].size = le32_to_cpu(buf[1].end) - >> + le32_to_cpu(buf[1].start); >> + >> + sharpsl_nand_parts[2].name = "home"; >> + sharpsl_nand_parts[2].offset = le32_to_cpu(buf[2].start); >> + sharpsl_nand_parts[2].size = le32_to_cpu(buf[2].end) - >> + le32_to_cpu(buf[2].start); >> + >> + *pparts = sharpsl_nand_parts; >> + return SHARPSL_NAND_PARTS; >> +} >> + >> +static struct mtd_part_parser sharpsl_mtd_parser = { >> + .parse_fn = sharpsl_parse_mtd_partitions, >> + .name = "sharpslpart", >> +}; >> +module_mtd_part_parser(sharpsl_mtd_parser); >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_AUTHOR("Andrea Adami <andrea.adami@gmail.com>"); >> +MODULE_DESCRIPTION("MTD partitioning for NAND flash on Sharp SL Series"); > Thank you very much. Code is getting better and better. Andrea
On Thu, 24 Aug 2017 11:19:56 +0200 Andrea Adami <andrea.adami@gmail.com> wrote: > >> +/** > >> + * struct sharpsl_ftl - Sharp FTL Logical Table > >> + * @logmax: number of logical blocks > >> + * @log2phy: the logical-to-physical table > >> + * > >> + * Stripped down from 2.4 sources for read-only purposes. > > > > Not sure this information is really useful, especially since you don't > > provide a link to the 2.4 sources (or maybe I missed it). > > Maybe here I can add that this FTL was originally tailored for devices > 16KiB erasesize matching the size of the PARAM_BLOCK_ above thus using > two separate blocks for the partition tables. Should I add an > ASCII-art? You can add an ASCII-art if you want but that's not mandatory if you can explain it with words :-). > Pls see > http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm > BTW the link was in the cover-letter 0/9, I'll put it here together > with the changelog. Please put it in the code (in a comment). The changelog will disappear as soon as I apply the patch. > > > > > You'd better describe what this struct is used for here. > Seems self-explanatory..2 fields commented. What would you add here? Just that the struct contains the logical-to-physical translation table used by the SHARPSL FTL. It's just that your initial comment didn't bring any useful information. > > > > >> + */ > >> +struct sharpsl_ftl { > >> + u_int logmax; > >> + u_int *log2phy; > > [...] > >> +/* > >> + * The logical block number assigned to a physical block is stored in the OOB > >> + * of the first page, in 3 16-bit copies with the following layout: > >> + * > >> + * 01234567 89abcdef > >> + * -------- -------- > >> + * ECC BB xyxyxy > >> + * > >> + * When reading we check that the first two copies agree. > >> + * In case of error, matching is tried using the following pairs. > >> + * Reserved values 0xffff mean the block is kept for wear leveling. > >> + * > >> + * 01234567 89abcdef > >> + * -------- -------- > >> + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9 > >> + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11 > >> + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13 > >> + * > >> + */ > >> +static u_int sharpsl_nand_get_logical_num(u_char *oob) > >> +{ > >> + u16 us; > >> + int good0, good1; > >> + > >> + if (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] && > >> + oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) { > >> + good0 = NAND_NOOB_LOGADDR_00; > >> + good1 = NAND_NOOB_LOGADDR_01; > >> + } else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] && > >> + oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) { > >> + good0 = NAND_NOOB_LOGADDR_10; > >> + good1 = NAND_NOOB_LOGADDR_11; > >> + } else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] && > >> + oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) { > >> + good0 = NAND_NOOB_LOGADDR_20; > >> + good1 = NAND_NOOB_LOGADDR_21; > >> + } else { > >> + /* wrong oob fingerprint, maybe here by mistake? */ > >> + return UINT_MAX; > >> + } > >> + > >> + us = oob[good0] | oob[good1] << 8; > >> + > >> + /* parity check */ > >> + if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT) > >> + return (UINT_MAX - 1); > > > > Why not making this function return an int and use regular error codes > > for the parity and wrong fingerprint errors? > > > Originally in case of error it did return the largest values. > I can return a negative int but then I'd have to check for log_no >0 > > Besides, what are the 'regular' error codes you'd use here? > For the missing FTL we do return -EINVAL later so I'd say this is the > error for the wrong oob fingerprint. EINVAL should be fine for all corruption/mismatch. > > About the parity error, it does return UINT_MAX -1 so to allow very > large log_num. This value is always bigger than log_max so it is > skipped in the actual code but the read continues. Well, a parity error is still an error, right? > Should we change the philosophy of the old 2.4 code and exit in case > of parity errors? Hm, you should not exit if the phys -> log information is corrupted, it just means you can't consider this physical block is containing a valid logical block, that's all. Pretty much like when there's an oob fingerprint mismatch. > > >> + > >> + /* reserved */ > >> + if (us == BLOCK_IS_RESERVED) > >> + return BLOCK_IS_RESERVED; > >> + else > > > > You can drop the 'else' here. > Done > > > >> + return (us & BLOCK_UNMASK) >> 1; > > > > So, this is a 10bit value, right? Why not doing the >> 1 first so that > > your BLOCK_UNMASK mask really looks like a valid mask (0x3ff, which can > > also be defined as GENMASK(9, 0)). > Right, very nice. Done. > Can I keep BLOCK_UNMASK COMPLEMENT = 1 ? Yes. > > > > >> +} > >> + > >> +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size, > > > > s/sharpsl_nand_init_logical/sharpsl_nand_init_ftl/ > Oh yes, renamed > > > > > partition_size is always set to SHARPSL_FTL_PART_SIZE. Please drop this > > argument. > > > Ok, done > > >> + struct sharpsl_ftl *ftl) > >> +{ > >> + u_int block_num, log_num, phymax; > >> + loff_t block_adr; > >> + u_char *oob; > >> + int i, ret; > >> + > >> + oob = kzalloc(mtd->oobsize, GFP_KERNEL); > >> + if (!oob) > >> + return -ENOMEM; > >> + > >> + /* initialize management structure */ > >> + phymax = (partition_size / mtd->erasesize); > > > > You can use mtd_div_by_eb() here. > > > Done for all occurrences > > >> + > >> + /* FTL reserves 5% of the blocks + 1 spare */ > >> + ftl->logmax = ((phymax * 95) / 100) - 1; > >> + > >> + ftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int), > > > > sizeof(*ftl->log2phy) > Ok, changed. > > > > >> + GFP_KERNEL); > >> + if (!ftl->log2phy) { > >> + ret = -ENOMEM; > >> + goto exit; > >> + } > >> + > >> + /* initialize ftl->log2phy */ > >> + for (i = 0; i < ftl->logmax; i++) > >> + ftl->log2phy[i] = UINT_MAX; > >> + > >> + /* create physical-logical table */ > >> + for (block_num = 0; block_num < phymax; block_num++) { > >> + block_adr = block_num * mtd->erasesize; > >> + > >> + if (mtd_block_isbad(mtd, block_adr)) > >> + continue; > >> + > >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) > >> + continue; > >> + > >> + /* get logical block */ > >> + log_num = sharpsl_nand_get_logical_num(oob); > >> + > >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */ > >> + if (log_num == UINT_MAX) { > >> + pr_info("sharpslpart: Sharp SL FTL not found.\n"); > >> + ret = -EINVAL; > >> + goto exit; > >> + } Okay, I overlooked that part. Why do you exit if there's a fingerprint mismatch? Can't you just ignore this physical block and continue scanning the remaining ones?
On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Thu, 24 Aug 2017 11:19:56 +0200 > Andrea Adami <andrea.adami@gmail.com> wrote: > >> >> +/** >> >> + * struct sharpsl_ftl - Sharp FTL Logical Table >> >> + * @logmax: number of logical blocks >> >> + * @log2phy: the logical-to-physical table >> >> + * >> >> + * Stripped down from 2.4 sources for read-only purposes. >> > >> > Not sure this information is really useful, especially since you don't >> > provide a link to the 2.4 sources (or maybe I missed it). >> >> Maybe here I can add that this FTL was originally tailored for devices >> 16KiB erasesize matching the size of the PARAM_BLOCK_ above thus using >> two separate blocks for the partition tables. Should I add an >> ASCII-art? > > You can add an ASCII-art if you want but that's not mandatory if you > can explain it with words :-). > >> Pls see >> http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm >> BTW the link was in the cover-letter 0/9, I'll put it here together >> with the changelog. > > Please put it in the code (in a comment). The changelog will disappear > as soon as I apply the patch. > >> >> > >> > You'd better describe what this struct is used for here. >> Seems self-explanatory..2 fields commented. What would you add here? > > Just that the struct contains the logical-to-physical translation > table used by the SHARPSL FTL. It's just that your initial comment > didn't bring any useful information. > >> >> > >> >> + */ >> >> +struct sharpsl_ftl { >> >> + u_int logmax; >> >> + u_int *log2phy; >> > > > [...] > >> >> +/* >> >> + * The logical block number assigned to a physical block is stored in the OOB >> >> + * of the first page, in 3 16-bit copies with the following layout: >> >> + * >> >> + * 01234567 89abcdef >> >> + * -------- -------- >> >> + * ECC BB xyxyxy >> >> + * >> >> + * When reading we check that the first two copies agree. >> >> + * In case of error, matching is tried using the following pairs. >> >> + * Reserved values 0xffff mean the block is kept for wear leveling. >> >> + * >> >> + * 01234567 89abcdef >> >> + * -------- -------- >> >> + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9 >> >> + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11 >> >> + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13 >> >> + * >> >> + */ >> >> +static u_int sharpsl_nand_get_logical_num(u_char *oob) >> >> +{ >> >> + u16 us; >> >> + int good0, good1; >> >> + >> >> + if (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] && >> >> + oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) { >> >> + good0 = NAND_NOOB_LOGADDR_00; >> >> + good1 = NAND_NOOB_LOGADDR_01; >> >> + } else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] && >> >> + oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) { >> >> + good0 = NAND_NOOB_LOGADDR_10; >> >> + good1 = NAND_NOOB_LOGADDR_11; >> >> + } else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] && >> >> + oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) { >> >> + good0 = NAND_NOOB_LOGADDR_20; >> >> + good1 = NAND_NOOB_LOGADDR_21; >> >> + } else { >> >> + /* wrong oob fingerprint, maybe here by mistake? */ >> >> + return UINT_MAX; >> >> + } >> >> + >> >> + us = oob[good0] | oob[good1] << 8; >> >> + >> >> + /* parity check */ >> >> + if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT) >> >> + return (UINT_MAX - 1); >> > >> > Why not making this function return an int and use regular error codes >> > for the parity and wrong fingerprint errors? >> > >> Originally in case of error it did return the largest values. >> I can return a negative int but then I'd have to check for log_no >0 >> >> Besides, what are the 'regular' error codes you'd use here? >> For the missing FTL we do return -EINVAL later so I'd say this is the >> error for the wrong oob fingerprint. > > EINVAL should be fine for all corruption/mismatch. > >> >> About the parity error, it does return UINT_MAX -1 so to allow very >> large log_num. This value is always bigger than log_max so it is >> skipped in the actual code but the read continues. > > Well, a parity error is still an error, right? > >> Should we change the philosophy of the old 2.4 code and exit in case >> of parity errors? > > Hm, you should not exit if the phys -> log information is corrupted, it > just means you can't consider this physical block is containing a valid > logical block, that's all. Pretty much like when there's an oob > fingerprint mismatch. For the partition parser purposes, we don't know in advance in which physical block the partition tables are stored. Maybe the parity is occurring on 'uninteresting' blocks, so I'll go head to maximize the probabilties to get the tables. > >> >> >> + >> >> + /* reserved */ >> >> + if (us == BLOCK_IS_RESERVED) >> >> + return BLOCK_IS_RESERVED; >> >> + else >> > >> > You can drop the 'else' here. >> Done >> > >> >> + return (us & BLOCK_UNMASK) >> 1; >> > >> > So, this is a 10bit value, right? Why not doing the >> 1 first so that >> > your BLOCK_UNMASK mask really looks like a valid mask (0x3ff, which can >> > also be defined as GENMASK(9, 0)). >> Right, very nice. Done. >> Can I keep BLOCK_UNMASK COMPLEMENT = 1 ? > > Yes. > >> >> > >> >> +} >> >> + >> >> +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size, >> > >> > s/sharpsl_nand_init_logical/sharpsl_nand_init_ftl/ >> Oh yes, renamed >> >> > >> > partition_size is always set to SHARPSL_FTL_PART_SIZE. Please drop this >> > argument. >> > >> Ok, done >> >> >> + struct sharpsl_ftl *ftl) >> >> +{ >> >> + u_int block_num, log_num, phymax; >> >> + loff_t block_adr; >> >> + u_char *oob; >> >> + int i, ret; >> >> + >> >> + oob = kzalloc(mtd->oobsize, GFP_KERNEL); >> >> + if (!oob) >> >> + return -ENOMEM; >> >> + >> >> + /* initialize management structure */ >> >> + phymax = (partition_size / mtd->erasesize); >> > >> > You can use mtd_div_by_eb() here. >> > >> Done for all occurrences >> >> >> + >> >> + /* FTL reserves 5% of the blocks + 1 spare */ >> >> + ftl->logmax = ((phymax * 95) / 100) - 1; >> >> + >> >> + ftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int), >> > >> > sizeof(*ftl->log2phy) >> Ok, changed. >> >> > >> >> + GFP_KERNEL); >> >> + if (!ftl->log2phy) { >> >> + ret = -ENOMEM; >> >> + goto exit; >> >> + } >> >> + >> >> + /* initialize ftl->log2phy */ >> >> + for (i = 0; i < ftl->logmax; i++) >> >> + ftl->log2phy[i] = UINT_MAX; >> >> + >> >> + /* create physical-logical table */ >> >> + for (block_num = 0; block_num < phymax; block_num++) { >> >> + block_adr = block_num * mtd->erasesize; >> >> + >> >> + if (mtd_block_isbad(mtd, block_adr)) >> >> + continue; >> >> + >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) >> >> + continue; >> >> + >> >> + /* get logical block */ >> >> + log_num = sharpsl_nand_get_logical_num(oob); >> >> + >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */ >> >> + if (log_num == UINT_MAX) { >> >> + pr_info("sharpslpart: Sharp SL FTL not found.\n"); >> >> + ret = -EINVAL; >> >> + goto exit; >> >> + } > > Okay, I overlooked that part. Why do you exit if there's a fingerprint > mismatch? Can't you just ignore this physical block and continue > scanning the remaining ones? Norris asked to quit immediately in this case. https://patchwork.kernel.org/patch/9758361/ "I wouldn't expect people to want to use this parser, but if we have a quick way to say "this doesn't match, skip me", then that would be helpful." "We don't want to waste too much time scanning for this partition table if possible." Now we are quitting ever before checking for parity erors ... Cheers Andrea
On Thu, 24 Aug 2017 12:30:02 +0200 Andrea Adami <andrea.adami@gmail.com> wrote: > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Thu, 24 Aug 2017 11:19:56 +0200 > > Andrea Adami <andrea.adami@gmail.com> wrote: > > > >> >> +/** > >> >> + * struct sharpsl_ftl - Sharp FTL Logical Table > >> >> + * @logmax: number of logical blocks > >> >> + * @log2phy: the logical-to-physical table > >> >> + * > >> >> + * Stripped down from 2.4 sources for read-only purposes. > >> > > >> > Not sure this information is really useful, especially since you don't > >> > provide a link to the 2.4 sources (or maybe I missed it). > >> > >> Maybe here I can add that this FTL was originally tailored for devices > >> 16KiB erasesize matching the size of the PARAM_BLOCK_ above thus using > >> two separate blocks for the partition tables. Should I add an > >> ASCII-art? > > > > You can add an ASCII-art if you want but that's not mandatory if you > > can explain it with words :-). > > > >> Pls see > >> http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm > >> BTW the link was in the cover-letter 0/9, I'll put it here together > >> with the changelog. > > > > Please put it in the code (in a comment). The changelog will disappear > > as soon as I apply the patch. > > > >> > >> > > >> > You'd better describe what this struct is used for here. > >> Seems self-explanatory..2 fields commented. What would you add here? > > > > Just that the struct contains the logical-to-physical translation > > table used by the SHARPSL FTL. It's just that your initial comment > > didn't bring any useful information. > > > >> > >> > > >> >> + */ > >> >> +struct sharpsl_ftl { > >> >> + u_int logmax; > >> >> + u_int *log2phy; > >> > > > > > [...] > > > >> >> +/* > >> >> + * The logical block number assigned to a physical block is stored in the OOB > >> >> + * of the first page, in 3 16-bit copies with the following layout: > >> >> + * > >> >> + * 01234567 89abcdef > >> >> + * -------- -------- > >> >> + * ECC BB xyxyxy > >> >> + * > >> >> + * When reading we check that the first two copies agree. > >> >> + * In case of error, matching is tried using the following pairs. > >> >> + * Reserved values 0xffff mean the block is kept for wear leveling. > >> >> + * > >> >> + * 01234567 89abcdef > >> >> + * -------- -------- > >> >> + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9 > >> >> + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11 > >> >> + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13 > >> >> + * > >> >> + */ > >> >> +static u_int sharpsl_nand_get_logical_num(u_char *oob) > >> >> +{ > >> >> + u16 us; > >> >> + int good0, good1; > >> >> + > >> >> + if (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] && > >> >> + oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) { > >> >> + good0 = NAND_NOOB_LOGADDR_00; > >> >> + good1 = NAND_NOOB_LOGADDR_01; > >> >> + } else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] && > >> >> + oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) { > >> >> + good0 = NAND_NOOB_LOGADDR_10; > >> >> + good1 = NAND_NOOB_LOGADDR_11; > >> >> + } else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] && > >> >> + oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) { > >> >> + good0 = NAND_NOOB_LOGADDR_20; > >> >> + good1 = NAND_NOOB_LOGADDR_21; > >> >> + } else { > >> >> + /* wrong oob fingerprint, maybe here by mistake? */ > >> >> + return UINT_MAX; > >> >> + } > >> >> + > >> >> + us = oob[good0] | oob[good1] << 8; > >> >> + > >> >> + /* parity check */ > >> >> + if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT) > >> >> + return (UINT_MAX - 1); > >> > > >> > Why not making this function return an int and use regular error codes > >> > for the parity and wrong fingerprint errors? > >> > > >> Originally in case of error it did return the largest values. > >> I can return a negative int but then I'd have to check for log_no >0 > >> > >> Besides, what are the 'regular' error codes you'd use here? > >> For the missing FTL we do return -EINVAL later so I'd say this is the > >> error for the wrong oob fingerprint. > > > > EINVAL should be fine for all corruption/mismatch. > > > >> > >> About the parity error, it does return UINT_MAX -1 so to allow very > >> large log_num. This value is always bigger than log_max so it is > >> skipped in the actual code but the read continues. > > > > Well, a parity error is still an error, right? > > > >> Should we change the philosophy of the old 2.4 code and exit in case > >> of parity errors? > > > > Hm, you should not exit if the phys -> log information is corrupted, it > > just means you can't consider this physical block is containing a valid > > logical block, that's all. Pretty much like when there's an oob > > fingerprint mismatch. > > For the partition parser purposes, we don't know in advance in which > physical block the partition tables are stored. > Maybe the parity is occurring on 'uninteresting' blocks, so I'll go > head to maximize the probabilties to get the tables. Yep, that's my point. You need to build the translation table before deciding whether a valid partinfo is present or not. > > > > >> > >> >> + > >> >> + /* reserved */ > >> >> + if (us == BLOCK_IS_RESERVED) > >> >> + return BLOCK_IS_RESERVED; > >> >> + else > >> > > >> > You can drop the 'else' here. > >> Done > >> > > >> >> + return (us & BLOCK_UNMASK) >> 1; > >> > > >> > So, this is a 10bit value, right? Why not doing the >> 1 first so that > >> > your BLOCK_UNMASK mask really looks like a valid mask (0x3ff, which can > >> > also be defined as GENMASK(9, 0)). > >> Right, very nice. Done. > >> Can I keep BLOCK_UNMASK COMPLEMENT = 1 ? > > > > Yes. > > > >> > >> > > >> >> +} > >> >> + > >> >> +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size, > >> > > >> > s/sharpsl_nand_init_logical/sharpsl_nand_init_ftl/ > >> Oh yes, renamed > >> > >> > > >> > partition_size is always set to SHARPSL_FTL_PART_SIZE. Please drop this > >> > argument. > >> > > >> Ok, done > >> > >> >> + struct sharpsl_ftl *ftl) > >> >> +{ > >> >> + u_int block_num, log_num, phymax; > >> >> + loff_t block_adr; > >> >> + u_char *oob; > >> >> + int i, ret; > >> >> + > >> >> + oob = kzalloc(mtd->oobsize, GFP_KERNEL); > >> >> + if (!oob) > >> >> + return -ENOMEM; > >> >> + > >> >> + /* initialize management structure */ > >> >> + phymax = (partition_size / mtd->erasesize); > >> > > >> > You can use mtd_div_by_eb() here. > >> > > >> Done for all occurrences > >> > >> >> + > >> >> + /* FTL reserves 5% of the blocks + 1 spare */ > >> >> + ftl->logmax = ((phymax * 95) / 100) - 1; > >> >> + > >> >> + ftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int), > >> > > >> > sizeof(*ftl->log2phy) > >> Ok, changed. > >> > >> > > >> >> + GFP_KERNEL); > >> >> + if (!ftl->log2phy) { > >> >> + ret = -ENOMEM; > >> >> + goto exit; > >> >> + } > >> >> + > >> >> + /* initialize ftl->log2phy */ > >> >> + for (i = 0; i < ftl->logmax; i++) > >> >> + ftl->log2phy[i] = UINT_MAX; > >> >> + > >> >> + /* create physical-logical table */ > >> >> + for (block_num = 0; block_num < phymax; block_num++) { > >> >> + block_adr = block_num * mtd->erasesize; > >> >> + > >> >> + if (mtd_block_isbad(mtd, block_adr)) > >> >> + continue; > >> >> + > >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) > >> >> + continue; > >> >> + > >> >> + /* get logical block */ > >> >> + log_num = sharpsl_nand_get_logical_num(oob); > >> >> + > >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */ > >> >> + if (log_num == UINT_MAX) { > >> >> + pr_info("sharpslpart: Sharp SL FTL not found.\n"); > >> >> + ret = -EINVAL; > >> >> + goto exit; > >> >> + } > > > > Okay, I overlooked that part. Why do you exit if there's a fingerprint > > mismatch? Can't you just ignore this physical block and continue > > scanning the remaining ones? > > Norris asked to quit immediately in this case. > https://patchwork.kernel.org/patch/9758361/ > > "I wouldn't expect people to want to use this parser, but if we have a > quick way to say "this doesn't match, skip me", then that would be > helpful." > "We don't want to waste too much time scanning for this partition > table if possible." Actually, you don't save much by exiting on "bad OOB fingerprint". If you look at the code you'll see that the only thing you check is whether some oob portions are equal or not, and most of the time the OOB area is left untouched by the upper layer, which means all free bytes will be left to 0xff, which in turn means the "is fingerprint good?" test will pass. > > Now we are quitting ever before checking for parity erors ... Honestly, I'd recommend not using this parser for anything but SHARPSL platforms, so I don't think we care much about the "scan all blocks" overhead. Moreover, the sharpsl parser is the last one in the part_parsers list, so it should be quite fast if the user specifies a valid mtdparts= on the cmdline or valid partitions in the DT.
On Tue, 22 Aug 2017 11:42:52 +0200 Andrea Adami <andrea.adami@gmail.com> wrote: > The Sharp SL Series (Zaurus) PXA handhelds have 16/64/128M of NAND flash > and share the same layout of the first 7M partition, managed by Sharp FTL. > > The purpose of this self-contained patch is to add a common parser and > remove the hardcoded sizes in the board files (these devices are not yet > converted to devicetree). > Users will have benefits because the mtdparts= tag will not be necessary > anymore and they will be free to repartition the little sized flash. > > The obsolete bootloader can not pass the partitioning info to modern > kernels anymore so it has to be read from flash at known logical addresses. > (see http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm ) > > In kernel, under arch/arm/mach-pxa we have already 8 machines: > MACH_POODLE, MACH_CORGI, MACH_SHEPERD, MACH_HUSKY, MACH_AKITA, MACH_SPITZ, > MACH_BORZOI, MACH_TOSA. > Lost after the 2.4 vendor kernel are MACH_BOXER and MACH_TERRIER. > > Almost every model has different factory partitioning: add to this the > units can be repartitioned by users with userspace tools (nandlogical) > and installers for popular (back then) linux distributions. > > The Parameter Area in the first (boot) partition extends from 0x00040000 to > 0x0007bfff (176k) and contains two copies of the partition table: > ... > 0x00060000: Partition Info1 16k > 0x00064000: Partition Info2 16k > 0x00668000: Model 16k > ... > > The first 7M partition is managed by the Sharp FTL reserving 5% + 1 blocks > for wear-leveling: some blocks are remapped and one layer of translation > (logical to physical) is necessary. > > There isn't much documentation about this FTL in the 2.4 sources, just the > MTD methods for reading and writing using logical addresses and the block > management (wear-leveling, use counter). > For the purpose of the MTD parser only the read part of the code was taken. > > The NAND drivers that can use this parser are sharpsl.c and tmio_nand.c. > > Signed-off-by: Andrea Adami <andrea.adami@gmail.com> > --- > drivers/mtd/parsers/Kconfig | 7 + > drivers/mtd/parsers/Makefile | 1 + > drivers/mtd/parsers/sharpslpart.c | 382 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 390 insertions(+) > create mode 100644 drivers/mtd/parsers/sharpslpart.c > > diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig > index d206b3c..f371022 100644 > --- a/drivers/mtd/parsers/Kconfig > +++ b/drivers/mtd/parsers/Kconfig > @@ -6,3 +6,10 @@ config MTD_PARSER_TRX > may contain up to 3/4 partitions (depending on the version). > This driver will parse TRX header and report at least two partitions: > kernel and rootfs. > +config MTD_SHARPSL_PARTS > + tristate "Sharp SL Series NAND flash partition parser" > + depends on MTD_NAND_SHARPSL || MTD_NAND_TMIO || COMPILE_TEST > + help > + This provides the read-only FTL logic necessary to read the partition > + table from the NAND flash of Sharp SL Series (Zaurus) and the MTD > + partition parser using this code. > diff --git a/drivers/mtd/parsers/Makefile b/drivers/mtd/parsers/Makefile > index 4d9024e..5b1bcc3 100644 > --- a/drivers/mtd/parsers/Makefile > +++ b/drivers/mtd/parsers/Makefile > @@ -1 +1,2 @@ > obj-$(CONFIG_MTD_PARSER_TRX) += parser_trx.o > +obj-$(CONFIG_MTD_SHARPSL_PARTS) += sharpslpart.o > diff --git a/drivers/mtd/parsers/sharpslpart.c b/drivers/mtd/parsers/sharpslpart.c > new file mode 100644 > index 0000000..b2333ae > --- /dev/null > +++ b/drivers/mtd/parsers/sharpslpart.c > @@ -0,0 +1,382 @@ > +/* > + * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL > + * for logical addressing, as used on the PXA models of the SHARP SL Series. > + * > + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com> > + * > + * Based on 2.4 sources: > + * drivers/mtd/nand/sharp_sl_logical.c > + * linux/include/asm-arm/sharp_nand_logical.h > + * > + * Copyright (C) 2002 SHARP > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/module.h> > +#include <linux/types.h> > +#include <linux/mtd/mtd.h> > +#include <linux/mtd/partitions.h> > + > +/* oob structure */ > +#define NAND_NOOB_LOGADDR_00 8 > +#define NAND_NOOB_LOGADDR_01 9 > +#define NAND_NOOB_LOGADDR_10 10 > +#define NAND_NOOB_LOGADDR_11 11 > +#define NAND_NOOB_LOGADDR_20 12 > +#define NAND_NOOB_LOGADDR_21 13 > + > +#define BLOCK_IS_RESERVED 0xffff > +#define BLOCK_UNMASK 0x07fe > +#define BLOCK_UNMASK_COMPLEMENT 1 > + > +/* factory defaults */ > +#define SHARPSL_NAND_PARTS 3 > +#define SHARPSL_FTL_PART_SIZE (7 * 1024 * 1024) > +#define PARAM_BLOCK_PARTITIONINFO1 0x00060000 > +#define PARAM_BLOCK_PARTITIONINFO2 0x00064000 > + > +#define BOOT_MAGIC 0x424f4f54 > +#define FSRO_MAGIC 0x4653524f > +#define FSRW_MAGIC 0x46535257 > + > +/** > + * struct sharpsl_ftl - Sharp FTL Logical Table > + * @logmax: number of logical blocks > + * @log2phy: the logical-to-physical table > + * > + * Stripped down from 2.4 sources for read-only purposes. > + */ > +struct sharpsl_ftl { > + u_int logmax; > + u_int *log2phy; > +}; > + > +static int sharpsl_nand_read_oob(struct mtd_info *mtd, loff_t offs, size_t len, > + uint8_t *buf) > +{ > + loff_t mask = mtd->writesize - 1; > + struct mtd_oob_ops ops; > + int ret; > + > + ops.mode = MTD_OPS_PLACE_OOB; > + ops.ooboffs = offs & mask; > + ops.ooblen = len; > + ops.oobbuf = buf; > + ops.datbuf = NULL; > + > + ret = mtd_read_oob(mtd, offs & ~mask, &ops); > + if (ret != 0 || len != ops.oobretlen) > + return -1; > + > + return 0; > +} > + > +/* > + * The logical block number assigned to a physical block is stored in the OOB > + * of the first page, in 3 16-bit copies with the following layout: > + * > + * 01234567 89abcdef > + * -------- -------- > + * ECC BB xyxyxy > + * > + * When reading we check that the first two copies agree. > + * In case of error, matching is tried using the following pairs. > + * Reserved values 0xffff mean the block is kept for wear leveling. > + * > + * 01234567 89abcdef > + * -------- -------- > + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9 > + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11 > + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13 I know there's a depends on "MTD_NAND_SHARPSL || MTD_NAND_TMIO" in the Kconfig entry, but one could enable those options just to use the sharpsl part parser even if the OOB layout is incompatible with the sharpsl FTL. I'd recommend that you check that OOB bytes 8 to 15 are actually free to be used by the MTD user in sharpsl_parse_mtd_partitions(). Can be done with something like that: static int sharpsl_nand_check_ooblayout(struct mtd_info *mtd) { u8 freebytes = 0; int section = 0; while (true) { struct mtd_oob_region oobfree = { }; int ret, i; ret = mtd_ooblayout_free(mtd, section++, &oobfree); if (ret) break; if (!oobfree.length || oobfree.offset > 15 || (oobfree.offset + oobfree.length) < 8) continue; i = oobfree.offset >= 8 ? : 8; for (; i < oobfree.offset + oobfree.length && i < 16; i++) freebytes |= BIT(i - 8); if (freebytes == 0xff) return 0; } return -ENOTSUPP; } > + * > + */ > +static u_int sharpsl_nand_get_logical_num(u_char *oob) > +{ > + u16 us; > + int good0, good1; > + > + if (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] && > + oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) { > + good0 = NAND_NOOB_LOGADDR_00; > + good1 = NAND_NOOB_LOGADDR_01; > + } else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] && > + oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) { > + good0 = NAND_NOOB_LOGADDR_10; > + good1 = NAND_NOOB_LOGADDR_11; > + } else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] && > + oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) { > + good0 = NAND_NOOB_LOGADDR_20; > + good1 = NAND_NOOB_LOGADDR_21; > + } else { > + /* wrong oob fingerprint, maybe here by mistake? */ > + return UINT_MAX; > + } > + > + us = oob[good0] | oob[good1] << 8; > + > + /* parity check */ > + if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT) > + return (UINT_MAX - 1); > + > + /* reserved */ > + if (us == BLOCK_IS_RESERVED) > + return BLOCK_IS_RESERVED; > + else > + return (us & BLOCK_UNMASK) >> 1; > +} > + > +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size, > + struct sharpsl_ftl *ftl) > +{ > + u_int block_num, log_num, phymax; > + loff_t block_adr; > + u_char *oob; > + int i, ret; > + > + oob = kzalloc(mtd->oobsize, GFP_KERNEL); > + if (!oob) > + return -ENOMEM; > + > + /* initialize management structure */ > + phymax = (partition_size / mtd->erasesize); > + > + /* FTL reserves 5% of the blocks + 1 spare */ > + ftl->logmax = ((phymax * 95) / 100) - 1; > + > + ftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int), > + GFP_KERNEL); > + if (!ftl->log2phy) { > + ret = -ENOMEM; > + goto exit; > + } > + > + /* initialize ftl->log2phy */ > + for (i = 0; i < ftl->logmax; i++) > + ftl->log2phy[i] = UINT_MAX; > + > + /* create physical-logical table */ > + for (block_num = 0; block_num < phymax; block_num++) { > + block_adr = block_num * mtd->erasesize; > + > + if (mtd_block_isbad(mtd, block_adr)) > + continue; > + > + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) > + continue; > + > + /* get logical block */ > + log_num = sharpsl_nand_get_logical_num(oob); > + > + /* FTL is not used? Exit here if the oob fingerprint is wrong */ > + if (log_num == UINT_MAX) { > + pr_info("sharpslpart: Sharp SL FTL not found.\n"); > + ret = -EINVAL; > + goto exit; > + } > + > + /* skip out of range and not unique values */ > + if (log_num < ftl->logmax) { > + if (ftl->log2phy[log_num] == UINT_MAX) > + ftl->log2phy[log_num] = block_num; > + } > + } > + > + pr_info("Sharp SL FTL: %d blocks used (%d logical, %d reserved)\n", > + phymax, ftl->logmax, > + phymax - ftl->logmax); > + > + ret = 0; > +exit: > + kfree(oob); > + return ret; > +} > + > +void sharpsl_nand_cleanup_logical(struct sharpsl_ftl *ftl) > +{ > + kfree(ftl->log2phy); > +} > + > +static int sharpsl_nand_read_laddr(struct mtd_info *mtd, > + loff_t from, > + size_t len, > + u_char *buf, > + struct sharpsl_ftl *ftl) > +{ > + u_int log_num, log_new; > + u_int block_num; > + loff_t block_adr; > + loff_t block_ofs; > + size_t retlen; > + int err; > + > + log_num = (u32)from / mtd->erasesize; > + log_new = ((u32)from + len - 1) / mtd->erasesize; > + > + if (len <= 0 || log_num >= ftl->logmax || log_new > log_num) > + return -EINVAL; > + > + block_num = ftl->log2phy[log_num]; > + block_adr = block_num * mtd->erasesize; > + block_ofs = (u32)from % mtd->erasesize; > + > + err = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf); > + /* Ignore corrected ECC errors */ > + if (mtd_is_bitflip(err)) > + err = 0; > + if (!err && retlen != len) > + err = -EIO; > + if (err) > + pr_err("sharpslpart: error, read failed at %#llx\n", > + block_adr + block_ofs); > + > + return err; > +} > + > +/* > + * MTD Partition Parser > + * > + * Sample values read from SL-C860 > + * > + * # cat /proc/mtd > + * dev: size erasesize name > + * mtd0: 006d0000 00020000 "Filesystem" > + * mtd1: 00700000 00004000 "smf" > + * mtd2: 03500000 00004000 "root" > + * mtd3: 04400000 00004000 "home" > + * > + * PARTITIONINFO1 > + * 0x00060000: 00 00 00 00 00 00 70 00 42 4f 4f 54 00 00 00 00 ......p.BOOT.... > + * 0x00060010: 00 00 70 00 00 00 c0 03 46 53 52 4f 00 00 00 00 ..p.....FSRO.... > + * 0x00060020: 00 00 c0 03 00 00 00 04 46 53 52 57 00 00 00 00 ........FSRW.... > + * 0x00060030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................ > + * > + */ > +struct sharpsl_nand_partinfo { > + __le32 start; > + __le32 end; > + __be32 magic; > + u32 reserved; > +}; > + > +static int sharpsl_nand_read_partinfo(struct mtd_info *master, > + loff_t from, > + size_t len, > + struct sharpsl_nand_partinfo *buf, > + struct sharpsl_ftl *ftl) > +{ > + int ret; > + > + ret = sharpsl_nand_read_laddr(master, (u32)from, len, (u_char *)buf, > + ftl); > + if (ret) > + goto exit; > + > + /* check for magics */ > + if (be32_to_cpu(buf[0].magic) != BOOT_MAGIC || > + be32_to_cpu(buf[1].magic) != FSRO_MAGIC || > + be32_to_cpu(buf[2].magic) != FSRW_MAGIC) { > + pr_err("sharpslpart: magic values mismatch\n"); > + ret = -EINVAL; > + goto exit; > + } > + > + /* fixup for hardcoded value 64 MiB (for older models) */ > + buf[2].end = cpu_to_le32(master->size); > + > + /* extra sanity check */ > + if (le32_to_cpu(buf[0].end) <= le32_to_cpu(buf[0].start) || > + le32_to_cpu(buf[1].start) < le32_to_cpu(buf[0].end) || > + le32_to_cpu(buf[1].end) <= le32_to_cpu(buf[1].start) || > + le32_to_cpu(buf[2].start) < le32_to_cpu(buf[1].end) || > + le32_to_cpu(buf[2].end) <= le32_to_cpu(buf[2].start)) { > + pr_err("sharpslpart: partition sizes mismatch\n"); > + ret = -EINVAL; > + goto exit; > + } > + > + ret = 0; > +exit: > + return ret; > +} > + > +static int sharpsl_parse_mtd_partitions(struct mtd_info *master, > + const struct mtd_partition **pparts, > + struct mtd_part_parser_data *data) > +{ > + struct sharpsl_ftl ftl; > + struct sharpsl_nand_partinfo buf[SHARPSL_NAND_PARTS]; > + struct mtd_partition *sharpsl_nand_parts; > + int err, ret; > + > + /* init logical mgmt (FTL) */ > + err = sharpsl_nand_init_logical(master, SHARPSL_FTL_PART_SIZE, &ftl); > + if (err) > + return err; > + > + /* read and validate first partition table */ > + pr_info("sharpslpart: using first partition table\n"); > + err = sharpsl_nand_read_partinfo(master, > + PARAM_BLOCK_PARTITIONINFO1, > + sizeof(buf), buf, &ftl); > + if (err) { > + /* fallback: read second partition table */ > + pr_warn("sharpslpart: retry using second partition table\n"); > + ret = sharpsl_nand_read_partinfo(master, > + PARAM_BLOCK_PARTITIONINFO2, > + sizeof(buf), buf, &ftl); > + if (ret) { > + pr_err("sharpslpart: partition tables are invalid\n"); > + sharpsl_nand_cleanup_logical(&ftl); > + return ret; > + } > + } > + > + /* cleanup logical mgmt (FTL) */ > + sharpsl_nand_cleanup_logical(&ftl); > + > + sharpsl_nand_parts = kzalloc(sizeof(*sharpsl_nand_parts) * > + SHARPSL_NAND_PARTS, GFP_KERNEL); > + if (!sharpsl_nand_parts) > + return -ENOMEM; > + > + /* original names */ > + sharpsl_nand_parts[0].name = "smf"; > + sharpsl_nand_parts[0].offset = le32_to_cpu(buf[0].start); > + sharpsl_nand_parts[0].size = le32_to_cpu(buf[0].end) - > + le32_to_cpu(buf[0].start); > + > + sharpsl_nand_parts[1].name = "root"; > + sharpsl_nand_parts[1].offset = le32_to_cpu(buf[1].start); > + sharpsl_nand_parts[1].size = le32_to_cpu(buf[1].end) - > + le32_to_cpu(buf[1].start); > + > + sharpsl_nand_parts[2].name = "home"; > + sharpsl_nand_parts[2].offset = le32_to_cpu(buf[2].start); > + sharpsl_nand_parts[2].size = le32_to_cpu(buf[2].end) - > + le32_to_cpu(buf[2].start); > + > + *pparts = sharpsl_nand_parts; > + return SHARPSL_NAND_PARTS; > +} > + > +static struct mtd_part_parser sharpsl_mtd_parser = { > + .parse_fn = sharpsl_parse_mtd_partitions, > + .name = "sharpslpart", > +}; > +module_mtd_part_parser(sharpsl_mtd_parser); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Andrea Adami <andrea.adami@gmail.com>"); > +MODULE_DESCRIPTION("MTD partitioning for NAND flash on Sharp SL Series");
On Thu, Aug 24, 2017 at 01:27:10PM +0200, Boris Brezillon wrote: > On Thu, 24 Aug 2017 12:30:02 +0200 > Andrea Adami <andrea.adami@gmail.com> wrote: > > > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon > > <boris.brezillon@free-electrons.com> wrote: > > > On Thu, 24 Aug 2017 11:19:56 +0200 > > > Andrea Adami <andrea.adami@gmail.com> wrote: ... > > >> >> + /* create physical-logical table */ > > >> >> + for (block_num = 0; block_num < phymax; block_num++) { > > >> >> + block_adr = block_num * mtd->erasesize; > > >> >> + > > >> >> + if (mtd_block_isbad(mtd, block_adr)) > > >> >> + continue; > > >> >> + > > >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) > > >> >> + continue; > > >> >> + > > >> >> + /* get logical block */ > > >> >> + log_num = sharpsl_nand_get_logical_num(oob); > > >> >> + > > >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */ > > >> >> + if (log_num == UINT_MAX) { > > >> >> + pr_info("sharpslpart: Sharp SL FTL not found.\n"); > > >> >> + ret = -EINVAL; > > >> >> + goto exit; > > >> >> + } > > > > > > Okay, I overlooked that part. Why do you exit if there's a fingerprint > > > mismatch? Can't you just ignore this physical block and continue > > > scanning the remaining ones? > > > > Norris asked to quit immediately in this case. > > https://patchwork.kernel.org/patch/9758361/ I didn't specifically ask for you to quit in *that* case. Quoting myself here, as you did: > > "I wouldn't expect people to want to use this parser, but if we have a > > quick way to say "this doesn't match, skip me", then that would be > > helpful." > > "We don't want to waste too much time scanning for this partition > > table if possible." That means, is there something (not necessarily writting in the "original code" that you're massaging) that could be used to reliably detect that this is/isn't a valid "Sharp FTL"? I don't think the check you wrote is a good one. Particularly, you *don't* want to just abort completely because there's one corrupt block. This check is a reliability check (so you can possibly ignore old/bad copies and skip onto better blocks), not a validity check. It is counter-productive to abort here, IIUC. > Actually, you don't save much by exiting on "bad OOB fingerprint". If > you look at the code you'll see that the only thing you check is > whether some oob portions are equal or not, and most of the time the > OOB area is left untouched by the upper layer, which means all free > bytes will be left to 0xff, which in turn means the "is fingerprint > good?" test will pass. Agreed. I'd drop this "abort early" check and just admit that it's not possible to do what I asked. > > Now we are quitting ever before checking for parity erors ... > > Honestly, I'd recommend not using this parser for anything but SHARPSL > platforms, so I don't think we care much about the "scan all blocks" > overhead. Sounds about right. > Moreover, the sharpsl parser is the last one in the > part_parsers list, so it should be quite fast if the user specifies a > valid mtdparts= on the cmdline or valid partitions in the DT. Brian P.S. I alluded to it earlier, but I figured I should write it down properly here sometime, as food for thought; you don't actually need any of this parser at all if you're willing to contruct an initramfs that will do the parsing in user space (e.g., some scripting and 'nanddump'; or link to libmtd) and then add partitions yourself (e.g., with 'mtdpart add ...', or calling the BLKPG ioctls directly). This would just require you have a kernel with CONFIG_MTD_PARTITIONED_MASTER=y.
On Fri, Aug 25, 2017 at 6:53 AM, Brian Norris <computersforpeace@gmail.com> wrote: > On Thu, Aug 24, 2017 at 01:27:10PM +0200, Boris Brezillon wrote: >> On Thu, 24 Aug 2017 12:30:02 +0200 >> Andrea Adami <andrea.adami@gmail.com> wrote: >> >> > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon >> > <boris.brezillon@free-electrons.com> wrote: >> > > On Thu, 24 Aug 2017 11:19:56 +0200 >> > > Andrea Adami <andrea.adami@gmail.com> wrote: > > ... > >> > >> >> + /* create physical-logical table */ >> > >> >> + for (block_num = 0; block_num < phymax; block_num++) { >> > >> >> + block_adr = block_num * mtd->erasesize; >> > >> >> + >> > >> >> + if (mtd_block_isbad(mtd, block_adr)) >> > >> >> + continue; >> > >> >> + >> > >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) >> > >> >> + continue; >> > >> >> + >> > >> >> + /* get logical block */ >> > >> >> + log_num = sharpsl_nand_get_logical_num(oob); >> > >> >> + >> > >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */ >> > >> >> + if (log_num == UINT_MAX) { >> > >> >> + pr_info("sharpslpart: Sharp SL FTL not found.\n"); >> > >> >> + ret = -EINVAL; >> > >> >> + goto exit; >> > >> >> + } >> > > >> > > Okay, I overlooked that part. Why do you exit if there's a fingerprint >> > > mismatch? Can't you just ignore this physical block and continue >> > > scanning the remaining ones? >> > >> > Norris asked to quit immediately in this case. >> > https://patchwork.kernel.org/patch/9758361/ > > I didn't specifically ask for you to quit in *that* case. Quoting myself > here, as you did: > >> > "I wouldn't expect people to want to use this parser, but if we have a >> > quick way to say "this doesn't match, skip me", then that would be >> > helpful." >> > "We don't want to waste too much time scanning for this partition >> > table if possible." > > That means, is there something (not necessarily writting in the > "original code" that you're massaging) that could be used to reliably > detect that this is/isn't a valid "Sharp FTL"? I don't think the check > you wrote is a good one. Particularly, you *don't* want to just abort > completely because there's one corrupt block. This check is a > reliability check (so you can possibly ignore old/bad copies and skip > onto better blocks), not a validity check. It is counter-productive to > abort here, IIUC. > >> Actually, you don't save much by exiting on "bad OOB fingerprint". If >> you look at the code you'll see that the only thing you check is >> whether some oob portions are equal or not, and most of the time the >> OOB area is left untouched by the upper layer, which means all free >> bytes will be left to 0xff, which in turn means the "is fingerprint >> good?" test will pass. > > Agreed. > > I'd drop this "abort early" check and just admit that it's not possible > to do what I asked. Ok then, I have misunderstood you. The only time-saving would be to skip the creation of the table. In the specific cases, the older devices with erasesize 16KiB have just 448 blocks and the routine doesn't really slow down. I can imagine it would be a breeze for modern devices. I will just return -EINVAl and this error, as well as the eventual parity-check error on a specific block, will be cut-off by the checks and the cycle will move to next block. So I understand the sharpsl_nand_check_ooblayout() could be also avoided. Please confirm this, thanks. > >> > Now we are quitting ever before checking for parity erors ... >> >> Honestly, I'd recommend not using this parser for anything but SHARPSL >> platforms, so I don't think we care much about the "scan all blocks" >> overhead. > > Sounds about right. > >> Moreover, the sharpsl parser is the last one in the >> part_parsers list, so it should be quite fast if the user specifies a >> valid mtdparts= on the cmdline or valid partitions in the DT. > > Brian > > P.S. I alluded to it earlier, but I figured I should write it down > properly here sometime, as food for thought; you don't actually need any > of this parser at all if you're willing to contruct an initramfs that > will do the parsing in user space (e.g., some scripting and 'nanddump'; > or link to libmtd) and then add partitions yourself (e.g., with > 'mtdpart add ...', or calling the BLKPG ioctls directly). This would > just require you have a kernel with CONFIG_MTD_PARTITIONED_MASTER=y. Brian, the first problem in this approach is the size: there are only 1264KiB available for the zImage. We accepted the challenge and wrote linux-kexecboot bootloader, a kernel + couple of klibc-static binaries in the initramfs and we special-cased the Zaurus adding the partition parser in userspace. Now, as widely discussed before, there are limits to this solution: - first, we cannot oblige to flash this kernel+initramfs. - second, we need to build one kernel+initramfs for each model - third, the machines are really the same from kernel pov, just mtdparts differ Soon we'll test a single kernel for the pxa25x and for the pxa27x models. Honestly I did just glimpse at BLKPG ioctl because resizing for mtd was added recently (iirc). As maintainer for the cross-toolchain and of kexecboot I am of the opinion that the cleanest solution is the in-kernel partition parser. Thanks for your time reviewing this...is just a partition parser for some of the first commercial linux handhelds. Cheers Andrea
On Fri, 25 Aug 2017 19:50:25 +0200 Andrea Adami <andrea.adami@gmail.com> wrote: > On Fri, Aug 25, 2017 at 6:53 AM, Brian Norris > <computersforpeace@gmail.com> wrote: > > On Thu, Aug 24, 2017 at 01:27:10PM +0200, Boris Brezillon wrote: > >> On Thu, 24 Aug 2017 12:30:02 +0200 > >> Andrea Adami <andrea.adami@gmail.com> wrote: > >> > >> > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon > >> > <boris.brezillon@free-electrons.com> wrote: > >> > > On Thu, 24 Aug 2017 11:19:56 +0200 > >> > > Andrea Adami <andrea.adami@gmail.com> wrote: > > > > ... > > > >> > >> >> + /* create physical-logical table */ > >> > >> >> + for (block_num = 0; block_num < phymax; block_num++) { > >> > >> >> + block_adr = block_num * mtd->erasesize; > >> > >> >> + > >> > >> >> + if (mtd_block_isbad(mtd, block_adr)) > >> > >> >> + continue; > >> > >> >> + > >> > >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) > >> > >> >> + continue; > >> > >> >> + > >> > >> >> + /* get logical block */ > >> > >> >> + log_num = sharpsl_nand_get_logical_num(oob); > >> > >> >> + > >> > >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */ > >> > >> >> + if (log_num == UINT_MAX) { > >> > >> >> + pr_info("sharpslpart: Sharp SL FTL not found.\n"); > >> > >> >> + ret = -EINVAL; > >> > >> >> + goto exit; > >> > >> >> + } > >> > > > >> > > Okay, I overlooked that part. Why do you exit if there's a fingerprint > >> > > mismatch? Can't you just ignore this physical block and continue > >> > > scanning the remaining ones? > >> > > >> > Norris asked to quit immediately in this case. > >> > https://patchwork.kernel.org/patch/9758361/ > > > > I didn't specifically ask for you to quit in *that* case. Quoting myself > > here, as you did: > > > >> > "I wouldn't expect people to want to use this parser, but if we have a > >> > quick way to say "this doesn't match, skip me", then that would be > >> > helpful." > >> > "We don't want to waste too much time scanning for this partition > >> > table if possible." > > > > That means, is there something (not necessarily writting in the > > "original code" that you're massaging) that could be used to reliably > > detect that this is/isn't a valid "Sharp FTL"? I don't think the check > > you wrote is a good one. Particularly, you *don't* want to just abort > > completely because there's one corrupt block. This check is a > > reliability check (so you can possibly ignore old/bad copies and skip > > onto better blocks), not a validity check. It is counter-productive to > > abort here, IIUC. > > > >> Actually, you don't save much by exiting on "bad OOB fingerprint". If > >> you look at the code you'll see that the only thing you check is > >> whether some oob portions are equal or not, and most of the time the > >> OOB area is left untouched by the upper layer, which means all free > >> bytes will be left to 0xff, which in turn means the "is fingerprint > >> good?" test will pass. > > > > Agreed. > > > > I'd drop this "abort early" check and just admit that it's not possible > > to do what I asked. > > Ok then, I have misunderstood you. The only time-saving would be to > skip the creation of the table. > In the specific cases, the older devices with erasesize 16KiB have > just 448 blocks and the routine doesn't really slow down. > I can imagine it would be a breeze for modern devices. > > I will just return -EINVAl and this error, as well as the eventual > parity-check error on a specific block, will be cut-off by the checks > and the cycle will move to next block. > > So I understand the sharpsl_nand_check_ooblayout() could be also avoided. > Please confirm this, thanks. No, that check is still needed to bail out if someone tries to use this parser with an incompatible NAND controller/ECC engine. Though it should only be called once from your sharpsl_parse_mtd_partitions() function (one of the first thing you should do actually). > > > > >> > Now we are quitting ever before checking for parity erors ... > >> > >> Honestly, I'd recommend not using this parser for anything but SHARPSL > >> platforms, so I don't think we care much about the "scan all blocks" > >> overhead. > > > > Sounds about right. > > > >> Moreover, the sharpsl parser is the last one in the > >> part_parsers list, so it should be quite fast if the user specifies a > >> valid mtdparts= on the cmdline or valid partitions in the DT. > > > > Brian > > > > P.S. I alluded to it earlier, but I figured I should write it down > > properly here sometime, as food for thought; you don't actually need any > > of this parser at all if you're willing to contruct an initramfs that > > will do the parsing in user space (e.g., some scripting and 'nanddump'; > > or link to libmtd) and then add partitions yourself (e.g., with > > 'mtdpart add ...', or calling the BLKPG ioctls directly). This would > > just require you have a kernel with CONFIG_MTD_PARTITIONED_MASTER=y. > > Brian, the first problem in this approach is the size: there are only > 1264KiB available for the zImage. > We accepted the challenge and wrote linux-kexecboot bootloader, a > kernel + couple of klibc-static binaries in the initramfs and we > special-cased the Zaurus adding the partition parser in userspace. > > Now, as widely discussed before, there are limits to this solution: > - first, we cannot oblige to flash this kernel+initramfs. Just like you cannot oblige people to update to a recent/mainline kernel. > - second, we need to build one kernel+initramfs for each model Why is that? If you have a userspace implementation of your part-parser only one initramfs is needed, or am I missing something? > - third, the machines are really the same from kernel pov, just mtdparts differ I don't get that one, sorry. If you have a userspace tool in your initramfs that creates the partitions from the partinfo definitions using BLKPG ioctls you'll still be able to keep one kernel and various partitioning. > > Soon we'll test a single kernel for the pxa25x and for the pxa27x models. Still don't see the link with the suggestion made by Brian. > > Honestly I did just glimpse at BLKPG ioctl because resizing for mtd > was added recently (iirc). > As maintainer for the cross-toolchain and of kexecboot I am of the > opinion that the cleanest solution is the in-kernel partition parser. We already had this discussion. As a maintainer of the MTD subsystem I'm concerned about adding support for an old FTL and part parser that has never been mainlined before and more importantly which you don't seem to understand. The question is, who is responsible for the code if something does not work. As I said many times, I'm not against adding new FTLs or part-parsers in the kernel, just find it worrisome that you failed to answer to some question about how it's supposed to work. > > Thanks for your time reviewing this...is just a partition parser for > some of the first commercial linux handhelds. Come on, not this argument again. On a final note, I'd like to say I spend time reviewing the code, and it looks good overall, but my concern about the FTL design still stands and I keep thinking that it's not such a good idea to support it in mainline just because some old devices used to use this FTL in their proprietary firmware. If people want to update their kernels why not forcing them to use an initramfs that hides all the ugly things in some specific userspace tools?
On Fri, Aug 25, 2017 at 11:48 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Fri, 25 Aug 2017 19:50:25 +0200 > Andrea Adami <andrea.adami@gmail.com> wrote: > >> On Fri, Aug 25, 2017 at 6:53 AM, Brian Norris >> <computersforpeace@gmail.com> wrote: >> > On Thu, Aug 24, 2017 at 01:27:10PM +0200, Boris Brezillon wrote: >> >> On Thu, 24 Aug 2017 12:30:02 +0200 >> >> Andrea Adami <andrea.adami@gmail.com> wrote: >> >> >> >> > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon >> >> > <boris.brezillon@free-electrons.com> wrote: >> >> > > On Thu, 24 Aug 2017 11:19:56 +0200 >> >> > > Andrea Adami <andrea.adami@gmail.com> wrote: >> > >> > ... >> > >> >> > >> >> + /* create physical-logical table */ >> >> > >> >> + for (block_num = 0; block_num < phymax; block_num++) { >> >> > >> >> + block_adr = block_num * mtd->erasesize; >> >> > >> >> + >> >> > >> >> + if (mtd_block_isbad(mtd, block_adr)) >> >> > >> >> + continue; >> >> > >> >> + >> >> > >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) >> >> > >> >> + continue; >> >> > >> >> + >> >> > >> >> + /* get logical block */ >> >> > >> >> + log_num = sharpsl_nand_get_logical_num(oob); >> >> > >> >> + >> >> > >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */ >> >> > >> >> + if (log_num == UINT_MAX) { >> >> > >> >> + pr_info("sharpslpart: Sharp SL FTL not found.\n"); >> >> > >> >> + ret = -EINVAL; >> >> > >> >> + goto exit; >> >> > >> >> + } >> >> > > >> >> > > Okay, I overlooked that part. Why do you exit if there's a fingerprint >> >> > > mismatch? Can't you just ignore this physical block and continue >> >> > > scanning the remaining ones? >> >> > >> >> > Norris asked to quit immediately in this case. >> >> > https://patchwork.kernel.org/patch/9758361/ >> > >> > I didn't specifically ask for you to quit in *that* case. Quoting myself >> > here, as you did: >> > >> >> > "I wouldn't expect people to want to use this parser, but if we have a >> >> > quick way to say "this doesn't match, skip me", then that would be >> >> > helpful." >> >> > "We don't want to waste too much time scanning for this partition >> >> > table if possible." >> > >> > That means, is there something (not necessarily writting in the >> > "original code" that you're massaging) that could be used to reliably >> > detect that this is/isn't a valid "Sharp FTL"? I don't think the check >> > you wrote is a good one. Particularly, you *don't* want to just abort >> > completely because there's one corrupt block. This check is a >> > reliability check (so you can possibly ignore old/bad copies and skip >> > onto better blocks), not a validity check. It is counter-productive to >> > abort here, IIUC. >> > >> >> Actually, you don't save much by exiting on "bad OOB fingerprint". If >> >> you look at the code you'll see that the only thing you check is >> >> whether some oob portions are equal or not, and most of the time the >> >> OOB area is left untouched by the upper layer, which means all free >> >> bytes will be left to 0xff, which in turn means the "is fingerprint >> >> good?" test will pass. >> > >> > Agreed. >> > >> > I'd drop this "abort early" check and just admit that it's not possible >> > to do what I asked. >> >> Ok then, I have misunderstood you. The only time-saving would be to >> skip the creation of the table. >> In the specific cases, the older devices with erasesize 16KiB have >> just 448 blocks and the routine doesn't really slow down. >> I can imagine it would be a breeze for modern devices. >> >> I will just return -EINVAl and this error, as well as the eventual >> parity-check error on a specific block, will be cut-off by the checks >> and the cycle will move to next block. >> >> So I understand the sharpsl_nand_check_ooblayout() could be also avoided. >> Please confirm this, thanks. > > No, that check is still needed to bail out if someone tries to use > this parser with an incompatible NAND controller/ECC engine. Though it > should only be called once from your sharpsl_parse_mtd_partitions() > function (one of the first thing you should do actually). > >> >> > >> >> > Now we are quitting ever before checking for parity erors ... >> >> >> >> Honestly, I'd recommend not using this parser for anything but SHARPSL >> >> platforms, so I don't think we care much about the "scan all blocks" >> >> overhead. >> > >> > Sounds about right. >> > >> >> Moreover, the sharpsl parser is the last one in the >> >> part_parsers list, so it should be quite fast if the user specifies a >> >> valid mtdparts= on the cmdline or valid partitions in the DT. >> > >> > Brian >> > >> > P.S. I alluded to it earlier, but I figured I should write it down >> > properly here sometime, as food for thought; you don't actually need any >> > of this parser at all if you're willing to contruct an initramfs that >> > will do the parsing in user space (e.g., some scripting and 'nanddump'; >> > or link to libmtd) and then add partitions yourself (e.g., with >> > 'mtdpart add ...', or calling the BLKPG ioctls directly). This would >> > just require you have a kernel with CONFIG_MTD_PARTITIONED_MASTER=y. >> >> Brian, the first problem in this approach is the size: there are only >> 1264KiB available for the zImage. >> We accepted the challenge and wrote linux-kexecboot bootloader, a >> kernel + couple of klibc-static binaries in the initramfs and we >> special-cased the Zaurus adding the partition parser in userspace. >> >> Now, as widely discussed before, there are limits to this solution: >> - first, we cannot oblige to flash this kernel+initramfs. > > Just like you cannot oblige people to update to a recent/mainline > kernel. > >> - second, we need to build one kernel+initramfs for each model > > Why is that? If you have a userspace implementation of your part-parser > only one initramfs is needed, or am I missing something? > >> - third, the machines are really the same from kernel pov, just mtdparts differ > > I don't get that one, sorry. If you have a userspace tool in your > initramfs that creates the partitions from the partinfo definitions > using BLKPG ioctls you'll still be able to keep one kernel and various > partitioning. > >> >> Soon we'll test a single kernel for the pxa25x and for the pxa27x models. > > Still don't see the link with the suggestion made by Brian. > >> >> Honestly I did just glimpse at BLKPG ioctl because resizing for mtd >> was added recently (iirc). >> As maintainer for the cross-toolchain and of kexecboot I am of the >> opinion that the cleanest solution is the in-kernel partition parser. > > We already had this discussion. As a maintainer of the MTD subsystem > I'm concerned about adding support for an old FTL and part parser that > has never been mainlined before and more importantly which you don't > seem to understand. > The question is, who is responsible for the code if something does not > work. As I said many times, I'm not against adding new FTLs or > part-parsers in the kernel, just find it worrisome that you failed to > answer to some question about how it's supposed to work. > >> >> Thanks for your time reviewing this...is just a partition parser for >> some of the first commercial linux handhelds. > > Come on, not this argument again. > > On a final note, I'd like to say I spend time reviewing the code, and > it looks good overall, but my concern about the FTL design still stands > and I keep thinking that it's not such a good idea to support it in > mainline just because some old devices used to use this FTL in their > proprietary firmware. If people want to update their kernels why not > forcing them to use an initramfs that hides all the ugly things in some > specific userspace tools? > Boris, I am really sorry but I won't answer again in detail about the issues with repartitioning and the fact that I have no control over other distributions, etc. etc. I don't understand where is your problem now, after 6 reviews. I'll be happy to send v7 with the added oob check and I'll have to add you to the authors now :) My point of view is: 1- Does Zaurus devices exist under arm/mach-pxa YES 2- Are the devices maintained YES 3- Is a new module/functionality provided YES What is wrong in this? Thanks Andrea
On Fri, 25 Aug 2017 00:11:29 +0200 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > +/* > > + * The logical block number assigned to a physical block is stored in the OOB > > + * of the first page, in 3 16-bit copies with the following layout: > > + * > > + * 01234567 89abcdef > > + * -------- -------- > > + * ECC BB xyxyxy > > + * > > + * When reading we check that the first two copies agree. > > + * In case of error, matching is tried using the following pairs. > > + * Reserved values 0xffff mean the block is kept for wear leveling. > > + * > > + * 01234567 89abcdef > > + * -------- -------- > > + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9 > > + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11 > > + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13 > > I know there's a depends on "MTD_NAND_SHARPSL || MTD_NAND_TMIO" in the > Kconfig entry, but one could enable those options just to use the sharpsl > part parser even if the OOB layout is incompatible with the sharpsl FTL. > > I'd recommend that you check that OOB bytes 8 to 15 are actually free > to be used by the MTD user in sharpsl_parse_mtd_partitions(). > > Can be done with something like that: > > static int sharpsl_nand_check_ooblayout(struct mtd_info *mtd) > { > u8 freebytes = 0; > int section = 0; > > while (true) { > struct mtd_oob_region oobfree = { }; > int ret, i; > > ret = mtd_ooblayout_free(mtd, section++, &oobfree); > if (ret) > break; > > if (!oobfree.length || oobfree.offset > 15 || > (oobfree.offset + oobfree.length) < 8) > continue; > > i = oobfree.offset >= 8 ? : 8; As you reported on IRC there's an mistake here, it should be: i = oobfree.offset >= 8 ? oobfree.offset : 8 > for (; i < oobfree.offset + oobfree.length && i < 16; i++) > freebytes |= BIT(i - 8); > > if (freebytes == 0xff) > return 0; > } > > return -ENOTSUPP; > }
On Sat, 26 Aug 2017 00:09:49 +0200 Andrea Adami <andrea.adami@gmail.com> wrote: > On Fri, Aug 25, 2017 at 11:48 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Fri, 25 Aug 2017 19:50:25 +0200 > > Andrea Adami <andrea.adami@gmail.com> wrote: > > > >> On Fri, Aug 25, 2017 at 6:53 AM, Brian Norris > >> <computersforpeace@gmail.com> wrote: > >> > On Thu, Aug 24, 2017 at 01:27:10PM +0200, Boris Brezillon wrote: > >> >> On Thu, 24 Aug 2017 12:30:02 +0200 > >> >> Andrea Adami <andrea.adami@gmail.com> wrote: > >> >> > >> >> > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon > >> >> > <boris.brezillon@free-electrons.com> wrote: > >> >> > > On Thu, 24 Aug 2017 11:19:56 +0200 > >> >> > > Andrea Adami <andrea.adami@gmail.com> wrote: > >> > > >> > ... > >> > > >> >> > >> >> + /* create physical-logical table */ > >> >> > >> >> + for (block_num = 0; block_num < phymax; block_num++) { > >> >> > >> >> + block_adr = block_num * mtd->erasesize; > >> >> > >> >> + > >> >> > >> >> + if (mtd_block_isbad(mtd, block_adr)) > >> >> > >> >> + continue; > >> >> > >> >> + > >> >> > >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) > >> >> > >> >> + continue; > >> >> > >> >> + > >> >> > >> >> + /* get logical block */ > >> >> > >> >> + log_num = sharpsl_nand_get_logical_num(oob); > >> >> > >> >> + > >> >> > >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */ > >> >> > >> >> + if (log_num == UINT_MAX) { > >> >> > >> >> + pr_info("sharpslpart: Sharp SL FTL not found.\n"); > >> >> > >> >> + ret = -EINVAL; > >> >> > >> >> + goto exit; > >> >> > >> >> + } > >> >> > > > >> >> > > Okay, I overlooked that part. Why do you exit if there's a fingerprint > >> >> > > mismatch? Can't you just ignore this physical block and continue > >> >> > > scanning the remaining ones? > >> >> > > >> >> > Norris asked to quit immediately in this case. > >> >> > https://patchwork.kernel.org/patch/9758361/ > >> > > >> > I didn't specifically ask for you to quit in *that* case. Quoting myself > >> > here, as you did: > >> > > >> >> > "I wouldn't expect people to want to use this parser, but if we have a > >> >> > quick way to say "this doesn't match, skip me", then that would be > >> >> > helpful." > >> >> > "We don't want to waste too much time scanning for this partition > >> >> > table if possible." > >> > > >> > That means, is there something (not necessarily writting in the > >> > "original code" that you're massaging) that could be used to reliably > >> > detect that this is/isn't a valid "Sharp FTL"? I don't think the check > >> > you wrote is a good one. Particularly, you *don't* want to just abort > >> > completely because there's one corrupt block. This check is a > >> > reliability check (so you can possibly ignore old/bad copies and skip > >> > onto better blocks), not a validity check. It is counter-productive to > >> > abort here, IIUC. > >> > > >> >> Actually, you don't save much by exiting on "bad OOB fingerprint". If > >> >> you look at the code you'll see that the only thing you check is > >> >> whether some oob portions are equal or not, and most of the time the > >> >> OOB area is left untouched by the upper layer, which means all free > >> >> bytes will be left to 0xff, which in turn means the "is fingerprint > >> >> good?" test will pass. > >> > > >> > Agreed. > >> > > >> > I'd drop this "abort early" check and just admit that it's not possible > >> > to do what I asked. > >> > >> Ok then, I have misunderstood you. The only time-saving would be to > >> skip the creation of the table. > >> In the specific cases, the older devices with erasesize 16KiB have > >> just 448 blocks and the routine doesn't really slow down. > >> I can imagine it would be a breeze for modern devices. > >> > >> I will just return -EINVAl and this error, as well as the eventual > >> parity-check error on a specific block, will be cut-off by the checks > >> and the cycle will move to next block. > >> > >> So I understand the sharpsl_nand_check_ooblayout() could be also avoided. > >> Please confirm this, thanks. > > > > No, that check is still needed to bail out if someone tries to use > > this parser with an incompatible NAND controller/ECC engine. Though it > > should only be called once from your sharpsl_parse_mtd_partitions() > > function (one of the first thing you should do actually). > > > >> > >> > > >> >> > Now we are quitting ever before checking for parity erors ... > >> >> > >> >> Honestly, I'd recommend not using this parser for anything but SHARPSL > >> >> platforms, so I don't think we care much about the "scan all blocks" > >> >> overhead. > >> > > >> > Sounds about right. > >> > > >> >> Moreover, the sharpsl parser is the last one in the > >> >> part_parsers list, so it should be quite fast if the user specifies a > >> >> valid mtdparts= on the cmdline or valid partitions in the DT. > >> > > >> > Brian > >> > > >> > P.S. I alluded to it earlier, but I figured I should write it down > >> > properly here sometime, as food for thought; you don't actually need any > >> > of this parser at all if you're willing to contruct an initramfs that > >> > will do the parsing in user space (e.g., some scripting and 'nanddump'; > >> > or link to libmtd) and then add partitions yourself (e.g., with > >> > 'mtdpart add ...', or calling the BLKPG ioctls directly). This would > >> > just require you have a kernel with CONFIG_MTD_PARTITIONED_MASTER=y. > >> > >> Brian, the first problem in this approach is the size: there are only > >> 1264KiB available for the zImage. > >> We accepted the challenge and wrote linux-kexecboot bootloader, a > >> kernel + couple of klibc-static binaries in the initramfs and we > >> special-cased the Zaurus adding the partition parser in userspace. > >> > >> Now, as widely discussed before, there are limits to this solution: > >> - first, we cannot oblige to flash this kernel+initramfs. > > > > Just like you cannot oblige people to update to a recent/mainline > > kernel. > > > >> - second, we need to build one kernel+initramfs for each model > > > > Why is that? If you have a userspace implementation of your part-parser > > only one initramfs is needed, or am I missing something? > > > >> - third, the machines are really the same from kernel pov, just mtdparts differ > > > > I don't get that one, sorry. If you have a userspace tool in your > > initramfs that creates the partitions from the partinfo definitions > > using BLKPG ioctls you'll still be able to keep one kernel and various > > partitioning. > > > >> > >> Soon we'll test a single kernel for the pxa25x and for the pxa27x models. > > > > Still don't see the link with the suggestion made by Brian. > > > >> > >> Honestly I did just glimpse at BLKPG ioctl because resizing for mtd > >> was added recently (iirc). > >> As maintainer for the cross-toolchain and of kexecboot I am of the > >> opinion that the cleanest solution is the in-kernel partition parser. > > > > We already had this discussion. As a maintainer of the MTD subsystem > > I'm concerned about adding support for an old FTL and part parser that > > has never been mainlined before and more importantly which you don't > > seem to understand. > > The question is, who is responsible for the code if something does not > > work. As I said many times, I'm not against adding new FTLs or > > part-parsers in the kernel, just find it worrisome that you failed to > > answer to some question about how it's supposed to work. > > > >> > >> Thanks for your time reviewing this...is just a partition parser for > >> some of the first commercial linux handhelds. > > > > Come on, not this argument again. > > > > On a final note, I'd like to say I spend time reviewing the code, and > > it looks good overall, but my concern about the FTL design still stands > > and I keep thinking that it's not such a good idea to support it in > > mainline just because some old devices used to use this FTL in their > > proprietary firmware. If people want to update their kernels why not > > forcing them to use an initramfs that hides all the ugly things in some > > specific userspace tools? > > > > Boris, > > I am really sorry but I won't answer again in detail about the issues > with repartitioning and the fact that I have no control over other > distributions, etc. etc. And do you have control on what those distributions enable in there kernel? Will they really enable this SHARPSL part parser? > > I don't understand where is your problem now, after 6 reviews. My concerns didn't vanish, and I still don't buy your different arguments which are either non-technical or are not related to the suggestions we do. In this regards, this thread is a good example, when Brian suggests to use a userspace program to parse partinfo and create partitions with the BLKPG ioctl, you answer that you'll require one initramfs per board, which AFAICT is not true. > I'll be happy to send v7 with the added oob check and I'll have to add > you to the authors now :) Hell no! Send a v7 if you want but don't flag me as one of the author. > > My point of view is: > 1- Does Zaurus devices exist under arm/mach-pxa YES > 2- Are the devices maintained YES > 3- Is a new module/functionality provided YES > > What is wrong in this? What is wrong?! Maybe the fact that you're trying to add support for something that sharp didn't even dare discussing with the community when they designed it. Also, no one seems to know enough about this FTL to explain some of the choices they made. And last, it seems this FTL was designed with old NANDs in mind (those 16K eraseblock NANDs) and then hacked to support bigger chips, which is not a good sign. Now, I'd like to stop this discussion here because we clearly disagree and I don't think one can convince the other. As I said from the beginning, if other MTD maintainers are fine with this parser I won't block its inclusion. I've actually done more than I initially planned to do: I reviewed the code, but don't expect me to agree with what you're doing, because I won't. Regards, Boris
diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig index d206b3c..f371022 100644 --- a/drivers/mtd/parsers/Kconfig +++ b/drivers/mtd/parsers/Kconfig @@ -6,3 +6,10 @@ config MTD_PARSER_TRX may contain up to 3/4 partitions (depending on the version). This driver will parse TRX header and report at least two partitions: kernel and rootfs. +config MTD_SHARPSL_PARTS + tristate "Sharp SL Series NAND flash partition parser" + depends on MTD_NAND_SHARPSL || MTD_NAND_TMIO || COMPILE_TEST + help + This provides the read-only FTL logic necessary to read the partition + table from the NAND flash of Sharp SL Series (Zaurus) and the MTD + partition parser using this code. diff --git a/drivers/mtd/parsers/Makefile b/drivers/mtd/parsers/Makefile index 4d9024e..5b1bcc3 100644 --- a/drivers/mtd/parsers/Makefile +++ b/drivers/mtd/parsers/Makefile @@ -1 +1,2 @@ obj-$(CONFIG_MTD_PARSER_TRX) += parser_trx.o +obj-$(CONFIG_MTD_SHARPSL_PARTS) += sharpslpart.o diff --git a/drivers/mtd/parsers/sharpslpart.c b/drivers/mtd/parsers/sharpslpart.c new file mode 100644 index 0000000..b2333ae --- /dev/null +++ b/drivers/mtd/parsers/sharpslpart.c @@ -0,0 +1,382 @@ +/* + * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL + * for logical addressing, as used on the PXA models of the SHARP SL Series. + * + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com> + * + * Based on 2.4 sources: + * drivers/mtd/nand/sharp_sl_logical.c + * linux/include/asm-arm/sharp_nand_logical.h + * + * Copyright (C) 2002 SHARP + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/types.h> +#include <linux/mtd/mtd.h> +#include <linux/mtd/partitions.h> + +/* oob structure */ +#define NAND_NOOB_LOGADDR_00 8 +#define NAND_NOOB_LOGADDR_01 9 +#define NAND_NOOB_LOGADDR_10 10 +#define NAND_NOOB_LOGADDR_11 11 +#define NAND_NOOB_LOGADDR_20 12 +#define NAND_NOOB_LOGADDR_21 13 + +#define BLOCK_IS_RESERVED 0xffff +#define BLOCK_UNMASK 0x07fe +#define BLOCK_UNMASK_COMPLEMENT 1 + +/* factory defaults */ +#define SHARPSL_NAND_PARTS 3 +#define SHARPSL_FTL_PART_SIZE (7 * 1024 * 1024) +#define PARAM_BLOCK_PARTITIONINFO1 0x00060000 +#define PARAM_BLOCK_PARTITIONINFO2 0x00064000 + +#define BOOT_MAGIC 0x424f4f54 +#define FSRO_MAGIC 0x4653524f +#define FSRW_MAGIC 0x46535257 + +/** + * struct sharpsl_ftl - Sharp FTL Logical Table + * @logmax: number of logical blocks + * @log2phy: the logical-to-physical table + * + * Stripped down from 2.4 sources for read-only purposes. + */ +struct sharpsl_ftl { + u_int logmax; + u_int *log2phy; +}; + +static int sharpsl_nand_read_oob(struct mtd_info *mtd, loff_t offs, size_t len, + uint8_t *buf) +{ + loff_t mask = mtd->writesize - 1; + struct mtd_oob_ops ops; + int ret; + + ops.mode = MTD_OPS_PLACE_OOB; + ops.ooboffs = offs & mask; + ops.ooblen = len; + ops.oobbuf = buf; + ops.datbuf = NULL; + + ret = mtd_read_oob(mtd, offs & ~mask, &ops); + if (ret != 0 || len != ops.oobretlen) + return -1; + + return 0; +} + +/* + * The logical block number assigned to a physical block is stored in the OOB + * of the first page, in 3 16-bit copies with the following layout: + * + * 01234567 89abcdef + * -------- -------- + * ECC BB xyxyxy + * + * When reading we check that the first two copies agree. + * In case of error, matching is tried using the following pairs. + * Reserved values 0xffff mean the block is kept for wear leveling. + * + * 01234567 89abcdef + * -------- -------- + * ECC BB xyxy oob[8]==oob[10] && oob[9]==oob[11] -> byte0=8 byte1=9 + * ECC BB xyxy oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10 byte1=11 + * ECC BB xy xy oob[12]==oob[8] && oob[13]==oob[9] -> byte0=12 byte1=13 + * + */ +static u_int sharpsl_nand_get_logical_num(u_char *oob) +{ + u16 us; + int good0, good1; + + if (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] && + oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) { + good0 = NAND_NOOB_LOGADDR_00; + good1 = NAND_NOOB_LOGADDR_01; + } else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] && + oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) { + good0 = NAND_NOOB_LOGADDR_10; + good1 = NAND_NOOB_LOGADDR_11; + } else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] && + oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) { + good0 = NAND_NOOB_LOGADDR_20; + good1 = NAND_NOOB_LOGADDR_21; + } else { + /* wrong oob fingerprint, maybe here by mistake? */ + return UINT_MAX; + } + + us = oob[good0] | oob[good1] << 8; + + /* parity check */ + if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT) + return (UINT_MAX - 1); + + /* reserved */ + if (us == BLOCK_IS_RESERVED) + return BLOCK_IS_RESERVED; + else + return (us & BLOCK_UNMASK) >> 1; +} + +static int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size, + struct sharpsl_ftl *ftl) +{ + u_int block_num, log_num, phymax; + loff_t block_adr; + u_char *oob; + int i, ret; + + oob = kzalloc(mtd->oobsize, GFP_KERNEL); + if (!oob) + return -ENOMEM; + + /* initialize management structure */ + phymax = (partition_size / mtd->erasesize); + + /* FTL reserves 5% of the blocks + 1 spare */ + ftl->logmax = ((phymax * 95) / 100) - 1; + + ftl->log2phy = kmalloc_array(ftl->logmax, sizeof(u_int), + GFP_KERNEL); + if (!ftl->log2phy) { + ret = -ENOMEM; + goto exit; + } + + /* initialize ftl->log2phy */ + for (i = 0; i < ftl->logmax; i++) + ftl->log2phy[i] = UINT_MAX; + + /* create physical-logical table */ + for (block_num = 0; block_num < phymax; block_num++) { + block_adr = block_num * mtd->erasesize; + + if (mtd_block_isbad(mtd, block_adr)) + continue; + + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) + continue; + + /* get logical block */ + log_num = sharpsl_nand_get_logical_num(oob); + + /* FTL is not used? Exit here if the oob fingerprint is wrong */ + if (log_num == UINT_MAX) { + pr_info("sharpslpart: Sharp SL FTL not found.\n"); + ret = -EINVAL; + goto exit; + } + + /* skip out of range and not unique values */ + if (log_num < ftl->logmax) { + if (ftl->log2phy[log_num] == UINT_MAX) + ftl->log2phy[log_num] = block_num; + } + } + + pr_info("Sharp SL FTL: %d blocks used (%d logical, %d reserved)\n", + phymax, ftl->logmax, + phymax - ftl->logmax); + + ret = 0; +exit: + kfree(oob); + return ret; +} + +void sharpsl_nand_cleanup_logical(struct sharpsl_ftl *ftl) +{ + kfree(ftl->log2phy); +} + +static int sharpsl_nand_read_laddr(struct mtd_info *mtd, + loff_t from, + size_t len, + u_char *buf, + struct sharpsl_ftl *ftl) +{ + u_int log_num, log_new; + u_int block_num; + loff_t block_adr; + loff_t block_ofs; + size_t retlen; + int err; + + log_num = (u32)from / mtd->erasesize; + log_new = ((u32)from + len - 1) / mtd->erasesize; + + if (len <= 0 || log_num >= ftl->logmax || log_new > log_num) + return -EINVAL; + + block_num = ftl->log2phy[log_num]; + block_adr = block_num * mtd->erasesize; + block_ofs = (u32)from % mtd->erasesize; + + err = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf); + /* Ignore corrected ECC errors */ + if (mtd_is_bitflip(err)) + err = 0; + if (!err && retlen != len) + err = -EIO; + if (err) + pr_err("sharpslpart: error, read failed at %#llx\n", + block_adr + block_ofs); + + return err; +} + +/* + * MTD Partition Parser + * + * Sample values read from SL-C860 + * + * # cat /proc/mtd + * dev: size erasesize name + * mtd0: 006d0000 00020000 "Filesystem" + * mtd1: 00700000 00004000 "smf" + * mtd2: 03500000 00004000 "root" + * mtd3: 04400000 00004000 "home" + * + * PARTITIONINFO1 + * 0x00060000: 00 00 00 00 00 00 70 00 42 4f 4f 54 00 00 00 00 ......p.BOOT.... + * 0x00060010: 00 00 70 00 00 00 c0 03 46 53 52 4f 00 00 00 00 ..p.....FSRO.... + * 0x00060020: 00 00 c0 03 00 00 00 04 46 53 52 57 00 00 00 00 ........FSRW.... + * 0x00060030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................ + * + */ +struct sharpsl_nand_partinfo { + __le32 start; + __le32 end; + __be32 magic; + u32 reserved; +}; + +static int sharpsl_nand_read_partinfo(struct mtd_info *master, + loff_t from, + size_t len, + struct sharpsl_nand_partinfo *buf, + struct sharpsl_ftl *ftl) +{ + int ret; + + ret = sharpsl_nand_read_laddr(master, (u32)from, len, (u_char *)buf, + ftl); + if (ret) + goto exit; + + /* check for magics */ + if (be32_to_cpu(buf[0].magic) != BOOT_MAGIC || + be32_to_cpu(buf[1].magic) != FSRO_MAGIC || + be32_to_cpu(buf[2].magic) != FSRW_MAGIC) { + pr_err("sharpslpart: magic values mismatch\n"); + ret = -EINVAL; + goto exit; + } + + /* fixup for hardcoded value 64 MiB (for older models) */ + buf[2].end = cpu_to_le32(master->size); + + /* extra sanity check */ + if (le32_to_cpu(buf[0].end) <= le32_to_cpu(buf[0].start) || + le32_to_cpu(buf[1].start) < le32_to_cpu(buf[0].end) || + le32_to_cpu(buf[1].end) <= le32_to_cpu(buf[1].start) || + le32_to_cpu(buf[2].start) < le32_to_cpu(buf[1].end) || + le32_to_cpu(buf[2].end) <= le32_to_cpu(buf[2].start)) { + pr_err("sharpslpart: partition sizes mismatch\n"); + ret = -EINVAL; + goto exit; + } + + ret = 0; +exit: + return ret; +} + +static int sharpsl_parse_mtd_partitions(struct mtd_info *master, + const struct mtd_partition **pparts, + struct mtd_part_parser_data *data) +{ + struct sharpsl_ftl ftl; + struct sharpsl_nand_partinfo buf[SHARPSL_NAND_PARTS]; + struct mtd_partition *sharpsl_nand_parts; + int err, ret; + + /* init logical mgmt (FTL) */ + err = sharpsl_nand_init_logical(master, SHARPSL_FTL_PART_SIZE, &ftl); + if (err) + return err; + + /* read and validate first partition table */ + pr_info("sharpslpart: using first partition table\n"); + err = sharpsl_nand_read_partinfo(master, + PARAM_BLOCK_PARTITIONINFO1, + sizeof(buf), buf, &ftl); + if (err) { + /* fallback: read second partition table */ + pr_warn("sharpslpart: retry using second partition table\n"); + ret = sharpsl_nand_read_partinfo(master, + PARAM_BLOCK_PARTITIONINFO2, + sizeof(buf), buf, &ftl); + if (ret) { + pr_err("sharpslpart: partition tables are invalid\n"); + sharpsl_nand_cleanup_logical(&ftl); + return ret; + } + } + + /* cleanup logical mgmt (FTL) */ + sharpsl_nand_cleanup_logical(&ftl); + + sharpsl_nand_parts = kzalloc(sizeof(*sharpsl_nand_parts) * + SHARPSL_NAND_PARTS, GFP_KERNEL); + if (!sharpsl_nand_parts) + return -ENOMEM; + + /* original names */ + sharpsl_nand_parts[0].name = "smf"; + sharpsl_nand_parts[0].offset = le32_to_cpu(buf[0].start); + sharpsl_nand_parts[0].size = le32_to_cpu(buf[0].end) - + le32_to_cpu(buf[0].start); + + sharpsl_nand_parts[1].name = "root"; + sharpsl_nand_parts[1].offset = le32_to_cpu(buf[1].start); + sharpsl_nand_parts[1].size = le32_to_cpu(buf[1].end) - + le32_to_cpu(buf[1].start); + + sharpsl_nand_parts[2].name = "home"; + sharpsl_nand_parts[2].offset = le32_to_cpu(buf[2].start); + sharpsl_nand_parts[2].size = le32_to_cpu(buf[2].end) - + le32_to_cpu(buf[2].start); + + *pparts = sharpsl_nand_parts; + return SHARPSL_NAND_PARTS; +} + +static struct mtd_part_parser sharpsl_mtd_parser = { + .parse_fn = sharpsl_parse_mtd_partitions, + .name = "sharpslpart", +}; +module_mtd_part_parser(sharpsl_mtd_parser); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Andrea Adami <andrea.adami@gmail.com>"); +MODULE_DESCRIPTION("MTD partitioning for NAND flash on Sharp SL Series");
The Sharp SL Series (Zaurus) PXA handhelds have 16/64/128M of NAND flash and share the same layout of the first 7M partition, managed by Sharp FTL. The purpose of this self-contained patch is to add a common parser and remove the hardcoded sizes in the board files (these devices are not yet converted to devicetree). Users will have benefits because the mtdparts= tag will not be necessary anymore and they will be free to repartition the little sized flash. The obsolete bootloader can not pass the partitioning info to modern kernels anymore so it has to be read from flash at known logical addresses. (see http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm ) In kernel, under arch/arm/mach-pxa we have already 8 machines: MACH_POODLE, MACH_CORGI, MACH_SHEPERD, MACH_HUSKY, MACH_AKITA, MACH_SPITZ, MACH_BORZOI, MACH_TOSA. Lost after the 2.4 vendor kernel are MACH_BOXER and MACH_TERRIER. Almost every model has different factory partitioning: add to this the units can be repartitioned by users with userspace tools (nandlogical) and installers for popular (back then) linux distributions. The Parameter Area in the first (boot) partition extends from 0x00040000 to 0x0007bfff (176k) and contains two copies of the partition table: ... 0x00060000: Partition Info1 16k 0x00064000: Partition Info2 16k 0x00668000: Model 16k ... The first 7M partition is managed by the Sharp FTL reserving 5% + 1 blocks for wear-leveling: some blocks are remapped and one layer of translation (logical to physical) is necessary. There isn't much documentation about this FTL in the 2.4 sources, just the MTD methods for reading and writing using logical addresses and the block management (wear-leveling, use counter). For the purpose of the MTD parser only the read part of the code was taken. The NAND drivers that can use this parser are sharpsl.c and tmio_nand.c. Signed-off-by: Andrea Adami <andrea.adami@gmail.com> --- drivers/mtd/parsers/Kconfig | 7 + drivers/mtd/parsers/Makefile | 1 + drivers/mtd/parsers/sharpslpart.c | 382 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 390 insertions(+) create mode 100644 drivers/mtd/parsers/sharpslpart.c