Message ID | 1337173272.24809.52.camel@sauron.fi.intel.com |
---|---|
State | RFC |
Headers | show |
On 16.05.2012 15:01, Artem Bityutskiy wrote: > I started looking at the code, and made some notes at the same time and > few minor changes - here is the diff which you can apply on top of your > patches. > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > index 2399511..2f5c362 100644 > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -869,15 +869,22 @@ static int attach_by_fastmap(struct ubi_device *ubi) > > fm_start = ubi_find_fastmap(ubi); > if (fm_start< 0) > + /* TODO: instead, return 1 which means that fall-back to > + * scanning is needed */ fm_start is the PEB number of the fastmap super block. It will return 1 if the super block was found at PEB 1. That's why it returns a negative values in case of an error. > + /* TODO: Before checking version - check the CRC. The CRC covers only the fastmap data, not the super block. > for (i = 0; i< nblocks; i++) { > + /* TODO: you basically perform the scanning here - you should > + * share the same code as we use in scan.c: use process_peb(). > + */ What exactly do you mean by that? process_eb() will not help much because the fastmap data is written "raw" to the flash. (Without any interaction from the WL sub-system) I'll release v6 now and address all other issues in v7. Thanks, //richard
On Mon, 2012-05-21 at 15:34 +0200, Richard Weinberger wrote: > > + /* TODO: Before checking version - check the CRC. > > The CRC covers only the fastmap data, not the super block. Everything should be protected with CRC. > > for (i = 0; i< nblocks; i++) { > > + /* TODO: you basically perform the scanning here - you should > > + * share the same code as we use in scan.c: use process_peb(). > > + */ > > What exactly do you mean by that? > process_eb() will not help much because the fastmap data is written > "raw" to the flash. (Without any interaction from the WL sub-system) It still contains EC and VID headers. So you can use the existing code for scanning it.
On 21.05.2012 16:00, Artem Bityutskiy wrote: > On Mon, 2012-05-21 at 15:34 +0200, Richard Weinberger wrote: >>> + /* TODO: Before checking version - check the CRC. >> >> The CRC covers only the fastmap data, not the super block. > > Everything should be protected with CRC. Okay, done. :) >>> for (i = 0; i< nblocks; i++) { >>> + /* TODO: you basically perform the scanning here - you should >>> + * share the same code as we use in scan.c: use process_peb(). >>> + */ >> >> What exactly do you mean by that? >> process_eb() will not help much because the fastmap data is written >> "raw" to the flash. (Without any interaction from the WL sub-system) > > It still contains EC and VID headers. So you can use the existing code > for scanning it. I use already ubi_io_read_vid_hdr() and ubi_io_read_ec_hdr() what should I reuse? Also checking the image sequence would make sense. Thanks, //richard
On Mon, 2012-05-21 at 16:16 +0200, Richard Weinberger wrote: > > It still contains EC and VID headers. So you can use the existing code > > for scanning it. > > I use already ubi_io_read_vid_hdr() and ubi_io_read_ec_hdr() what should > I reuse? As I wrote: 'process_eb()'. Now it is named 'scan_peb()' after the renames. It does all the work to scan PEB headers. Create a _separate_ 'struct ubi_attach_info' run 'scan_peb()' for each PEB of the fastmap. Check for errors, and sanity (all headers are there, no errors, etc, may be an extra self-check function would be appropriate here). Then go through the resulting fastmap volume attach info, read and process the fastmap _contents_. Of course you need the second (real) attach info for the data you read from the fastmep. This is what I call "reuse the existing" code. How does this sound to you? > Also checking the image sequence would make sense. Yes, you should check everything. If you reuse the code, you will get this automatically, no?
On Mon, 2012-05-21 at 15:34 +0200, Richard Weinberger wrote: > > fm_start = ubi_find_fastmap(ubi); > > if (fm_start< 0) > > + /* TODO: instead, return 1 which means that fall-back > to > > + * scanning is needed */ > > fm_start is the PEB number of the fastmap super block. > It will return 1 if the super block was found at PEB 1. > That's why it returns a negative values in case of an error OK, if 1 does not work - use 2, introduce your own constant. Using -EINVAL is simply incorrect. Check what happens if the MTD driver returns -EINVAL when you call 'ubi_io_read_vid_hdr()'.
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index 2399511..2f5c362 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -869,15 +869,22 @@ static int attach_by_fastmap(struct ubi_device *ubi) fm_start = ubi_find_fastmap(ubi); if (fm_start < 0) + /* TODO: instead, return 1 which means that fall-back to + * scanning is needed */ return -ENOENT; + /* TODO: we need to re-name scan.h to attach.h, scan_info to + * attach_info, si to ai, seb to aeb, and so on - because we share the + * same data structures between scanning and fastmap, so the word + * "scan" is not very sensible to have in the name any more. + */ si = ubi_read_fastmap(ubi, fm_start); if (IS_ERR(si)) return PTR_ERR(si); - ubi->bad_peb_count = 0; + /* TODO: the rest looks very much like attach_by_scanning() - we + * need to re-structure the code and avoid duplication */ ubi->good_peb_count = ubi->peb_count; - ubi->corr_peb_count = 0; ubi->max_ec = si->max_ec; ubi->mean_ec = si->mean_ec; ubi_msg("max. sequence number: %llu", si->max_sqnum); @@ -888,6 +895,7 @@ static int attach_by_fastmap(struct ubi_device *ubi) goto out_si; } + /* TODO: the _scan suffix makes no sens anymore - kill it */ err = ubi_wl_init_scan(ubi, si); if (err) { ubi_err("ubi_wl_init_scan failed!"); @@ -1022,7 +1030,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset) err = attach_by_fastmap(ubi); if (err) { if (err != -ENOENT) - ubi_msg("falling back to attach by scanning mode!"); + ubi_msg("falling back to attach by scanning"); ubi->attached_by_scanning = true; err = attach_by_scanning(ubi); diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c index 2ea1682..7c5f1f7 100644 --- a/drivers/mtd/ubi/fastmap.c +++ b/drivers/mtd/ubi/fastmap.c @@ -582,7 +582,12 @@ struct ubi_scan_info *ubi_read_fastmap(struct ubi_device *ubi, goto out; } + /* TODO: Before checking version - check the CRC. + * I think you need to add a separate helper function in io.c, similar + * to ubi_io_read_vid_hdr(). */ if (fmsb->version != UBI_FM_FMT_VERSION) { + /* TODO: we can fall back to scanning instead of saying good + * bye! */ ubi_err("Unknown fastmap format version!"); si = ERR_PTR(-EINVAL); kfree(fmsb); @@ -606,6 +611,7 @@ struct ubi_scan_info *ubi_read_fastmap(struct ubi_device *ubi, if (!fm_raw) { si = ERR_PTR(-ENOMEM); kfree(fmsb); + /* TODO: goto? */ } vh = ubi_zalloc_vid_hdr(ubi, GFP_KERNEL); @@ -617,6 +623,9 @@ struct ubi_scan_info *ubi_read_fastmap(struct ubi_device *ubi, } for (i = 0; i < nblocks; i++) { + /* TODO: you basically perform the scanning here - you should + * share the same code as we use in scan.c: use process_peb(). + */ ret = ubi_io_read_vid_hdr(ubi, be32_to_cpu(fmsb->block_loc[i]), vh, 0); if (ret) { @@ -653,6 +662,7 @@ struct ubi_scan_info *ubi_read_fastmap(struct ubi_device *ubi, i, be32_to_cpu(fmsb->block_loc[i])); si = ERR_PTR(ret); + /* TODO: fsmb is not freed? */ goto free_vhdr; } }