Supporting flash that is locked by default

Supporting flash that is locked by default

[Todd Poynor]

Various flash chips may be locked (at a chip or block level) at power-on 
(perhaps depending on the board wiring), requiring an unlock before 
writing to the device.  It doesn't seem unreasonable to require one to 
run flash_unlock on appropriate partitions before writing to the device, 
at least in the case of initially mounting filesystems during system 
startup.  Platforms that power-cycle the device during a suspend/resume 
make this a little more complicated, since there's not really a standard 
way to insert the necessary flash_unlock commands into the system resume 
sequence, especially for a writeable root filesystem.   (For example, 
the Intel XScale PXA27x uses K3 StrataFlash, which powers up with all 
blocks locked and power-cycles the chip on suspend.)

This could all be handled inside the kernel, unlocking blocks as written 
to (if the block has not been explicitly locked).  But keeping track of 
which blocks are supposed to be unlockable is a messy business, and 
could fairly be accused of placing intelligence or policy in the kernel 
that is better left to userspace.  I don't, however, have an easy answer 
for how to handle the suspend/resume case.  The embedded engineers I've 
talked to felt this was a hardware detail the kernel should take care of 
for them, and it might not be that bad of an idea to have the kernel 
restore the flash to pre-suspend state by keeping track of 
previously-unlocked blocks.

So I thought I'd ask whether an architectural direction for linux-mtd 
has been set in this regard, or whether anybody has an opinion on this. 
  Thanks for any insight you can add,

-- 
Todd Poynor
MontaVista Software

RE: Supporting flash that is locked by default

[Christopher Hoover]

> Various flash chips may be locked (at a chip or block level) 
> at power-on 
> (perhaps depending on the board wiring), requiring an unlock before 
> writing to the device.

I raised this issue several years ago.  There wasn't (to me at least) a
satisfactory answer.  I carry a private patch forward from release to
release that performs the unlock operation in the mtd map code.  

Basically I do this which gets called in the probe:

static void __init xxxx_unlock_mtd(struct mtd_info *mtd)
{
	int i;

	/* argh, no way to get to the slaves that make up the parts of
	   our device, so instead cruise over all the available
	   devices */
	for (i = 0;; i++) {
		struct mtd_info *info;

		info = get_mtd_device(0, i);
		if (!info)
			break;

		if ((info->flags & MTD_WRITEABLE) &&
		    (info->unlock)) {
			int ret;
		
			printk(KERN_DEBUG "unlocking mtd device %s\n",
info->name);
			ret = info->unlock(info, 0, info->size);
			if (ret)
				printk(KERN_ERR "failed (%d) to unlock mtd
device %s\n", ret, info->name);
		}

		put_mtd_device(info);
	}
}               

Somone (either rmk or David -- I don't recall whom) didn't feel the unlock
belonged in the mapping code and didn't want the patch.   It would be nice
to come up with a solution that pleased everyone.

-ch

RE: Supporting flash that is locked by default

[David Woodhouse]

On Fri, 2004-09-24 at 19:18 -0700, Christopher Hoover wrote:
> Somone (either rmk or David -- I don't recall whom) didn't feel the unlock
> belonged in the mapping code and didn't want the patch.   It would be nice
> to come up with a solution that pleased everyone.

This locking scheme is daft. My original reaction was to hide it in the
mapping driver for such idiotic flashes, but they're more common now and
it really is a function of the flash not of the board. I suspect we
ought to add a quirk entry for these flashes, and automatically unlock
them on probe.

-- 
dwmw2

Re: Supporting flash that is locked by default

[Todd Poynor]

On Sat, Sep 25, 2004 at 11:33:36AM +0100, David Woodhouse wrote:
> This locking scheme is daft. My original reaction was to hide it in the
> mapping driver for such idiotic flashes, but they're more common now and
> it really is a function of the flash not of the board. I suspect we
> ought to add a quirk entry for these flashes, and automatically unlock
> them on probe.

Here's a try at automatically unlocking all blocks of Intel CFI flash at
probe time and at system resume time.  The new logic is in the chip
driver, but perhaps it would be better to move it generic, adding a new
chip driver callback for "is unlock needed?"; can rework things that way
if desired (was originally hoping to hide this "feature" from generic
code).

I did not notice a query feature bit that identifies whether the flash
has the "all blocks locked at power on" property in the datasheets, so
the patch checks whether block 0 is locked and assumes its one of those
flashes if so.  I've tried booting a board with K3 StrataFlash (which
powers up locked) and a board with J3 StrataFlash (which does not) and
it seems to do the right thing.

Since all blocks may be unlocked at resume time, it would be up to a
userspace action to relock any previously explicitly locked blocks soon
afterwards.  The chip driver (or generic layer) *could* keep track of
locked blocks and only unlock those not previously locked, can send a
patch for that if there's interest.

Comments?  Thanks. -- Todd

Index: drivers/mtd/chips/cfi_cmdset_0001.c
===================================================================
RCS file: /home/cvs/mtd/drivers/mtd/chips/cfi_cmdset_0001.c,v
retrieving revision 1.156
diff -u -r1.156 cfi_cmdset_0001.c
--- drivers/mtd/chips/cfi_cmdset_0001.c	17 Sep 2004 11:45:05 -0000	1.156
+++ drivers/mtd/chips/cfi_cmdset_0001.c	28 Sep 2004 22:56:49 -0000
@@ -45,6 +45,12 @@
 #define MANUFACTURER_ST         0x0020
 #define M50LPW080       0x002F
 
+/* 
+ * Block status register bits
+ */
+
+#define BSR_LOCK	0x1	/* Block lock status */
+
 static int cfi_intelext_read (struct mtd_info *, loff_t, size_t, size_t *, u_char *);
 //static int cfi_intelext_read_user_prot_reg (struct mtd_info *, loff_t, size_t, size_t *, u_char *);
 //static int cfi_intelext_read_fact_prot_reg (struct mtd_info *, loff_t, size_t, size_t *, u_char *);
@@ -299,6 +305,41 @@
 	return cfi_intelext_setup(mtd);
 }
 
+static int checklockstatus(struct map_info *map, struct flchip *chip,
+			   unsigned long adr, int len, void *thunk)
+{
+	struct cfi_private *cfi = map->fldrv_priv;
+	int ofs_factor = cfi->interleave * cfi->device_type;
+	int ret;
+
+	cfi_send_gen_cmd(0x90, 0x55, 0, map, cfi, cfi->device_type, NULL);
+	ret =  cfi_read_query(map, adr+(2*ofs_factor)) & BSR_LOCK;
+	chip->state = FL_JEDEC_QUERY;
+	return ret;
+}
+
+static void cfi_intelext_unlock_all(struct mtd_info *mtd)
+{
+	int i, locked;
+
+	locked = cfi_varsize_frob(mtd, checklockstatus,
+				  0, mtd->eraseregions[0].erasesize, 0);
+
+	if (locked != 1)
+		return;
+
+	printk(KERN_DEBUG "Unlocking locked CFI flash blocks.\n");
+	for (i = 0; i < mtd->numeraseregions; i++) {
+		int j;
+
+		for (j = 0; j < mtd->eraseregions[i].numblocks; j++){
+			cfi_intelext_unlock(mtd, mtd->eraseregions[i].offset +
+					    j * mtd->eraseregions[i].erasesize,
+					    mtd->eraseregions[i].erasesize);
+		}
+	}
+}
+
 static struct mtd_info *cfi_intelext_setup(struct mtd_info *mtd)
 {
 	struct map_info *map = mtd->priv;
@@ -358,6 +399,7 @@
 	if (cfi_intelext_partition_fixup(map, &cfi) != 0)
 		goto setup_err;
 
+	cfi_intelext_unlock_all(mtd);
 	__module_get(THIS_MODULE);
 	return mtd;
 
@@ -1756,6 +1798,8 @@
 
 		spin_unlock(chip->mutex);
 	}
+
+	cfi_intelext_unlock_all(mtd);
 }
 
 static void cfi_intelext_destroy(struct mtd_info *mtd)

Re: Supporting flash that is locked by default

[Nicolas Pitre]

On Tue, 28 Sep 2004, Todd Poynor wrote:

> On Sat, Sep 25, 2004 at 11:33:36AM +0100, David Woodhouse wrote:
> > This locking scheme is daft. My original reaction was to hide it in the
> > mapping driver for such idiotic flashes, but they're more common now and
> > it really is a function of the flash not of the board. I suspect we
> > ought to add a quirk entry for these flashes, and automatically unlock
> > them on probe.
> 
> Here's a try at automatically unlocking all blocks of Intel CFI flash at
> probe time and at system resume time.  The new logic is in the chip
> driver, but perhaps it would be better to move it generic, adding a new
> chip driver callback for "is unlock needed?"; can rework things that way
> if desired (was originally hoping to hide this "feature" from generic
> code).
> 
> I did not notice a query feature bit that identifies whether the flash
> has the "all blocks locked at power on" property in the datasheets, so
> the patch checks whether block 0 is locked and assumes its one of those
> flashes if so.

This is a rather bad idea since block 0 is often the bootloader and most 
likely to be locked on purpose.

I'm a bit unconfortable with a feature like this as well.  What if I 
want some partitions to remain locked at all times at the hw level?

I think the decision of unlocking the chip and what to unlock should 
remain in the map driver since this is where the knowledge of what the 
flash is meant to be used for is the most likely to be found.

> Since all blocks may be unlocked at resume time, it would be up to a
> userspace action to relock any previously explicitly locked blocks soon
> afterwards.

This sounds really really fishy to me.

If you can query which blocks are or are not locked before suspending, 
then why don't you just create a bitmap with the state of the entire 
flash and restore it upon resume?

> The chip driver (or generic layer) *could* keep track of
> locked blocks and only unlock those not previously locked, can send a
> patch for that if there's interest.

I think this is the only sane approach to this problem IMHO.


Nicolas

Re: Supporting flash that is locked by default

[Todd Poynor]

Nicolas Pitre wrote:
> On Tue, 28 Sep 2004, Todd Poynor wrote:
>>Here's a try at automatically unlocking all blocks of Intel CFI flash at
>>probe time and at system resume time.  The new logic is in the chip
>>driver, but perhaps it would be better to move it generic, adding a new
>>chip driver callback for "is unlock needed?"; can rework things that way
>>if desired (was originally hoping to hide this "feature" from generic
>>code).
>>
>>I did not notice a query feature bit that identifies whether the flash
>>has the "all blocks locked at power on" property in the datasheets, so
>>the patch checks whether block 0 is locked and assumes its one of those
>>flashes if so.
> 
> 
> This is a rather bad idea since block 0 is often the bootloader and most 
> likely to be locked on purpose.
> 
> I'm a bit unconfortable with a feature like this as well.  What if I 
> want some partitions to remain locked at all times at the hw level?

The intention was to catch the power-on behavior and normalize it to 
"all unlocked" like traditional flashes, then allow system-specific 
actions (or map partition software locking) to lock blocks, as is 
commonly done on a Rev. J flash, for example.  But it may be the case 
that some folks like the default-locked CFI behavior of these flashes 
and don't want the "all unlocked by default" common denominator, so 
agreed this should be rethought.

> I think the decision of unlocking the chip and what to unlock should 
> remain in the map driver since this is where the knowledge of what the 
> flash is meant to be used for is the most likely to be found.

Or require all the policy on this matter to reside in userspace, 
requiring an explicit flash_unlock for partitions, and not have the 
kernel automatically unlock anything.  If the map partition writeable 
flags are proof enough the partition should be automatically unlocked 
then sounds good to me, I'll send a patch.

>>Since all blocks may be unlocked at resume time, it would be up to a
>>userspace action to relock any previously explicitly locked blocks soon
>>afterwards.
> 
> 
> This sounds really really fishy to me.
> 
> If you can query which blocks are or are not locked before suspending, 
> then why don't you just create a bitmap with the state of the entire 
> flash and restore it upon resume?

I was planning what you describe for "phase 2" (the userspace actions to 
re-lock are actually the converse of how it works today, where actions 
are required to unlock), thought maybe adding this would be 
controversial.  I'll send a patch to do this.  Thanks,

-- 
Todd

Re: Supporting flash that is locked by default

[Todd Poynor]

Another stab at handling flashes that bootup/resume with blocks locked:
The partition driver intercepts an EROFS return from the chip driver,
unlocks blocks, and retries the write/erase.  If the flash is not
writeable per the partition map, or if the partition has been locked,
then leave locked blocks alone.  This avoids the need to detect whether
the flash has the property that blocks for a partition intended to be
writeable may be locked anyway, and avoids a loop to unlock all
appropriate blocks at init time, as well as the need to save/restore
block lock status via a bitmap at suspend/resume time.

In order to allow a writeable partition to be setup but still explicitly
locked via flash_lock, a partition lock flag is added that is
set/cleared by flash_lock/unlock.  Blocks in a partition locked via
flash_lock will not be automatically cleared.  flash_lock will thus
trigger software locking a la the MTD_WRITEABLE flag in maps.  This
might be a good thing, regardless of the merits of the previous
paragraph.

Comments?  Thanks -- Todd

Index: drivers/mtd/mtdpart.c
===================================================================
RCS file: /home/cvs/mtd/drivers/mtd/mtdpart.c,v
retrieving revision 1.50
diff -u -r1.50 mtdpart.c
--- drivers/mtd/mtdpart.c	10 Aug 2004 16:18:34 -0000	1.50
+++ drivers/mtd/mtdpart.c	8 Oct 2004 01:06:52 -0000
@@ -33,8 +33,11 @@
 	int index;
 	struct list_head list;
 	int registered;
+	int flags;
 };
 
+#define PART_LOCKED 1
+
 /*
  * Given a pointer to the MTD object in the mtd_part structure, we can retrieve
  * the pointer to that structure with this macro.
@@ -124,23 +127,40 @@
 					len, retlen, buf);
 }
 
+static int part_do_write (struct mtd_info *mtd, loff_t to, size_t len,
+			  size_t *retlen, const u_char *buf)
+{
+	struct mtd_part *part = PART(mtd);
+
+	if (part->master->write_ecc == NULL)	
+		return part->master->write (part->master, to + part->offset, 
+					    len, retlen, buf);
+	else
+		return part->master->write_ecc (part->master, to + part->offset,
+						len, retlen, buf, NULL,
+						&mtd->oobinfo);
+}
+
 static int part_write (struct mtd_info *mtd, loff_t to, size_t len,
 			size_t *retlen, const u_char *buf)
 {
 	struct mtd_part *part = PART(mtd);
-	if (!(mtd->flags & MTD_WRITEABLE))
+	int ret;
+	if (! (mtd->flags & MTD_WRITEABLE) || part->flags & PART_LOCKED)
 		return -EROFS;
 	if (to >= mtd->size)
 		len = 0;
 	else if (to + len > mtd->size)
 		len = mtd->size - to;
-	if (part->master->write_ecc == NULL)	
-		return part->master->write (part->master, to + part->offset, 
-					len, retlen, buf);
-	else
-		return part->master->write_ecc (part->master, to + part->offset, 
-					len, retlen, buf, NULL, &mtd->oobinfo);
-							
+
+	if (((ret = part_do_write (mtd, to, len, retlen, buf)) == -EROFS) &&
+	    part->master->unlock) {
+		loff_t baddr = (to + part->offset) & ~(mtd->erasesize - 1);
+		if (! part->master->unlock (part->master, baddr,
+					    mtd->erasesize))
+			ret = part_do_write (mtd, to, len, retlen, buf);
+	}
+	return ret;			
 }
 
 static int part_write_ecc (struct mtd_info *mtd, loff_t to, size_t len,
@@ -148,7 +168,8 @@
 			 u_char *eccbuf, struct nand_oobinfo *oobsel)
 {
 	struct mtd_part *part = PART(mtd);
-	if (!(mtd->flags & MTD_WRITEABLE))
+	int ret;
+	if (! (mtd->flags & MTD_WRITEABLE) || part->flags & PART_LOCKED)
 		return -EROFS;
 	if (oobsel == NULL)
 		oobsel = &mtd->oobinfo;
@@ -156,22 +177,44 @@
 		len = 0;
 	else if (to + len > mtd->size)
 		len = mtd->size - to;
-	return part->master->write_ecc (part->master, to + part->offset, 
-					len, retlen, buf, eccbuf, oobsel);
+	if (((ret = part->master->write_ecc (part->master, to + part->offset,
+					     len, retlen, buf, eccbuf, 
+					     oobsel)) == -EROFS) &&
+	    part->master->unlock) {
+		loff_t baddr = (to + part->offset) & ~(mtd->erasesize - 1);
+		if (! part->master->unlock (part->master, baddr,
+					    mtd->erasesize))
+			ret = part->master->write_ecc (part->master,
+						       to + part->offset,
+						       len, retlen, buf, 
+						       eccbuf, oobsel);
+	}
+	return ret;			
 }
 
 static int part_write_oob (struct mtd_info *mtd, loff_t to, size_t len,
 			size_t *retlen, const u_char *buf)
 {
 	struct mtd_part *part = PART(mtd);
-	if (!(mtd->flags & MTD_WRITEABLE))
+	int ret;
+	if (! (mtd->flags & MTD_WRITEABLE) || part->flags & PART_LOCKED)
 		return -EROFS;
 	if (to >= mtd->size)
 		len = 0;
 	else if (to + len > mtd->size)
 		len = mtd->size - to;
-	return part->master->write_oob (part->master, to + part->offset, 
-					len, retlen, buf);
+	if (((ret = part->master->write_oob (part->master, to + part->offset,
+					     len, retlen, buf)) == -EROFS) &&
+	    part->master->unlock) {
+		loff_t baddr = (to + part->offset) & ~(mtd->erasesize - 1);
+		if (! part->master->unlock (part->master, baddr,
+					    mtd->erasesize))
+			ret = part->master->write_oob (part->master, 
+						       to + part->offset, len,
+						       retlen, buf);
+	}
+
+	return ret;
 }
 
 static int part_write_user_prot_reg (struct mtd_info *mtd, loff_t from, size_t len, 
@@ -182,12 +225,10 @@
 					len, retlen, buf);
 }
 
-static int part_writev (struct mtd_info *mtd,  const struct kvec *vecs,
-			 unsigned long count, loff_t to, size_t *retlen)
+static int part_do_writev (struct mtd_info *mtd,  const struct kvec *vecs,
+			   unsigned long count, loff_t to, size_t *retlen)
 {
 	struct mtd_part *part = PART(mtd);
-	if (!(mtd->flags & MTD_WRITEABLE))
-		return -EROFS;
 	if (part->master->writev_ecc == NULL)	
 		return part->master->writev (part->master, vecs, count,
 					to + part->offset, retlen);
@@ -197,6 +238,24 @@
 					NULL, &mtd->oobinfo);
 }
 
+static int part_writev (struct mtd_info *mtd,  const struct kvec *vecs,
+			 unsigned long count, loff_t to, size_t *retlen)
+{
+	struct mtd_part *part = PART(mtd);
+	int ret;
+	if (! (mtd->flags & MTD_WRITEABLE) || part->flags & PART_LOCKED)
+		return -EROFS;
+
+	if (((ret = part_do_writev (mtd, vecs, count, to, retlen)) == -EROFS)
+	    && part->master->unlock) {
+		loff_t baddr = (to + part->offset) & ~(mtd->erasesize - 1);
+		if (! part->master->unlock (part->master, baddr,
+					    mtd->erasesize))
+			ret = part_do_writev (mtd, vecs, count, to, retlen);
+	}
+	return ret;			
+}
+
 static int part_readv (struct mtd_info *mtd,  struct kvec *vecs,
 			 unsigned long count, loff_t from, size_t *retlen)
 {
@@ -215,13 +274,25 @@
 			 u_char *eccbuf,  struct nand_oobinfo *oobsel)
 {
 	struct mtd_part *part = PART(mtd);
-	if (!(mtd->flags & MTD_WRITEABLE))
+	int ret;
+	if (! (mtd->flags & MTD_WRITEABLE) || part->flags & PART_LOCKED)
 		return -EROFS;
 	if (oobsel == NULL)
 		oobsel = &mtd->oobinfo;
-	return part->master->writev_ecc (part->master, vecs, count,
-					to + part->offset, retlen,
-					eccbuf, oobsel);
+	if (((ret = part->master->writev_ecc (part->master, vecs, count,
+					      to + part->offset, retlen,
+					      eccbuf, oobsel)) == -EROFS) &&
+	    part->master->unlock) {
+		loff_t baddr = (to + part->offset) & ~(mtd->erasesize - 1);
+		if (! part->master->unlock (part->master, baddr,
+					    mtd->erasesize))
+			ret = part->master->writev_ecc (part->master, vecs, 
+							count,
+							to + part->offset, 
+							retlen, eccbuf, oobsel);
+	}
+	return ret;			
+
 }
 
 static int part_readv_ecc (struct mtd_info *mtd,  struct kvec *vecs,
@@ -240,12 +311,17 @@
 {
 	struct mtd_part *part = PART(mtd);
 	int ret;
-	if (!(mtd->flags & MTD_WRITEABLE))
+	if (! (mtd->flags & MTD_WRITEABLE) || part->flags & PART_LOCKED)
 		return -EROFS;
 	if (instr->addr >= mtd->size)
 		return -EINVAL;
 	instr->addr += part->offset;
-	ret = part->master->erase(part->master, instr);
+	if ((ret = part->master->erase(part->master, instr) == -EROFS) &&
+	    part->master->unlock &&
+	    (! part->master->unlock (part->master, instr->addr,
+				     mtd->erasesize)))
+		ret = part->master->erase(part->master, instr);
+
 	return ret;
 }
 
@@ -268,6 +344,7 @@
 	struct mtd_part *part = PART(mtd);
 	if ((len + ofs) > mtd->size) 
 		return -EINVAL;
+	part->flags |= PART_LOCKED;
 	return part->master->lock(part->master, ofs + part->offset, len);
 }
 
@@ -276,6 +353,7 @@
 	struct mtd_part *part = PART(mtd);
 	if ((len + ofs) > mtd->size) 
 		return -EINVAL;
+	part->flags &= ~PART_LOCKED;
 	return part->master->unlock(part->master, ofs + part->offset, len);
 }
 
@@ -309,7 +387,7 @@
 static int part_block_markbad (struct mtd_info *mtd, loff_t ofs)
 {
 	struct mtd_part *part = PART(mtd);
-	if (!(mtd->flags & MTD_WRITEABLE))
+	if (! (mtd->flags & MTD_WRITEABLE) || part->flags & PART_LOCKED)
 		return -EROFS;
 	if (ofs >= mtd->size)
 		return -EINVAL;