From 6293b0361d9816dc5286cd766d4865a30ebdfb6f Mon Sep 17 00:00:00 2001 From: Miquel Raynal Date: Thu, 11 Oct 2018 17:45:42 +0200 Subject: mtd: nand: pxa3xx: add raw read support Raw read support is added by editing a few code sections: ->handle_data_pio() includes the ECC bytes that are not consumed anymore by the ECC engine. ->prepare_set_command() is changed so that the ECC bytes are requested as part of the data I/O length. ->drain_fifo() shall also avoid checking the R/B pin too often when in raw mode. ->read_page_raw()/->read_oob_raw() are written from scratch. Signed-off-by: Miquel Raynal Acked-by: Jagan Teki --- drivers/mtd/nand/raw/pxa3xx_nand.c | 98 +++++++++++++++++++++++++++++++++++--- 1 file changed, 92 insertions(+), 6 deletions(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/nand/raw/pxa3xx_nand.c b/drivers/mtd/nand/raw/pxa3xx_nand.c index 4c783f1e1e4..454597355b7 100644 --- a/drivers/mtd/nand/raw/pxa3xx_nand.c +++ b/drivers/mtd/nand/raw/pxa3xx_nand.c @@ -195,6 +195,7 @@ struct pxa3xx_nand_info { int cs; int use_ecc; /* use HW ECC ? */ + int force_raw; /* prevent use_ecc to be set */ int ecc_bch; /* using BCH ECC? */ int use_spare; /* use spare ? */ int need_wait; @@ -579,7 +580,7 @@ static void disable_int(struct pxa3xx_nand_info *info, uint32_t int_mask) static void drain_fifo(struct pxa3xx_nand_info *info, void *data, int len) { - if (info->ecc_bch) { + if (info->ecc_bch && !info->force_raw) { u32 ts; /* @@ -612,12 +613,22 @@ static void drain_fifo(struct pxa3xx_nand_info *info, void *data, int len) static void handle_data_pio(struct pxa3xx_nand_info *info) { + int data_len = info->step_chunk_size; + + /* + * In raw mode, include the spare area and the ECC bytes that are not + * consumed by the controller in the data section. Do not reorganize + * here, do it in the ->read_page_raw() handler instead. + */ + if (info->force_raw) + data_len += info->step_spare_size + info->ecc_size; + switch (info->state) { case STATE_PIO_WRITING: if (info->step_chunk_size) writesl(info->mmio_base + NDDB, info->data_buff + info->data_buff_pos, - DIV_ROUND_UP(info->step_chunk_size, 4)); + DIV_ROUND_UP(data_len, 4)); if (info->step_spare_size) writesl(info->mmio_base + NDDB, @@ -628,7 +639,10 @@ static void handle_data_pio(struct pxa3xx_nand_info *info) if (info->step_chunk_size) drain_fifo(info, info->data_buff + info->data_buff_pos, - DIV_ROUND_UP(info->step_chunk_size, 4)); + DIV_ROUND_UP(data_len, 4)); + + if (info->force_raw) + break; if (info->step_spare_size) drain_fifo(info, @@ -642,7 +656,7 @@ static void handle_data_pio(struct pxa3xx_nand_info *info) } /* Update buffer pointers for multi-page read/write */ - info->data_buff_pos += info->step_chunk_size; + info->data_buff_pos += data_len; info->oob_buff_pos += info->step_spare_size; } @@ -796,7 +810,8 @@ static void prepare_start_command(struct pxa3xx_nand_info *info, int command) case NAND_CMD_READ0: case NAND_CMD_READOOB: case NAND_CMD_PAGEPROG: - info->use_ecc = 1; + if (!info->force_raw) + info->use_ecc = 1; break; case NAND_CMD_PARAM: info->use_spare = 0; @@ -866,7 +881,13 @@ static int prepare_set_command(struct pxa3xx_nand_info *info, int command, * which is either naked-read or last-read according to the * state. */ - if (mtd->writesize == info->chunk_size) { + if (info->force_raw) { + info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8) | + NDCB0_LEN_OVRD | + NDCB0_EXT_CMD_TYPE(ext_cmd_type); + info->ndcb3 = info->step_chunk_size + + info->step_spare_size + info->ecc_size; + } else if (mtd->writesize == info->chunk_size) { info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8); } else if (mtd->writesize > info->chunk_size) { info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8) @@ -1238,6 +1259,69 @@ static int pxa3xx_nand_read_page_hwecc(struct mtd_info *mtd, return info->max_bitflips; } +static int pxa3xx_nand_read_page_raw(struct mtd_info *mtd, + struct nand_chip *chip, uint8_t *buf, + int oob_required, int page) +{ + struct pxa3xx_nand_host *host = chip->priv; + struct pxa3xx_nand_info *info = host->info_data; + int chunk, ecc_off_buf; + + if (!info->ecc_bch) + return -ENOTSUPP; + + /* + * Set the force_raw boolean, then re-call ->cmdfunc() that will run + * pxa3xx_nand_start(), which will actually disable the ECC engine. + */ + info->force_raw = true; + chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); + + ecc_off_buf = (info->nfullchunks * info->spare_size) + + info->last_spare_size; + for (chunk = 0; chunk < info->nfullchunks; chunk++) { + chip->read_buf(mtd, + buf + (chunk * info->chunk_size), + info->chunk_size); + chip->read_buf(mtd, + chip->oob_poi + + (chunk * (info->spare_size)), + info->spare_size); + chip->read_buf(mtd, + chip->oob_poi + ecc_off_buf + + (chunk * (info->ecc_size)), + info->ecc_size - 2); + } + + if (info->ntotalchunks > info->nfullchunks) { + chip->read_buf(mtd, + buf + (info->nfullchunks * info->chunk_size), + info->last_chunk_size); + chip->read_buf(mtd, + chip->oob_poi + + (info->nfullchunks * (info->spare_size)), + info->last_spare_size); + chip->read_buf(mtd, + chip->oob_poi + ecc_off_buf + + (info->nfullchunks * (info->ecc_size)), + info->ecc_size - 2); + } + + info->force_raw = false; + + return 0; +} + +static int pxa3xx_nand_read_oob_raw(struct mtd_info *mtd, + struct nand_chip *chip, int page) +{ + /* Invalidate page cache */ + chip->pagebuf = -1; + + return chip->ecc.read_page_raw(mtd, chip, chip->buffers->databuf, true, + page); +} + static uint8_t pxa3xx_nand_read_byte(struct mtd_info *mtd) { struct nand_chip *chip = mtd_to_nand(mtd); @@ -1669,6 +1753,8 @@ static int alloc_nand_resource(struct pxa3xx_nand_info *info) nand_set_controller_data(chip, host); chip->ecc.read_page = pxa3xx_nand_read_page_hwecc; + chip->ecc.read_page_raw = pxa3xx_nand_read_page_raw; + chip->ecc.read_oob_raw = pxa3xx_nand_read_oob_raw; chip->ecc.write_page = pxa3xx_nand_write_page_hwecc; chip->controller = &info->controller; chip->waitfunc = pxa3xx_nand_waitfunc; -- cgit v1.3.1 From af61ea27f51fce62188276d7b5682ac51b03a705 Mon Sep 17 00:00:00 2001 From: Miquel Raynal Date: Thu, 11 Oct 2018 17:45:43 +0200 Subject: mtd: nand: pxa3xx: re-read a page in raw mode on uncorrectable error This only applies on BCH path. When an empty page is read, it triggers an uncorrectable error. While this is expected, the ECC engine might produce itself bitflips in the read data under certain layouts. To overcome this situation, always re-read the entire page in raw mode and check for the whole page to be empty. Also report the right number of bitflips if there are any. Signed-off-by: Miquel Raynal Acked-by: Jagan Teki --- drivers/mtd/nand/raw/pxa3xx_nand.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/nand/raw/pxa3xx_nand.c b/drivers/mtd/nand/raw/pxa3xx_nand.c index 454597355b7..492485b1d0c 100644 --- a/drivers/mtd/nand/raw/pxa3xx_nand.c +++ b/drivers/mtd/nand/raw/pxa3xx_nand.c @@ -1237,6 +1237,7 @@ static int pxa3xx_nand_read_page_hwecc(struct mtd_info *mtd, { struct pxa3xx_nand_host *host = nand_get_controller_data(chip); struct pxa3xx_nand_info *info = host->info_data; + int bf; chip->read_buf(mtd, buf, mtd->writesize); chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); @@ -1244,12 +1245,30 @@ static int pxa3xx_nand_read_page_hwecc(struct mtd_info *mtd, if (info->retcode == ERR_CORERR && info->use_ecc) { mtd->ecc_stats.corrected += info->ecc_err_cnt; - } else if (info->retcode == ERR_UNCORERR) { + } else if (info->retcode == ERR_UNCORERR && info->ecc_bch) { /* - * for blank page (all 0xff), HW will calculate its ECC as - * 0, which is different from the ECC information within - * OOB, ignore such uncorrectable errors + * Empty pages will trigger uncorrectable errors. Re-read the + * entire page in raw mode and check for bits not being "1". + * If there are more than the supported strength, then it means + * this is an actual uncorrectable error. */ + chip->ecc.read_page_raw(mtd, chip, buf, oob_required, page); + bf = nand_check_erased_ecc_chunk(buf, mtd->writesize, + chip->oob_poi, mtd->oobsize, + NULL, 0, chip->ecc.strength); + if (bf < 0) { + mtd->ecc_stats.failed++; + } else if (bf) { + mtd->ecc_stats.corrected += bf; + info->max_bitflips = max_t(unsigned int, + info->max_bitflips, bf); + info->retcode = ERR_CORERR; + } else { + info->retcode = ERR_NONE; + } + + } else if (info->retcode == ERR_UNCORERR && !info->ecc_bch) { + /* Raw read is not supported with Hamming ECC engine */ if (is_buf_blank(buf, mtd->writesize)) info->retcode = ERR_NONE; else -- cgit v1.3.1 From c907464a0ad5f1327a3873e9d0ffd617a0182a44 Mon Sep 17 00:00:00 2001 From: Miquel Raynal Date: Thu, 11 Oct 2018 17:45:44 +0200 Subject: mtd: rawnand: pxa3xx: fix 2kiB pages with 8b strength chips layout The initial layout for such NAND chips was the following: +----------------------------------------------------------------------------+ | 1024 (data) | 30 (ECC) | 1024 (data) | 30 (ECC) | 32 (free OOB) | 30 (ECC) | +----------------------------------------------------------------------------+ This layout has a weakness: reading empty pages trigger ECC errors (this is expected), but the hardware ECC engine tries to correct the data anyway and creates itself bitflips, hence bitflips are detected in erased pages while actually there are none in the NAND chip. Two solutions have been found at the same time. One was to enlarge the free OOB area to 64 bytes, changing the layout to be: +----------------------------------------------------------------------------+ | 1024 (data) | 30 (ECC) | 1024 (data) | 30 (ECC) | 64 (free OOB) | 30 (ECC) | +----------------------------------------------------------------------------+ ^^ The very big drawbacks of this solution are: 1/ It prevents booting from NAND. 2/ The current Linux driver (marvell_nand) does not have such problem because it already re-reads possible empty pages in raw mode before checking for bitflips. Using different layouts in U-Boot and Linux would simply not work. As this driver does support raw reads now and uses it to check for empty pages, let's forget about this broken hack and return to the initial layout with only 32 free OOB bytes. Fixes: ac56a3b30c ("mtd: nand: pxa3xx: add support for 2KB 8-bit flash") Signed-off-by: Miquel Raynal Acked-by: Jagan Teki --- drivers/mtd/nand/raw/pxa3xx_nand.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/nand/raw/pxa3xx_nand.c b/drivers/mtd/nand/raw/pxa3xx_nand.c index 492485b1d0c..4d2712df4c7 100644 --- a/drivers/mtd/nand/raw/pxa3xx_nand.c +++ b/drivers/mtd/nand/raw/pxa3xx_nand.c @@ -327,14 +327,14 @@ static struct nand_ecclayout ecc_layout_2KB_bch4bit = { static struct nand_ecclayout ecc_layout_2KB_bch8bit = { .eccbytes = 64, .eccpos = { - 64, 65, 66, 67, 68, 69, 70, 71, - 72, 73, 74, 75, 76, 77, 78, 79, - 80, 81, 82, 83, 84, 85, 86, 87, - 88, 89, 90, 91, 92, 93, 94, 95, - 96, 97, 98, 99, 100, 101, 102, 103, - 104, 105, 106, 107, 108, 109, 110, 111, - 112, 113, 114, 115, 116, 117, 118, 119, - 120, 121, 122, 123, 124, 125, 126, 127}, + 32, 33, 34, 35, 36, 37, 38, 39, + 40, 41, 42, 43, 44, 45, 46, 47, + 48, 49, 50, 51, 52, 53, 54, 55, + 56, 57, 58, 59, 60, 61, 62, 63, + 64, 65, 66, 67, 68, 69, 70, 71, + 72, 73, 74, 75, 76, 77, 78, 79, + 80, 81, 82, 83, 84, 85, 86, 87, + 88, 89, 90, 91, 92, 93, 94, 95}, .oobfree = { {1, 4}, {6, 26} } }; @@ -1591,7 +1591,7 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info, info->chunk_size = 1024; info->spare_size = 0; info->last_chunk_size = 1024; - info->last_spare_size = 64; + info->last_spare_size = 32; info->ecc_size = 32; ecc->mode = NAND_ECC_HW; ecc->size = info->chunk_size; -- cgit v1.3.1 From 4c47fd0b6bce62162e11b8a22e2eaf0d8f6673b1 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:22 +0100 Subject: mtd: Add a function to report when the MTD dev list has been updated We need to parse mtdparts/mtids again everytime a device has been added/removed from the MTD list, but there's currently no way to know when such an update has been done. Add an ->updated field to the idr struct that we set to true every time a device is added/removed and expose a function returning the value of this field and resetting it to false. Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher --- drivers/mtd/mtdcore.c | 16 +++++++++++++++- include/linux/mtd/mtd.h | 1 + 2 files changed, 16 insertions(+), 1 deletion(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index fb6c779abbf..7a15ded8c88 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -87,14 +87,17 @@ struct idr_layer { struct idr { struct idr_layer id[MAX_IDR_ID]; + bool updated; }; #define DEFINE_IDR(name) struct idr name; void idr_remove(struct idr *idp, int id) { - if (idp->id[id].used) + if (idp->id[id].used) { idp->id[id].used = 0; + idp->updated = true; + } return; } @@ -134,6 +137,7 @@ int idr_alloc(struct idr *idp, void *ptr, int start, int end, gfp_t gfp_mask) if (idl->used == 0) { idl->used = 1; idl->ptr = ptr; + idp->updated = true; return i; } i++; @@ -155,6 +159,16 @@ struct mtd_info *__mtd_next_device(int i) } EXPORT_SYMBOL_GPL(__mtd_next_device); +bool mtd_dev_list_updated(void) +{ + if (mtd_idr.updated) { + mtd_idr.updated = false; + return true; + } + + return false; +} + #ifndef __UBOOT__ static LIST_HEAD(mtd_notifiers); diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 68e59153249..d20ebd82028 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -581,6 +581,7 @@ int mtd_arg_off_size(int argc, char *const argv[], int *idx, loff_t *off, void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset, const uint64_t length, uint64_t *len_incl_bad, int *truncated); +bool mtd_dev_list_updated(void); /* drivers/mtd/mtd_uboot.c */ int mtd_search_alternate_name(const char *mtdname, char *altname, -- cgit v1.3.1 From 779c9c0565a44e7dc5f72919d88f67fb7e280880 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:23 +0100 Subject: mtd: Parse mtdparts/mtdids again when the MTD list has been updated Updates to the MTD device list should trigger a new parsing of the mtdids/mtdparts vars even if those vars haven't changed. Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher --- drivers/mtd/mtd_uboot.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index 5ca560c9687..6a3e64395de 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -161,9 +161,13 @@ int mtd_probe_devices(void) mtd_probe_uclass_mtd_devs(); - /* Check if mtdparts/mtdids changed since last call, otherwise: exit */ + /* + * Check if mtdparts/mtdids changed or if the MTD dev list was updated + * since last call, otherwise: exit + */ if ((!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) || (mtdparts && old_mtdparts && mtdids && old_mtdids && + !mtd_dev_list_updated() && !strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))) return 0; @@ -201,6 +205,12 @@ int mtd_probe_devices(void) } } + /* + * Call mtd_dev_list_updated() to clear updates generated by our own + * parts removal loop. + */ + mtd_dev_list_updated(); + /* If either mtdparts or mtdids is empty, then exit */ if (!mtdparts || !mtdids) return 0; @@ -281,6 +291,12 @@ int mtd_probe_devices(void) put_mtd_device(mtd); } + /* + * Call mtd_dev_list_updated() to clear updates generated by our own + * parts registration loop. + */ + mtd_dev_list_updated(); + return 0; } #else -- cgit v1.3.1 From a02820fca90ce9ccf243b3fce59c04dabd5671a8 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:24 +0100 Subject: mtd: Delete partitions attached to the device when a device is deleted If we don't do that, partitions might still be exposed while the underlying device is gone. Fixes: 2a74930da57f ("mtd: mtdpart: implement proper partition handling") Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher --- drivers/mtd/mtdcore.c | 7 +++++++ include/linux/mtd/mtd.h | 15 +++++++++++++++ 2 files changed, 22 insertions(+) (limited to 'drivers/mtd') diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 7a15ded8c88..cb7ca38d074 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -528,6 +528,13 @@ int del_mtd_device(struct mtd_info *mtd) struct mtd_notifier *not; #endif + ret = del_mtd_partitions(mtd); + if (ret) { + debug("Failed to delete MTD partitions attached to %s (err %d)\n", + mtd->name, ret); + return ret; + } + mutex_lock(&mtd_table_mutex); if (idr_find(&mtd_idr, mtd->index) != mtd) { diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index d20ebd82028..4d0096d9f1d 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -562,8 +562,23 @@ unsigned mtd_mmap_capabilities(struct mtd_info *mtd); /* drivers/mtd/mtdcore.h */ int add_mtd_device(struct mtd_info *mtd); int del_mtd_device(struct mtd_info *mtd); + +#ifdef CONFIG_MTD_PARTITIONS int add_mtd_partitions(struct mtd_info *, const struct mtd_partition *, int); int del_mtd_partitions(struct mtd_info *); +#else +static inline int add_mtd_partitions(struct mtd_info *mtd, + const struct mtd_partition *parts, + int nparts) +{ + return 0; +} + +static inline int del_mtd_partitions(struct mtd_info *mtd) +{ + return 0; +} +#endif struct mtd_info *__mtd_next_device(int i); #define mtd_for_each_device(mtd) \ -- cgit v1.3.1 From 492151b2093ee4df173f7e10da4938ef7c2a2156 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:25 +0100 Subject: mtd: sf: Make sure we don't register the same device twice spi_flash_mtd_register() can be called several times and each time it will register the same mtd_info instance like if it was a new one. The MTD ID allocation gets crazy when that happens, so let's track the status of the sf_mtd_info object to avoid that. Fixes: 9fe6d8716e09 ("mtd, spi: Add MTD layer driver") Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher Reviewed-by: Jagan Teki --- drivers/mtd/spi/sf_mtd.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c index 58d7e443990..aabbc358943 100644 --- a/drivers/mtd/spi/sf_mtd.c +++ b/drivers/mtd/spi/sf_mtd.c @@ -10,6 +10,7 @@ #include static struct mtd_info sf_mtd_info; +static bool sf_mtd_registered; static char sf_mtd_name[8]; static int spi_flash_mtd_erase(struct mtd_info *mtd, struct erase_info *instr) @@ -73,6 +74,12 @@ static int spi_flash_mtd_number(void) int spi_flash_mtd_register(struct spi_flash *flash) { + int ret; + + if (sf_mtd_registered) + del_mtd_device(&sf_mtd_info); + + sf_mtd_registered = false; memset(&sf_mtd_info, 0, sizeof(sf_mtd_info)); sprintf(sf_mtd_name, "nor%d", spi_flash_mtd_number()); @@ -94,7 +101,11 @@ int spi_flash_mtd_register(struct spi_flash *flash) sf_mtd_info.numeraseregions = 0; sf_mtd_info.erasesize = flash->sector_size; - return add_mtd_device(&sf_mtd_info); + ret = add_mtd_device(&sf_mtd_info); + if (!ret) + sf_mtd_registered = true; + + return ret; } void spi_flash_mtd_unregister(void) -- cgit v1.3.1 From 96b06434e56ce5dca391e8417d293132ff7164e0 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:26 +0100 Subject: mtd: Use get_mtdids() instead of env_get("mtdids") in mtd_search_alternate_name() The environment is not guaranteed to contain a valid mtdids variable when called from mtd_search_alternate_name(). Call get_mtdids() instead of env_get("mtdids"). Fixes: ff4afa8a981e ("mtd: uboot: search for an equivalent MTD name with the mtdids") Signed-off-by: Boris Brezillon Reviewed-by: Miquel Raynal Tested-by: Heiko Schocher --- drivers/mtd/mtd_uboot.c | 49 ++++++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 25 deletions(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index 6a3e64395de..c4434d70520 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -13,6 +13,29 @@ #define MTD_NAME_MAX_LEN 20 +void board_mtdparts_default(const char **mtdids, const char **mtdparts); + +static const char *get_mtdids(void) +{ + __maybe_unused const char *mtdparts = NULL; + const char *mtdids = env_get("mtdids"); + + if (mtdids) + return mtdids; + +#if defined(CONFIG_SYS_MTDPARTS_RUNTIME) + board_mtdparts_default(&mtdids, &mtdparts); +#elif defined(MTDIDS_DEFAULT) + mtdids = MTDIDS_DEFAULT; +#elif defined(CONFIG_MTDIDS_DEFAULT) + mtdids = CONFIG_MTDIDS_DEFAULT; +#endif + + if (mtdids) + env_set("mtdids", mtdids); + + return mtdids; +} /** * mtd_search_alternate_name - Search an alternate name for @mtdname thanks to @@ -34,7 +57,7 @@ int mtd_search_alternate_name(const char *mtdname, char *altname, const char *mtdids, *equal, *comma, *dev_id, *mtd_id; int dev_id_len, mtd_id_len; - mtdids = env_get("mtdids"); + mtdids = get_mtdids(); if (!mtdids) return -EINVAL; @@ -92,30 +115,6 @@ static void mtd_probe_uclass_mtd_devs(void) { } #endif #if defined(CONFIG_MTD_PARTITIONS) -extern void board_mtdparts_default(const char **mtdids, - const char **mtdparts); - -static const char *get_mtdids(void) -{ - __maybe_unused const char *mtdparts = NULL; - const char *mtdids = env_get("mtdids"); - - if (mtdids) - return mtdids; - -#if defined(CONFIG_SYS_MTDPARTS_RUNTIME) - board_mtdparts_default(&mtdids, &mtdparts); -#elif defined(MTDIDS_DEFAULT) - mtdids = MTDIDS_DEFAULT; -#elif defined(CONFIG_MTDIDS_DEFAULT) - mtdids = CONFIG_MTDIDS_DEFAULT; -#endif - - if (mtdids) - env_set("mtdids", mtdids); - - return mtdids; -} #define MTDPARTS_MAXLEN 512 -- cgit v1.3.1 From 429e048e4190b68b078c37c0012c59804e32818b Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:27 +0100 Subject: mtd: Be more strict on the "mtdparts=" prefix check strstr() does not guarantee that the string we're searching for is placed at the beginning. Use strncmp() instead. Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher --- drivers/mtd/mtd_uboot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index c4434d70520..d551aee2020 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -215,7 +215,7 @@ int mtd_probe_devices(void) return 0; /* Start the parsing by ignoring the extra 'mtdparts=' prefix, if any */ - if (strstr(mtdparts, "mtdparts=")) + if (!strncmp(mtdparts, "mtdparts=", sizeof("mtdparts=") - 1)) mtdparts += 9; /* For each MTD device in mtdparts */ -- cgit v1.3.1 From 772aa9799353d1d6bd6d9e2682945d4d7122539a Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:28 +0100 Subject: mtd: Make sure the name passed in mtdparts fits in mtd_name[] The local mtd_name[] variable is limited in size. Return an error if the name passed in mtdparts does not fit in this local var. Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher --- drivers/mtd/mtd_uboot.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index d551aee2020..0eda3627830 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -222,8 +222,8 @@ int mtd_probe_devices(void) while (mtdparts[0] != '\0') { char mtd_name[MTD_NAME_MAX_LEN], *colon; struct mtd_partition *parts; - int mtd_name_len, nparts; - int ret; + unsigned int mtd_name_len; + int nparts, ret; colon = strchr(mtdparts, ':'); if (!colon) { @@ -231,7 +231,12 @@ int mtd_probe_devices(void) return -EINVAL; } - mtd_name_len = colon - mtdparts; + mtd_name_len = (unsigned int)(colon - mtdparts); + if (mtd_name_len + 1 > sizeof(mtd_name)) { + printf("MTD name too long: %s\n", mtdparts); + return -EINVAL; + } + strncpy(mtd_name, mtdparts, mtd_name_len); mtd_name[mtd_name_len] = '\0'; /* Move the pointer forward (including the ':') */ -- cgit v1.3.1 From 2428d9160b80375870c1fff6cbb0214639628282 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:29 +0100 Subject: mtd: Make sure we don't parse MTD partitions belonging to another dev The mtdparts variable might contain partition definitions for several MTD devices. Each partition layout is separated by a ';', so let's make sure we don't pick a wrong name when mtdparts is malformed. Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher --- drivers/mtd/mtd_uboot.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index 0eda3627830..6a36948b917 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -155,6 +155,7 @@ int mtd_probe_devices(void) static char *old_mtdids; const char *mtdparts = get_mtdparts(); const char *mtdids = get_mtdids(); + const char *mtdparts_next = mtdparts; bool remaining_partitions = true; struct mtd_info *mtd; @@ -219,13 +220,22 @@ int mtd_probe_devices(void) mtdparts += 9; /* For each MTD device in mtdparts */ - while (mtdparts[0] != '\0') { + for (; mtdparts[0] != '\0'; mtdparts = mtdparts_next) { char mtd_name[MTD_NAME_MAX_LEN], *colon; struct mtd_partition *parts; unsigned int mtd_name_len; int nparts, ret; + mtdparts_next = strchr(mtdparts, ';'); + if (!mtdparts_next) + mtdparts_next = mtdparts + strlen(mtdparts); + else + mtdparts_next++; + colon = strchr(mtdparts, ':'); + if (colon > mtdparts_next) + colon = NULL; + if (!colon) { printf("Wrong mtdparts: %s\n", mtdparts); return -EINVAL; @@ -263,10 +273,7 @@ int mtd_probe_devices(void) if (ret || IS_ERR_OR_NULL(mtd)) { printf("Could not find a valid device for %s\n", mtd_name); - mtdparts = strchr(mtdparts, ';'); - if (mtdparts) - mtdparts++; - + mtdparts = mtdparts_next; continue; } } -- cgit v1.3.1 From 4a5594fa20d0fa6479f477d2bd67967aca201c2f Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:30 +0100 Subject: mtd: Don't stop MTD partition creation when it fails on one device MTD partition creation code is a bit tricky. It tries to figure out when things have changed (either MTD dev list or mtdparts/mtdids vars) and when that happens it first deletes all the partitions that had been previously created and then creates the new ones based on the new mtdparts/mtdids values. But before deleting the old partitions, it ensures that none of the currently registered parts are being used and bails out when that's not the case. So, we end up in a situation where, if at least one MTD dev has one of its partitions used by someone (UBI for instance), the partitions update logic no longer works for other devs. Rework the code to relax the logic and allow updates of MTD parts on devices that are not being used (we still refuse to updates parts on devices who have at least one of their partitions used by someone). Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher --- drivers/mtd/mtd_uboot.c | 96 ++++++++++++++++++++++++++++++++++--------------- drivers/mtd/mtdpart.c | 12 +++++++ include/linux/mtd/mtd.h | 2 ++ 3 files changed, 82 insertions(+), 28 deletions(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index 6a36948b917..d638f700d04 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -149,6 +149,54 @@ static const char *get_mtdparts(void) return mtdparts; } +static int mtd_del_parts(struct mtd_info *mtd, bool quiet) +{ + int ret; + + if (!mtd_has_partitions(mtd)) + return 0; + + /* do not delete partitions if they are in use. */ + if (mtd_partitions_used(mtd)) { + if (!quiet) + printf("\"%s\" partitions still in use, can't delete them\n", + mtd->name); + return -EACCES; + } + + ret = del_mtd_partitions(mtd); + if (ret) + return ret; + + return 1; +} + +static bool mtd_del_all_parts_failed; + +static void mtd_del_all_parts(void) +{ + struct mtd_info *mtd; + int ret = 0; + + mtd_del_all_parts_failed = false; + + /* + * It is not safe to remove entries from the mtd_for_each_device loop + * as it uses idr indexes and the partitions removal is done in bulk + * (all partitions of one device at the same time), so break and + * iterate from start each time a new partition is found and deleted. + */ + do { + mtd_for_each_device(mtd) { + ret = mtd_del_parts(mtd, false); + if (ret > 0) + break; + else if (ret < 0) + mtd_del_all_parts_failed = true; + } + } while (ret > 0); +} + int mtd_probe_devices(void) { static char *old_mtdparts; @@ -156,18 +204,19 @@ int mtd_probe_devices(void) const char *mtdparts = get_mtdparts(); const char *mtdids = get_mtdids(); const char *mtdparts_next = mtdparts; - bool remaining_partitions = true; struct mtd_info *mtd; mtd_probe_uclass_mtd_devs(); /* - * Check if mtdparts/mtdids changed or if the MTD dev list was updated - * since last call, otherwise: exit + * Check if mtdparts/mtdids changed, if the MTD dev list was updated + * or if our previous attempt to delete existing partititions failed. + * In any of these cases we want to update the partitions, otherwise, + * everything is up-to-date and we can return 0 directly. */ if ((!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) || (mtdparts && old_mtdparts && mtdids && old_mtdids && - !mtd_dev_list_updated() && + !mtd_dev_list_updated() && !mtd_del_all_parts_failed && !strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))) return 0; @@ -178,32 +227,12 @@ int mtd_probe_devices(void) old_mtdparts = strdup(mtdparts); old_mtdids = strdup(mtdids); - /* If at least one partition is still in use, do not delete anything */ - mtd_for_each_device(mtd) { - if (mtd->usecount) { - printf("Partition \"%s\" already in use, aborting\n", - mtd->name); - return -EACCES; - } - } - /* - * Everything looks clear, remove all partitions. It is not safe to - * remove entries from the mtd_for_each_device loop as it uses idr - * indexes and the partitions removal is done in bulk (all partitions of - * one device at the same time), so break and iterate from start each - * time a new partition is found and deleted. + * Remove all old parts. Note that partition removal can fail in case + * one of the partition is still being used by an MTD user, so this + * does not guarantee that all old partitions are gone. */ - while (remaining_partitions) { - remaining_partitions = false; - mtd_for_each_device(mtd) { - if (!mtd_is_partition(mtd) && mtd_has_partitions(mtd)) { - del_mtd_partitions(mtd); - remaining_partitions = true; - break; - } - } - } + mtd_del_all_parts(); /* * Call mtd_dev_list_updated() to clear updates generated by our own @@ -278,6 +307,17 @@ int mtd_probe_devices(void) } } + /* + * Call mtd_del_parts() again, even if it's already been called + * in mtd_del_all_parts(). We need to know if old partitions are + * still around (because they are still being used by someone), + * and if they are, we shouldn't create new partitions, so just + * skip this MTD device and try the next one. + */ + ret = mtd_del_parts(mtd, true); + if (ret < 0) + continue; + /* * Parse the MTD device partitions. It will update the mtdparts * pointer, create an array of parts (that must be freed), and diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 4d2ac8107f0..fd8d8e5ea72 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -63,6 +63,18 @@ char *kstrdup(const char *s, gfp_t gfp) #define MTD_SIZE_REMAINING (~0LLU) #define MTD_OFFSET_NOT_SPECIFIED (~0LLU) +bool mtd_partitions_used(struct mtd_info *master) +{ + struct mtd_info *slave; + + list_for_each_entry(slave, &master->partitions, node) { + if (slave->usecount) + return true; + } + + return false; +} + /** * mtd_parse_partition - Parse @mtdparts partition definition, fill @partition * with it and update the @mtdparts string pointer. diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 4d0096d9f1d..cd1f557a2f3 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -366,6 +366,8 @@ static inline bool mtd_has_partitions(const struct mtd_info *mtd) return !list_empty(&mtd->partitions); } +bool mtd_partitions_used(struct mtd_info *master); + int mtd_ooblayout_ecc(struct mtd_info *mtd, int section, struct mtd_oob_region *oobecc); int mtd_ooblayout_find_eccregion(struct mtd_info *mtd, int eccbyte, -- cgit v1.3.1 From 7371944a71690abafd0717b5d5f72c67e9f0f414 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:31 +0100 Subject: mtd: sf: Unregister the MTD device prior to removing the spi_flash obj The DM implementation of spi_flash_free() does not unregister the MTD device before removing the spi dev object. This leads to a use-after-free bug when the MTD device is later accessed by a MTD user (observed when attaching the device to UBI after env_sf_load() has called spi_flash_free()). Implement ->remove() and call spi_flash_mtd_unregister() from there. Fixes: 9fe6d8716e09 ("mtd, spi: Add MTD layer driver") Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher Reviewed-by: Jagan Teki --- drivers/mtd/spi/sf_probe.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'drivers/mtd') diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 94fde2ae7a3..4d7320fe8c1 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -137,6 +137,14 @@ static int spi_flash_std_probe(struct udevice *dev) return spi_flash_probe_slave(flash); } +static int spi_flash_std_remove(struct udevice *dev) +{ +#ifdef CONFIG_SPI_FLASH_MTD + spi_flash_mtd_unregister(); +#endif + return 0; +} + static const struct dm_spi_flash_ops spi_flash_std_ops = { .read = spi_flash_std_read, .write = spi_flash_std_write, @@ -153,6 +161,7 @@ U_BOOT_DRIVER(spi_flash_std) = { .id = UCLASS_SPI_FLASH, .of_match = spi_flash_std_ids, .probe = spi_flash_std_probe, + .remove = spi_flash_std_remove, .priv_auto_alloc_size = sizeof(struct spi_flash), .ops = &spi_flash_std_ops, }; -- cgit v1.3.1 From 08898e8b22d74a4511eadee9b06b11aab43e809c Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sun, 2 Dec 2018 10:54:32 +0100 Subject: mtd: sf: Make sf_mtd.c more robust SPI flash based MTD devs can be registered/unregistered at any time through the sf probe command or the spi_flash_free() function. This commit does not try to fix the root cause as it would probably require rewriting most of the code and have an mtd_info object instance per spi_flash object (not to mention that the the spi-flash layer is likely to be replaced by a spi-nor layer ported from Linux). Instead, we try to be as safe as can be by checking the code returned by del_mtd_device() and complain loudly when there's nothing we can do about the deregistration failure. When that happens we also reset sf_mtd_info.priv to NULL, and check for NULL pointer in the mtd hooks so that -ENODEV is returned instead of hitting a NULL pointer dereference exception when the MTD instance is later accessed by a user. Signed-off-by: Boris Brezillon Tested-by: Heiko Schocher --- drivers/mtd/spi/sf_mtd.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c index aabbc358943..68c36002bee 100644 --- a/drivers/mtd/spi/sf_mtd.c +++ b/drivers/mtd/spi/sf_mtd.c @@ -18,6 +18,9 @@ static int spi_flash_mtd_erase(struct mtd_info *mtd, struct erase_info *instr) struct spi_flash *flash = mtd->priv; int err; + if (!flash) + return -ENODEV; + instr->state = MTD_ERASING; err = spi_flash_erase(flash, instr->addr, instr->len); @@ -39,6 +42,9 @@ static int spi_flash_mtd_read(struct mtd_info *mtd, loff_t from, size_t len, struct spi_flash *flash = mtd->priv; int err; + if (!flash) + return -ENODEV; + err = spi_flash_read(flash, from, len, buf); if (!err) *retlen = len; @@ -52,6 +58,9 @@ static int spi_flash_mtd_write(struct mtd_info *mtd, loff_t to, size_t len, struct spi_flash *flash = mtd->priv; int err; + if (!flash) + return -ENODEV; + err = spi_flash_write(flash, to, len, buf); if (!err) *retlen = len; @@ -76,8 +85,13 @@ int spi_flash_mtd_register(struct spi_flash *flash) { int ret; - if (sf_mtd_registered) - del_mtd_device(&sf_mtd_info); + if (sf_mtd_registered) { + ret = del_mtd_device(&sf_mtd_info); + if (ret) + return ret; + + sf_mtd_registered = false; + } sf_mtd_registered = false; memset(&sf_mtd_info, 0, sizeof(sf_mtd_info)); @@ -110,5 +124,24 @@ int spi_flash_mtd_register(struct spi_flash *flash) void spi_flash_mtd_unregister(void) { - del_mtd_device(&sf_mtd_info); + int ret; + + if (!sf_mtd_registered) + return; + + ret = del_mtd_device(&sf_mtd_info); + if (!ret) { + sf_mtd_registered = false; + return; + } + + /* + * Setting mtd->priv to NULL is the best we can do. Thanks to that, + * the MTD layer can still call mtd hooks without risking a + * use-after-free bug. Still, things should be fixed to prevent the + * spi_flash object from being destroyed when del_mtd_device() fails. + */ + sf_mtd_info.priv = NULL; + printf("Failed to unregister MTD %s and the spi_flash object is going away: you're in deep trouble!", + sf_mtd_info.name); } -- cgit v1.3.1