Re: pull request: wireless-next-2.6 2010-09-21

[Christian Lamparter <chunkeey@googlemail.com>]

On Wednesday 22 September 2010 18:22:52 David Miller wrote:
> From: Christian Lamparter <chunkeey@googlemail.com>
> Date: Wed, 22 Sep 2010 11:58:13 +0200
> 
> > On Wednesday 22 September 2010 03:36:14 David Miller wrote:
> >> From: "John W. Linville" <linville@tuxdriver.com>
> >> Date: Tue, 21 Sep 2010 16:17:05 -0400
> >> 
> >> Pulled, but I suspect the 'packed' attribute usage is wrong in
> >> ath/carl9170 and can just be deleted.
> > 
> > which __packed do you think can be removed?
> > 
> > Note:
> > The overzealous use of __packed is a result of a report from Al Viro
> > for ar9170usb: "arm will pad even between u8's".
> >
> > as decribed in http://tinyurl.com/2ww8c53 [git.kernel.org]
> 
> Then only the structure that has the "u8's" needs the __packed attribute
> not every single other structure it is included in.

Which exactly is the "every single other structure"?
All structs which are internal to the driver are not packed
(e.g.: device context, tid & sta info, tx info, etc... - carl9170.h).

Only the structs which deal with hardware (hw.h, eeprom.h, wlan.h)
or firmware (fwcmd.h, fwdesc.h & wlan.h) interface have the
__packed attribute. And there are several good reasons.

e.g.:
 * prevent gcc from aligning the elements to the architecture boundary,
   which would be fatal because it can cause the device to malfunction.

 * prevent possibly fatal unaligned memory access bugs on
   architectures that don't allow/support unaligned memory access.
   (as described in Documentation/unaligned-memory-access.txt )

 * __packed does not only affect a structs element position, but
   also prevent gcc from adding additional padding at the end.
   This is bad because it breaks BUILD_BUG_ON asserts (as reported by Al Viro)

 * consistency and future-proofing changes.
   The firmware is open source, and other people which are not familiar with
   the point above are working/adding new stuff to the code.
   Therefore the firmware descriptor (fwdesc.h), firmware command  interface
   (fwcmd.h) and the super tx/rx descriptors (wlan.h) can change _alot_. 
   So even it looks like some structs that currently don't necessarily need
   to be __packed they need it in the future.
   (no stable API nonsense!)

In my opinion __packed is necessary for these *interface definitions/API*
as long as it can't be 100% proven that the questionable structure would
not cause problems with any compiler on any platform or architecture at
any time.

But If anyone thinks: "That's all just utter rubbish, you have no idea
what you are talking about!" Then he/she is entitled to take action and
draft a new guild-line which lists valid technical(not religious!) reasons
why the use of __packed is discouraged for interface protocol definitions.

This worked before, take a look at the rants in:
volatile-considered-harmful.txt. Now check-patch.pl warns as soon
as a volatile is buried deep between the lines.

Best Regards,
	Chr