On Wed, May 2, 2012 at 10:44 AM, David Laight <David.Laight@xxxxxxxxxx> wrote: >> [mailto:netdev-owner@xxxxxxxxxxxxxxx] On Behalf Of Linus Walleij >> The header file <linux/usb/rndis_host.h> used a number of #defines >> that included the cpu_to_le32() macro to assure the result will be >> in LE endianness. Inlining this into the code instead of using it >> in the code definitions yields consolidation opportunities later >> on as you will see in the following patches. The individual >> drivers also used local defines - all are switched over to the >> pattern of doing the conversion at the call sites instead. >> >> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx> >> diff --git a/drivers/net/usb/rndis_host.c >> b/drivers/net/usb/rndis_host.c >> index c8f1b5b..05cad0b 100644 >> --- a/drivers/net/usb/rndis_host.c >> +++ b/drivers/net/usb/rndis_host.c >> @@ -78,10 +78,10 @@ static void rndis_msg_indicate(struct >> usbnet *dev, struct rndis_indicate *msg, >> dev->driver_info->indication(dev, msg, buflen); >> } else { >> switch (msg->status) { >> - case RNDIS_STATUS_MEDIA_CONNECT: >> + case cpu_to_le32(RNDIS_STATUS_MEDIA_CONNECT): >> dev_info(udev, "rndis media connect\n"); >> break; > > I don't actually see how this helps! It helps to unify the #defines, so we can consolidate them in patch 2/13. I.e today we have: include/linux/usb/rndis_host.h #define RNDIS_STATUS_MEDIA_CONNECT cpu_to_le32(0x4001000b) drivers/usb/gadget/rndis.h #define RNDIS_STATUS_MEDIA_CONNECT 0x4001000BU drivers/net/hyperv/hyperv_net.h #define RNDIS_STATUS_MEDIA_CONNECT (0x4001000BL) Notice the odd bird on the top. After this patch I can collapse these three defines into one. As the overall goal is to start to unify the four different RNDIS implementations in the kernel today. > It seems to make the code more unreadable and more likely to > be wrong (due to typos/coding slipups etc). > I didn't see anything in the earlier versions of the patches > where leaving the constants 'host endian' helped. OK I guess this means I should simply wrap all the #defines into cpu_to_le32() in the new <linux/rndis.h>? I think the reason it looks as it does is that the rndis host is the only thing that actually works on bigendian, such as when Mac:s, SPARCs or PowerPC:s want to use RNDIS dongles. I suspect using a BE system to produce a gadget driver won't work, and that the hypervisor code is implicitly LE and has never ever been designed with BE in mind. > I also actually suspect that the generated code for some of the > switch statements would be better if the 'status' were byteswapped. > In particular it might allow the compiler to use a jump table > (when all the case labels are in one set of values). Hm not sure if I'm following what you want me to do here... I could cook a patch set with all defines wrapped in the cpu_to_le32() macro to begin with. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
