mtd: ofpart: drop undocumented, nonfunctional 'lock' property

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

---
 drivers/mtd/ofpart.c | 3 ---
 1 file changed, 3 deletions(-)

index aa26c32e1bc2..6bab6b5863e5 100644
@@ -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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Brian,

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 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Sascha,

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.

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/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Brian,

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html