| 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
| ||
|
Date: Thu, 26 Feb 2009 15:15:21 -0800
From: David Brownell <david-b@...bell.net>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
davinci-linux-open-source@...ux.davincidsp.com
Subject: Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver
On Thursday 26 February 2009, Andrew Morton wrote:
> > + * Copyright (C) 2006 Texas Instruments.
> > + *
> > + * Ported to 2.6.23 Copyright (C) 2008 by
> > + * Sander Huijsen <Shuijsen@...elecom-nkf.com>
> > + * Troy Kisky <troy.kisky@...ndarydevices.com>
> > + * Dirk Behme <Dirk.Behme@...il.com>
>
> hm. What's the story with authorship, attributions and signoffs here?
Written by TI (PSP team in India, ISTR) with no individual
authorship credited, and shipped with a MontaVista 2.6.10
kernel. Ported as noted; I could presumably add my own
copyright given recent updates I've made. Likewise Felipe
Balbi. Kevin Hilman has signed off on various patches as
part of merging them to the DaVinci tree.
(To the TI team reading this via the DaVinci list: I think
Andrew is hinting that a Signed-off-By from a TI person
would be a Nice Thing. Same for Dirk, and maybe others.)
> > ...
> >
> > +#ifdef CONFIG_MTD_PARTITIONS
> > +static inline int mtd_has_partitions(void) { return 1; }
> > +#else
> > +static inline int mtd_has_partitions(void) { return 0; }
> > +#endif
> > +
> > +#ifdef CONFIG_MTD_CMDLINE_PARTS
> > +static inline int mtd_has_cmdlinepart(void) { return 1; }
> > +#else
> > +static inline int mtd_has_cmdlinepart(void) { return 0; }
> > +#endif
>
> These definitions shouldn't be buried in a .c file.
I will send along a patch to move them to <linux/mtd/...> headers,
now that there seems to be a bit of recognition that the current
ifdef-centric approach in the MTD mapping drivers is trouble. ;)
> >
> > ...
> >
> > +static void nand_davinci_hwctl_1bit(struct mtd_info *mtd, int mode)
> > +{
> > + struct davinci_nand_info *info;
> > + u32 retval;
>
> The identifier `retval' is usually used to identify the value which
> this function will return.
True; resolved in the appended fixup patch.
> > +static int nand_davinci_calculate_1bit(struct mtd_info *mtd,
> > + const u_char *dat, u_char *ecc_code)
> > +{
> > + unsigned int ecc_val = nand_davinci_readecc_1bit(mtd);
> > + unsigned int tmp = (ecc_val & 0x0fff) | ((ecc_val & 0x0fff0000) >> 4);
>
> argh.
It seems the best-dressed pirates have parrots named "argh"! ;)
> > + /* invert so that erased block ecc is correct */
> > + tmp = ~tmp;
> > + ecc_code[0] = (u_char)(tmp);
> > + ecc_code[1] = (u_char)(tmp >> 8);
> > + ecc_code[2] = (u_char)(tmp >> 16);
>
> Is there some library facility which is being open-coded here?
I don't know of such a facility: 24-bit integer into 3-byte buffer.
> > + return 0;
> > +}
> > +
> >
> > ...
> >
> > +static int __init nand_davinci_probe(struct platform_device *pdev)
> > +{
> > + ... deletia ...
> > +
> > + info = kzalloc(sizeof(*info), GFP_KERNEL);
> > + if (!info) {
> > + dev_err(&pdev->dev, "unable to allocate memory\n");
> > + ret = -ENOMEM;
> > + goto err_nomem;
> > + }
> > +
> > + platform_set_drvdata(pdev, info);
> > +
> > + res1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + res2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (!res1 || !res2) {
> > + dev_err(&pdev->dev, "resource missing\n");
> > + ret = -EINVAL;
> > + goto err_res;
>
> This leaks `info'.
>
> Please check all the error path unwinding here.
OK -- that does look buggish.
(Kevin -- I suggest you merge this to the DaVinci tree to
make the eventual resync-with-mainline easier.)
=====================
From: David Brownell <dbrownell@...rs.sourceforge.net>
Minor fixes to the davinci-nand driver patch.
Signed-off-by: David Brownell <dbrownell@...rs.sourceforge.net>
---
drivers/mtd/nand/davinci_nand.c | 35 ++++++++++++++++-------------------
1 file changed, 16 insertions(+), 19 deletions(-)
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -161,7 +161,7 @@ static inline u32 nand_davinci_readecc_1
static void nand_davinci_hwctl_1bit(struct mtd_info *mtd, int mode)
{
struct davinci_nand_info *info;
- u32 retval;
+ u32 nandcfr;
unsigned long flags;
info = to_davinci_nand(mtd);
@@ -172,9 +172,9 @@ static void nand_davinci_hwctl_1bit(stru
spin_lock_irqsave(&davinci_nand_lock, flags);
/* Restart ECC hardware */
- retval = davinci_nand_readl(info, NANDFCR_OFFSET);
- retval |= BIT(8 + info->core_chipsel);
- davinci_nand_writel(info, NANDFCR_OFFSET, retval);
+ nandcfr = davinci_nand_readl(info, NANDFCR_OFFSET);
+ nandcfr |= BIT(8 + info->core_chipsel);
+ davinci_nand_writel(info, NANDFCR_OFFSET, nandcfr);
spin_unlock_irqrestore(&davinci_nand_lock, flags);
}
@@ -186,13 +186,13 @@ static int nand_davinci_calculate_1bit(s
const u_char *dat, u_char *ecc_code)
{
unsigned int ecc_val = nand_davinci_readecc_1bit(mtd);
- unsigned int tmp = (ecc_val & 0x0fff) | ((ecc_val & 0x0fff0000) >> 4);
+ unsigned int ecc24 = (ecc_val & 0x0fff) | ((ecc_val & 0x0fff0000) >> 4);
/* invert so that erased block ecc is correct */
- tmp = ~tmp;
- ecc_code[0] = (u_char)(tmp);
- ecc_code[1] = (u_char)(tmp >> 8);
- ecc_code[2] = (u_char)(tmp >> 16);
+ ecc24 = ~ecc24;
+ ecc_code[0] = (u_char)(ecc24);
+ ecc_code[1] = (u_char)(ecc24 >> 8);
+ ecc_code[2] = (u_char)(ecc24 >> 16);
return 0;
}
@@ -278,7 +278,7 @@ static int nand_davinci_dev_ready(struct
static void __init nand_dm6446evm_flash_init(struct davinci_nand_info *info)
{
- u32 regval, tmp;
+ u32 regval, a1cr;
/*
* NAND FLASH timings @ PLL1 == 459 MHz
@@ -297,11 +297,11 @@ static void __init nand_dm6446evm_flash_
| (3 << 2) /* turnAround ?? ns */
| (0 << 0) /* asyncSize 8-bit bus */
;
- tmp = davinci_nand_readl(info, A1CR_OFFSET);
- if (tmp != regval) {
+ a1cr = davinci_nand_readl(info, A1CR_OFFSET);
+ if (a1cr != regval) {
dev_dbg(info->dev, "Warning: NAND config: Set A1CR " \
"reg to 0x%08x, was 0x%08x, should be done by " \
- "bootloader.\n", regval, tmp);
+ "bootloader.\n", regval, a1cr);
davinci_nand_writel(info, A1CR_OFFSET, regval);
}
}
@@ -338,7 +338,7 @@ static int __init nand_davinci_probe(str
if (!res1 || !res2) {
dev_err(&pdev->dev, "resource missing\n");
ret = -EINVAL;
- goto err_res;
+ goto err_nomem;
}
vaddr = ioremap(res1->start, res1->end - res1->start);
@@ -347,7 +347,6 @@ static int __init nand_davinci_probe(str
dev_err(&pdev->dev, "ioremap failed\n");
ret = -EINVAL;
goto err_ioremap;
-
}
info->dev = &pdev->dev;
@@ -525,16 +524,14 @@ err_clk_enable:
err_ecc:
err_clk:
+err_ioremap:
if (base)
iounmap(base);
if (vaddr)
iounmap(vaddr);
-err_ioremap:
- kfree(info);
-
err_nomem:
-err_res:
+ kfree(info);
return ret;
}
--
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