* Bernhard Reutner-Fischer | 2009-02-13 21:53:29 [+0100]:
>Please add the output of ./scripts/bloat-o-meter and size(1) of
>miscutils/flash_eraseall.o
okay.
>>Index: busybox/include/usage.h
>>===================================================================
>>--- busybox.orig/include/usage.h
>>+++ busybox/include/usage.h
>>@@ -1217,6 +1217,12 @@
>> "$ find / -name passwd\n" \
>> "/etc/passwd\n"
>>
>>+#define flash_eraseall_trivial_usage \
>>+ "[-jq] MTD_DEVICE"
>>+#define flash_eraseall_full_usage "\n\n" \
>>+ " -j format the device for jffs2\n" \
>>+ " -q don't display progress messages"
>
>use a tab instead of ' ', twice.
done.
>>+
>> #define fold_trivial_usage \
>> "[-bs] [-w WIDTH] [FILE]"
>> #define fold_full_usage "\n\n" \
>>Index: busybox/miscutils/flash_eraseall.c
>>===================================================================
>>--- /dev/null
>>+++ busybox/miscutils/flash_eraseall.c
>>@@ -0,0 +1,198 @@
>
>>+static struct jffs2_unknown_node cleanmarker;
>>+int target_endian = __BYTE_ORDER;
>>+static uint32_t *crc32_table;
>>+
>>+static uint32_t crc32(uint32_t val, const void *ss, int len)
>>+{
>>+ const unsigned char *s = ss;
>>+ while (--len >= 0)
>>+ val = crc32_table[(val ^ *s++) & 0xff] ^ (val >> 8);
>>+ return val;
>>+}
>
>Don't we have a crc impl in there already?
Yes, and I use it. There is a function which computes the crc table. I
haven't seen a function which computes the resulting crc.
>>+static void show_progress (mtd_info_t *meminfo, erase_info_t *erase)
>>+{
>>+ printf("\rErasing %d Kibyte @ %x -- %2llu %% complete.",
>>+ meminfo->erasesize / 1024, erase->start,
>>+ (unsigned long long) erase->start * 100 / meminfo->size);
>>+ fflush(stdout);
>>+}
>
>I think that we already have a progress-meter in there, perhaps you could
>use that instead.
I looked at wget & pipe_progress and greped and all of them implement it
on its own.
>>+int flash_eraseall_main(int argc, char **argv);
>
>missing MAIN_EXTERNALLY_VISIBLE above.
>See any other applet in the tree, taskset.c is in the same directory, for
>example.
done
>
>>+int flash_eraseall_main(int argc, char **argv)
>>+{
>>+ int quiet;
>>+ int jffs2;
>
>wow, perhaps just #define quiet (flags & OPTION_Q) and the same for jffs2.
I don't like a define here but maybe I can get rid of it.
>
>>+ mtd_info_t meminfo;
>>+ int fd, clmpos = 0, clmlen = 8;
>>+ erase_info_t erase;
>>+ struct stat st;
>>+ int isNAND, bbtest = 1;
>
>isNAND can be a simple bool and could just be ORed into e.g. flags.
I don't like the idea of mixing command line args/flags and other
informations just to save 4 bytes in case the compiler can't optimize
that one away.
>>+ unsigned int flags;
>>+ char *mtd_name;
>>+
>>+ if (argc < 2) {
>>+ bb_show_usage();
>>+ }
>>+
>>+ flags = getopt32(argv, "jq");
>>+ jffs2 = flags & OPTION_J;
>>+ quiet = flags & OPTION_Q;
>>+
>>+ mtd_name = *(argv + optind);
>>+
>>+ xstat(mtd_name, &st);
>>+ if (!S_ISCHR(st.st_mode)) {
>>+ bb_error_msg_and_die("%s: not a char device", mtd_name);
>>+ }
>>+
>>+ fd = xopen(mtd_name, O_RDWR);
>>+
>>+ xioctl(fd, MEMGETINFO, &meminfo);
>>+
>>+ erase.length = meminfo.erasesize;
>>+ isNAND = meminfo.type == MTD_NANDFLASH ? 1 : 0;
>>+
>>+ if (jffs2) {
>>+ crc32_table = crc32_filltable(NULL, 0);
>>+
>>+ cleanmarker.magic = cpu_to_je16 (JFFS2_MAGIC_BITMASK);
>>+ cleanmarker.nodetype = cpu_to_je16 (JFFS2_NODETYPE_CLEANMARKER);
>>+ if (!isNAND)
>>+ cleanmarker.totlen = cpu_to_je32 (sizeof (struct
>>jffs2_unknown_node));
>>+ else {
>>+ struct nand_oobinfo oobinfo;
>>+
>>+ xioctl(fd, MEMGETOOBSEL, &oobinfo);
>>+
>>+ /* Check for autoplacement */
>>+ if (oobinfo.useecc == MTD_NANDECC_AUTOPLACE) {
>>+ /* Get the position of the free bytes */
>>+ if (!oobinfo.oobfree[0][1]) {
>>+ fprintf (stderr, " Eeep. Autoplacement
>>selected and no empty space in oob\n");
>>+ return 1;
>bb_error_msg_and_die()
done.
>>+ }
>>+ clmpos = oobinfo.oobfree[0][0];
>>+ clmlen = oobinfo.oobfree[0][1];
>>+ if (clmlen > 8)
>>+ clmlen = 8;
>>+ } else {
>>+ /* Legacy mode */
>>+ switch (meminfo.oobsize) {
>>+ case 8:
>>+ clmpos = 6;
>>+ clmlen = 2;
>>+ break;
>>+ case 16:
>>+ clmpos = 8;
>>+ clmlen = 8;
>>+ break;
>>+ case 64:
>>+ clmpos = 16;
>>+ clmlen = 8;
>>+ break;
>>+ }
>
>I don't think that there is a pass that can optimize that properly, so better
>spell that out explicitely with an if-else chain.
I don't actually get what you mean here. The switch() and if() statement
should not make much of a difference to a recent compiler.
>>+ }
>>+ cleanmarker.totlen = cpu_to_je32(8);
>>+ }
>>+
>>+ cleanmarker.hdr_crc = cpu_to_je32 (crc32 (0, &cleanmarker,
>>sizeof (struct jffs2_unknown_node) - 4));
>>+ }
>>+
>>+ for (erase.start = 0; erase.start < meminfo.size;
>>+ erase.start += meminfo.erasesize) {
>>+ if (bbtest) {
>>+ int ret;
>>+
>>+ loff_t offset = erase.start;
>>+ ret = ioctl(fd, MEMGETBADBLOCK, &offset);
>>+ if (ret > 0) {
>>+ if (!quiet)
>>+ printf("\nSkipping bad block at
>>0x%08x\n", erase.start);
>
>bb_info_msg, perhaps?
yep, done.
>>+ continue;
>>+ } else if (ret < 0) {
>>+ /* Black block table is not available on
>>certain flash
>>+ * types e.g. NOR
>>+ */
>>+ if (errno == EOPNOTSUPP) {
>>+ bbtest = 0;
>>+ if (isNAND) {
>>+ fprintf(stderr, "%s: %s: Bad
>>block check not available\n",
>>+ argv[0],
>>mtd_name);
>>+ return 1;
>bb_error_msg_and_die().
>and return 1 is never adequate for a main, there is EXIT_{SUCCESS,FAILURE} in
>stdlib.h for that.
done
>>+ }
>>+ } else {
>>+ fprintf(stderr, "\n%s: %s: MTD get bad
>>block failed: %s\n",
>>+ argv[0], mtd_name,
>>strerror(errno));
>>+ return 1;
>bb_error_msg_and_die().
done.
>>+ }
>>+ }
>>+ }
>>+
>>+ if (!quiet)
>>+ show_progress(&meminfo, &erase);
>>+
>>+ xioctl(fd, MEMERASE, &erase);
>>+
>>+ /* format for JFFS2 ? */
>>+ if (!jffs2)
>>+ continue;
>>+
>>+ /* write cleanmarker */
>>+ if (isNAND) {
>>+ struct mtd_oob_buf oob;
>>+ oob.ptr = (unsigned char *) &cleanmarker;
>>+ oob.start = erase.start + clmpos;
>>+ oob.length = clmlen;
>>+ xioctl (fd, MEMWRITEOOB, &oob);
>>+ } else {
>>+ if (lseek (fd, erase.start, SEEK_SET) < 0) {
>>+ fprintf(stderr, "\n%s: MTD lseek failure:
>>%s\n", mtd_name, strerror(errno));
>>+ continue;
>>+ }
>>+ if (write (fd , &cleanmarker, sizeof (cleanmarker)) !=
>>sizeof (cleanmarker)) {
>>+ fprintf(stderr, "\n%s: MTD write failure:
>>%s\n", mtd_name, strerror(errno));
>>+ continue;
>>+ }
>>+ }
>>+ if (!quiet)
>>+ printf (" Cleanmarker written at %x.", erase.start);
>>+ }
>>+ if (!quiet) {
>>+ show_progress(&meminfo, &erase);
>>+ printf("\n");
>
>putchar
with -O2 gcc replaces printf with putchar if there is no format arg.
>>+ }
>>+
>>+ return 0;
>
>EXIT_SUCCESS
done.
>>+}
Thanks for the review.
Sebastian
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
