| Message ID | 20170522111704.52ce1c40@bbrezillon |
|---|---|
| State | Changes Requested |
| Delegated to: | Boris Brezillon |
| Headers | show |
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
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
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
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; }