netdev - Re: [PATCH 11/11] benet: Kconfig, MAINTAINETS, drivers/net Makefile

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 for Android: free password hash cracker in your pocket

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

Message-Id: <20081209.225453.183512699.davem@davemloft.net>
Date:	Tue, 09 Dec 2008 22:54:53 -0800 (PST)
From:	David Miller <davem@...emloft.net>
To:	sathyap@...verengines.com
Cc:	netdev@...r.kernel.org, jgarzik@...ox.com, subbus@...verengines.com
Subject: Re: [PATCH 11/11] benet: Kconfig, MAINTAINETS, drivers/net Makefile

Ok I looked at this driver some more and I have many comments.

The hardware library abstraction has to go.  You aren't writing a
native linux driver if you split things up like this.  I know you want
code sharing with your other supported platforms, but that's too bad.
If we let every vendor do this the whole tree would be one big
unmaintainable mess.

Many structures have all-caps or capitalized names.  Good coding style
indicates that only CPP macros are to be named with capital letters.
(capital letterd symbols say to the reader "I'm a CPP macro and
probably have side-effects, beware") What's happening here looks ugly
and is inconsistent with the rest of the linux kernel.

The definition of the access to the chip registers is overly
obfuscated.  All of this structure stuff and offsetof() business
adds complexity to the driver and makes it harder to understand.

The bit twiddling is difficult to understand and makes the compiler
work too hard.

I would suggest to fix all of this by simply using macros which
define chip register offsets, and next to those offset define
macros which define the bit values within the register.  Endianness
is not an issue, and all read*()/write*() calls will write out
to the chip in little endian regardless of cpu endianness.

See other drivers such as drivers/net/tg3.[ch] or drivers/net/niu.[ch]
As you'll notice even such huge drivers as those can be done cleanly
in a single source file with no hardware abstraction library layer and
no funny register access structures and bit twiddling, so you can
strive for that as well.

So, this driver still needs a lot of work. :)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html