mtd/sf: Various fixes

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 <[email protected]>

---
Changes in v2:
- None

---
  drivers/mtd/mtdcore.c   | 16 +++++++++++++++-
  include/linux/mtd/mtd.h |  1 +
  2 files changed, 16 insertions(+), 1 deletion(-)

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 <[email protected]>

---
Changes in v3:
- None

Changes in v2:
- None

---
  drivers/mtd/mtd_uboot.c | 18 +++++++++++++++++-
  1 file changed, 17 insertions(+), 1 deletion(-)

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 <[email protected]>

---
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(+)

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 <[email protected]>

---
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(+)

drivers/mtd/mtdcore.c +0 -0

index 7a15ded8c883..46657fe7c949 100644

drivers/mtd/mtdcore.c +0 -0

 	mutex_lock(&mtd_table_mutex);
 
 	if (idr_find(&mtd_idr, mtd->index) != mtd) {

include/linux/mtd/mtd.h +15 -0

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

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 <[email protected]>

---
Changes in v3:
- None

Changes in v2
- None

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

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 <[email protected]>

---
Changes in v3:
- None

Changes in v2
- None
---

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 <[email protected]>

Reviewed-by Miquel Raynal <[email protected]>

---
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(-)

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 <[email protected]>

---
Changes in v3:
- None

Changes in v2:
- New patch

---
  drivers/mtd/mtd_uboot.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

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 <[email protected]>

---
Changes in v3:
- None

Changes in v2:
- New patch

---
  drivers/mtd/mtd_uboot.c | 11 ++++++++---
  1 file changed, 8 insertions(+), 3 deletions(-)

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 <[email protected]>

---
Changes in v3:
- None

Changes in v2:
- New patch

---
  drivers/mtd/mtd_uboot.c | 17 ++++++++++++-----
  1 file changed, 12 insertions(+), 5 deletions(-)

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 <[email protected]>

---
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(-)

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 <[email protected]>

---
Changes in v3:
- New patch

---
  drivers/mtd/spi/sf_probe.c | 9 +++++++++
  1 file changed, 9 insertions(+)

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 <[email protected]>

---

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 <[email protected]>

---
Changes in v3:
- New patch

---
  drivers/mtd/spi/sf_mtd.c | 39 ++++++++++++++++++++++++++++++++++++---
  1 file changed, 36 insertions(+), 3 deletions(-)

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 <[email protected]>

---
Changes in v3:
- New patch

---
 drivers/mtd/spi/sf_mtd.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

drivers/mtd/spi/sf_mtd.c +26 -3

index aabbc3589435..68c36002bee2 100644
@@ -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;
+       }

Quoted 93 lines · 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 <[email protected]>

---
Changes in v3:
- New patch

---
 drivers/mtd/spi/sf_mtd.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

drivers/mtd/spi/sf_mtd.c +30 -3

index aabbc3589435..68c36002bee2 100644
@@ -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

drivers/mtd/spi/sf_mtd.c int spi_flash_mtd_register(struct spi_flash *flash) · line 146

Quoted 6 lines

drivers/mtd/spi/sf_mtd.c +6 -0

+        * 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.

Quoted 11 lines

drivers/mtd/spi/sf_mtd.c int spi_flash_mtd_register(struct spi_flash *flash) · line 146

Quoted 9 lines

drivers/mtd/spi/sf_mtd.c +9 -0

+       /*
+        * 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).

Quoted 2 lines

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.

Hi Jagan,

Quoted 18 lines · On Thu, 22 Nov 2018 09:40:56 +0100 Boris Brezillon…

Quoted 11 lines

drivers/mtd/spi/sf_mtd.c int spi_flash_mtd_register(struct spi_flash *flash) · line 146

Quoted 9 lines

drivers/mtd/spi/sf_mtd.c +9 -0

+       /*
+        * 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).

Quoted 2 lines

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

Quoted 6 lines

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).

Quoted 30 lines · Boris Brezillon

Hi Jagan,

Quoted 24 lines · On Thu, 22 Nov 2018 09:40:56 +0100 Boris Brezillon…

Quoted 11 lines

drivers/mtd/spi/sf_mtd.c int spi_flash_mtd_register(struct spi_flash *flash) · line 146

Quoted 9 lines

drivers/mtd/spi/sf_mtd.c +9 -0

+       /*
+        * 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).

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 37 lines · On Mon, 26 Nov 2018 16:42:48 +0530 Jagan Teki <[email protected]> wrote:

Quoted 34 lines · Boris Brezillon

Hi Jagan,

Quoted 18 lines · On Thu, 22 Nov 2018 09:40:56 +0100 Boris Brezillon…

Quoted 11 lines

drivers/mtd/spi/sf_mtd.c int spi_flash_mtd_register(struct spi_flash *flash) · line 146

Quoted 9 lines

drivers/mtd/spi/sf_mtd.c +9 -0

+       /*
+        * 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).

Quoted 2 lines

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

Quoted 6 lines

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).

Quoted 2 lines

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

Quoted 40 lines · On Mon, 26 Nov 2018 13:37:46 +0100 Boris Brezillon…

Quoted 37 lines · On Mon, 26 Nov 2018 16:42:48 +0530 Jagan Teki <[email protected]> wrote:

Quoted 34 lines · Boris Brezillon

Hi Jagan,

Quoted 18 lines · On Thu, 22 Nov 2018 09:40:56 +0100 Boris Brezillon…

Quoted 11 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+       /*
+        * 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).

Quoted 2 lines

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

Quoted 6 lines

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).

Quoted 4 lines

Quoted 2 lines

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.

Quoted 48 lines

Quoted 38 lines · On Mon, 26 Nov 2018 13:37:46 +0100 Boris Brezillon…

Quoted 35 lines · On Mon, 26 Nov 2018 16:42:48 +0530 Jagan Teki <[email protected]> wrote:

Quoted 32 lines · Boris Brezillon

Hi Jagan,

On Thu, 22 Nov 2018 09:40:56 +0100
Boris Brezillon <[email protected]> wrote:

     >>>>>>> +       /*

Quoted 14 lines

Quoted 10 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+        * 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).

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

can't we do the same thing in MTD core

Quoted 8 lines

Quoted 1 line

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).

Quoted 4 lines

Quoted 2 lines

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 :-/.

Quoted 9 lines

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?

Hi Jagan,

Jagan Teki <[email protected]> wrote on Mon, 26 Nov 2018 18:35:01 +0530:

Quoted 59 lines · Boris Brezillon

Quoted 48 lines

Quoted 38 lines · On Mon, 26 Nov 2018 13:37:46 +0100 Boris Brezillon…

Quoted 35 lines · On Mon, 26 Nov 2018 16:42:48 +0530 Jagan Teki <[email protected]> wrote:

Quoted 32 lines · Boris Brezillon

Hi Jagan,

On Thu, 22 Nov 2018 09:40:56 +0100
Boris Brezillon <[email protected]> wrote:

     >>>>>>> +       /*  

Quoted 14 lines

Quoted 10 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+        * 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).

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

can't we do the same thing in MTD core

Quoted 8 lines

Quoted 1 line

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).

Quoted 4 lines

Quoted 2 lines

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 :-/.

Quoted 9 lines

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.

Quoted 1 line

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.

Quoted 66 lines · On Mon, 26 Nov 2018 14:25:44 +0100 Miquel Raynal <[email protected]>…

Hi Jagan,

Jagan Teki <[email protected]> wrote on Mon, 26 Nov 2018 18:35:01 +0530:

Quoted 59 lines · Boris Brezillon

Quoted 48 lines

Quoted 38 lines · On Mon, 26 Nov 2018 13:37:46 +0100 Boris Brezillon…

Quoted 35 lines · On Mon, 26 Nov 2018 16:42:48 +0530 Jagan Teki <[email protected]> wrote:

Quoted 32 lines · Boris Brezillon

Hi Jagan,

On Thu, 22 Nov 2018 09:40:56 +0100
Boris Brezillon <[email protected]> wrote:

     >>>>>>> +       /*

Quoted 14 lines

Quoted 10 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+        * 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).

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

can't we do the same thing in MTD core

Quoted 8 lines

Quoted 1 line

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).

Quoted 4 lines

Quoted 2 lines

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 :-/.

Quoted 9 lines

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.

Quoted 1 line

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?

Quoted 7 lines

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)?

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(-)

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,

Hello Boris,

Am 19.11.2018 um 21:59 schrieb Boris Brezillon:

Quoted 9 lines

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)

Quoted 1 line

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

Yup, works fine now for me!

Quoted 1 line

Thanks,

Thanks!