James Ketrenos wrote:
Randy Dunlap wrote:
2. Use of bitfields: bitfields are not (guaranteed to be) portable
across a wire interface to some other CPU architecture.
Are all bitfield uses always local to the running driver?
They should be.
The bitfields are between the hw/ucode (__le) and the driver. Are you
saying the bitfields won't be compatible if the driver is compiled and
running on big endian? Shouldn't le16_to_cpu() fix that? I just
noticed the types in iwl-4965-hw.h weren't endian specific; I've updated
that (u16, u32, u64 -> __le16, __le32, __le64)
Use of bitfields, as defined in the C standard, should be avoided at all
costs. It requires you to define structures twice -- once for LE, and
once for BE. It generates demonstrably sub-optimal code on all known
compilers. And in general, people tend to screw them up.
Drivers should be using bitwise integer math rather than bitfields.
As for u8/u16/u32/u64 integer mixes, you must still pay attention to
structure layout and compiler padding, in addition to endian issues
(which are lessened once bitfields are gone).
3. What are all of those "volatile" C keywords doing in struct
iwl_shared?
See http://lwn.net/Articles/232961/ : "The trouble with volatile"
If a reference shared through memory mapped IO with a device that can
change the value at any time, doesn't it need to be volatile? 'struct
iwl_shared' is allocated with pci_alloc_consistent and the physical
address is provided to the hardware for it to update whenever it sees fit.
volatile should not be in your driver at all.
See the innumerable threads on LKML, including one current one.
8. Too many BUG_ON()s. Try to recover or return -ERRORs instead.
I pulled out two that shouldn't have been there.
The rest are currently there map to bugs which are easily introduced
through code changes but which may not cause an immediately visible bug
at run-time. Typically they are placed where an eventual printk() at
that location during debugging of a user bug resulted in the critical
"ah-ha!" that led to fixing it. I've added comments to the BUG_ON's
that didn't have comments already.
I suppose we could try and introduce a new driver recovery mechanism
that, upon detection, throws a stack trace and then shuts down the
driver (vs. throwing a kernel OOPs) requiring user space to unload and
reload the driver. That would still be a sucky situation, but at least
it wouldn't cause a system failure requiring a reboot... does such a
mechanism already exist?
In general, you should not be checking for programmer mistakes at every
level. Only use BUG_ON() for mistakes that have happened in the past in
this driver, AND are likely to occur in the future, AND do not cause
problems easily detectable simply by allowing the code to screw up.
9. printk() calls should use KERN_* levels.
Fixed.
dev_xxx stuff is preferred over printk(), actually :)
Jeff
-
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
