| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1340887459.3070.100.camel@sauron.fi.intel.com>
Date: Thu, 28 Jun 2012 15:44:19 +0300
From: Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>
To: Richard Weinberger <richard@....at>
Cc: linux-mtd@...ts.infradead.org, nyoushchenko@...sta.com,
linux-kernel@...r.kernel.org, adrian.hunter@...el.com,
Heinz.Egger@...utronix.de, thomas.wucher@...utronix.de,
shmulik.ladkani@...il.com, tglx@...utronix.de,
Marius.Mazarel@...l.ro, tim.bird@...sony.com
Subject: Re: [PATCH 04/16] UBI: Fastmap: Rename self_check_fastmap()
On Wed, 2012-06-27 at 17:57 +0200, Richard Weinberger wrote:
> Signed-off-by: Richard Weinberger <richard@....at>
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 <artem.bityutskiy@...ux.intel.com>
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 <artem.bityutskiy@...ux.intel.com>
---
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 <artem.bityutskiy@...ux.intel.com>
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 <artem.bityutskiy@...ux.intel.com>
---
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 <linux/random.h>
#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;
}
--
1.7.10
--
Best Regards,
Artem Bityutskiy
Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)
Powered by blists - more mailing lists