Supporting flash that is locked by default
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,
Signature · 2 lines
Todd Poynor MontaVista Software
Quoted 4 lines
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
Quoted 3 lines · Christopher Hoover
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.
Signature · 1 line
dwmw2
Quoted 5 lines · David Woodhouse
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
mtd/chips/cfi_cmdset_0001.c +44 -0
@@ -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)
Quoted 14 lines · Todd Poynor
Quoted 5 lines · David Woodhouse
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).
Quoted 4 lines
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.
Quoted 3 lines
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?
Quoted 3 lines
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
Quoted 14 lines · Nicolas Pitre
Quoted 11 lines · Todd Poynor
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.
Quoted 3 lines
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.
Quoted 1 line
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.
Quoted 8 lines
Quoted 3 lines
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,
Signature · 1 line
Todd
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
mtd/mtdpart.c +103 -25
@@ -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 ;