mtd: keep original flags for every struct mtd_info

PATCH Applied Rafał Miłecki 3 participants 5 messages Nov 20, 2018 – Dec 3, 2018 Memory technology devices (MTD)

lore

Share & tools

lore

Rafał Miłecki

author

2018-11-20 08:57 UTC #0 lore raw

PATCH 1/1 mtd: keep original flags for every struct mtd_info

From: Rafał Miłecki <[email protected]>

When allocating a new partition mtd subsystem runs internal tests in the allocate_partition(). They may result in modifying specified flags (e.g. dropping some /features/ like write access).

Those constraints don't have to be necessary true for subpartitions. It may happen parent partition isn't block aligned (effectively disabling write access) while subpartition may fit blocks nicely. In such case all checks should be run again (starting with original flags value).

Signed-off-by Rafał Miłecki <[email protected]>

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

drivers/mtd/mtdcore.c +2 -0

index 97ac219c082e..2d24701255e5 100644
@@ -665,6 +665,8 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
 	} else {
 		pr_debug("mtd device won't show a device symlink in sysfs\n");
 	}
+
+	mtd->orig_flags = mtd->flags;
 }
 
 /**

drivers/mtd/mtdpart.c +2 -1

index 99c460facd5e..2b6e53af47da 100644
@@ -346,7 +346,8 @@ static struct mtd_part *allocate_partition(struct mtd_info *parent,
 
 	/* set up the MTD object for this partition */
 	slave->mtd.type = parent->type;
-	slave->mtd.flags = parent->flags & ~part->mask_flags;
+	slave->mtd.flags = parent->orig_flags & ~part->mask_flags;
+	slave->mtd.orig_flags = slave->mtd.flags;
 	slave->mtd.size = part->size;
 	slave->mtd.writesize = parent->writesize;
 	slave->mtd.writebufsize = parent->writebufsize;

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

index cd0be91bdefa..b491a08e87e5 100644
@@ -207,6 +207,7 @@ struct mtd_debug_info {
 struct mtd_info {
 	u_char type;
 	uint32_t flags;
+	uint32_t orig_flags; /* Flags as before running mtd checks */
 	uint64_t size;	 // Total size of the MTD
 
 	/* "Major" erase size for the device. Naïve users may take this

Signature · 1 line

2.13.7

Boris Brezillon

2018-11-20 09:13 UTC #1 lore raw

Quoted 50 lines · On Tue, 20 Nov 2018 09:55:45 +0100 Rafał Miłecki <[email protected]> wrote:

From: Rafał Miłecki <[email protected]>
When allocating a new partition mtd subsystem runs internal tests in the allocate_partition(). They may result in modifying specified flags (e.g. dropping some /features/ like write access).
Those constraints don't have to be necessary true for subpartitions. It may happen parent partition isn't block aligned (effectively disabling write access) while subpartition may fit blocks nicely. In such case all checks should be run again (starting with original flags value).
Signed-off-by Rafał Miłecki <[email protected]>
---
 drivers/mtd/mtdcore.c   | 2 ++
 drivers/mtd/mtdpart.c   | 3 ++-
 include/linux/mtd/mtd.h | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)
drivers/mtd/mtdcore.c +2 -0
index 97ac219c082e..2d24701255e5 100644
@@ -665,6 +665,8 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
 	} else {
 		pr_debug("mtd device won't show a device symlink in sysfs\n");
 	}
+
+	mtd->orig_flags = mtd->flags;
 }
 
 /**
drivers/mtd/mtdpart.c +2 -1
index 99c460facd5e..2b6e53af47da 100644
@@ -346,7 +346,8 @@ static struct mtd_part *allocate_partition(struct mtd_info *parent,
 
 	/* set up the MTD object for this partition */
 	slave->mtd.type = parent->type;
-	slave->mtd.flags = parent->flags & ~part->mask_flags;
+	slave->mtd.flags = parent->orig_flags & ~part->mask_flags;
+	slave->mtd.orig_flags = slave->mtd.flags;
 	slave->mtd.size = part->size;
 	slave->mtd.writesize = parent->writesize;
 	slave->mtd.writebufsize = parent->writebufsize;
include/linux/mtd/mtd.h +0 -0
index cd0be91bdefa..b491a08e87e5 100644

include/linux/mtd/mtd.h struct mtd_debug_info { · line 210

Quoted 4 lines

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

@@ -207,6 +207,7 @@ struct mtd_debug_info {
 struct mtd_info {
 	u_char type;
 	uint32_t flags;
+	uint32_t orig_flags; /* Flags as before running mtd checks */

Hm, I'm not a big fan of this orig_flags field. I'd prefer to have Miquel's patch series [1] ported to Linus, so that master can be retrieved with a simple loop:

struct mtd_info *mtd_get_master(struct mtd_info *mtd)

{
	struct mtd_info *master = mtd;

	while (master->parent)
		master = master->parent;
}

[1]https://www.mail-archive.com/[email protected]/msg297465.html

Rafał Miłecki

2018-11-20 09:44 UTC #2 lore raw

Quoted 65 lines · Boris Brezillon

Quoted 52 lines · On Tue, 20 Nov 2018 09:55:45 +0100 Rafał Miłecki <[email protected]> wrote:
From: Rafał Miłecki <[email protected]>
When allocating a new partition mtd subsystem runs internal tests in the allocate_partition(). They may result in modifying specified flags (e.g. dropping some /features/ like write access).
Those constraints don't have to be necessary true for subpartitions. It may happen parent partition isn't block aligned (effectively disabling write access) while subpartition may fit blocks nicely. In such case all checks should be run again (starting with original flags value).
Signed-off-by Rafał Miłecki <[email protected]>
---
 drivers/mtd/mtdcore.c   | 2 ++
 drivers/mtd/mtdpart.c   | 3 ++-
 include/linux/mtd/mtd.h | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)
drivers/mtd/mtdcore.c +2 -0
index 97ac219c082e..2d24701255e5 100644
@@ -665,6 +665,8 @@ static void mtd_set_dev_defaults(struct mtd_info 
 *mtd)
 	} else {
 		pr_debug("mtd device won't show a device symlink in sysfs\n");
 	}
+
+	mtd->orig_flags = mtd->flags;
 }
 
/**
drivers/mtd/mtdpart.c +2 -1
index 99c460facd5e..2b6e53af47da 100644
@@ -346,7 +346,8 @@ static struct mtd_part *allocate_partition(struct 
 mtd_info *parent,
 
 	/* set up the MTD object for this partition */
 	slave->mtd.type = parent->type;
-	slave->mtd.flags = parent->flags & ~part->mask_flags;
+	slave->mtd.flags = parent->orig_flags & ~part->mask_flags;
+	slave->mtd.orig_flags = slave->mtd.flags;
 	slave->mtd.size = part->size;
 	slave->mtd.writesize = parent->writesize;
drivers/mtd/mtdpart.c +0 -0
 	slave->mtd.writebufsize = parent->writebufsize;
include/linux/mtd/mtd.h +0 -0
index cd0be91bdefa..b491a08e87e5 100644
include/linux/mtd/mtd.h struct mtd_debug_info { · line 210
Quoted 4 lines
include/linux/mtd/mtd.h +1 -0
@@ -207,6 +207,7 @@ struct mtd_debug_info {
 struct mtd_info {
 	u_char type;
 	uint32_t flags;
+	uint32_t orig_flags; /* Flags as before running mtd checks */
Hm, I'm not a big fan of this orig_flags field. I'd prefer to have Miquel's patch series [1] ported to Linus, so that master can be retrieved with a simple loop:
struct mtd_info *mtd_get_master(struct mtd_info *mtd)

Quoted 8 lines

{
	struct mtd_info *master = mtd;

	while (master->parent)
		master = master->parent;
}
[1]https://www.mail-archive.com/[email protected]/msg297465.html

Retrieving master is not a big deal, but I don't think it's going to help. Just because master is writeable doesn't mean every subpartition should be.

Let me try to provide some example covering both cases.

. "spi0.0" orig_flags:MTD_WRITEABLE flags:MTD_WRITEABLE

├── "boot" orig_flags:0 flags:0
├── "firmware" orig_flags:MTD_WRITEABLE flags:0 (misaligned partition)
    ├── "header" flags: ? (misaligned partition)
    ├── "rootfs" flags: ? (aligned partition)
    └── "kernel" flags: ? (aligned partition)
└── "calibration" flags:0
    ├── "wifi0" flags: ?
    └── "wifi1" flags: ?

My patch is intended to fix "rootfs" and "kernel" not having MTD_WRITEABLE flag. The reason for lacking flag is mtd subsystem (correctly) dropping it in the allocate_partition() due to misalignment.

If we start checking flags of the master however that will result in "wifi0" and "wifi1" receiving MTD_WRITEABLE. The intention of "calibration" was to make all calibration data (unit specific thing) RO only.

I don't have any better idea than "orig_flags" that would help solving this problem :(

Boris Brezillon

2018-11-20 10:08 UTC #3 lore raw

Quoted 95 lines · On Tue, 20 Nov 2018 10:43:38 +0100 Rafał Miłecki <[email protected]> wrote:

Quoted 69 lines · Boris Brezillon
Quoted 56 lines · On Tue, 20 Nov 2018 09:55:45 +0100 Rafał Miłecki <[email protected]> wrote:
From: Rafał Miłecki <[email protected]>
When allocating a new partition mtd subsystem runs internal tests in the
allocate_partition(). They may result in modifying specified flags (e.g.
dropping some /features/ like write access).
Those constraints don't have to be necessary true for subpartitions. It
may happen parent partition isn't block aligned (effectively disabling write access) while subpartition may fit blocks nicely. In such case all
checks should be run again (starting with original flags value).
Signed-off-by Rafał Miłecki <[email protected]>
---
 drivers/mtd/mtdcore.c   | 2 ++
 drivers/mtd/mtdpart.c   | 3 ++-
 include/linux/mtd/mtd.h | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)
drivers/mtd/mtdcore.c +2 -0
index 97ac219c082e..2d24701255e5 100644
@@ -665,6 +665,8 @@ static void mtd_set_dev_defaults(struct mtd_info 
 *mtd)
 	} else {
 		pr_debug("mtd device won't show a device symlink in sysfs\n");
 	}
+
+	mtd->orig_flags = mtd->flags;
 }
 
/**
drivers/mtd/mtdpart.c +2 -1
index 99c460facd5e..2b6e53af47da 100644
@@ -346,7 +346,8 @@ static struct mtd_part *allocate_partition(struct 
 mtd_info *parent,
 
 	/* set up the MTD object for this partition */
 	slave->mtd.type = parent->type;
-	slave->mtd.flags = parent->flags & ~part->mask_flags;
+	slave->mtd.flags = parent->orig_flags & ~part->mask_flags;
+	slave->mtd.orig_flags = slave->mtd.flags;
 	slave->mtd.size = part->size;
 	slave->mtd.writesize = parent->writesize;
drivers/mtd/mtdpart.c +0 -0
 	slave->mtd.writebufsize = parent->writebufsize;
include/linux/mtd/mtd.h +0 -0
index cd0be91bdefa..b491a08e87e5 100644
include/linux/mtd/mtd.h struct mtd_debug_info { · line 210
Quoted 4 lines
include/linux/mtd/mtd.h +1 -0
@@ -207,6 +207,7 @@ struct mtd_debug_info {
 struct mtd_info {
 	u_char type;
 	uint32_t flags;
+	uint32_t orig_flags; /* Flags as before running mtd checks */  
Hm, I'm not a big fan of this orig_flags field. I'd prefer to have Miquel's patch series [1] ported to Linus, so that master can be retrieved with a simple loop:
struct mtd_info *mtd_get_master(struct mtd_info *mtd)
Quoted 8 lines
{
	struct mtd_info *master = mtd;

	while (master->parent)
		master = master->parent;
}
[1]https://www.mail-archive.com/[email protected]/msg297465.html
Retrieving master is not a big deal, but I don't think it's going to help. Just because master is writeable doesn't mean every subpartition should be.
Let me try to provide some example covering both cases.
. "spi0.0" orig_flags:MTD_WRITEABLE flags:MTD_WRITEABLE
├── "boot" orig_flags:0 flags:0
├── "firmware" orig_flags:MTD_WRITEABLE flags:0 (misaligned partition)
     ├── "header" flags: ? (misaligned partition)
     ├── "rootfs" flags: ? (aligned partition)
     └── "kernel" flags: ? (aligned partition)
└── "calibration" flags:0
     ├── "wifi0" flags: ?
     └── "wifi1" flags: ?

Quoted 8 lines

My patch is intended to fix "rootfs" and "kernel" not having MTD_WRITEABLE flag. The reason for lacking flag is mtd subsystem (correctly) dropping it in the allocate_partition() due to misalignment.
If we start checking flags of the master however that will result in "wifi0" and "wifi1" receiving MTD_WRITEABLE. The intention of "calibration" was to make all calibration data (unit specific thing) RO only.

I see.

Quoted 3 lines

I don't have any better idea than "orig_flags" that would help solving this problem :(

Me neither. I just misunderstood what you were trying to do.

Boris Brezillon

Applied

2018-12-03 07:53 UTC #4 lore raw

In reply to Rafał Miłecki

Quoted 6 lines · On Tue, 20 Nov 2018 09:55:45 +0100 Rafał Miłecki <[email protected]> wrote:

From: Rafał Miłecki <[email protected]>
When allocating a new partition mtd subsystem runs internal tests in the allocate_partition(). They may result in modifying specified flags (e.g. dropping some /features/ like write access).

Quoted 6 lines

Those constraints don't have to be necessary true for subpartitions. It may happen parent partition isn't block aligned (effectively disabling write access) while subpartition may fit blocks nicely. In such case all checks should be run again (starting with original flags value).
Signed-off-by Rafał Miłecki <[email protected]>

Queued to mtd/next.

Thanks,

Boris

Quoted 45 lines

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

drivers/mtd/mtdcore.c +2 -0

index 97ac219c082e..2d24701255e5 100644
@@ -665,6 +665,8 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
 	} else {
 		pr_debug("mtd device won't show a device symlink in sysfs\n");
 	}
+
+	mtd->orig_flags = mtd->flags;
 }
 
 /**

drivers/mtd/mtdpart.c +2 -1

index 99c460facd5e..2b6e53af47da 100644
@@ -346,7 +346,8 @@ static struct mtd_part *allocate_partition(struct mtd_info *parent,
 
 	/* set up the MTD object for this partition */
 	slave->mtd.type = parent->type;
-	slave->mtd.flags = parent->flags & ~part->mask_flags;
+	slave->mtd.flags = parent->orig_flags & ~part->mask_flags;
+	slave->mtd.orig_flags = slave->mtd.flags;
 	slave->mtd.size = part->size;
 	slave->mtd.writesize = parent->writesize;
 	slave->mtd.writebufsize = parent->writebufsize;

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

index cd0be91bdefa..b491a08e87e5 100644
@@ -207,6 +207,7 @@ struct mtd_debug_info {
 struct mtd_info {
 	u_char type;
 	uint32_t flags;
+	uint32_t orig_flags; /* Flags as before running mtd checks */
 	uint64_t size;	 // Total size of the MTD
 
 	/* "Major" erase size for the device. Naïve users may take this