Re: coding style lesson: iwlwifi vs. endianness

[Dan Williams <dcbw@redhat.com>]

On Wed, 2007-11-28 at 20:20 +0200, Tomas Winkler wrote:
> If you would compose your email in less arrogant tone I would answer
> you why your assumptions are wrong.
> I know it is tempting to teach BIG Intel, but please try to keep good
> spirit on this mailing list as it was so far.
> Thanks
> Tomas.

Ignoring the email tones and focusing on the problem, could you
elaborate your reasons?  Doing endian conversions at the boundaries is
quite a bit simpler and does lead to cleaner, more readable code.  The
bulk of the work being done with a softmac card is in the driver +
stack, so the clearer those are, the better for everyone.

Dan

> On Nov 27, 2007 8:44 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > Today's lesson is brought to you by the motivation to teach you how to
> > write simpler code since apparently the only way you found so far to get
> > drivers working on big endian platforms is to use a big endian
> > conversion hammer (ambiguity intentional :) ). Which is not at all
> > necessary. Maybe you don't want to do this as it's a lot of work to
> > start with, but the current way you do things more than suboptimal.
> >
> > But let me explain. And since examples are always good, I picked one at
> > random from the iwl4965 driver.
> >
> > Consider the phy_flags field in the iwl4965_rx_phy_res structure which
> > is defined as follows:
> >         __le16 phy_flags;       /* general phy flags: band, modulation, ... */
> >
> > along with these definitions:
> >
> > #define RX_RES_PHY_FLAGS_BAND_24_MSK            __constant_cpu_to_le16(1 << 0)
> > #define RX_RES_PHY_FLAGS_MOD_CCK_MSK            __constant_cpu_to_le16(1 << 1)
> > #define RX_RES_PHY_FLAGS_SHORT_PREAMBLE_MSK     __constant_cpu_to_le16(1 << 2)
> > #define RX_RES_PHY_FLAGS_NARROW_BAND_MSK        __constant_cpu_to_le16(1 << 3)
> > #define RX_RES_PHY_FLAGS_ANTENNA_MSK            __constant_cpu_to_le16(0xf0)    [1]
> >
> > This is very complex. Why? Because you continually have to use __le16
> > for any phy flags field. To extract the antenna, here's what you need to
> > do:
> >
> > __le16 phy_flags_hw = phy_res->phy_flags;
> > [...]
> >
> > antenna = le16_to_cpu(phy_flags_hw & ANTENNA_MSK) >> 4;
> >
> > Also, on a big-endian architecture that expands to this beast:
> > rt_antenna =
> >   (__builtin_constant_p((__u16)(( __u16)(__le16)(phy_flags_hw &
> > (( __le16)((__u16)( (((__u16)((0xf0)) & (__u16)0x00ffU) << 8) |
> > (((__u16)((0xf0)) & (__u16)0xff00U) >> 8) )))))) ?
> > ((__u16)( (((__u16)((( __u16)(__le16)(phy_flags_hw &
> > (( __le16)((__u16)( (((__u16)((0xf0)) & (__u16)0x00ffU) << 8) |
> > (((__u16)((0xf0)) & (__u16)0xff00U) >> 8) )))))) & (__u16)0x00ffU) << 8)
> > | (((__u16)((( __u16)(__le16)(phy_flags_hw &
> > (( __le16)((__u16)( (((__u16)((0xf0)) & (__u16)0x00ffU) << 8) |
> > (((__u16)((0xf0)) & (__u16)0xff00U) >> 8) )))))) & (__u16)0xff00U) >>
> > 8) )) : __fswab16((( __u16)(__le16)(phy_flags_hw &
> > (( __le16)((__u16)( (((__u16)((0xf0)) & (__u16)0x00ffU) << 8) |
> > (((__u16)((0xf0)) & (__u16)0xff00U) >> 8) ))))))) >> 4;
> >
> > It also results in a compiler warning:
> > [...] warning: integer overflow in expression
> >
> > although I have to admit that right now I do not know where in that
> > beast the compiler thinks it got an overflow. Twice, in fact. In any
> > case, it's pretty hard on the compiler.
> >
> > Additionally, doing it this way means that programmers continually need
> > to think about endianness *all over the code*. Literally *everywhere*
> > that touches values coming from hardware/going to hardware, which is
> > pretty much everywhere in a driver.
> >
> > Now, here's what I want to teach you to do instead.
> >
> > Keep the phy_flags field defined as it was:
> >         __le16 phy_flags;       /* general phy flags: band, modulation, ... */
> >
> > but do it like everybody else and define the values as they are in the
> > phy_flags field without thinking about endianness at all:
> >
> > #define RX_RES_PHY_FLAGS_BAND_24_MSK            (1 << 0)
> > #define RX_RES_PHY_FLAGS_MOD_CCK_MSK            (1 << 1)
> > #define RX_RES_PHY_FLAGS_SHORT_PREAMBLE_MSK     (1 << 2)
> > #define RX_RES_PHY_FLAGS_NARROW_BAND_MSK        (1 << 3)
> > #define RX_RES_PHY_FLAGS_ANTENNA_MSK            0xf0
> >
> > This is easier for the person writing the definitions and looks much
> > cleaner to boot.
> >
> > Then, when it comes to using a value, simply do:
> >
> > u16 phy_flags = le16_to_cpu(phy_res->phy_flags);
> >
> > (and if you get it wrong, sparse will warn here.)
> >
> > Then, you can do the easy:
> > antenna = (phy_flags & ANTENNA_MASK) >> 4;
> >
> > without having to think about endianness. And if you use the field again
> > and again you never have any conversion functions.
> >
> > Presto. As long as you think in terms of
> >         this is the phy_flags field that contains X, Y and Z at
> >         positions x, y, z
> >
> > rather than
> >
> >         this is 16 bits that are laid out in such and such way
> >
> > (which I think everybody except hardware designers does), you win.
> >
> > As a bonus, your code is much easier to read and much smaller (C/header
> > file size I mean). Also, it's a lot more efficient for fields that
> > contain multiple fields like the antenna field (with more than one bit)
> > because you do the conversion only once.
> >
> > Think of it this way:
> >
> > Your current style of taking care of endianness requires thinking about
> > it everywhere, is thus prone to errors and looks ugly.
> >
> > The way I'm suggesting is to convert all data to native (CPU) endianness
> > _as it enters the system_, i.e. at the driver/hardware boundary and then
> > think in terms of the "field" after that, never bothering to think about
> > endianness again. In fact, you shouldn't ever need to use cpu_to_le*()
> > in the RX path, only in any control/TX paths where you need to push a
> > value down to the hardware.
> >
> > The iwlwifi code is a big mess this way. I'm willing to help with the
> > conversion, it should be a pretty mechanical removal of many many
> > cpu_to_le16/32 calls and some fixups and will definitely make the code
> > much easier to maintain. In fact, it would probably have been easier if
> > you'd written the code without respect for endianness and then we'd
> > simply annotated all structures that are shared with the hardware and
> > fixed the sparse warnings at the system boundaries.
> >
> > I hope this helps. Let me know if you have any questions.
> >
> > I have one for you: Whatever gave you the idea of doing it this way?
> > Isn't it common sense that converting data at system boundaries is much
> > easier and less prone to errors than trying to pull it through the whole
> > second system in a non-native format? Whether it's endianness, character
> > set conversion, ...
> >
> > johannes
> >
> > [1] By the way, using __constant_cpu_to_le16() is not useful because
> > cpu_to_le16() already checks whether the argument is a constant. You
> > should only use it if absolutely necessary for some reason. That's why
> > it has two underscores.
> >
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html