From patchwork Mon May 22 09:17:04 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Boris Brezillon X-Patchwork-Id: 765295 X-Patchwork-Delegate: boris.brezillon@free-electrons.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wWY394RtVz9s4q for ; Mon, 22 May 2017 19:17:41 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="YVMtj9Ve"; dkim-atps=neutral DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=CC+GQ+sApaySgEGu7Taln15MRzl5ZtjrxIUIfebjZTk=; b=YVMtj9VebmEALX vl+vVdRZ9dHI/B32OOX3/FYScU1iPtnFej0v/Hqb18Qw26Vwcl2TiTjFcSS2NUE37HHcnvXrTHJ1h Au9cPH1VfsF24up6d7CSl1NGSt7eGSzLqJzIrerzi0bBO1iJt6UXri4TDjCvxVF8WpWHU/+ksyD4G 4tHhUfKOuAnG+e0YI+XxdN71MfTyJbLD/EB723V9bCJDQYx8UqsAthvanYPrAY/daC5XuNOBtosNw 83OwcQSB9pkohNJoQeMp4vg7YJhbOpAKuOgP1IGStKLdsDwTHhrmgfJnlwWVuSA7qBvcAO5sM+6hK Ob65MHTfRABXBCOzxzwQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dCjT8-00076i-FB; Mon, 22 May 2017 09:17:38 +0000 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 Received: by mail.free-electrons.com (Postfix, from userid 110) id 9E8972070F; Mon, 22 May 2017 11:17:14 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on mail.free-electrons.com X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT shortcircuit=ham autolearn=disabled version=3.4.0 Received: from bbrezillon (LStLambert-657-1-97-87.w90-63.abo.wanadoo.fr [90.63.216.87]) by mail.free-electrons.com (Postfix) with ESMTPSA id 69628206E2; Mon, 22 May 2017 11:17:04 +0200 (CEST) Date: Mon, 22 May 2017 11:17:04 +0200 From: Boris Brezillon To: Honza =?UTF-8?B?UGV0cm91xaE=?= Subject: Re: [PATCH] mtd:nor:ppb_unlock: remove repeated chip unlock Message-ID: <20170522111704.52ce1c40@bbrezillon> In-Reply-To: References: X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170522_021736_076904_0D3D7284 X-CRM114-Status: GOOD ( 24.15 ) X-Spam-Score: -1.9 (-) X-Spam-Report: SpamAssassin version 3.4.1 on bombadil.infradead.org summary: Content analysis details: (-1.9 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 SPF_PASS SPF: sender matches SPF record -0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Richard Weinberger , Marek Vasut , linux-mtd@lists.infradead.org, Cyrille Pitchen , Brian Norris , David Woodhouse Sender: "linux-mtd" Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org Hi Honza, On Wed, 17 May 2017 09:25:18 +0200 Honza Petrouš 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 > --- > 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--- Tested-by: Honza Petrous 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; }