From patchwork Thu Jun 28 12:44:19 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Artem Bityutskiy X-Patchwork-Id: 167867 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from merlin.infradead.org (unknown [IPv6:2001:4978:20e::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id B8ACAB6FFF for ; Thu, 28 Jun 2012 22:42:01 +1000 (EST) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1SkE1G-0006ty-Iy; Thu, 28 Jun 2012 12:40:22 +0000 Received: from mga11.intel.com ([192.55.52.93]) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1SkE1C-0006sy-UQ for linux-mtd@lists.infradead.org; Thu, 28 Jun 2012 12:40:20 +0000 Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga102.fm.intel.com with ESMTP; 28 Jun 2012 05:40:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.71,315,1320652800"; d="asc'?scan'208";a="171089890" Received: from linux.jf.intel.com (HELO linux.intel.com) ([10.23.219.25]) by fmsmga001.fm.intel.com with ESMTP; 28 Jun 2012 05:40:15 -0700 Received: from [10.237.72.167] (sauron.fi.intel.com [10.237.72.167]) by linux.intel.com (Postfix) with ESMTP id 972F12C8001; Thu, 28 Jun 2012 05:40:14 -0700 (PDT) Message-ID: <1340887459.3070.100.camel@sauron.fi.intel.com> Subject: Re: [PATCH 04/16] UBI: Fastmap: Rename self_check_fastmap() From: Artem Bityutskiy To: Richard Weinberger Date: Thu, 28 Jun 2012 15:44:19 +0300 In-Reply-To: <1340812676-14460-5-git-send-email-richard@nod.at> References: <1340812676-14460-1-git-send-email-richard@nod.at> <1340812676-14460-5-git-send-email-richard@nod.at> X-Mailer: Evolution 3.2.3 (3.2.3-3.fc16) Mime-Version: 1.0 X-Spam-Note: CRM114 invocation failed X-Spam-Score: -6.9 (------) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-6.9 points) pts rule name description ---- ---------------------- -------------------------------------------------- -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high trust [192.55.52.93 listed in list.dnswl.org] -0.0 T_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] Cc: nyoushchenko@mvista.com, linux-kernel@vger.kernel.org, adrian.hunter@intel.com, Heinz.Egger@linutronix.de, thomas.wucher@linutronix.de, linux-mtd@lists.infradead.org, shmulik.ladkani@gmail.com, tglx@linutronix.de, Marius.Mazarel@ugal.ro, tim.bird@am.sony.com X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: artem.bityutskiy@linux.intel.com List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-mtd-bounces@lists.infradead.org Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org On Wed, 2012-06-27 at 17:57 +0200, Richard Weinberger wrote: > Signed-off-by: Richard Weinberger Hi, pushed this and added a couple of TODOs. Could you please take a look? Most TODOs are non-essential and easy, but one is more important - the one about regression in attach time of non-fastmap images. Thanks! From b43ceb01a33b322898232fb05e5cc7f754656421 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Thu, 28 Jun 2012 15:08:29 +0300 Subject: [PATCH 1/2] UBI: Fastmap: rename new_ai to alloc_ia Just more consistent, we do not use "new" in, e.g., 'ubi_zalloc_vid_hdr()'. Also move the code to avoid forward references, which we also do not use. Also get rid of unneeded goto in alloc_ia(). Nothing major. Signed-off-by: Artem Bityutskiy --- drivers/mtd/ubi/attach.c | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c index 7552d25..8204b2d 100644 --- a/drivers/mtd/ubi/attach.c +++ b/drivers/mtd/ubi/attach.c @@ -91,7 +91,6 @@ static int self_check_ai(struct ubi_device *ubi, struct ubi_attach_info *ai); static void destroy_ai(struct ubi_device *ubi, struct ubi_attach_info *ai); -static struct ubi_attach_info *new_ai(void); /* Temporary variables used during scanning */ static struct ubi_ec_hdr *ech; @@ -1216,6 +1215,22 @@ out_ai: return err; } +static struct ubi_attach_info *alloc_ai(void) +{ + static struct ubi_attach_info *ai; + + ai = kzalloc(sizeof(struct ubi_attach_info), GFP_KERNEL); + if (ai) { + INIT_LIST_HEAD(&ai->corr); + INIT_LIST_HEAD(&ai->free); + INIT_LIST_HEAD(&ai->erase); + INIT_LIST_HEAD(&ai->alien); + ai->volumes = RB_ROOT; + } + + return ai; +} + /** * ubi_attach - attach an MTD device. * @ubi: UBI device descriptor @@ -1229,7 +1244,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan) int err; struct ubi_attach_info *ai; - ai = new_ai(); + ai = alloc_ai(); if (!ai) return -ENOMEM; @@ -1239,7 +1254,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan) err = ubi_scan_fastmap(ubi, ai); if (err > 0) { destroy_ai(ubi, ai); - ai = new_ai(); + ai = alloc_ai(); if (!ai) return -ENOMEM; @@ -1279,7 +1294,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan) if (ubi->fm && ubi->dbg->chk_gen) { struct ubi_attach_info *scan_ai; - scan_ai = new_ai(); + scan_ai = alloc_ai(); if (!scan_ai) goto out_ai; @@ -1395,24 +1410,6 @@ static void destroy_ai(struct ubi_device *ubi, struct ubi_attach_info *ai) kfree(ai); } -static struct ubi_attach_info *new_ai(void) -{ - static struct ubi_attach_info *ai; - - ai = kzalloc(sizeof(struct ubi_attach_info), GFP_KERNEL); - if (!ai) - goto out; - - INIT_LIST_HEAD(&ai->corr); - INIT_LIST_HEAD(&ai->free); - INIT_LIST_HEAD(&ai->erase); - INIT_LIST_HEAD(&ai->alien); - ai->volumes = RB_ROOT; - -out: - return ai; -} - /** * self_check_ai - check the attaching information. * @ubi: UBI device description object -- 1.7.10 From 400a911b104d88bbf6cc593e8bb24c9865a1c449 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Thu, 28 Jun 2012 15:34:17 +0300 Subject: [PATCH 2/2] UBI: fastmap: some todo and random changes Add few consmetic changes and a bunch of TODOs. Signed-off-by: Artem Bityutskiy --- drivers/mtd/ubi/attach.c | 29 +++++++++++++++++++++++++++++ drivers/mtd/ubi/eba.c | 7 ++++--- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c index 8204b2d..3d9be42 100644 --- a/drivers/mtd/ubi/attach.c +++ b/drivers/mtd/ubi/attach.c @@ -89,6 +89,13 @@ #include #include "ubi.h" +/* + * TODO: please, no forward declarations. We do not use them in UBI code. + * Actually initially I did use them a lot, but when upstreaming, I was asked + * to remove. Please, follow this convention as well. Please, change globally. + * I mean, I am already used to that _all_ the code is upside-down, let's keep + * it that way, or re-structure all the code. :-) + */ static int self_check_ai(struct ubi_device *ubi, struct ubi_attach_info *ai); static void destroy_ai(struct ubi_device *ubi, struct ubi_attach_info *ai); @@ -1170,6 +1177,7 @@ static int scan_all(struct ubi_device *ubi, struct ubi_attach_info *ai) if (ai->ec_count) ai->mean_ec = div_u64(ai->ec_sum, ai->ec_count); + /* TODO: if we attach by fastmap, we do not execute this? */ err = late_analysis(ubi, ai); if (err) goto out_vidh; @@ -1251,6 +1259,25 @@ int ubi_attach(struct ubi_device *ubi, int force_scan) if (force_scan) err = scan_all(ubi, ai); else { + /* TODO: this is a regression. If I have an old image, and I do + * not want to use fastmap, I will be forced to waste time for + * useless scan of 64 first eraseblocks. Not good. + * + * Can you teach ubi_scan_fastmap() to use 'scan_peb()' + * function for scanning and build normal ai information? If it + * finds fastmap - it can destroy the collected ai. If it does + * not find, it returns ai. Then you just confinue scanning. + * + * I buess what we'll need is: + * 1. scan_all() -> scan_range(..., int pnum1, int pnum2); + * 2. ubi_scan_fastmap() returns the pnum of the last scanned + * eraseblock if fastmap was not found; + * Also 'ubi_scan_fastmap()' uses scan_peb() for scanning. + * 3. You call 'scan_range(..., pnum, c->peb_cnt - 1)' and + * it continues. + * + * And no regressions. + */ err = ubi_scan_fastmap(ubi, ai); if (err > 0) { destroy_ai(ubi, ai); @@ -1276,6 +1303,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan) if (err) goto out_ai; + /* TODO: Hmm why this code is not hidden in 'ubi_scan_fastmap()' ? */ if (ubi->fm) { ubi->fm_pool.max_size = ubi->fm->max_pool_size; ubi->fm_wl_pool.max_size = ubi->fm->max_wl_pool_size; @@ -1294,6 +1322,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan) if (ubi->fm && ubi->dbg->chk_gen) { struct ubi_attach_info *scan_ai; + scan_ai = alloc_ai(); if (!scan_ai) goto out_ai; diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c index cb0139c..6d6e301 100644 --- a/drivers/mtd/ubi/eba.c +++ b/drivers/mtd/ubi/eba.c @@ -1218,11 +1218,12 @@ static void print_rsvd_warning(struct ubi_device *ubi, } /** - * self_check_eba - run a self check on the EBA table construected by fastmap. - * + * self_check_eba - run a self check on the EBA table constructed by fastmap. * @ubi: UBI device description object * @ai_fastmap: UBI attach info object created by fastmap * @ai_scan: UBI attach info object created by scanning + * + * TODO: what we do and what return. */ int self_check_eba(struct ubi_device *ubi, struct ubi_attach_info *ai_fastmap, struct ubi_attach_info *ai_scan) @@ -1290,6 +1291,7 @@ int self_check_eba(struct ubi_device *ubi, struct ubi_attach_info *ai_fastmap, ubi_err("LEB:%i:%i is PEB:%i instead of %i!", vol->vol_id, i, fm_eba[i][j], scan_eba[i][j]); + /* TODO: no, please, return error instead */ BUG(); } } @@ -1306,7 +1308,6 @@ out_free: kfree(scan_eba); kfree(fm_eba); - return ret; }