[PATCH] mtd: ofpart: drop undocumented, nonfunctional 'lock' property

[PATCH] mtd: ofpart: drop undocumented, nonfunctional 'lock' property

[Brian Norris]

The 'lock' property of a partition does nothing, because it effectively
only sets the flags for the partition device, not the master device. And
no MTD driver checks for MTD_POWERUP_LOCK in the partition device, only
the master device.

Michal noticed that this flag was undocumented, but rather than document
a non-functioning DT property, let's just kill it, since obviously no
one cared about it.

Note that this logic does not apply to all MTD flags (e.g., the
"read-only" flags above the "lock" one), since certain flags *do* get
checked in the partition, at least in some cases (particularly, when
checking file permissions in mtdchar.c, but notably *not* in the core
APIs like mtd_write()).

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Michal Suchanek <hramrach@gmail.com>
---
 drivers/mtd/ofpart.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
index aa26c32e1bc2..6bab6b5863e5 100644
--- a/drivers/mtd/ofpart.c
+++ b/drivers/mtd/ofpart.c
@@ -86,9 +86,6 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 		if (of_get_property(pp, "read-only", &len))
 			(*pparts)[i].mask_flags |= MTD_WRITEABLE;
 
-		if (of_get_property(pp, "lock", &len))
-			(*pparts)[i].mask_flags |= MTD_POWERUP_LOCK;
-
 		i++;
 	}
 
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH] mtd: ofpart: drop undocumented, nonfunctional 'lock' property

[Sascha Hauer]

Hi Brian,

On Mon, Oct 26, 2015 at 06:56:06PM -0700, Brian Norris wrote:
> The 'lock' property of a partition does nothing, because it effectively
> only sets the flags for the partition device, not the master device. And
> no MTD driver checks for MTD_POWERUP_LOCK in the partition device, only
> the master device.

This is not true. The flag gets checked in add_mtd_device:

	/* Some chips always power up locked. Unlock them now */
	if ((mtd->flags & MTD_WRITEABLE) && (mtd->flags & MTD_POWERUP_LOCK)) {
		error = mtd_unlock(mtd, 0, mtd->size);
		...
	}

add_mtd_device() is called either for the master mtd device or for each
partition (if there are partitions). The flow is like that:

- Some flash devices come up locked from power on. Drivers for these
  devices set the MTD_POWERUP_LOCK flag (cfi_cmdset_0001.c,
  cfi_cmdset_0002.c)
- The partitions inherit the flags from the master device, but not the
  ones set in mask_flags:

	ofpart.c:

	if (of_get_property(pp, "lock", &len))
		(*pparts)[i].mask_flags |= MTD_POWERUP_LOCK;

	mtdpart.c:

	slave->mtd.flags = master->flags & ~part->mask_flags;

- So if the lock property is not set then the MTD_POWERUP_LOCK flag is
  still set in add_mtd_device() for the partition and the corresponding
  partition is not unlocked. Otherwise the partition is unlocked.

So how I see it the mechanism is still functional, except that it's
broken when the recently introduced option MTD_PARTITIONED_MASTER is
set. When it's set then add_mtd_device() is called for the master device
aswell and the whole device gets unlocked. When the partitions are
registered later then it makes no difference whether they are unlocked
or not, they are already unlocked anyway.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] mtd: ofpart: drop undocumented, nonfunctional 'lock' property

[Brian Norris]

Hi Sascha,

On Tue, Oct 27, 2015 at 08:11:06AM +0100, Sascha Hauer wrote:
> On Mon, Oct 26, 2015 at 06:56:06PM -0700, Brian Norris wrote:
> > The 'lock' property of a partition does nothing, because it effectively
> > only sets the flags for the partition device, not the master device. And
> > no MTD driver checks for MTD_POWERUP_LOCK in the partition device, only
> > the master device.
> 
> This is not true. The flag gets checked in add_mtd_device:
> 
> 	/* Some chips always power up locked. Unlock them now */
> 	if ((mtd->flags & MTD_WRITEABLE) && (mtd->flags & MTD_POWERUP_LOCK)) {
> 		error = mtd_unlock(mtd, 0, mtd->size);
> 		...
> 	}
> 
> add_mtd_device() is called either for the master mtd device or for each
> partition (if there are partitions). The flow is like that:
> 
> - Some flash devices come up locked from power on. Drivers for these
>   devices set the MTD_POWERUP_LOCK flag (cfi_cmdset_0001.c,
>   cfi_cmdset_0002.c)
> - The partitions inherit the flags from the master device, but not the
>   ones set in mask_flags:
> 
> 	ofpart.c:
> 
> 	if (of_get_property(pp, "lock", &len))
> 		(*pparts)[i].mask_flags |= MTD_POWERUP_LOCK;
> 
> 	mtdpart.c:
> 
> 	slave->mtd.flags = master->flags & ~part->mask_flags;
> 
> - So if the lock property is not set then the MTD_POWERUP_LOCK flag is
>   still set in add_mtd_device() for the partition and the corresponding
>   partition is not unlocked. Otherwise the partition is unlocked.

Wow, you're absolutely correct, and I'm not sure how I overlooked that.
So I guess this patch [1] should be rewritten to be clearer, and the
$subject patch dropped.

> So how I see it the mechanism is still functional, except that it's
> broken when the recently introduced option MTD_PARTITIONED_MASTER is
> set. When it's set then add_mtd_device() is called for the master device
> aswell and the whole device gets unlocked. When the partitions are
> registered later then it makes no difference whether they are unlocked
> or not, they are already unlocked anyway.

It's also broken for overlapping partitions. Similarly to some previous
proposals for per-partition ECC handling for NAND, I think.

When first looking at this property (and not understanding how exactly
it worked), I was wondering why there's a per-partition property but not
a per-device property. It seems that if there were a whole device
property, then one could use 'lock' for the whole device (and then
provide partition(s) which do not have the 'lock' property) in order to
get this behavior on PARTITIONED_MASTER.

I guess if one is really interested in fixing 'lock' for
PARTITIONED_MASTER, and we don't want to introduce DT "ABI" changes, we
can just have any partition that has the 'lock' flag mask the
POWERUP_LOCK flag in its parent (master) device. That seems like a
reasonable request.

(It also still seems reasonable to support the 'lock' flag in the parent
device, in case someone is not using ofpart.)

Brian

[1] http://patchwork.ozlabs.org/patch/508356/

Re: [PATCH] mtd: ofpart: drop undocumented, nonfunctional 'lock' property

[Boris Brezillon]

Hi Brian,

On Tue, 27 Oct 2015 11:32:44 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi Sascha,
> 
> On Tue, Oct 27, 2015 at 08:11:06AM +0100, Sascha Hauer wrote:
> > On Mon, Oct 26, 2015 at 06:56:06PM -0700, Brian Norris wrote:
> > > The 'lock' property of a partition does nothing, because it effectively
> > > only sets the flags for the partition device, not the master device. And
> > > no MTD driver checks for MTD_POWERUP_LOCK in the partition device, only
> > > the master device.
> > 
> > This is not true. The flag gets checked in add_mtd_device:
> > 
> > 	/* Some chips always power up locked. Unlock them now */
> > 	if ((mtd->flags & MTD_WRITEABLE) && (mtd->flags & MTD_POWERUP_LOCK)) {
> > 		error = mtd_unlock(mtd, 0, mtd->size);
> > 		...
> > 	}
> > 
> > add_mtd_device() is called either for the master mtd device or for each
> > partition (if there are partitions). The flow is like that:
> > 
> > - Some flash devices come up locked from power on. Drivers for these
> >   devices set the MTD_POWERUP_LOCK flag (cfi_cmdset_0001.c,
> >   cfi_cmdset_0002.c)
> > - The partitions inherit the flags from the master device, but not the
> >   ones set in mask_flags:
> > 
> > 	ofpart.c:
> > 
> > 	if (of_get_property(pp, "lock", &len))
> > 		(*pparts)[i].mask_flags |= MTD_POWERUP_LOCK;
> > 
> > 	mtdpart.c:
> > 
> > 	slave->mtd.flags = master->flags & ~part->mask_flags;
> > 
> > - So if the lock property is not set then the MTD_POWERUP_LOCK flag is
> >   still set in add_mtd_device() for the partition and the corresponding
> >   partition is not unlocked. Otherwise the partition is unlocked.
> 
> Wow, you're absolutely correct, and I'm not sure how I overlooked that.
> So I guess this patch [1] should be rewritten to be clearer, and the
> $subject patch dropped.
> 
> > So how I see it the mechanism is still functional, except that it's
> > broken when the recently introduced option MTD_PARTITIONED_MASTER is
> > set. When it's set then add_mtd_device() is called for the master device
> > aswell and the whole device gets unlocked. When the partitions are
> > registered later then it makes no difference whether they are unlocked
> > or not, they are already unlocked anyway.
> 
> It's also broken for overlapping partitions. Similarly to some previous
> proposals for per-partition ECC handling for NAND, I think.

But do we really care about this case? I mean, if someone is stupid
enough to put different values for two overlapping partitions it
won't work correctly. The only thing we could do is complain loudly
about this mismatch.
For the per-partition proposal I posted a few weeks ago I assumed
overlapping partitions with different ECC configs were valid, which
means data will only be readable from one of the partitions: the one
who last wrote data on it (note that this is not true if you're using
raw access mode).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com