[PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock
Honza Petrouš
jpetrous at gmail.com
Thu May 25 01:11:46 PDT 2017
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++) {
>> + 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>
>
> 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