| 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: <1278571021.12733.47.camel@localhost>
Date: Thu, 08 Jul 2010 09:37:01 +0300
From: Artem Bityutskiy <Artem.Bityutskiy@...ia.com>
To: Neal Buckendahl <nealb001@...il.com>
Cc: dwmw2@...radead.org, rmk+kernel@....linux.org.uk,
cascardo@...oscopio.com, mohanlaljangir@...il.com,
maximlevitsky@...il.com, julia@...u.dk, jkosina@...e.cz,
linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
Neal Buckendahl <nealb@...net.com>
Subject: Re: [PATCH] Mtd: fixed coding style issues in afs.c ar7part.c
cmdlinepart.c inftlcore.c This is a patch to the files afs.c ar7part.c
cmdlinepart.c inftlcore.c that fixes up warnings found by the checkpatch.pl
tool Signed-off-by: Neal Buckendahl <nealb001@...il.com>
On Sun, 2010-06-20 at 03:01 -0500, Neal Buckendahl wrote:
> From: Neal Buckendahl <nealb@...net.com>
>
> ---
> drivers/mtd/afs.c | 9 ++-
> drivers/mtd/ar7part.c | 2 +-
> drivers/mtd/cmdlinepart.c | 167 ++++++++++++++++++++-------------------------
> drivers/mtd/inftlcore.c | 47 ++++++++-----
> 4 files changed, 109 insertions(+), 116 deletions(-)
Many fixes are debatable. Could you please split your patch on smaller
patches, at least per-file basis, but also better on per-change-type
basis.
Additionally, please, do not put so much crap into the subject line.
Thanks!
Some comments below.
> diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
> index cec7ab9..d127171 100644
> --- a/drivers/mtd/afs.c
> +++ b/drivers/mtd/afs.c
> @@ -162,8 +162,8 @@ afs_read_iis(struct mtd_info *mtd, struct image_info_struct *iis, u_int ptr)
> }
>
> static int parse_afs_partitions(struct mtd_info *mtd,
> - struct mtd_partition **pparts,
> - unsigned long origin)
> + struct mtd_partition **pparts,
> + unsigned long origin)
> {
> struct mtd_partition *parts;
> u_int mask, off, idx, sz;
> @@ -235,11 +235,12 @@ static int parse_afs_partitions(struct mtd_info *mtd,
> strcpy(str, iis.name);
>
> parts[idx].name = str;
> - parts[idx].size = (iis.length + mtd->erasesize - 1) & ~(mtd->erasesize - 1);
> + parts[idx].size = (iis.length + mtd->erasesize - 1)&
> + ~(mtd->erasesize - 1);
Use ALIGN()
> parts[idx].offset = img_ptr;
> parts[idx].mask_flags = 0;
>
> - printk(" mtd%d: at 0x%08x, %5lluKiB, %8u, %s\n",
> + printk(KERN_INFO " mtd%d: at 0x%08x, %5lluKiB, %8u, %s\n",
> idx, img_ptr, parts[idx].size / 1024,
> iis.imageNumber, str);
>
> diff --git a/drivers/mtd/ar7part.c b/drivers/mtd/ar7part.c
> index 6697a1e..7ccfdca 100644
> --- a/drivers/mtd/ar7part.c
> +++ b/drivers/mtd/ar7part.c
> @@ -148,6 +148,6 @@ static int __init ar7_parser_init(void)
> module_init(ar7_parser_init);
>
> MODULE_LICENSE("GPL");
> -MODULE_AUTHOR( "Felix Fietkau <nbd@...nwrt.org>, "
> +MODULE_AUTHOR("Felix Fietkau <nbd@...nwrt.org>, "
> "Eugene Konev <ejka@...nwrt.org>");
> MODULE_DESCRIPTION("MTD partitioning for TI AR7");
> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
> index 1479da6..ef5ea7e 100644
> --- a/drivers/mtd/cmdlinepart.c
> +++ b/drivers/mtd/cmdlinepart.c
> @@ -34,7 +34,8 @@
>
> /* debug macro */
> #if 0
> -#define dbg(x) do { printk("DEBUG-CMDLINE-PART: "); printk x; } while(0)
> +#define dbg(x) do { printk(KERN_DEBUG "DEBUG-CMDLINE-PART: "); printk x;
> + } while (0)
> #else
> #define dbg(x)
> #endif
Can you just kill this whole dbg() stuff - I t is useless nowadays.
> @@ -56,7 +57,7 @@ static struct cmdline_mtd_partition *partitions;
>
> /* the command line passed to mtdpart_setupd() */
> static char *cmdline;
> -static int cmdline_parsed = 0;
> +static int cmdline_parsed = 1;
1 ?
> /*
> * Parse one partition definition for an MTD. Since there can be many
> @@ -66,12 +67,12 @@ static int cmdline_parsed = 0;
> * is allocated upon the last definition being found. At that point the
> * syntax has been verified ok.
> */
> -static struct mtd_partition * newpart(char *s,
> - char **retptr,
> - int *num_parts,
> - int this_part,
> - unsigned char **extra_mem_ptr,
> - int extra_mem_size)
> +static struct mtd_partition *newpart(char *s,
> + char **retptr,
> + int *num_parts,
> + int this_part,
> + unsigned char **extra_mem_ptr,
> + int extra_mem_size)
Merge some lines
> {
> struct mtd_partition *parts;
> unsigned long size;
> @@ -83,17 +84,15 @@ static struct mtd_partition * newpart(char *s,
> unsigned int mask_flags;
>
> /* fetch the partition size */
> - if (*s == '-')
> - { /* assign all remaining space to this partition */
> + if (*s == '-') {
> + /* assign all remaining space to this partition */
> size = SIZE_REMAINING;
> s++;
> - }
> - else
> - {
> + } else {
> size = memparse(s, &s);
> - if (size < PAGE_SIZE)
> - {
> - printk(KERN_ERR ERRP "partition size too small (%lx)\n", size);
> + if (size < PAGE_SIZE) {
> + printk(KERN_ERR ERRP "partition size too small (%lx)\n"
> + , size);
> return NULL;
> }
> }
> @@ -101,61 +100,51 @@ static struct mtd_partition * newpart(char *s,
> /* fetch partition name and flags */
> mask_flags = 0; /* this is going to be a regular partition */
> delim = 0;
> - /* check for offset */
> - if (*s == '@')
> - {
> - s++;
> - offset = memparse(s, &s);
> - }
> - /* now look for name */
> + /* check for offset */
> + if (*s == '@') {
> + s++;
> + offset = memparse(s, &s);
> + }
> + /* now look for name */
> if (*s == '(')
> - {
> delim = ')';
> - }
>
> - if (delim)
> - {
> + if (delim) {
> char *p;
> -
> - name = ++s;
> + name = ++s;
> p = strchr(name, delim);
> - if (!p)
> - {
> - printk(KERN_ERR ERRP "no closing %c found in partition name\n", delim);
> + if (!p) {
> + printk(KERN_ERR ERRP
> + "no closing %c found in partition name\n", delim);
> return NULL;
> }
> name_len = p - name;
> s = p + 1;
> - }
> - else
> - {
> - name = NULL;
> + } else {
> + name = NULL;
> name_len = 13; /* Partition_000 */
> }
>
> /* record name length for memory allocation later */
> extra_mem_size += name_len + 1;
>
> - /* test for options */
> - if (strncmp(s, "ro", 2) == 0)
> - {
> + /* test for options */
> + if (strncmp(s, "ro", 2) == 0) {
> mask_flags |= MTD_WRITEABLE;
> s += 2;
> - }
> + }
>
> - /* if lk is found do NOT unlock the MTD partition*/
> - if (strncmp(s, "lk", 2) == 0)
> - {
> + /* if lk is found do NOT unlock the MTD partition*/
> + if (strncmp(s, "lk", 2) == 0) {
> mask_flags |= MTD_POWERUP_LOCK;
> s += 2;
> - }
> + }
>
> /* test if more partitions are following */
> - if (*s == ',')
> - {
> - if (size == SIZE_REMAINING)
> - {
> - printk(KERN_ERR ERRP "no partitions allowed after a fill-up partition\n");
> + if (*s == ',') {
> + if (size == SIZE_REMAINING) {
> + printk(KERN_ERR ERRP
> + "no partitions allowed after a fill-up partition\n");
> return NULL;
> }
> /* more partitions follow, parse them */
> @@ -163,34 +152,29 @@ static struct mtd_partition * newpart(char *s,
> &extra_mem, extra_mem_size);
> if (!parts)
> return NULL;
> - }
> - else
> - { /* this is the last partition: allocate space for all */
> + } else {
> + /* this is the last partition: allocate space for all */
> int alloc_size;
>
> *num_parts = this_part + 1;
> alloc_size = *num_parts * sizeof(struct mtd_partition) +
> extra_mem_size;
> parts = kzalloc(alloc_size, GFP_KERNEL);
> - if (!parts)
> - {
> + if (!parts) {
> printk(KERN_ERR ERRP "out of memory\n");
> return NULL;
> }
> extra_mem = (unsigned char *)(parts + *num_parts);
> }
All the { } stuff should be in a separate patch.
The patch is too large, no one is going to merge it because it is
difficult to get confident it is doing a good thing. Split it.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists