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

[Honza Petrouš]

2017-05-17 9:25 GMT+02:00 Honza Petrouš <jpetrous at 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 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 = &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++) {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