From: Christoph Hellwig <hch@lst.de>
To: Matteo Croce <technoboy85@gmail.com>
Cc: linux-mips@linux-mips.org, Felix Fietkau <nbd@openwrt.org>,
Eugene Konev <ejka@imfi.kspu.ru>,
dwmw2@infradead.org, linux-mtd@lists.infradead.org,
Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH][MIPS][2/7] AR7: mtd partition map
Date: Thu, 20 Sep 2007 19:52:05 +0200 [thread overview]
Message-ID: <20070920175204.GA26132@lst.de> (raw)
In-Reply-To: <200709201755.06712.technoboy85@gmail.com>
> +#include <linux/squashfs_fs.h>
As Ralf mentioned this is not in mainline, so you can't include it.
Fortunately for you including it seems to be a rather dumb idea anyway,
see below for further comments.
> +static struct mtd_partition ar7_parts[5];
Can we please get a symbolic name for that 5? Then again we only
seems to use 3 anyway, and create_mtd_partitions returns 4..
> + unsigned int root_offset = 0xe0000;
Please provide a symbolic name for this one.
> + printk(KERN_INFO "Parsing AR7 partition map...\n");
Do we really need this printk?
> + if (header.checksum == 0xfeedfa42)
> + break;
> + if (header.checksum == 0xfeed1281)
> + break;
These two want symbolic names, please:
enum {
/* some comment */
FOO_PART_MAGIC = 0xfeedfa42,
/* more comment */
BAR_PART_MAGIC = 0xfeed1281,
};
> + switch (header.checksum) {
> + case 0xfeedfa42:
> + while (header.length) {
> + offset += sizeof(header) + header.length;
> + master->read(master, offset, sizeof(header),
> + &len, (u_char *)&header);
> + }
> + root_offset = offset + sizeof(header) + 4;
> + break;
> + case 0xfeed1281:
Especially as you use them here again.
> + default:
> + printk(KERN_WARNING "Unknown magic: %08x\n", header.checksum);
> + break;
> + }
> +
> + master->read(master, root_offset,
> + sizeof(header), &len, (u_char *)&header);
> + if (header.checksum != SQUASHFS_MAGIC) {
> + root_offset += master->erasesize - 1;
> + root_offset &= ~(master->erasesize - 1);
> + }
And after this I'm pretty sure either of the two hex constants above
is SQUASHFS_MAGIC. But not actually the magic in the squashfs
superblock, but someone decided to reuse it for the partitions magic.
So one of the comments for *_PART_MAGIC becomes:
/* same as SQUASHFS_MAGIC for some odd reason.. */
WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@lst.de>
To: Matteo Croce <technoboy85@gmail.com>
Cc: linux-mips@linux-mips.org, Felix Fietkau <nbd@openwrt.org>,
Eugene Konev <ejka@imfi.kspu.ru>,
linux-mtd@lists.infradead.org,
Andrew Morton <akpm@linux-foundation.org>,
dwmw2@infradead.org
Subject: Re: [PATCH][MIPS][2/7] AR7: mtd partition map
Date: Thu, 20 Sep 2007 19:52:05 +0200 [thread overview]
Message-ID: <20070920175204.GA26132@lst.de> (raw)
In-Reply-To: <200709201755.06712.technoboy85@gmail.com>
> +#include <linux/squashfs_fs.h>
As Ralf mentioned this is not in mainline, so you can't include it.
Fortunately for you including it seems to be a rather dumb idea anyway,
see below for further comments.
> +static struct mtd_partition ar7_parts[5];
Can we please get a symbolic name for that 5? Then again we only
seems to use 3 anyway, and create_mtd_partitions returns 4..
> + unsigned int root_offset = 0xe0000;
Please provide a symbolic name for this one.
> + printk(KERN_INFO "Parsing AR7 partition map...\n");
Do we really need this printk?
> + if (header.checksum == 0xfeedfa42)
> + break;
> + if (header.checksum == 0xfeed1281)
> + break;
These two want symbolic names, please:
enum {
/* some comment */
FOO_PART_MAGIC = 0xfeedfa42,
/* more comment */
BAR_PART_MAGIC = 0xfeed1281,
};
> + switch (header.checksum) {
> + case 0xfeedfa42:
> + while (header.length) {
> + offset += sizeof(header) + header.length;
> + master->read(master, offset, sizeof(header),
> + &len, (u_char *)&header);
> + }
> + root_offset = offset + sizeof(header) + 4;
> + break;
> + case 0xfeed1281:
Especially as you use them here again.
> + default:
> + printk(KERN_WARNING "Unknown magic: %08x\n", header.checksum);
> + break;
> + }
> +
> + master->read(master, root_offset,
> + sizeof(header), &len, (u_char *)&header);
> + if (header.checksum != SQUASHFS_MAGIC) {
> + root_offset += master->erasesize - 1;
> + root_offset &= ~(master->erasesize - 1);
> + }
And after this I'm pretty sure either of the two hex constants above
is SQUASHFS_MAGIC. But not actually the magic in the squashfs
superblock, but someone decided to reuse it for the partitions magic.
So one of the comments for *_PART_MAGIC becomes:
/* same as SQUASHFS_MAGIC for some odd reason.. */
next prev parent reply other threads:[~2007-09-20 17:53 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-09-20 15:28 [PATCH][MIPS][0/7] AR7: 4th effort Matteo Croce
2007-09-20 15:43 ` [PATCH][MIPS][1/7] AR7: core support Matteo Croce
2007-09-21 1:57 ` Matteo Croce
2007-09-22 16:18 ` Atsushi Nemoto
2007-09-20 15:55 ` [PATCH][MIPS][2/7] AR7: mtd partition map Matteo Croce
2007-09-20 15:55 ` Matteo Croce
2007-09-20 16:53 ` Ralf Baechle
2007-09-20 16:53 ` Ralf Baechle
2007-09-20 17:52 ` Christoph Hellwig [this message]
2007-09-20 17:52 ` Christoph Hellwig
2007-09-20 18:34 ` Matteo Croce
2007-09-20 18:34 ` Matteo Croce
2007-09-20 19:29 ` Matteo Croce
2007-09-20 19:29 ` Matteo Croce
2007-09-20 19:35 ` Christoph Hellwig
2007-09-20 19:35 ` Christoph Hellwig
2007-09-20 20:00 ` Jörn Engel
2007-09-20 20:00 ` Jörn Engel
2007-09-21 2:09 ` Matteo Croce
2007-09-21 2:09 ` Matteo Croce
2007-09-21 2:20 ` Jörn Engel
2007-09-21 2:20 ` Jörn Engel
2007-09-21 8:03 ` Christoph Hellwig
2007-09-21 8:03 ` Christoph Hellwig
2007-09-21 8:18 ` Geert Uytterhoeven
2007-09-21 8:18 ` Geert Uytterhoeven
2007-09-20 16:00 ` [PATCH][MIPS][3/7] AR7: gpio char device Matteo Croce
2007-09-22 16:42 ` Atsushi Nemoto
2007-09-22 18:59 ` David Brownell
2007-09-23 15:47 ` Atsushi Nemoto
2007-09-20 16:03 ` [PATCH][MIPS][4/7] AR7: leds driver Matteo Croce
2007-09-20 22:54 ` Richard Purdie
2007-09-20 16:06 ` [PATCH][MIPS][5/7] AR7: watchdog timer Matteo Croce
2007-10-03 19:24 ` Wim Van Sebroeck
2007-10-03 20:17 ` Matteo Croce
2007-10-07 20:31 ` Wim Van Sebroeck
2007-09-20 16:11 ` [PATCH][MIPS][6/7] AR7: serial hack Matteo Croce
2007-09-20 16:13 ` [PATCH][MIPS][7/7] AR7: ethernet Matteo Croce
2007-09-29 5:39 ` Jeff Garzik
[not found] <200709080143.12345.technoboy85@gmail.com>
2007-09-08 0:19 ` [PATCH][MIPS][2/7] AR7: mtd partition map Matteo Croce
2007-09-08 0:19 ` Matteo Croce
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20070920175204.GA26132@lst.de \
--to=hch@lst.de \
--cc=akpm@linux-foundation.org \
--cc=dwmw2@infradead.org \
--cc=ejka@imfi.kspu.ru \
--cc=linux-mips@linux-mips.org \
--cc=linux-mtd@lists.infradead.org \
--cc=nbd@openwrt.org \
--cc=technoboy85@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.