[for-4.14.y+] mtd: keep original flags for every struct mtd_info

Message ID 1550493012-5959-3-git-send-email-amit.pundir@linaro.org
State New
Headers show
Series
  • [for-4.14.y+] mtd: keep original flags for every struct mtd_info
Related show

Commit Message

Amit Pundir Feb. 18, 2019, 12:30 p.m.
From: Rafał Miłecki <rafal@milecki.pl>


commit 1186af457cc186c5ed01708da71b1ffbdf0a2638 upstream.

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 <rafal@milecki.pl>

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Signed-off-by: Amit Pundir <amit.pundir@linaro.org>

---
I understand that it doesn't exactly fit into Stable rules but I didn't
want to assume anything, so sending it up for review.
Cherry-picked from lede tree https://git.lede-project.org/?p=source.git
and build tested on v4.14.97 and v4.19.19 for ARCH=arm/arm64 defconfig.

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

-- 
2.7.4

Comments

Rafał Miłecki Feb. 18, 2019, 1:01 p.m. | #1
On 2019-02-18 13:30, Amit Pundir wrote:
> From: Rafał Miłecki <rafal@milecki.pl>

> 

> commit 1186af457cc186c5ed01708da71b1ffbdf0a2638 upstream.

> 

> 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 <rafal@milecki.pl>

> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

> Signed-off-by: Amit Pundir <amit.pundir@linaro.org>

> ---

> I understand that it doesn't exactly fit into Stable rules but I didn't

> want to assume anything, so sending it up for review.

> Cherry-picked from lede tree https://git.lede-project.org/?p=source.git

> and build tested on v4.14.97 and v4.19.19 for ARCH=arm/arm64 defconfig.


This was a pre-requirement for some later-added *feature*. I see to
reason to pick this one for stable. We won't be picking that feature
(whatever it was), so what's the point?
Amit Pundir Feb. 18, 2019, 1:38 p.m. | #2
On Mon, 18 Feb 2019 at 18:31, Rafał Miłecki <rafal@milecki.pl> wrote:
>

> On 2019-02-18 13:30, Amit Pundir wrote:

> > From: Rafał Miłecki <rafal@milecki.pl>

> >

> > commit 1186af457cc186c5ed01708da71b1ffbdf0a2638 upstream.

> >

> > 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 <rafal@milecki.pl>

> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

> > Signed-off-by: Amit Pundir <amit.pundir@linaro.org>

> > ---

> > I understand that it doesn't exactly fit into Stable rules but I didn't

> > want to assume anything, so sending it up for review.

> > Cherry-picked from lede tree https://git.lede-project.org/?p=source.git

> > and build tested on v4.14.97 and v4.19.19 for ARCH=arm/arm64 defconfig.

>

> This was a pre-requirement for some later-added *feature*. I see to

> reason to pick this one for stable. We won't be picking that feature

> (whatever it was), so what's the point?


Thank you for the review. I knew I was missing something. I sent it
out of curiosity based on commit log, which talks about re-running all
checks as a fix for internal tests likely dropping write access to new
partition. And I didn't read it is a pre-requisite for later-added
feature. I'm sure lede/openwrt would have picked/backported the whole
feature, but as you mentioned it doesn't make sense for Stable. Thank
you again for the feedback.

Regards,
Amit Pundir

Patch

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 97ac219c082e..2d24701255e5 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -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;
 }
 
 /**
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 99c460facd5e..2b6e53af47da 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -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;
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index cd0be91bdefa..b491a08e87e5 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -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