linux-kernel - Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <82ecf08e0902250505l59f6ddebo9789d205e46b7751@mail.gmail.com>
Date:	Wed, 25 Feb 2009 10:05:18 -0300
From:	Thiago Galesi <thiagogalesi@...il.com>
To:	David Brownell <david-b@...bell.net>
Cc:	Linux MTD <linux-mtd@...ts.infradead.org>,
	lkml <linux-kernel@...r.kernel.org>,
	DaVinci <davinci-linux-open-source@...ux.davincidsp.com>
Subject: Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

On Wed, Feb 25, 2009 at 1:12 AM, David Brownell <david-b@...bell.net> wrote:
> On Tuesday 24 February 2009, Thiago Galesi wrote:
>> OK a couple of things
>>  +
>> > +
>> > +#ifdef CONFIG_MTD_PARTITIONS
>> > +static inline int mtd_has_partitions(void) { return 1; }
>> > +#else
>> > +static inline int mtd_has_partitions(void) { return 0; }
>> > +#endif
>
> My preference would be to have those functions live in the
> MTD headers.
>
> Needing #ifdefs in the body of probe() is *extremely* error
> prone.  Having function versions makes it harder to commit
> a lot of the common errors I've seen, like having some mix
> of options gratuitously break compiles because of missing
> code fragments or variable declarations.  It also makes it
> easier to see when code is obviously doing the wrong thing.

Agreed. Especially about having #ifdefs in probe, no question about
that, this way is _much_better_

>
> Note that #ifdefs-in-functions is contrary to standard
> kernel coding practices.

Yes, I know (and agree with) that 100% :)

>
> I too would like to see <linux/mtd/*.h> use u8/u32/etc.  But
> in this case, it's just doing what the MTD framework does.
> If I used u8/u32/etc the usual feedback would be "do what
> all the other drivers do", "match the interface decls", etc.

Yes, that happens a lot. But a movement towards 'the right way' is
always welcome.

>
>> > +static void nand_davinci_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
>> > +{
>> > +       struct nand_chip *chip = mtd->priv;
>> > +
>> > +       if ((0x03 & ((unsigned)buf)) == 0 && (0x03 & len) == 0)
>> > +               ioread32_rep(chip->IO_ADDR_R, buf, len >> 2);
>> > +       else if ((0x01 & ((unsigned)buf)) == 0 && (0x01 & len) == 0)
>>
>> What are those 0x03 and 0x01 (and other places as well), you'll have
>> to spell out those, preferably using defines.
>
> The "0x03" is "low two bits", "0x01" is "low one bit", etc.
> Those functions can't be used except when memory address
> is "naturally" aligned, and the data length likewise.
>
> You might have an argument if you suggested a comment were
> appropriate ... but those are very common idioms,

Oh, ok, now I get it. But this is still confusing. (Yes, put a comment there)
No need for the ifdefs though.

Also, in the case of non-aligned acesses what is commonly done goes like this:
you write the small unaligned part with 8/16 bit ops, then the rest
with 32 bit ops.

Maybe it's really not worth speedwise to do all of this, but this is
ARM after all :)


-- 
-
Thiago Galesi
--
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/