From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.free-electrons.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dCjT5-000746-Nw for linux-mtd@lists.infradead.org; Mon, 22 May 2017 09:17:37 +0000 Date: Mon, 22 May 2017 11:17:04 +0200 From: Boris Brezillon To: Honza =?UTF-8?B?UGV0cm91xaE=?= Cc: David Woodhouse , Brian Norris , Marek Vasut , Richard Weinberger , Cyrille Pitchen , linux-mtd@lists.infradead.org Subject: Re: [PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock Message-ID: <20170522111704.52ce1c40@bbrezillon> In-Reply-To: References: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Honza, On Wed, 17 May 2017 09:25:18 +0200 Honza Petrou=C5=A1 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 >=20 > 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. >=20 > Signed-off-by: Honza Petrous > --- > drivers/mtd/chips/cfi_cmdset_0002.c | 37 +++++++++++++++++++++++++++++--= ------ > 1 file changed, 29 insertions(+), 8 deletions(-) >=20 > 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; > }; >=20 > +#define MAX_CHIPS 16 > #define MAX_SECTORS 512 >=20 > #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 =3D mtd->priv; > struct cfi_private *cfi =3D 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; >=20 > @@ -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 =3D kzalloc(MAX_SECTORS * sizeof(struct ppb_lock), GFP_KERNEL); > + sect =3D kzalloc((MAX_SECTORS + MAX_CHIPS) * sizeof(struct ppb_lock), > + GFP_KERNEL); > if (!sect) > return -ENOMEM; >=20 > + chips =3D §[MAX_SECTORS]; > + > /* > * This code to walk all sectors is a slightly modified version > * of the cfi_varsize_frob() code. > */ > i =3D 0; > + j =3D -1; > chipnum =3D 0; > adr =3D 0; > sectors =3D 0; > @@ -2671,6 +2678,18 @@ static int __maybe_unused cfi_ppb_unlock(struct > mtd_info *mtd, loff_t ofs, > sect[sectors].locked =3D do_ppb_xxlock( > map, &cfi->chips[chipnum], adr, 0, > DO_XXLOCK_ONEBLOCK_GETLOCK); > + } else { > + if (j < 0 || chips[j].chip !=3D &cfi->chips[chipnum]) { > + j++; > + if (j >=3D MAX_CHIPS) { > + printk(KERN_ERR "Only %d chips for PPB locking > supported!\n", > + MAX_CHIPS); > + kfree(sect); > + return -EINVAL; > + } > + chips[j].chip =3D &cfi->chips[chipnum]; > + chips[j].erasesize =3D size; > + } > } >=20 > adr +=3D size; > @@ -2697,12 +2716,14 @@ static int __maybe_unused > cfi_ppb_unlock(struct mtd_info *mtd, loff_t ofs, > } > } >=20 > - /* Now unlock the whole chip */ > - ret =3D 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 =3D 0; i <=3D j; i++) { > + ret =3D do_ppb_xxlock(map, chips[i].chip, 0, chips[i].erasesize, > + DO_XXLOCK_ONEBLOCK_UNLOCK); > + if (ret) { > + kfree(sect); > + return ret; > + } > } >=20 > /* 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_cm= dset_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, } =20 /* Now unlock the whole chip */ - ret =3D cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len, - DO_XXLOCK_ONEBLOCK_UNLOCK); - if (ret) { - kfree(sect); - return ret; + for (chipnum =3D 0; chipnum < cfi->numchips; chipnum++) { + ret =3D do_ppb_xxlock(map, &cfi->chips[chipnum], + (loff_t)chipnum << cfi->chipshift, + 1 << cfi->chipshift, + DO_XXLOCK_ONEBLOCK_UNLOCK); + if (ret) + goto out; } =20 /* @@ -2715,6 +2717,7 @@ static int __maybe_unused cfi_ppb_unlock(struct mtd_i= nfo *mtd, loff_t ofs, DO_XXLOCK_ONEBLOCK_LOCK); } =20 +out: kfree(sect); return ret; }