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

[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++) {
+               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;
 }