On Sat, 2011-02-12 at 18:39 -0800, Michael Chan wrote: > James Bottomley wrote: > > > On Fri, 2011-01-28 at 18:00 -0800, Bhanu Gollapudi wrote: > > > This patch contains header files for bnx2fc driver. > > > > Do we have to have this litter of > > > > #ifdef (__BIG_ENDIAN) > > ... > > #else > > ... > > #endif > > > > throughout all the structures ... it really makes quite a mess, and is > > very fragile (you have two entries to change and test each time, not > > just one). You can use the cpu neutral format for all the headers and > > then use the cpu_to... accessors to make the conversions. > > > > To see how to do all of this, here's an ancient driver that used to > > look > > like yours and got converted: > > > > http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commit;h=0 > > 353305208ae64b5d4a27957bb3e25c6045b3f68 > > > > I think our situation is a little different. Our chip does automatic > endian swapping on these structures during DMA, so we don't need to do > any cpu_to_xxx()/xxx_to_cpu() when writing or reading these fields. On > a big endian system, we set a special bit in a DMA control register to > tell the chip to DMA these control structures with swapping. But the > structure definition needs to be different on big endian and little > endian when the field is not a 32-bit field. > > Here's an example: > > +struct fcoe_bd_ctx { > + u32 buf_addr_hi; > + u32 buf_addr_lo; > +#if defined(__BIG_ENDIAN) > + u16 rsrv0; > + u16 buf_len; > +#elif defined(__LITTLE_ENDIAN) > + u16 buf_len; > + u16 rsrv0; > +#endif > +#if defined(__BIG_ENDIAN) > + u16 rsrv1; > + u16 flags; > +#elif defined(__LITTLE_ENDIAN) > + u16 flags; > + u16 rsrv1; > +#endif > > The 32-bit buf_addr_hi/buf_addr_lo fields are the same. But the 16-bit fields > have to be reversed in their order. In the code, we reference these fields as- > is directly and the chip does the endian swapping when DMA'ing the whole > structure. > > If we want to get rid of even the #ifdef in the .h file, we'll need to redefine > all 8-bit and 16-bit fields and combine them as 32-bit fields. When assigning > 8-bit or 16-bit quantities, we'll have to mask and shift them in properly. > Again, no cpu_to_xxx is needed because of the DMA endian swap done by the chip. > > So we trade clean .h files for shifting and masking operations in the code. > These .h files are generated by automatic scripts to match the firmware > structures. I understand that, but you're creating a complete mess. Your hardware swapper clearly only operates on 32 bit quantities, hence the header nightmare. This is actually very similar to the qla1280 situation. The consequence is that every header needs two separate definitions for the DMA quantities which doubles the amount of testing and makes the headers very hard to verify. I also don't quite get what you think this buys (other than added complexity). The standard way to do this is to DMA bus endian structures (i.e. little endian for PCI). You do the swaps as you load using the cpu_to_leX() macros. These macros are pretty much free on all architectures (they're nops on the LE ones and the BE ones mostly have load and swap which costs the same as a load). James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
