* HFI driver...
@ 2011-04-26 19:06 David Miller
0 siblings, 0 replies; only message in thread
From: David Miller @ 2011-04-26 19:06 UTC (permalink / raw)
To: netdev; +Cc: dykmanj
I started reviewing this monster, here is what I have to say so far:
1) Get rid of this character device.
If you want to dump traces, and allow the user to submit commands,
you can create a generic netlink family to do this.
Netlink will get rid of the ugly char device, and allow you to
extend your interfaces in a backwards compatible way in the future.
2) You add casts to of_get_property() return values. This should never
be necessary.
of_get_property() returns "const void *" which means as long as
you declare your pointers "const" (which you don't) no cast is
necessary.
You are bypassing the read-only protection property values have
by doing these casts, get rid of them and declare your pointers
with "const".
3) Most uses of "long long" and "int" are suspect.
If you need a 64-bit type, use "u64". If you need a 32-bit
type, use "u32". If you need them to be signed use "s64"
and "s32" respectively.
4) When you add the "is_userspace" variable to hfidd_cmd_write() there
are not users. This will produce warnings with gcc-4.6.0, do not
add function variables that are set but never used.
5) Similar comment about using non-fixed-sized types in the definition
of base_hdr, hfi_rdma_extended_hdr et al. Never use generic C types
like "unsigned int" and "unsigned long long". Instead, always use
well defined, fixed sized types like "u32", "u64", etc.
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2011-04-26 19:06 UTC | newest]
Thread overview: (only message) (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-04-26 19:06 HFI driver David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).