| 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: <87k57ba4z7.fsf@deeprootsystems.com>
Date: Fri, 27 Feb 2009 14:29:32 -0800
From: Kevin Hilman <khilman@...prootsystems.com>
To: David Brownell <david-b@...bell.net>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
davinci-linux-open-source@...ux.davincidsp.com,
linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver
David Brownell <david-b@...bell.net> writes:
> 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.)
>
Done.
It also needs this cleanup bit below which is in DaVinci git now that
we've deprecated the use of the davinci cpu_is_* macros in drivers.
This could be just folded into current patch if desired.
Thanks,
Kevin
commit 1bacc33ccc9bd0f3c109bf8a8550e9b6f99397bd
Author: Kevin Hilman <khilman@...prootsystems.com>
Date: Thu Feb 26 17:15:18 2009 -0800
MTD: NAND: drop usage of cpu_is_* macro
Usage of davinci-specific cpu_is macros is not allowed in drivers.
These options should be passed in through platform_data.
Signed-off-by: Kevin Hilman <khilman@...prootsystems.com>
diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index aa70b4e..a2f78ad 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -33,7 +33,6 @@
#include <linux/mtd/nand.h>
#include <linux/mtd/partitions.h>
-#include <mach/cpu.h>
#include <mach/nand.h>
#include <asm/mach-types.h>
@@ -392,8 +391,6 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
/* use board-specific ECC config; else, the best available */
if (pdata)
ecc_mode = pdata->ecc_mode;
- else if (cpu_is_davinci_dm355())
- ecc_mode = NAND_ECC_HW_SYNDROME;
else
ecc_mode = NAND_ECC_HW;
--
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