[PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock
Boris Brezillon
boris.brezillon at free-electrons.com
Fri May 26 09:31:26 PDT 2017
Le Thu, 25 May 2017 10:11:46 +0200,
Honza Petrouš <jpetrous at gmail.com> a écrit :
> Hi Boris
>
> 2017-05-23 8:45 GMT+02:00 Honza Petrouš <jpetrous at gmail.com>:
> > 2017-05-22 11:17 GMT+02:00 Boris Brezillon <boris.brezillon at free-electrons.com>:
> >> Hi Honza,
> >>
> >> On Wed, 17 May 2017 09:25:18 +0200
> >> Honza Petrouš <jpetrous at 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 at 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 at 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
More information about the linux-mtd
mailing list