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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html - 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
