From patchwork Sun Jul 8 11:47:45 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Shmulik Ladkani X-Patchwork-Id: 169632 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from merlin.infradead.org (merlin.infradead.org [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 115B82C020C for ; Sun, 8 Jul 2012 21:50:40 +1000 (EST) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1SnpyB-0002w3-Qb; Sun, 08 Jul 2012 11:48:08 +0000 Received: from mail-wg0-f49.google.com ([74.125.82.49]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1Snpy5-0002vo-7B for linux-mtd@lists.infradead.org; Sun, 08 Jul 2012 11:48:03 +0000 Received: by wgbds1 with SMTP id ds1so7400943wgb.18 for ; Sun, 08 Jul 2012 04:47:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; bh=HXK9G/cDQ1rpkNYDl06REhd30/uZAnZ61sqTpTlEs+s=; b=xUMEKM4hZWnnv4nYl0cha4P6YBrFrVBytsA8ouMee4DJO2gwHg7Vujld4w+NHcglFj SQVwr2b1Bx78MG9fJGVyTE0ujtz/H3ZVH/gUKla6MZg0l7eXFTEkVJahgb8ZgQMKwQUN dwhwqCqUj4PamWqUNPvq2X37SHrX1JrNV/v9FdAoQsUTfcxI1/4bg+B/NnnSSQWYSibS sOZIeaks+SCzyLfBoNUxUMe/MCl9hbaR2jHgKWu+T/3u5c1AOcxbD96+aG7hu1yewtpu sFMbmUbHIsW+l4jDFKwEBZ96vQtsdo1OLX1WojNgsbbxNrQWRXyuWk9q7AmrroE+mf0a J4WQ== Received: by 10.217.3.209 with SMTP id r59mr13115928wes.108.1341748073390; Sun, 08 Jul 2012 04:47:53 -0700 (PDT) Received: from pixies.home.jungo.com (212-150-239-254.bb.netvision.net.il. [212.150.239.254]) by mx.google.com with ESMTPS id j6sm25776726wiy.4.2012.07.08.04.47.50 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 08 Jul 2012 04:47:52 -0700 (PDT) Date: Sun, 8 Jul 2012 14:47:45 +0300 From: Shmulik Ladkani To: Richard Weinberger Subject: Re: UBI fastmap updates Message-ID: <20120708144745.36109029@pixies.home.jungo.com> In-Reply-To: <1340982869-77042-1-git-send-email-richard@nod.at> References: <1340982869-77042-1-git-send-email-richard@nod.at> Mime-Version: 1.0 X-Spam-Note: CRM114 invocation failed X-Spam-Score: -2.7 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (shmulik.ladkani[at]gmail.com) -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [74.125.82.49 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature Cc: nyoushchenko@mvista.com, artem.bityutskiy@linux.intel.com, linux-kernel@vger.kernel.org, adrian.hunter@intel.com, Heinz.Egger@linutronix.de, thomas.wucher@linutronix.de, linux-mtd@lists.infradead.org, tim.bird@am.sony.com, tglx@linutronix.de, Marius.Mazarel@ugal.ro X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list 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 Hi Richard, On Fri, 29 Jun 2012 17:14:18 +0200 Richard Weinberger wrote: > This is the next round of UBI fastmap updates. Please examine some TODOs (and questions) I've spotted while diffing against "vanilla" ubi. This patch should apply to linux-ubi at d41a140 Sorry I couldn't review entirely, diff has gotten really enormous... I'll try to pick it up again when you'll get to the final merge submitting incremental patches. Regards, Shmulik diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c index a343a41..dae9674 100644 --- a/drivers/mtd/ubi/attach.c +++ b/drivers/mtd/ubi/attach.c @@ -999,6 +999,13 @@ static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai, if (!find_fastmap && (vol_id > UBI_MAX_VOLUMES && vol_id != UBI_LAYOUT_VOLUME_ID)) { + + /* TODO: if find_fastmap==1, we do not enter this block at all. + * shouldn't we? shouldn't we care of compatability of unknown + * internal volumes OTHER than the fastmap ones, even if + * find_fastmap==1? + */ + int lnum = be32_to_cpu(vidh->lnum); /* Unsupported internal volume */ @@ -1221,6 +1228,7 @@ static void destroy_ai(struct ubi_attach_info *ai) * scan_all - scan entire MTD device. * @ubi: UBI device description object * @ai: attach info object + * TODO: document @start * * This function does full scanning of an MTD device and returns complete * information about it in form of a "struct ubi_attach_info" object. In case @@ -1350,6 +1358,11 @@ out_vidh: out_ech: kfree(ech); out_ai: + /* TODO: doesn't seem clean to destroy 'ai' as it was allocated by the + * caller, its his responsibility. + * also looks like it leads to double freee in case 'err' returned is + * negative + */ destroy_ai(ai); return err; } @@ -1441,6 +1454,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan) err = scan_all(ubi, scan_ai, 0); if (err) { + /* TODO: hmm... kfree or destroy_ai ? */ kfree(scan_ai); goto out_wl; } diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index 50b7590..f769c22 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -1053,6 +1053,7 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway) ubi_notify_all(ubi, UBI_VOLUME_REMOVED, NULL); dbg_msg("detaching mtd%d from ubi%d", ubi->mtd->index, ubi_num); + /* TODO: any action on failure? */ ubi_update_fastmap(ubi); /* @@ -1070,7 +1071,6 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway) ubi_debugfs_exit_dev(ubi); uif_close(ubi); - ubi_wl_close(ubi); ubi_free_internal_volumes(ubi); vfree(ubi->vtbl); diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h index 9f766ff..0b2d0cf 100644 --- a/drivers/mtd/ubi/ubi.h +++ b/drivers/mtd/ubi/ubi.h @@ -219,7 +219,7 @@ struct ubi_volume_desc; * @size: size of the fastmap in bytes * @used_blocks: number of used PEBs * @max_pool_size: maximal size of the user pool - * @max_wl_pool_size: maximal size of the pooly used by the WL sub-system + * @max_wl_pool_size: maximal size of the pool used by the WL sub-system * @raw: the fastmap itself as byte array (only valid while attaching) */ struct ubi_fastmap_layout { @@ -242,7 +242,6 @@ struct ubi_fastmap_layout { * A pool gets filled with up to max_size. * If all PEBs within the pool are used a new fastmap will be written * to the flash and the pool gets refilled with empty PEBs. - * */ struct ubi_fm_pool { int pebs[UBI_FM_MAX_POOL_SIZE]; diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c index 6c69017..688881b 100644 --- a/drivers/mtd/ubi/wl.c +++ b/drivers/mtd/ubi/wl.c @@ -272,6 +272,10 @@ static int produce_free_peb(struct ubi_device *ubi) dbg_wl("do one work synchronously"); err = do_work(ubi); if (err) + /* TODO: in the new locking scheme, produce_free_peb is + * called under wl_lock taken. + * so when returning, should reacquire the lock + */ return err; spin_lock(&ubi->wl_lock); @@ -377,6 +381,7 @@ static struct ubi_wl_entry *find_wl_entry(struct ubi_device *ubi, * as anchor PEB, hold it back and return the second best WL entry * such that fastmap can use the anchor PEB later. */ if (!ubi->fm_disabled && !ubi->fm && e->pnum < UBI_FM_MAX_START) + /* TODO: do we have a risk returning NULL here? */ return prev_e; return e; @@ -405,6 +410,7 @@ static struct ubi_wl_entry *find_mean_wl_entry(struct ubi_device *ubi, /* If no fastmap has been written and this WL entry can be used * as anchor PEB, hold it back and return the second best * WL entry such that fastmap can use the anchor PEB later. */ + /* TODO: why this is specific just to < WL_FREE_MAX_DIFF case? */ if (e && !ubi->fm_disabled && !ubi->fm && e->pnum < UBI_FM_MAX_START) e = rb_entry(rb_next(root->rb_node),