From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from a.ns.miles-group.at ([95.130.255.143] helo=radon.swed.at) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1ScNnx-0003Ig-M1 for linux-mtd@lists.infradead.org; Wed, 06 Jun 2012 21:30:16 +0000 Message-ID: <4FCFCBDD.8030907@nod.at> Date: Wed, 06 Jun 2012 23:30:05 +0200 From: Richard Weinberger MIME-Version: 1.0 To: Artem Bityutskiy Subject: Re: [PATCH 1/5] UBI: fastmap: add more TODOs References: <1338909119-5188-1-git-send-email-dedekind1@gmail.com> <1338909119-5188-2-git-send-email-dedekind1@gmail.com> In-Reply-To: <1338909119-5188-2-git-send-email-dedekind1@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigB9671153E72EC156501FFA42" Cc: MTD Maling List , Shmulik Ladkani List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigB9671153E72EC156501FFA42 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Am 05.06.2012 17:11, schrieb Artem Bityutskiy: > From: Artem Bityutskiy >=20 > I've spent 10 minutes looking at the code and added few TODOs. Many of = them are > nit-picks, though, but I'd like them to be fixed - should not be diffic= ult. > Some are more than just nit-picks. >=20 > Signed-off-by: Artem Bityutskiy > --- > TODO | 1 - > drivers/mtd/ubi/attach.c | 21 +++++++++++++++------ > drivers/mtd/ubi/fastmap.c | 26 ++++++++++++++++++++++++++ > 3 files changed, 41 insertions(+), 7 deletions(-) >=20 > diff --git a/TODO b/TODO > index 63a22b9..17e30b6 100644 > --- a/TODO > +++ b/TODO > @@ -9,4 +9,3 @@ to the ubi-utils.git repository, to a separate branch a= t the beginning > test UBI + fastmap with it. > 3. Test the autoresize feature > 4. Test 'ubi_flush()' > -5. Test > diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c > index acf7db3..2a0c1ba 100644 > --- a/drivers/mtd/ubi/attach.c > +++ b/drivers/mtd/ubi/attach.c > @@ -1228,12 +1228,21 @@ int ubi_attach(struct ubi_device *ubi) > } else if (err < 0) > return err; > =20 > - /* TODO: When you create an image with ubinize - you do not know the > - * amount of PEBs. So you need to initialize this field with '-1' at > - * ubinize time. And here you need to check for -1 and initialize it = if > - * needed. Then store it at fastmap. This special value has to be als= o > - * documented at ubi-media.h. You also have to amend 'nused' etc. > - * Probably this can be done later. */ > + /* TODO: currently the fastmap code assumes that the fastmap data > + * structures are created only by the kernel when the kernel attaches= > + * an fastmap-less image. However, this assumption is too limiting an= d > + * for sure people will want to pre-create UBI images with fastmap > + * using the ubinize tool. Then they wont have to waste a lot of time= > + * waiting for full scan and fastmap initializetion during the first > + * boot. This is a very important feature for the factory production > + * line where every additional minute per device costs a lot. > + * > + * When you are attaching an MTD device which contains an image > + * generated by ubinize with a fastmap, you will not know the > + * 'bad_peb_count' value. Most probably it will contain something lik= e > + * -1. The same is true for the per-PEB information in the fastmap - = it > + * won't tell which PEBs are bad. So we need to detect this and itera= te > + * over all PEBs, find out which are bad, and update 'ai' here. */ I'm confused to find bad PEBs I'd still need a full scan, right? > ubi->bad_peb_count =3D ai->bad_peb_count; > ubi->good_peb_count =3D ubi->peb_count - ubi->bad_peb_count; > ubi->corr_peb_count =3D ai->corr_peb_count; > diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c > index a8143da..09629c2 100644 > --- a/drivers/mtd/ubi/fastmap.c > +++ b/drivers/mtd/ubi/fastmap.c > @@ -718,6 +718,17 @@ out: > * ubi_scan_fastmap - scan the fastmap > * @ubi: UBI device object > * @ai: UBI attach info to be filled > + * > + * TODO: not urgent, but at some point - check the code with kernel do= c and fix > + * its complaints. Okay. > + * TODO: not urgent, but for consistency, follow the UBI/UBIFS style a= nd put a > + * dot at the end of the first short description sentence (globally): > + * ubi_scan_fastmap - scan the fastmap. (<-dot). Will fix! > + * TODO: not urgent, but it is desireble to document error codes in th= e header > + * comments and probably describe what the function does, if there is = something > + * to say (globally). > */ > int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *a= i) > { > @@ -744,8 +755,13 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struc= t ubi_attach_info *ai) > goto out; > } > =20 > + /* TODO: If 'ubi_io_read()' returns you UBI_IO_BITFLIP, this means th= at > + * the PEB has a bit-flip and has to be scrubbed. How will the > + * superblock be scrubbed or how is the bit-flip guaranteed to be tak= en > + * care of? */ At this stage it's a bit difficult. But I can add this information to the in-memory fastmap structure such th= at the PEB will be scrubbed upon it's returned to the WL sub-system. > ret =3D ubi_io_read(ubi, fmsb, sb_pnum, ubi->leb_start, sizeof(*fmsb)= ); > if (ret && ret !=3D UBI_IO_BITFLIPS) { > + /* TODO: what are the error codes > 0 ? Why is this check? */ To catch UBI_IO_BAD_HDR, UBI_IO_BAD_HDR_EBADMSG and friends. If we fail to read from a fastmap PEB we have to fail with UBI_BAD_FASTMA= P. > if (ret > 0) > ret =3D UBI_BAD_FASTMAP; > =20 > @@ -754,7 +770,17 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struc= t ubi_attach_info *ai) > goto out; > } > =20 > + /* TODO: please, use 'be32_to_cpu()' _every_ time you access a __be32= / > + * etc field. Please, look how things are done in io.c. Please, check= > + * and fix globally. */ AFAIK ->magic is the only __b32 field where I not use be32_to_cpu() becau= se it's useless here. The magic value will be always the same. (It's fixed) I know UBI does also use be32_to_cpu for the EC and HDR magic. Anyway, I'll change it. :-) > if (fmsb->magic !=3D UBI_FM_SB_MAGIC) { > + /* TODO: not urgent, but examine all the error messages and > + * print more information there. Here you should print what was > + * read and what was expected. See io.c and do similarly or > + * better. > + * Please, change globally. E.g., when you print about bad > + * version - print what was expected and what was actually > + * found. */ Okay, this is a good idea! Thanks, //richard --------------enigB9671153E72EC156501FFA42 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.18 (GNU/Linux) iQEcBAEBAgAGBQJPz8vdAAoJEN9758yqZn9elncH/2MEBlKd2cvBYlITlVmRyT1g +UGAUau8YkcH8mS6Tr80ntnYnXrlZ+TrKpF+pkfKWJLKDwTTlDisLm1+NnwQMuvo hFFlUkWkazTsx3A71OgYv0JDV1dKMKsClk8Af9WYzrGP20zNVARdjUD/3AP1h25j oEBO/BeCQprpmSEWS8+u/t7qAHdg1fG6Grb5+RhCNk3wGpLReXtgbw7PCm76Pn3p HPzszeKMVZajQwuPdZnhQBNXGlEHCEqSx+Y3O3+sAbUOf+UwPVGCu4jbJnX+uKeQ HMAWA4GXIf/UvTY03ipXqvvWWeyk2ICXZBtDJ/INN0Du+x05TUrhedP9/U+eXMQ= =gvAk -----END PGP SIGNATURE----- --------------enigB9671153E72EC156501FFA42--