Search Linux Wireless

Re: [PATCH] Add iwlwifi wireless drivers [part 2/2]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]
  Powered by Linux