[U-Boot] [PATCH v3 00/11] mtd/sf: Various fixes

[U-Boot] [PATCH v3 00/11] mtd/sf: Various fixes

[Boris Brezillon]

Hello,

This is the 3rd version of the mtd / sf fixes patchset. This v3
contains 2 new patches fixing bugs in the spi-flash layer.

With this version I hope to have addressed all the weird interactions
between the spi-flash and MTD layer (tested on a platform with a
spi-nor and a spi-nand)

Heiko, let me know if still issues with this version.

Thanks,

Boris

Boris Brezillon (11):
  mtd: Add a function to report when the MTD dev list has been updated
  mtd: Parse mtdparts/mtdids again when the MTD list has been updated
  mtd: Delete partitions attached to the device when a device is deleted
  mtd: sf: Make sure we don't register the same device twice
  mtd: Use get_mtdids() instead of env_get("mtdids") in
    mtd_search_alternate_name()
  mtd: Be more strict on the "mtdparts=" prefix check
  mtd: Make sure the name passed in mtdparts fits in mtd_name[]
  mtd: Make sure we don't parse MTD partitions belonging to another dev
  mtd: Don't stop MTD partition creation when it fails on one device
  mtd: sf: Unregister the MTD device prior to removing the spi_flash obj
  mtd: sf: Make sf_mtd.c more robust

 drivers/mtd/mtd_uboot.c    | 185 +++++++++++++++++++++++++------------
 drivers/mtd/mtdcore.c      |  17 +++-
 drivers/mtd/mtdpart.c      |  12 +++
 drivers/mtd/spi/sf_mtd.c   |  48 +++++++++-
 drivers/mtd/spi/sf_probe.c |   9 ++
 include/linux/mtd/mtd.h    |  18 ++++
 6 files changed, 227 insertions(+), 62 deletions(-)

-- 
2.17.1

[U-Boot] [PATCH v3 01/11] mtd: Add a function to report when the MTD dev list has been updated

[Boris Brezillon]

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 <boris.brezillon@bootlin.com>
---
Changes in v2:
- None
---
 drivers/mtd/mtdcore.c   | 16 +++++++++++++++-
 include/linux/mtd/mtd.h |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index fb6c779abbfe..7a15ded8c883 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 68e591532492..d20ebd820289 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,
-- 
2.17.1

[U-Boot] [PATCH v3 01/11] mtd: Add a function to report when the MTD dev list has been updated

[Heiko Schocher]

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v2:
> - None
> ---
>   drivers/mtd/mtdcore.c   | 16 +++++++++++++++-
>   include/linux/mtd/mtd.h |  1 +
>   2 files changed, 16 insertions(+), 1 deletion(-)

Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v3 02/11] mtd: Parse mtdparts/mtdids again when the MTD list has been updated

[Boris Brezillon]

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 <boris.brezillon@bootlin.com>
---
Changes in v3:
- None

Changes in v2:
- None
---
 drivers/mtd/mtd_uboot.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c
index 5ca560c96879..6a3e64395de2 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
-- 
2.17.1

[U-Boot] [PATCH v3 02/11] mtd: Parse mtdparts/mtdids again when the MTD list has been updated

[Heiko Schocher]

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - None
> 
> Changes in v2:
> - None
> ---
>   drivers/mtd/mtd_uboot.c | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)


Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v3 03/11] mtd: Delete partitions attached to the device when a device is deleted

[Boris Brezillon]

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 <boris.brezillon@bootlin.com>
---
Changes in v3:
- None

Changes in v2:
- Fix build failures when CONFIG_MTD_PARTITIONS is not enabled
---
 drivers/mtd/mtdcore.c   |  1 +
 include/linux/mtd/mtd.h | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 7a15ded8c883..46657fe7c949 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -528,6 +528,7 @@ int del_mtd_device(struct mtd_info *mtd)
 	struct mtd_notifier *not;
 #endif
 
+	del_mtd_partitions(mtd);
 	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 d20ebd820289..4d0096d9f1d8 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)			\
-- 
2.17.1

[U-Boot] [PATCH v3 03/11] mtd: Delete partitions attached to the device when a device is deleted

[Heiko Schocher]

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - None
> 
> Changes in v2:
> - Fix build failures when CONFIG_MTD_PARTITIONS is not enabled
> ---
>   drivers/mtd/mtdcore.c   |  1 +
>   include/linux/mtd/mtd.h | 15 +++++++++++++++
>   2 files changed, 16 insertions(+)


Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v3 03/11] mtd: Delete partitions attached to the device when a device is deleted

[Boris Brezillon]

On Mon, 19 Nov 2018 21:59:47 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - None
> 
> Changes in v2:
> - Fix build failures when CONFIG_MTD_PARTITIONS is not enabled
> ---
>  drivers/mtd/mtdcore.c   |  1 +
>  include/linux/mtd/mtd.h | 15 +++++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 7a15ded8c883..46657fe7c949 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -528,6 +528,7 @@ int del_mtd_device(struct mtd_info *mtd)
>  	struct mtd_notifier *not;
>  #endif
>  
> +	del_mtd_partitions(mtd);

We should check and propagate the error (if any).

	ret = del_mtd_partitions(mtd);
	if (ret)
		return ret;

I'll fix that in v4.

>  	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 d20ebd820289..4d0096d9f1d8 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)			\

[U-Boot] [PATCH v3 04/11] mtd: sf: Make sure we don't register the same device twice

[Boris Brezillon]

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 <boris.brezillon@bootlin.com>
---
Changes in v3:
- None

Changes in v2
- None
---
 drivers/mtd/spi/sf_mtd.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c
index 58d7e4439903..aabbc3589435 100644
--- a/drivers/mtd/spi/sf_mtd.c
+++ b/drivers/mtd/spi/sf_mtd.c
@@ -10,6 +10,7 @@
 #include <spi_flash.h>
 
 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)
-- 
2.17.1

[U-Boot] [PATCH v3 04/11] mtd: sf: Make sure we don't register the same device twice

[Heiko Schocher]

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - None
> 
> Changes in v2
> - None
> ---
>   drivers/mtd/spi/sf_mtd.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)

Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v3 04/11] mtd: sf: Make sure we don't register the same device twice

[Jagan Teki]

On Tue, Nov 20, 2018 at 2:35 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - None
>
> Changes in v2
> - None
> ---

Reviewed-by: Jagan Teki <jagan@openedev.com>

[U-Boot] [PATCH v3 05/11] mtd: Use get_mtdids() instead of env_get("mtdids") in mtd_search_alternate_name()

[Boris Brezillon]

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 <boris.brezillon@bootlin.com>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
Changes in v3:
- None

Changes in v2:
- Add Miquel's R-b
- Move board_mtdparts_default() prototype def to fix a build failure
---
 drivers/mtd/mtd_uboot.c | 49 ++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c
index 6a3e64395de2..c4434d70520d 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
 
-- 
2.17.1

[U-Boot] [PATCH v3 05/11] mtd: Use get_mtdids() instead of env_get("mtdids") in mtd_search_alternate_name()

[Heiko Schocher]

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> 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 <boris.brezillon@bootlin.com>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> Changes in v3:
> - None
> 
> Changes in v2:
> - Add Miquel's R-b
> - Move board_mtdparts_default() prototype def to fix a build failure
> ---
>   drivers/mtd/mtd_uboot.c | 49 ++++++++++++++++++++---------------------
>   1 file changed, 24 insertions(+), 25 deletions(-)

Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v3 06/11] mtd: Be more strict on the "mtdparts=" prefix check

[Boris Brezillon]

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 <boris.brezillon@bootlin.com>
---
Changes in v3:
- None

Changes in v2:
- New patch
---
 drivers/mtd/mtd_uboot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c
index c4434d70520d..d551aee20203 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 */
-- 
2.17.1

[U-Boot] [PATCH v3 06/11] mtd: Be more strict on the "mtdparts=" prefix check

[Heiko Schocher]

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - None
> 
> Changes in v2:
> - New patch
> ---
>   drivers/mtd/mtd_uboot.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v3 07/11] mtd: Make sure the name passed in mtdparts fits in mtd_name[]

[Boris Brezillon]

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 <boris.brezillon@bootlin.com>
---
Changes in v3:
- None

Changes in v2:
- New patch
---
 drivers/mtd/mtd_uboot.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c
index d551aee20203..0eda36278309 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 ':') */
-- 
2.17.1

[U-Boot] [PATCH v3 07/11] mtd: Make sure the name passed in mtdparts fits in mtd_name[]

[Heiko Schocher]

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - None
> 
> Changes in v2:
> - New patch
> ---
>   drivers/mtd/mtd_uboot.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)

Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v3 08/11] mtd: Make sure we don't parse MTD partitions belonging to another dev

[Boris Brezillon]

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 <boris.brezillon@bootlin.com>
---
Changes in v3:
- None

Changes in v2:
- New patch
---
 drivers/mtd/mtd_uboot.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c
index 0eda36278309..6a36948b917b 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;
 			}
 		}
-- 
2.17.1

[U-Boot] [PATCH v3 08/11] mtd: Make sure we don't parse MTD partitions belonging to another dev

[Heiko Schocher]

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - None
> 
> Changes in v2:
> - New patch
> ---
>   drivers/mtd/mtd_uboot.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)

Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v3 09/11] mtd: Don't stop MTD partition creation when it fails on one device

[Boris Brezillon]

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 <boris.brezillon@bootlin.com>
---
Changes in v3:
- None

Changes in v2:
- New patch

Changes in v3:
- Re-create partitions when our last attempt do delete all existing
  parts failed. This way we can update parts after ubi detach has
  been called without having to change mtdparts/mtdids.
---
 drivers/mtd/mtd_uboot.c | 96 +++++++++++++++++++++++++++++------------
 drivers/mtd/mtdpart.c   | 12 ++++++
 include/linux/mtd/mtd.h |  2 +
 3 files changed, 82 insertions(+), 28 deletions(-)

diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c
index 6a36948b917b..d638f700d041 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 4d2ac8107f01..fd8d8e5ea729 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 4d0096d9f1d8..cd1f557a2f31 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,
-- 
2.17.1

[U-Boot] [PATCH v3 09/11] mtd: Don't stop MTD partition creation when it fails on one device

[Heiko Schocher]

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - None
> 
> Changes in v2:
> - New patch
> 
> Changes in v3:
> - Re-create partitions when our last attempt do delete all existing
>    parts failed. This way we can update parts after ubi detach has
>    been called without having to change mtdparts/mtdids.
> ---
>   drivers/mtd/mtd_uboot.c | 96 +++++++++++++++++++++++++++++------------
>   drivers/mtd/mtdpart.c   | 12 ++++++
>   include/linux/mtd/mtd.h |  2 +
>   3 files changed, 82 insertions(+), 28 deletions(-)

Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v3 10/11] mtd: sf: Unregister the MTD device prior to removing the spi_flash obj

[Boris Brezillon]

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 <boris.brezillon@bootlin.com>
---
Changes in v3:
- New patch
---
 drivers/mtd/spi/sf_probe.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
index 94fde2ae7a36..4d7320fe8c14 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,
 };
-- 
2.17.1

[U-Boot] [PATCH v3 10/11] mtd: sf: Unregister the MTD device prior to removing the spi_flash obj

[Heiko Schocher]

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - New patch
> ---
>   drivers/mtd/spi/sf_probe.c | 9 +++++++++
>   1 file changed, 9 insertions(+)

Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v3 10/11] mtd: sf: Unregister the MTD device prior to removing the spi_flash obj

[Jagan Teki]

On Tue, Nov 20, 2018 at 2:37 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> 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 <boris.brezillon@bootlin.com>
> ---

Reviewed-by: Jagan Teki <jagan@openedev.com>

[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Boris Brezillon]

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 <boris.brezillon@bootlin.com>
---
Changes in v3:
- New patch
---
 drivers/mtd/spi/sf_mtd.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c
index aabbc3589435..68c36002bee2 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);
 }
-- 
2.17.1

[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Heiko Schocher]

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - New patch
> ---
>   drivers/mtd/spi/sf_mtd.c | 39 ++++++++++++++++++++++++++++++++++++---
>   1 file changed, 36 insertions(+), 3 deletions(-)

Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Jagan Teki]

On Tue, Nov 20, 2018 at 2:33 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> 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 <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - New patch
> ---
>  drivers/mtd/spi/sf_mtd.c | 39 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c
> index aabbc3589435..68c36002bee2 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);

Why do we need this print? can't we do the same thing in MTD core
itself, so-that it can be generic for all flash objects.

[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Boris Brezillon]

On Thu, 22 Nov 2018 12:40:57 +0530
Jagan Teki <jagan@amarulasolutions.com> wrote:

> On Tue, Nov 20, 2018 at 2:33 AM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > 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 <boris.brezillon@bootlin.com>
> > ---
> > Changes in v3:
> > - New patch
> > ---
> >  drivers/mtd/spi/sf_mtd.c | 39 ++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 36 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c
> > index aabbc3589435..68c36002bee2 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);  
> 
> Why do we need this print?

Yes we do, just to keep the user informed that something bad happened
and its spi-flash is no longer usable (at least through the MTD layer).

> can't we do the same thing in MTD core
> itself, so-that it can be generic for all flash objects.

del_mtd_device() can fail, so it's the caller responsibility to decide
what to do when that happens. Some users will propagate the error to
the upper layer and maybe cancel the device removal (AFAICT,
driver->remove() can return an error, not sure what happens in this
case though). For others, like spi-flash, the device will go away, and
all subsequent accesses will fail.

[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Boris Brezillon]

Hi Jagan,

On Thu, 22 Nov 2018 09:40:56 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> > > +       /*
> > > +        * 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);    
> > 
> > Why do we need this print?  
> 
> Yes we do, just to keep the user informed that something bad happened
> and its spi-flash is no longer usable (at least through the MTD layer).
> 
> > can't we do the same thing in MTD core
> > itself, so-that it can be generic for all flash objects.  
> 
> del_mtd_device() can fail, so it's the caller responsibility to decide
> what to do when that happens. Some users will propagate the error to
> the upper layer and maybe cancel the device removal (AFAICT,
> driver->remove() can return an error, not sure what happens in this
> case though). For others, like spi-flash, the device will go away, and
> all subsequent accesses will fail.

I'm about to send a new version fixing the problem I mentioned in patch
3, but before doing that, I'd like to know if my answer convinced you or
if you'd still prefer this message to go away (or be placed in
mtdcore/mtdpart.c).

Regards,

Boris

[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Jagan Teki]

On 26/11/18 2:12 PM, Boris Brezillon wrote:
> Hi Jagan,
> 
> On Thu, 22 Nov 2018 09:40:56 +0100
> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> 
>>>> +       /*
>>>> +        * 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);
>>>
>>> Why do we need this print?
>>
>> Yes we do, just to keep the user informed that something bad happened
>> and its spi-flash is no longer usable (at least through the MTD layer).
>>
>>> can't we do the same thing in MTD core
>>> itself, so-that it can be generic for all flash objects.
>>
>> del_mtd_device() can fail, so it's the caller responsibility to decide
>> what to do when that happens. Some users will propagate the error to
>> the upper layer and maybe cancel the device removal (AFAICT,
>> driver->remove() can return an error, not sure what happens in this
>> case though). For others, like spi-flash, the device will go away, and
>> all subsequent accesses will fail.
> 
> I'm about to send a new version fixing the problem I mentioned in patch
> 3, but before doing that, I'd like to know if my answer convinced you or
> if you'd still prefer this message to go away (or be placed in
> mtdcore/mtdpart.c).


I'm thinking of having the message still in MTD by showing which 
interface it would belongs, along with the details.

[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Boris Brezillon]

On Mon, 26 Nov 2018 16:42:48 +0530
Jagan Teki <jagan@openedev.com> wrote:

> On 26/11/18 2:12 PM, Boris Brezillon wrote:
> > Hi Jagan,
> > 
> > On Thu, 22 Nov 2018 09:40:56 +0100
> > Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> >   
> >>>> +       /*
> >>>> +        * 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);  
> >>>
> >>> Why do we need this print?  
> >>
> >> Yes we do, just to keep the user informed that something bad happened
> >> and its spi-flash is no longer usable (at least through the MTD layer).
> >>  
> >>> can't we do the same thing in MTD core
> >>> itself, so-that it can be generic for all flash objects.  
> >>
> >> del_mtd_device() can fail, so it's the caller responsibility to decide
> >> what to do when that happens. Some users will propagate the error to
> >> the upper layer and maybe cancel the device removal (AFAICT,
> >> driver->remove() can return an error, not sure what happens in this
> >> case though). For others, like spi-flash, the device will go away, and
> >> all subsequent accesses will fail.  
> > 
> > I'm about to send a new version fixing the problem I mentioned in patch
> > 3, but before doing that, I'd like to know if my answer convinced you or
> > if you'd still prefer this message to go away (or be placed in
> > mtdcore/mtdpart.c).  
> 
> 
> I'm thinking of having the message still in MTD by showing which 
> interface it would belongs, along with the details.

Then we'd need something less 

[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Boris Brezillon]

On Mon, 26 Nov 2018 13:37:46 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Mon, 26 Nov 2018 16:42:48 +0530
> Jagan Teki <jagan@openedev.com> wrote:
> 
> > On 26/11/18 2:12 PM, Boris Brezillon wrote:  
> > > Hi Jagan,
> > > 
> > > On Thu, 22 Nov 2018 09:40:56 +0100
> > > Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> > >     
> > >>>> +       /*
> > >>>> +        * 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);    
> > >>>
> > >>> Why do we need this print?    
> > >>
> > >> Yes we do, just to keep the user informed that something bad happened
> > >> and its spi-flash is no longer usable (at least through the MTD layer).
> > >>    
> > >>> can't we do the same thing in MTD core
> > >>> itself, so-that it can be generic for all flash objects.    
> > >>
> > >> del_mtd_device() can fail, so it's the caller responsibility to decide
> > >> what to do when that happens. Some users will propagate the error to
> > >> the upper layer and maybe cancel the device removal (AFAICT,
> > >> driver->remove() can return an error, not sure what happens in this
> > >> case though). For others, like spi-flash, the device will go away, and
> > >> all subsequent accesses will fail.    
> > > 
> > > I'm about to send a new version fixing the problem I mentioned in patch
> > > 3, but before doing that, I'd like to know if my answer convinced you or
> > > if you'd still prefer this message to go away (or be placed in
> > > mtdcore/mtdpart.c).    
> > 
> > 
> > I'm thinking of having the message still in MTD by showing which 
> > interface it would belongs, along with the details.  
> 
> Then we'd need something less 

Sorry, I inadvertently hit the send button :-/.

So, I was about to say that we need something less worrisome than the
message I added in the SF layer if we move it to the MTD layer because
some drivers might actually do the right thing when del_mtd_device()
returns an error. I keep thinking that putting an error message in
mtdcore.c is not the right thing to do, but if you insist, I'll add
one (maybe a debug()). In any case I'd like to keep the one we have
here, because in this specific case, there's simply nothing we can do
when MTD dev removal fails.

[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Jagan Teki]

On 26/11/18 6:12 PM, Boris Brezillon wrote:
> On Mon, 26 Nov 2018 13:37:46 +0100
> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> 
>> On Mon, 26 Nov 2018 16:42:48 +0530
>> Jagan Teki <jagan@openedev.com> wrote:
>>
>>> On 26/11/18 2:12 PM, Boris Brezillon wrote:
>>>> Hi Jagan,
>>>>
>>>> On Thu, 22 Nov 2018 09:40:56 +0100
>>>> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
>>>>      
>>>>>>> +       /*
>>>>>>> +        * 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);
>>>>>>
>>>>>> Why do we need this print?
>>>>>
>>>>> Yes we do, just to keep the user informed that something bad happened
>>>>> and its spi-flash is no longer usable (at least through the MTD layer).
>>>>>     
>>>>>> can't we do the same thing in MTD core
>>>>>> itself, so-that it can be generic for all flash objects.
>>>>>
>>>>> del_mtd_device() can fail, so it's the caller responsibility to decide
>>>>> what to do when that happens. Some users will propagate the error to
>>>>> the upper layer and maybe cancel the device removal (AFAICT,
>>>>> driver->remove() can return an error, not sure what happens in this
>>>>> case though). For others, like spi-flash, the device will go away, and
>>>>> all subsequent accesses will fail.
>>>>
>>>> I'm about to send a new version fixing the problem I mentioned in patch
>>>> 3, but before doing that, I'd like to know if my answer convinced you or
>>>> if you'd still prefer this message to go away (or be placed in
>>>> mtdcore/mtdpart.c).
>>>
>>>
>>> I'm thinking of having the message still in MTD by showing which
>>> interface it would belongs, along with the details.
>>
>> Then we'd need something less
> 
> Sorry, I inadvertently hit the send button :-/.
> 
> So, I was about to say that we need something less worrisome than the
> message I added in the SF layer if we move it to the MTD layer because
> some drivers might actually do the right thing when del_mtd_device()
> returns an error. I keep thinking that putting an error message in
> mtdcore.c is not the right thing to do, but if you insist, I'll add
> one (maybe a debug()). In any case I'd like to keep the one we have
> here, because in this specific case, there's simply nothing we can do
> when MTD dev removal fails.
> 

Look like other flashes were deleting only via mtd_partitions. how would 
they know and does it not need for them to print the same information?

[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Miquel Raynal]

Hi Jagan,

Jagan Teki <jagan@openedev.com> wrote on Mon, 26 Nov 2018 18:35:01
+0530:

> On 26/11/18 6:12 PM, Boris Brezillon wrote:
> > On Mon, 26 Nov 2018 13:37:46 +0100
> > Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> > 
> >> On Mon, 26 Nov 2018 16:42:48 +0530
> >> Jagan Teki <jagan@openedev.com> wrote:
> >>
> >>> On 26/11/18 2:12 PM, Boris Brezillon wrote:
> >>>> Hi Jagan,
> >>>>
> >>>> On Thu, 22 Nov 2018 09:40:56 +0100
> >>>> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> >>>>      >>>>>>> +       /*
> >>>>>>> +        * 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);
> >>>>>>
> >>>>>> Why do we need this print?
> >>>>>
> >>>>> Yes we do, just to keep the user informed that something bad happened
> >>>>> and its spi-flash is no longer usable (at least through the MTD layer).
> >>>>>     >>>>>> can't we do the same thing in MTD core
> >>>>>> itself, so-that it can be generic for all flash objects.
> >>>>>
> >>>>> del_mtd_device() can fail, so it's the caller responsibility to decide
> >>>>> what to do when that happens. Some users will propagate the error to
> >>>>> the upper layer and maybe cancel the device removal (AFAICT,
> >>>>> driver->remove() can return an error, not sure what happens in this
> >>>>> case though). For others, like spi-flash, the device will go away, and
> >>>>> all subsequent accesses will fail.
> >>>>
> >>>> I'm about to send a new version fixing the problem I mentioned in patch
> >>>> 3, but before doing that, I'd like to know if my answer convinced you or
> >>>> if you'd still prefer this message to go away (or be placed in
> >>>> mtdcore/mtdpart.c).
> >>>
> >>>
> >>> I'm thinking of having the message still in MTD by showing which
> >>> interface it would belongs, along with the details.
> >>
> >> Then we'd need something less
> > 
> > Sorry, I inadvertently hit the send button :-/.
> > 
> > So, I was about to say that we need something less worrisome than the
> > message I added in the SF layer if we move it to the MTD layer because
> > some drivers might actually do the right thing when del_mtd_device()
> > returns an error. I keep thinking that putting an error message in
> > mtdcore.c is not the right thing to do, but if you insist, I'll add
> > one (maybe a debug()). In any case I'd like to keep the one we have
> > here, because in this specific case, there's simply nothing we can do
> > when MTD dev removal fails.
> > 
> 
> Look like other flashes were deleting only via mtd_partitions. how would they know and does it not need for them to print the same information?

Do you see that sf does not use MTD in a consistent manner? The whole
point of this commit is to handle sf's lightnesses in a "more robust"
manner. This warning belongs to sf and not to the mtdcore, because
as stated in the comment above, we should "prevent the spi_flash
object from being destroyed when del_mtd_device() fails". If you don't
want such warning in sf, I think the best thing to do is to fix the
root issue.


Thanks,
Miquèl

[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Boris Brezillon]

On Mon, 26 Nov 2018 14:25:44 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Jagan,
> 
> Jagan Teki <jagan@openedev.com> wrote on Mon, 26 Nov 2018 18:35:01
> +0530:
> 
> > On 26/11/18 6:12 PM, Boris Brezillon wrote:  
> > > On Mon, 26 Nov 2018 13:37:46 +0100
> > > Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> > >   
> > >> On Mon, 26 Nov 2018 16:42:48 +0530
> > >> Jagan Teki <jagan@openedev.com> wrote:
> > >>  
> > >>> On 26/11/18 2:12 PM, Boris Brezillon wrote:  
> > >>>> Hi Jagan,
> > >>>>
> > >>>> On Thu, 22 Nov 2018 09:40:56 +0100
> > >>>> Boris Brezillon <boris.brezillon@bootlin.com> wrote:  
> > >>>>      >>>>>>> +       /*  
> > >>>>>>> +        * 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);  
> > >>>>>>
> > >>>>>> Why do we need this print?  
> > >>>>>
> > >>>>> Yes we do, just to keep the user informed that something bad happened
> > >>>>> and its spi-flash is no longer usable (at least through the MTD layer).  
> > >>>>>     >>>>>> can't we do the same thing in MTD core  
> > >>>>>> itself, so-that it can be generic for all flash objects.  
> > >>>>>
> > >>>>> del_mtd_device() can fail, so it's the caller responsibility to decide
> > >>>>> what to do when that happens. Some users will propagate the error to
> > >>>>> the upper layer and maybe cancel the device removal (AFAICT,
> > >>>>> driver->remove() can return an error, not sure what happens in this
> > >>>>> case though). For others, like spi-flash, the device will go away, and
> > >>>>> all subsequent accesses will fail.  
> > >>>>
> > >>>> I'm about to send a new version fixing the problem I mentioned in patch
> > >>>> 3, but before doing that, I'd like to know if my answer convinced you or
> > >>>> if you'd still prefer this message to go away (or be placed in
> > >>>> mtdcore/mtdpart.c).  
> > >>>
> > >>>
> > >>> I'm thinking of having the message still in MTD by showing which
> > >>> interface it would belongs, along with the details.  
> > >>
> > >> Then we'd need something less  
> > > 
> > > Sorry, I inadvertently hit the send button :-/.
> > > 
> > > So, I was about to say that we need something less worrisome than the
> > > message I added in the SF layer if we move it to the MTD layer because
> > > some drivers might actually do the right thing when del_mtd_device()
> > > returns an error. I keep thinking that putting an error message in
> > > mtdcore.c is not the right thing to do, but if you insist, I'll add
> > > one (maybe a debug()). In any case I'd like to keep the one we have
> > > here, because in this specific case, there's simply nothing we can do
> > > when MTD dev removal fails.
> > >   
> > 
> > Look like other flashes were deleting only via mtd_partitions. how would they know and does it not need for them to print the same information?  
> 
> Do you see that sf does not use MTD in a consistent manner? The whole
> point of this commit is to handle sf's lightnesses in a "more robust"
> manner. This warning belongs to sf and not to the mtdcore, because
> as stated in the comment above, we should "prevent the spi_flash
> object from being destroyed when del_mtd_device() fails". If you don't
> want such warning in sf, I think the best thing to do is to fix the
> root issue.

Jagan, are you okay with this suggestion (add a debug() message in the
del_mtd_device() path and keep the one we already have in sf_mtd.c)?

[U-Boot] [PATCH v3 11/11] mtd: sf: Make sf_mtd.c more robust

[Jagan Teki]

On Tue, Nov 27, 2018 at 6:06 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Mon, 26 Nov 2018 14:25:44 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> > Hi Jagan,
> >
> > Jagan Teki <jagan@openedev.com> wrote on Mon, 26 Nov 2018 18:35:01
> > +0530:
> >
> > > On 26/11/18 6:12 PM, Boris Brezillon wrote:
> > > > On Mon, 26 Nov 2018 13:37:46 +0100
> > > > Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> > > >
> > > >> On Mon, 26 Nov 2018 16:42:48 +0530
> > > >> Jagan Teki <jagan@openedev.com> wrote:
> > > >>
> > > >>> On 26/11/18 2:12 PM, Boris Brezillon wrote:
> > > >>>> Hi Jagan,
> > > >>>>
> > > >>>> On Thu, 22 Nov 2018 09:40:56 +0100
> > > >>>> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> > > >>>>      >>>>>>> +       /*
> > > >>>>>>> +        * 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);
> > > >>>>>>
> > > >>>>>> Why do we need this print?
> > > >>>>>
> > > >>>>> Yes we do, just to keep the user informed that something bad happened
> > > >>>>> and its spi-flash is no longer usable (at least through the MTD layer).
> > > >>>>>     >>>>>> can't we do the same thing in MTD core
> > > >>>>>> itself, so-that it can be generic for all flash objects.
> > > >>>>>
> > > >>>>> del_mtd_device() can fail, so it's the caller responsibility to decide
> > > >>>>> what to do when that happens. Some users will propagate the error to
> > > >>>>> the upper layer and maybe cancel the device removal (AFAICT,
> > > >>>>> driver->remove() can return an error, not sure what happens in this
> > > >>>>> case though). For others, like spi-flash, the device will go away, and
> > > >>>>> all subsequent accesses will fail.
> > > >>>>
> > > >>>> I'm about to send a new version fixing the problem I mentioned in patch
> > > >>>> 3, but before doing that, I'd like to know if my answer convinced you or
> > > >>>> if you'd still prefer this message to go away (or be placed in
> > > >>>> mtdcore/mtdpart.c).
> > > >>>
> > > >>>
> > > >>> I'm thinking of having the message still in MTD by showing which
> > > >>> interface it would belongs, along with the details.
> > > >>
> > > >> Then we'd need something less
> > > >
> > > > Sorry, I inadvertently hit the send button :-/.
> > > >
> > > > So, I was about to say that we need something less worrisome than the
> > > > message I added in the SF layer if we move it to the MTD layer because
> > > > some drivers might actually do the right thing when del_mtd_device()
> > > > returns an error. I keep thinking that putting an error message in
> > > > mtdcore.c is not the right thing to do, but if you insist, I'll add
> > > > one (maybe a debug()). In any case I'd like to keep the one we have
> > > > here, because in this specific case, there's simply nothing we can do
> > > > when MTD dev removal fails.
> > > >
> > >
> > > Look like other flashes were deleting only via mtd_partitions. how would they know and does it not need for them to print the same information?
> >
> > Do you see that sf does not use MTD in a consistent manner? The whole
> > point of this commit is to handle sf's lightnesses in a "more robust"
> > manner. This warning belongs to sf and not to the mtdcore, because
> > as stated in the comment above, we should "prevent the spi_flash
> > object from being destroyed when del_mtd_device() fails". If you don't
> > want such warning in sf, I think the best thing to do is to fix the
> > root issue.
>
> Jagan, are you okay with this suggestion (add a debug() message in the
> del_mtd_device() path and keep the one we already have in sf_mtd.c)?

Please proceed.

[U-Boot] [PATCH v3 00/11] mtd/sf: Various fixes

[Boris Brezillon]

On Mon, 19 Nov 2018 21:59:44 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> Hello,
> 
> This is the 3rd version of the mtd / sf fixes patchset. This v3
> contains 2 new patches fixing bugs in the spi-flash layer.
> 
> With this version I hope to have addressed all the weird interactions
> between the spi-flash and MTD layer (tested on a platform with a
> spi-nor and a spi-nand)
> 
> Heiko, let me know if still issues with this version.

			^ you still have

> 
> Thanks,
> 
> Boris
> 
> Boris Brezillon (11):
>   mtd: Add a function to report when the MTD dev list has been updated
>   mtd: Parse mtdparts/mtdids again when the MTD list has been updated
>   mtd: Delete partitions attached to the device when a device is deleted
>   mtd: sf: Make sure we don't register the same device twice
>   mtd: Use get_mtdids() instead of env_get("mtdids") in
>     mtd_search_alternate_name()
>   mtd: Be more strict on the "mtdparts=" prefix check
>   mtd: Make sure the name passed in mtdparts fits in mtd_name[]
>   mtd: Make sure we don't parse MTD partitions belonging to another dev
>   mtd: Don't stop MTD partition creation when it fails on one device
>   mtd: sf: Unregister the MTD device prior to removing the spi_flash obj
>   mtd: sf: Make sf_mtd.c more robust
> 
>  drivers/mtd/mtd_uboot.c    | 185 +++++++++++++++++++++++++------------
>  drivers/mtd/mtdcore.c      |  17 +++-
>  drivers/mtd/mtdpart.c      |  12 +++
>  drivers/mtd/spi/sf_mtd.c   |  48 +++++++++-
>  drivers/mtd/spi/sf_probe.c |   9 ++
>  include/linux/mtd/mtd.h    |  18 ++++
>  6 files changed, 227 insertions(+), 62 deletions(-)
> 

[U-Boot] [PATCH v3 00/11] mtd/sf: Various fixes

[Heiko Schocher]

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> Hello,
> 
> This is the 3rd version of the mtd / sf fixes patchset. This v3
> contains 2 new patches fixing bugs in the spi-flash layer.
> 
> With this version I hope to have addressed all the weird interactions
> between the spi-flash and MTD layer (tested on a platform with a
> spi-nor and a spi-nand)
> 
> Heiko, let me know if still issues with this version.

Yup, works fine now for me!

> Thanks,

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH v3 00/11] mtd/sf: Various fixes

[Boris Brezillon]

On Wed, 21 Nov 2018 07:43:17 +0100
Heiko Schocher <hs@denx.de> wrote:

> Hello Boris,
> 
> Am 19.11.2018 um 21:59 schrieb Boris Brezillon:
> > Hello,
> > 
> > This is the 3rd version of the mtd / sf fixes patchset. This v3
> > contains 2 new patches fixing bugs in the spi-flash layer.
> > 
> > With this version I hope to have addressed all the weird interactions
> > between the spi-flash and MTD layer (tested on a platform with a
> > spi-nor and a spi-nand)
> > 
> > Heiko, let me know if still issues with this version.  
> 
> Yup, works fine now for me!
> 
> > Thanks,  
> 
> Thanks!

Thanks for testing!

Boris