* [PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock @ 2017-05-17 7:25 Honza Petrouš 2017-05-22 8:30 ` Honza Petrouš 2017-05-22 9:17 ` Boris Brezillon 0 siblings, 2 replies; 6+ messages in thread From: Honza Petrouš @ 2017-05-17 7:25 UTC (permalink / raw) To: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd The Persistent Protection Bits (PPB) locking of cfi_cmdset_0002.c doesn't support per-sector-unlocking, so any unlock request unlocks the whole chip. Because of that limitation the driver does the unlock in three steps: 1) remember all locked sector 2) do the whole chip unlock 3) lock back only the necessary sectors Unfortunately in step 2 (unlocking the chip) there is used cfi_varsize_frob() for per-sector unlock, what ends up in multiple chip unlocking calls (exactly the chip unlock is called for every sector in the unlock area) even the only one unlock per chip is enough. Signed-off-by: Honza Petrous <jpetrous@gmail.com> --- drivers/mtd/chips/cfi_cmdset_0002.c | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index 56aa6b7..53c842a 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -2534,8 +2534,10 @@ struct ppb_lock { struct flchip *chip; loff_t offset; int locked; + unsigned int erasesize; }; +#define MAX_CHIPS 16 #define MAX_SECTORS 512 #define DO_XXLOCK_ONEBLOCK_LOCK ((void *)1) @@ -2628,11 +2630,12 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, struct map_info *map = mtd->priv; struct cfi_private *cfi = map->fldrv_priv; struct ppb_lock *sect; + struct ppb_lock *chips; unsigned long adr; loff_t offset; uint64_t length; int chipnum; - int i; + int i, j; int sectors; int ret; @@ -2642,15 +2645,19 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, * first check the locking status of all sectors and save * it for future use. */ - sect = kzalloc(MAX_SECTORS * sizeof(struct ppb_lock), GFP_KERNEL); + sect = kzalloc((MAX_SECTORS + MAX_CHIPS) * sizeof(struct ppb_lock), + GFP_KERNEL); if (!sect) return -ENOMEM; + chips = §[MAX_SECTORS]; + /* * This code to walk all sectors is a slightly modified version * of the cfi_varsize_frob() code. */ i = 0; + j = -1; chipnum = 0; adr = 0; sectors = 0; @@ -2671,6 +2678,18 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, sect[sectors].locked = do_ppb_xxlock( map, &cfi->chips[chipnum], adr, 0, DO_XXLOCK_ONEBLOCK_GETLOCK); + } else { + if (j < 0 || chips[j].chip != &cfi->chips[chipnum]) { + j++; + if (j >= MAX_CHIPS) { + printk(KERN_ERR "Only %d chips for PPB locking supported!\n", + MAX_CHIPS); + kfree(sect); + return -EINVAL; + } + chips[j].chip = &cfi->chips[chipnum]; + chips[j].erasesize = size; + } } adr += size; @@ -2697,12 +2716,14 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, } } - /* Now unlock the whole chip */ - ret = cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len, - DO_XXLOCK_ONEBLOCK_UNLOCK); - if (ret) { - kfree(sect); - return ret; + /* Now unlock all involved chip(s) */ + for (i = 0; i <= j; i++) { + ret = do_ppb_xxlock(map, chips[i].chip, 0, chips[i].erasesize, + DO_XXLOCK_ONEBLOCK_UNLOCK); + if (ret) { + kfree(sect); + return ret; + } } /* -- 2.9.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock 2017-05-17 7:25 [PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock Honza Petrouš @ 2017-05-22 8:30 ` Honza Petrouš 2017-05-22 9:17 ` Boris Brezillon 1 sibling, 0 replies; 6+ messages in thread From: Honza Petrouš @ 2017-05-22 8:30 UTC (permalink / raw) To: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd 2017-05-17 9:25 GMT+02:00 Honza Petrouš <jpetrous@gmail.com>: > The Persistent Protection Bits (PPB) locking of cfi_cmdset_0002.c > doesn't support per-sector-unlocking, so any unlock request > unlocks the whole chip. Because of that limitation the driver > does the unlock in three steps: > 1) remember all locked sector > 2) do the whole chip unlock > 3) lock back only the necessary sectors > > Unfortunately in step 2 (unlocking the chip) there is used > cfi_varsize_frob() for per-sector unlock, what ends up > in multiple chip unlocking calls (exactly the chip unlock > is called for every sector in the unlock area) even the only one > unlock per chip is enough. > > Signed-off-by: Honza Petrous <jpetrous@gmail.com> > --- > drivers/mtd/chips/cfi_cmdset_0002.c | 37 +++++++++++++++++++++++++++++-------- > 1 file changed, 29 insertions(+), 8 deletions(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c > b/drivers/mtd/chips/cfi_cmdset_0002.c > index 56aa6b7..53c842a 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -2534,8 +2534,10 @@ struct ppb_lock { > struct flchip *chip; > loff_t offset; > int locked; > + unsigned int erasesize; > }; > > +#define MAX_CHIPS 16 > #define MAX_SECTORS 512 > > #define DO_XXLOCK_ONEBLOCK_LOCK ((void *)1) > @@ -2628,11 +2630,12 @@ static int __maybe_unused > cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > struct map_info *map = mtd->priv; > struct cfi_private *cfi = map->fldrv_priv; > struct ppb_lock *sect; > + struct ppb_lock *chips; > unsigned long adr; > loff_t offset; > uint64_t length; > int chipnum; > - int i; > + int i, j; > int sectors; > int ret; > > @@ -2642,15 +2645,19 @@ static int __maybe_unused > cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > * first check the locking status of all sectors and save > * it for future use. > */ > - sect = kzalloc(MAX_SECTORS * sizeof(struct ppb_lock), GFP_KERNEL); > + sect = kzalloc((MAX_SECTORS + MAX_CHIPS) * sizeof(struct ppb_lock), > + GFP_KERNEL); > if (!sect) > return -ENOMEM; > > + chips = §[MAX_SECTORS]; > + > /* > * This code to walk all sectors is a slightly modified version > * of the cfi_varsize_frob() code. > */ > i = 0; > + j = -1; > chipnum = 0; > adr = 0; > sectors = 0; > @@ -2671,6 +2678,18 @@ static int __maybe_unused cfi_ppb_unlock(struct > mtd_info *mtd, loff_t ofs, > sect[sectors].locked = do_ppb_xxlock( > map, &cfi->chips[chipnum], adr, 0, > DO_XXLOCK_ONEBLOCK_GETLOCK); > + } else { > + if (j < 0 || chips[j].chip != &cfi->chips[chipnum]) { > + j++; > + if (j >= MAX_CHIPS) { > + printk(KERN_ERR "Only %d chips for PPB locking > supported!\n", > + MAX_CHIPS); > + kfree(sect); > + return -EINVAL; > + } > + chips[j].chip = &cfi->chips[chipnum]; > + chips[j].erasesize = size; > + } > } > > adr += size; > @@ -2697,12 +2716,14 @@ static int __maybe_unused > cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > } > } > > - /* Now unlock the whole chip */ > - ret = cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len, > - DO_XXLOCK_ONEBLOCK_UNLOCK); > - if (ret) { > - kfree(sect); > - return ret; > + /* Now unlock all involved chip(s) */ > + for (i = 0; i <= j; i++) {UBI and UBIFS do not work on top of block devices > + ret = do_ppb_xxlock(map, chips[i].chip, 0, chips[i].erasesize, > + DO_XXLOCK_ONEBLOCK_UNLOCK); > + if (ret) { > + kfree(sect); > + return ret; > + } > } > > /* > -- > 2.9.3 Ping. No any volunteer for review? PS: Even it seems not to be so crucial for most cases, I still think it is not correct to do multiple unlocking if know that cmd0002 chips have support only for full chip unlock (so no per-sector unlock is possible). I have detected that incorrect behavior on Spansion S29GL01GS, which has really crazy unlocking timing (at least on our custom boards). /Honza ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock 2017-05-17 7:25 [PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock Honza Petrouš 2017-05-22 8:30 ` Honza Petrouš @ 2017-05-22 9:17 ` Boris Brezillon 2017-05-23 6:45 ` Honza Petrouš 1 sibling, 1 reply; 6+ messages in thread From: Boris Brezillon @ 2017-05-22 9:17 UTC (permalink / raw) To: Honza Petrouš Cc: David Woodhouse, Brian Norris, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd Hi Honza, On Wed, 17 May 2017 09:25:18 +0200 Honza Petrouš <jpetrous@gmail.com> wrote: > The Persistent Protection Bits (PPB) locking of cfi_cmdset_0002.c > doesn't support per-sector-unlocking, so any unlock request > unlocks the whole chip. Because of that limitation the driver > does the unlock in three steps: > 1) remember all locked sector > 2) do the whole chip unlock > 3) lock back only the necessary sectors > > Unfortunately in step 2 (unlocking the chip) there is used > cfi_varsize_frob() for per-sector unlock, what ends up > in multiple chip unlocking calls (exactly the chip unlock > is called for every sector in the unlock area) even the only one > unlock per chip is enough. > > Signed-off-by: Honza Petrous <jpetrous@gmail.com> > --- > drivers/mtd/chips/cfi_cmdset_0002.c | 37 +++++++++++++++++++++++++++++-------- > 1 file changed, 29 insertions(+), 8 deletions(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c > b/drivers/mtd/chips/cfi_cmdset_0002.c > index 56aa6b7..53c842a 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -2534,8 +2534,10 @@ struct ppb_lock { > struct flchip *chip; > loff_t offset; > int locked; > + unsigned int erasesize; > }; > > +#define MAX_CHIPS 16 > #define MAX_SECTORS 512 > > #define DO_XXLOCK_ONEBLOCK_LOCK ((void *)1) > @@ -2628,11 +2630,12 @@ static int __maybe_unused > cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > struct map_info *map = mtd->priv; > struct cfi_private *cfi = map->fldrv_priv; > struct ppb_lock *sect; > + struct ppb_lock *chips; > unsigned long adr; > loff_t offset; > uint64_t length; > int chipnum; > - int i; > + int i, j; > int sectors; > int ret; > > @@ -2642,15 +2645,19 @@ static int __maybe_unused > cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > * first check the locking status of all sectors and save > * it for future use. > */ > - sect = kzalloc(MAX_SECTORS * sizeof(struct ppb_lock), GFP_KERNEL); > + sect = kzalloc((MAX_SECTORS + MAX_CHIPS) * sizeof(struct ppb_lock), > + GFP_KERNEL); > if (!sect) > return -ENOMEM; > > + chips = §[MAX_SECTORS]; > + > /* > * This code to walk all sectors is a slightly modified version > * of the cfi_varsize_frob() code. > */ > i = 0; > + j = -1; > chipnum = 0; > adr = 0; > sectors = 0; > @@ -2671,6 +2678,18 @@ static int __maybe_unused cfi_ppb_unlock(struct > mtd_info *mtd, loff_t ofs, > sect[sectors].locked = do_ppb_xxlock( > map, &cfi->chips[chipnum], adr, 0, > DO_XXLOCK_ONEBLOCK_GETLOCK); > + } else { > + if (j < 0 || chips[j].chip != &cfi->chips[chipnum]) { > + j++; > + if (j >= MAX_CHIPS) { > + printk(KERN_ERR "Only %d chips for PPB locking > supported!\n", > + MAX_CHIPS); > + kfree(sect); > + return -EINVAL; > + } > + chips[j].chip = &cfi->chips[chipnum]; > + chips[j].erasesize = size; > + } > } > > adr += size; > @@ -2697,12 +2716,14 @@ static int __maybe_unused > cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > } > } > > - /* Now unlock the whole chip */ > - ret = cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len, > - DO_XXLOCK_ONEBLOCK_UNLOCK); > - if (ret) { > - kfree(sect); > - return ret; > + /* Now unlock all involved chip(s) */ > + for (i = 0; i <= j; i++) { > + ret = do_ppb_xxlock(map, chips[i].chip, 0, chips[i].erasesize, > + DO_XXLOCK_ONEBLOCK_UNLOCK); > + if (ret) { > + kfree(sect); > + return ret; > + } > } > > /* Hm, this logic looks over-complicated. How about the following changes? Would that work? And if it doesn't, can you detail why? --->8--- diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index 56aa6b75213d..370c063c3d4d 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -2698,11 +2698,13 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, } /* Now unlock the whole chip */ - ret = cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len, - DO_XXLOCK_ONEBLOCK_UNLOCK); - if (ret) { - kfree(sect); - return ret; + for (chipnum = 0; chipnum < cfi->numchips; chipnum++) { + ret = do_ppb_xxlock(map, &cfi->chips[chipnum], + (loff_t)chipnum << cfi->chipshift, + 1 << cfi->chipshift, + DO_XXLOCK_ONEBLOCK_UNLOCK); + if (ret) + goto out; } /* @@ -2715,6 +2717,7 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, DO_XXLOCK_ONEBLOCK_LOCK); } +out: kfree(sect); return ret; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock 2017-05-22 9:17 ` Boris Brezillon @ 2017-05-23 6:45 ` Honza Petrouš 2017-05-25 8:11 ` Honza Petrouš 0 siblings, 1 reply; 6+ messages in thread From: Honza Petrouš @ 2017-05-23 6:45 UTC (permalink / raw) To: Boris Brezillon Cc: David Woodhouse, Brian Norris, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd 2017-05-22 11:17 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > Hi Honza, > > On Wed, 17 May 2017 09:25:18 +0200 > Honza Petrouš <jpetrous@gmail.com> wrote: > >> The Persistent Protection Bits (PPB) locking of cfi_cmdset_0002.c >> doesn't support per-sector-unlocking, so any unlock request >> unlocks the whole chip. Because of that limitation the driver >> does the unlock in three steps: >> 1) remember all locked sector >> 2) do the whole chip unlock >> 3) lock back only the necessary sectors >> >> Unfortunately in step 2 (unlocking the chip) there is used >> cfi_varsize_frob() for per-sector unlock, what ends up >> in multiple chip unlocking calls (exactly the chip unlock >> is called for every sector in the unlock area) even the only one >> unlock per chip is enough. >> >> Signed-off-by: Honza Petrous <jpetrous@gmail.com> >> --- >> drivers/mtd/chips/cfi_cmdset_0002.c | 37 +++++++++++++++++++++++++++++-------- >> 1 file changed, 29 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c >> b/drivers/mtd/chips/cfi_cmdset_0002.c >> index 56aa6b7..53c842a 100644 >> --- a/drivers/mtd/chips/cfi_cmdset_0002.c >> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c >> @@ -2534,8 +2534,10 @@ struct ppb_lock { >> struct flchip *chip; >> loff_t offset; >> int locked; >> + unsigned int erasesize; >> }; >> >> +#define MAX_CHIPS 16 >> #define MAX_SECTORS 512 >> >> #define DO_XXLOCK_ONEBLOCK_LOCK ((void *)1) >> @@ -2628,11 +2630,12 @@ static int __maybe_unused >> cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, >> struct map_info *map = mtd->priv; >> struct cfi_private *cfi = map->fldrv_priv; >> struct ppb_lock *sect; >> + struct ppb_lock *chips; >> unsigned long adr; >> loff_t offset; >> uint64_t length; >> int chipnum; >> - int i; >> + int i, j; >> int sectors; >> int ret; >> >> @@ -2642,15 +2645,19 @@ static int __maybe_unused >> cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, >> * first check the locking status of all sectors and save >> * it for future use. >> */ >> - sect = kzalloc(MAX_SECTORS * sizeof(struct ppb_lock), GFP_KERNEL); >> + sect = kzalloc((MAX_SECTORS + MAX_CHIPS) * sizeof(struct ppb_lock), >> + GFP_KERNEL); >> if (!sect) >> return -ENOMEM; >> >> + chips = §[MAX_SECTORS]; >> + >> /* >> * This code to walk all sectors is a slightly modified version >> * of the cfi_varsize_frob() code. >> */ >> i = 0; >> + j = -1; >> chipnum = 0; >> adr = 0; >> sectors = 0; >> @@ -2671,6 +2678,18 @@ static int __maybe_unused cfi_ppb_unlock(struct >> mtd_info *mtd, loff_t ofs, >> sect[sectors].locked = do_ppb_xxlock( >> map, &cfi->chips[chipnum], adr, 0, >> DO_XXLOCK_ONEBLOCK_GETLOCK); >> + } else { >> + if (j < 0 || chips[j].chip != &cfi->chips[chipnum]) { >> + j++; >> + if (j >= MAX_CHIPS) { >> + printk(KERN_ERR "Only %d chips for PPB locking >> supported!\n", >> + MAX_CHIPS); >> + kfree(sect); >> + return -EINVAL; >> + } >> + chips[j].chip = &cfi->chips[chipnum]; >> + chips[j].erasesize = size; >> + } >> } >> >> adr += size; >> @@ -2697,12 +2716,14 @@ static int __maybe_unused >> cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, >> } >> } >> >> - /* Now unlock the whole chip */ >> - ret = cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len, >> - DO_XXLOCK_ONEBLOCK_UNLOCK); >> - if (ret) { >> - kfree(sect); >> - return ret; >> + /* Now unlock all involved chip(s) */ >> + for (i = 0; i <= j; i++) { >> + ret = do_ppb_xxlock(map, chips[i].chip, 0, chips[i].erasesize, >> + DO_XXLOCK_ONEBLOCK_UNLOCK); >> + if (ret) { >> + kfree(sect); >> + return ret; >> + } >> } >> >> /* > > Hm, this logic looks over-complicated. How about the following changes? > Would that work? And if it doesn't, can you detail why? > > --->8--- > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c > index 56aa6b75213d..370c063c3d4d 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -2698,11 +2698,13 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > } > > /* Now unlock the whole chip */ > - ret = cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len, > - DO_XXLOCK_ONEBLOCK_UNLOCK); > - if (ret) { > - kfree(sect); > - return ret; > + for (chipnum = 0; chipnum < cfi->numchips; chipnum++) { > + ret = do_ppb_xxlock(map, &cfi->chips[chipnum], > + (loff_t)chipnum << cfi->chipshift, > + 1 << cfi->chipshift, > + DO_XXLOCK_ONEBLOCK_UNLOCK); > + if (ret) > + goto out; > } > > /* > @@ -2715,6 +2717,7 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > DO_XXLOCK_ONEBLOCK_LOCK); > } > > +out: > kfree(sect); > return ret; > } Well, your fix should work (I'm going to verify it on our hw asap) and I agree it is much more simple :) But I found another use case, when it is not fully optimized - it not cover the multi-chip setting when the requested unlock area not involve all chips. In that case it execute few unneeded commands (both full chip unlock and every-sector re-lock) on chips which are out of requested area. Though, I can agree it is very minor use case, so might be not worth of caught it. /Honza ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock 2017-05-23 6:45 ` Honza Petrouš @ 2017-05-25 8:11 ` Honza Petrouš 2017-05-26 16:31 ` Boris Brezillon 0 siblings, 1 reply; 6+ messages in thread From: Honza Petrouš @ 2017-05-25 8:11 UTC (permalink / raw) To: Boris Brezillon Cc: David Woodhouse, Brian Norris, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd Hi Boris 2017-05-23 8:45 GMT+02:00 Honza Petrouš <jpetrous@gmail.com>: > 2017-05-22 11:17 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>: >> Hi Honza, >> >> On Wed, 17 May 2017 09:25:18 +0200 >> Honza Petrouš <jpetrous@gmail.com> wrote: >> >>> The Persistent Protection Bits (PPB) locking of cfi_cmdset_0002.c >>> doesn't support per-sector-unlocking, so any unlock request >>> unlocks the whole chip. Because of that limitation the driver >>> does the unlock in three steps: >>> 1) remember all locked sector >>> 2) do the whole chip unlock >>> 3) lock back only the necessary sectors >>> >>> Unfortunately in step 2 (unlocking the chip) there is used >>> cfi_varsize_frob() for per-sector unlock, what ends up >>> in multiple chip unlocking calls (exactly the chip unlock >>> is called for every sector in the unlock area) even the only one >>> unlock per chip is enough. >>> >>> Signed-off-by: Honza Petrous <jpetrous@gmail.com> >>> --- >>> drivers/mtd/chips/cfi_cmdset_0002.c | 37 +++++++++++++++++++++++++++++-------- >>> 1 file changed, 29 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c >>> b/drivers/mtd/chips/cfi_cmdset_0002.c >>> index 56aa6b7..53c842a 100644 >>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c >>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c >>> @@ -2534,8 +2534,10 @@ struct ppb_lock { >>> struct flchip *chip; >>> loff_t offset; >>> int locked; >>> + unsigned int erasesize; >>> }; >>> >>> +#define MAX_CHIPS 16 >>> #define MAX_SECTORS 512 >>> >>> #define DO_XXLOCK_ONEBLOCK_LOCK ((void *)1) >>> @@ -2628,11 +2630,12 @@ static int __maybe_unused >>> cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, >>> struct map_info *map = mtd->priv; >>> struct cfi_private *cfi = map->fldrv_priv; >>> struct ppb_lock *sect; >>> + struct ppb_lock *chips; >>> unsigned long adr; >>> loff_t offset; >>> uint64_t length; >>> int chipnum; >>> - int i; >>> + int i, j; >>> int sectors; >>> int ret; >>> >>> @@ -2642,15 +2645,19 @@ static int __maybe_unused >>> cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, >>> * first check the locking status of all sectors and save >>> * it for future use. >>> */ >>> - sect = kzalloc(MAX_SECTORS * sizeof(struct ppb_lock), GFP_KERNEL); >>> + sect = kzalloc((MAX_SECTORS + MAX_CHIPS) * sizeof(struct ppb_lock), >>> + GFP_KERNEL); >>> if (!sect) >>> return -ENOMEM; >>> >>> + chips = §[MAX_SECTORS]; >>> + >>> /* >>> * This code to walk all sectors is a slightly modified version >>> * of the cfi_varsize_frob() code. >>> */ >>> i = 0; >>> + j = -1; >>> chipnum = 0; >>> adr = 0; >>> sectors = 0; >>> @@ -2671,6 +2678,18 @@ static int __maybe_unused cfi_ppb_unlock(struct >>> mtd_info *mtd, loff_t ofs, >>> sect[sectors].locked = do_ppb_xxlock( >>> map, &cfi->chips[chipnum], adr, 0, >>> DO_XXLOCK_ONEBLOCK_GETLOCK); >>> + } else { >>> + if (j < 0 || chips[j].chip != &cfi->chips[chipnum]) { >>> + j++; >>> + if (j >= MAX_CHIPS) { >>> + printk(KERN_ERR "Only %d chips for PPB locking >>> supported!\n", >>> + MAX_CHIPS); >>> + kfree(sect); >>> + return -EINVAL; >>> + } >>> + chips[j].chip = &cfi->chips[chipnum]; >>> + chips[j].erasesize = size; >>> + } >>> } >>> >>> adr += size; >>> @@ -2697,12 +2716,14 @@ static int __maybe_unused >>> cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, >>> } >>> } >>> >>> - /* Now unlock the whole chip */ >>> - ret = cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len, >>> - DO_XXLOCK_ONEBLOCK_UNLOCK); >>> - if (ret) { >>> - kfree(sect); >>> - return ret; >>> + /* Now unlock all involved chip(s) */ >>> + for (i = 0; i <= j; i++) { >>> + ret = do_ppb_xxlock(map, chips[i].chip, 0, chips[i].erasesize, >>> + DO_XXLOCK_ONEBLOCK_UNLOCK); >>> + if (ret) { >>> + kfree(sect); >>> + return ret; >>> + } >>> } >>> >>> /* >> >> Hm, this logic looks over-complicated. How about the following changes? >> Would that work? And if it doesn't, can you detail why? >> >> --->8--- >> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c >> index 56aa6b75213d..370c063c3d4d 100644 >> --- a/drivers/mtd/chips/cfi_cmdset_0002.c >> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c >> @@ -2698,11 +2698,13 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, >> } >> >> /* Now unlock the whole chip */ >> - ret = cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len, >> - DO_XXLOCK_ONEBLOCK_UNLOCK); >> - if (ret) { >> - kfree(sect); >> - return ret; >> + for (chipnum = 0; chipnum < cfi->numchips; chipnum++) { >> + ret = do_ppb_xxlock(map, &cfi->chips[chipnum], >> + (loff_t)chipnum << cfi->chipshift, >> + 1 << cfi->chipshift, >> + DO_XXLOCK_ONEBLOCK_UNLOCK); >> + if (ret) >> + goto out; >> } >> >> /* >> @@ -2715,6 +2717,7 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, >> DO_XXLOCK_ONEBLOCK_LOCK); >> } >> >> +out: >> kfree(sect); >> return ret; >> } I just tested your fix and it works as expected. So you can add my: Tested-by: Honza Petrous <jpetrous@gmail.com> > > Well, your fix should work (I'm going to verify it on our hw asap) and I agree > it is much more simple :) > > But I found another use case, when it is not fully optimized > - it not cover the multi-chip setting when the requested unlock area > not involve all chips. In that case it execute few unneeded commands > (both full chip unlock and every-sector re-lock) on chips which > are out of requested area. > > Though, I can agree it is very minor use case, so might be not worth > of caught it. > > /Honza ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock 2017-05-25 8:11 ` Honza Petrouš @ 2017-05-26 16:31 ` Boris Brezillon 0 siblings, 0 replies; 6+ messages in thread From: Boris Brezillon @ 2017-05-26 16:31 UTC (permalink / raw) To: Honza Petrouš Cc: David Woodhouse, Brian Norris, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd Le Thu, 25 May 2017 10:11:46 +0200, Honza Petrouš <jpetrous@gmail.com> a écrit : > Hi Boris > > 2017-05-23 8:45 GMT+02:00 Honza Petrouš <jpetrous@gmail.com>: > > 2017-05-22 11:17 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > >> Hi Honza, > >> > >> On Wed, 17 May 2017 09:25:18 +0200 > >> Honza Petrouš <jpetrous@gmail.com> wrote: > >> > >>> The Persistent Protection Bits (PPB) locking of cfi_cmdset_0002.c > >>> doesn't support per-sector-unlocking, so any unlock request > >>> unlocks the whole chip. Because of that limitation the driver > >>> does the unlock in three steps: > >>> 1) remember all locked sector > >>> 2) do the whole chip unlock > >>> 3) lock back only the necessary sectors > >>> > >>> Unfortunately in step 2 (unlocking the chip) there is used > >>> cfi_varsize_frob() for per-sector unlock, what ends up > >>> in multiple chip unlocking calls (exactly the chip unlock > >>> is called for every sector in the unlock area) even the only one > >>> unlock per chip is enough. > >>> > >>> Signed-off-by: Honza Petrous <jpetrous@gmail.com> > >>> --- > >>> drivers/mtd/chips/cfi_cmdset_0002.c | 37 +++++++++++++++++++++++++++++-------- > >>> 1 file changed, 29 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c > >>> b/drivers/mtd/chips/cfi_cmdset_0002.c > >>> index 56aa6b7..53c842a 100644 > >>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c > >>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > >>> @@ -2534,8 +2534,10 @@ struct ppb_lock { > >>> struct flchip *chip; > >>> loff_t offset; > >>> int locked; > >>> + unsigned int erasesize; > >>> }; > >>> > >>> +#define MAX_CHIPS 16 > >>> #define MAX_SECTORS 512 > >>> > >>> #define DO_XXLOCK_ONEBLOCK_LOCK ((void *)1) > >>> @@ -2628,11 +2630,12 @@ static int __maybe_unused > >>> cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > >>> struct map_info *map = mtd->priv; > >>> struct cfi_private *cfi = map->fldrv_priv; > >>> struct ppb_lock *sect; > >>> + struct ppb_lock *chips; > >>> unsigned long adr; > >>> loff_t offset; > >>> uint64_t length; > >>> int chipnum; > >>> - int i; > >>> + int i, j; > >>> int sectors; > >>> int ret; > >>> > >>> @@ -2642,15 +2645,19 @@ static int __maybe_unused > >>> cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > >>> * first check the locking status of all sectors and save > >>> * it for future use. > >>> */ > >>> - sect = kzalloc(MAX_SECTORS * sizeof(struct ppb_lock), GFP_KERNEL); > >>> + sect = kzalloc((MAX_SECTORS + MAX_CHIPS) * sizeof(struct ppb_lock), > >>> + GFP_KERNEL); > >>> if (!sect) > >>> return -ENOMEM; > >>> > >>> + chips = §[MAX_SECTORS]; > >>> + > >>> /* > >>> * This code to walk all sectors is a slightly modified version > >>> * of the cfi_varsize_frob() code. > >>> */ > >>> i = 0; > >>> + j = -1; > >>> chipnum = 0; > >>> adr = 0; > >>> sectors = 0; > >>> @@ -2671,6 +2678,18 @@ static int __maybe_unused cfi_ppb_unlock(struct > >>> mtd_info *mtd, loff_t ofs, > >>> sect[sectors].locked = do_ppb_xxlock( > >>> map, &cfi->chips[chipnum], adr, 0, > >>> DO_XXLOCK_ONEBLOCK_GETLOCK); > >>> + } else { > >>> + if (j < 0 || chips[j].chip != &cfi->chips[chipnum]) { > >>> + j++; > >>> + if (j >= MAX_CHIPS) { > >>> + printk(KERN_ERR "Only %d chips for PPB locking > >>> supported!\n", > >>> + MAX_CHIPS); > >>> + kfree(sect); > >>> + return -EINVAL; > >>> + } > >>> + chips[j].chip = &cfi->chips[chipnum]; > >>> + chips[j].erasesize = size; > >>> + } > >>> } > >>> > >>> adr += size; > >>> @@ -2697,12 +2716,14 @@ static int __maybe_unused > >>> cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > >>> } > >>> } > >>> > >>> - /* Now unlock the whole chip */ > >>> - ret = cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len, > >>> - DO_XXLOCK_ONEBLOCK_UNLOCK); > >>> - if (ret) { > >>> - kfree(sect); > >>> - return ret; > >>> + /* Now unlock all involved chip(s) */ > >>> + for (i = 0; i <= j; i++) { > >>> + ret = do_ppb_xxlock(map, chips[i].chip, 0, chips[i].erasesize, > >>> + DO_XXLOCK_ONEBLOCK_UNLOCK); > >>> + if (ret) { > >>> + kfree(sect); > >>> + return ret; > >>> + } > >>> } > >>> > >>> /* > >> > >> Hm, this logic looks over-complicated. How about the following changes? > >> Would that work? And if it doesn't, can you detail why? > >> > >> --->8--- > >> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c > >> index 56aa6b75213d..370c063c3d4d 100644 > >> --- a/drivers/mtd/chips/cfi_cmdset_0002.c > >> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > >> @@ -2698,11 +2698,13 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > >> } > >> > >> /* Now unlock the whole chip */ > >> - ret = cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len, > >> - DO_XXLOCK_ONEBLOCK_UNLOCK); > >> - if (ret) { > >> - kfree(sect); > >> - return ret; > >> + for (chipnum = 0; chipnum < cfi->numchips; chipnum++) { Hm, I think I was wrong here. It should be: for (chipnum = ofs >> cfi->chipshift; chipnum <= (ofs + len - 1) >> cfi->chipshift; chipnum++) { > >> + ret = do_ppb_xxlock(map, &cfi->chips[chipnum], > >> + (loff_t)chipnum << cfi->chipshift, > >> + 1 << cfi->chipshift, > >> + DO_XXLOCK_ONEBLOCK_UNLOCK); > >> + if (ret) > >> + goto out; > >> } > >> > >> /* > >> @@ -2715,6 +2717,7 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > >> DO_XXLOCK_ONEBLOCK_LOCK); > >> } > >> > >> +out: > >> kfree(sect); > >> return ret; > >> } > > I just tested your fix and it works as expected. > > So you can add my: > > Tested-by: Honza Petrous <jpetrous@gmail.com> Hm, actually I was expecting you to send a v2 :-), I was just suggesting to do something simpler, that's all. > > > > > Well, your fix should work (I'm going to verify it on our hw asap) and I agree > > it is much more simple :) > > > > But I found another use case, when it is not fully optimized > > - it not cover the multi-chip setting when the requested unlock area > > not involve all chips. In that case it execute few unneeded commands > > (both full chip unlock and every-sector re-lock) on chips which > > are out of requested area. > > > > Though, I can agree it is very minor use case, so might be not worth > > of caught it. > > > > /Honza ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-26 16:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-17 7:25 [PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock Honza Petrouš 2017-05-22 8:30 ` Honza Petrouš 2017-05-22 9:17 ` Boris Brezillon 2017-05-23 6:45 ` Honza Petrouš 2017-05-25 8:11 ` Honza Petrouš 2017-05-26 16:31 ` Boris Brezillon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox