linux-kernel - Re: [PATCH V4] mtd: Add DiskOnChip G3 support

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: <878vp96fhl.fsf@free.fr>
Date:	Tue, 27 Sep 2011 19:51:18 +0200
From:	Robert Jarzmik <robert.jarzmik@...e.fr>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	dedekind1@...il.com, dwmw2@...radead.org,
	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4] mtd: Add DiskOnChip G3 support

Arnd Bergmann <arnd@...db.de> writes:

> On Thursday 22 September 2011, Robert Jarzmik wrote:
>> >> +#define doc_flashSequence(seq)                                              \
>> >> +do {                                                                        \
>> >> +    doc_dbg("doc_flashSequence: %02x " #seq "\n", DoC_Seq_##seq);   \
>> >> +    doc_writeb(DoC_Seq_##seq, DoC_FlashSequence);                   \
>> >> +} while (0)
>> >> +
>> ...zip...
>> >
>> > Could you please turn these macros into 'static inline' function - this
>> > is one of the modern patterns of kernel programming - we try to use
>> > functions for better type checking.
>>
>> No sorry, that I cannot. If you look closely, the ##seq is not something you can
>> convert with an inline function, neither the #seq.
>
> Better not obfuscate the code like that then ;-)
>
> Really, passing the entire register name into an inline function is much
> preferred over string concatenation, because it lets you grep for where
> certain definitions are used.
Right. But how do handle the "#seq" then ?

The whole point here is to write humanly readable debug messages. A message of
the like "doc_flashsequence: 48 SET_PLANE1" is much better than
"doc_flashsequence: 48", don't you think ?

If we consider to loose that debugging messages, it's way easier to drop the "do
.. while(0)" sequences, and just keep the register write.

Now if you're convinced removing the "SET_PLANE1" debugging message is less
important that ability to grep the full register name, why not, but from a
maintenance point of view I would prefer letting the "macroness" in.

> You can also convert them to ALL_CAPS
> identifiers instead of cAmeLCAsE.
Point taken for V6.

>
> Finally, don't use the __raw_readb() style functions but instead use
> the regular readb() style, which is the correct one to use in device
> drivers.
Point taken for V6.

Cheers.

-- 
Robert
--
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/