Re: [PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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 = &sect[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