On Sun, Sep 7, 2008 at 2:36 AM, Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote: > Chris Leech wrote: >> Both iSCSI and Fibre Channel make use of 24-bit big-endian values in >> frame headers. This patch defines __be24 and __le24 typedefs for a >> structure wrapped around a 3-byte array, and macros to convert back and >> forth to a 32-bit integer. >> >> The undefs in iscsi_proto.h are because of the different calling >> convention for the existing hton24 macro in the iSCSI code. iSCSI will >> be converted in a subsequent patch. >> >> Signed-off-by: Chris Leech <christopher.leech@xxxxxxxxx> > > I like what this patch wants to accomplish, but I disagree with the > implementation. > > First why is the double definition, one in include/linux/byteorder.h > and one in include/linux/byteorder/generic.h ? Because there are currently two byteorder/swab implementations in the kernel. As you said Harvey Harrison has done a lot of work in this area, but right now only the arm and avr32 architectures make use of the new linux/byteorder.h. I could put the 24-bit support in it's own header instead. > Second and most important, in both these files all routines are inline's > not MACROs, and rightly so. There is no place for a macro and the MACRO > works bad for these. I understand the benefits of inline functions, but I disagree that a macro is a bad choice in this case. An inline implementation of cpu_to_be24/hton24 would require a different interface than the rest of the byteorder functions, looking more like the existing iSCSI hton24 macro by taking a pointer to a memory location to store the result in. By using a macro that expands to a compound literal, I was able to maintain the same calling convention of X = cpu_to_be24(Y); Attempting to do so with an inline results in a function definition that returns a structure by value, and even when inlined generates much worse assembly (yes, I tried it). > One - the definition of a local variable in a {} scope in the middle of anywhere. That was done in order to only evaluate the macro operand only once, making it safe to call with an expression that may have side effects. Macros that create scoped variables are all over the place, like min, max and container_of. I consider it necessary in creating a good macro implementation for this functionality, and my motivation for using macros was given above. > Second - type safety. Actually, the assignment to a locally scoped variable gives just as much type checking as an inline would :-) Chris > I CC: Harvey Harrison which did lots of work in these area's. > > For me this patch is totally unacceptable. > > Thanks for working on this > Boaz -- 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