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

[Honza Petrouš]

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 = &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;
>  }

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