Re: [PATCH 1/1] aacraid: big endian issues
On Jan 8, 2008 12:48 PM, Salyzyn, Mark <[EMAIL PROTECTED]> wrote: > Big endian systems issues discovered in the aacraid driver. ... --- a/drivers/scsi/aacraid/comminit.c 2008-01-08 15:32:28.329810853 -0500 +++ b/drivers/scsi/aacraid/comminit.c 2008-01-08 15:37:35.633163607 -0500 @@ -301,10 +301,10 @@ if ((!aac_adapter_sync_cmd(dev, GET_ADAPTER_PROPERTIES, 0, 0, 0, 0, 0, 0, status+0, status+1, status+2, NULL, NULL)) && (status[0] == 0x0001)) { - if (status[1] & AAC_OPT_NEW_COMM_64) + if (status[1] & le32_to_cpu(AAC_OPT_NEW_COMM_64)) ... Why apply le32_to_cpu() to the constant instead of the variable? On systems were le32_to_cpu() is doing something, can gcc or preprocessor optimize the constant? I've always assumed it could not but that might be wrong. thanks, grant - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/1] aacraid: big endian issues
I've always assumed that byte swapping of constants would be optimized where a variable would not :-) I have confirmed in assembler output of the compiler that constant merely become byte reversed constants optimized or no in at least one architectural case. I have *not* confirmed that a variable byte reversal requires processing overhead, as one could possibly expect the compiler to instead optimize by byte reversing the constant when comparing. However, I will guarantee you that if optimization is turned off in the compiler that such an optimization will not take place... I do not think this turns into a readability issue in either case and view this as a simple cosmetic coding precaution much like likely()/unlikely() offers hints to the compiler on code intent... Sincerely -- Mark Salyzyn > -Original Message- > From: Grant Grundler [mailto:[EMAIL PROTECTED] > Sent: Tuesday, January 08, 2008 4:17 PM > To: Salyzyn, Mark > Cc: linux-scsi@vger.kernel.org > Subject: Re: [PATCH 1/1] aacraid: big endian issues > > On Jan 8, 2008 12:48 PM, Salyzyn, Mark > <[EMAIL PROTECTED]> wrote: > > Big endian systems issues discovered in the aacraid driver. > > ... > --- a/drivers/scsi/aacraid/comminit.c 2008-01-08 > 15:32:28.329810853 -0500 > +++ b/drivers/scsi/aacraid/comminit.c 2008-01-08 > 15:37:35.633163607 -0500 > @@ -301,10 +301,10 @@ > if ((!aac_adapter_sync_cmd(dev, GET_ADAPTER_PROPERTIES, > 0, 0, 0, 0, 0, 0, status+0, status+1, > status+2, NULL, NULL)) && > (status[0] == 0x0001)) { > - if (status[1] & AAC_OPT_NEW_COMM_64) > + if (status[1] & le32_to_cpu(AAC_OPT_NEW_COMM_64)) > ... > > Why apply le32_to_cpu() to the constant instead of the variable? > On systems were le32_to_cpu() is doing something, can gcc or > preprocessor optimize the constant? > I've always assumed it could not but that might be wrong. > > thanks, > grant - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] aacraid: big endian issues
On Tue, Jan 08, 2008 at 01:17:15PM -0800, Grant Grundler wrote: > Why apply le32_to_cpu() to the constant instead of the variable? > On systems were le32_to_cpu() is doing something, can gcc or > preprocessor optimize the constant? > I've always assumed it could not but that might be wrong. $ grep constant_p include/linux/byteorder/* include/linux/byteorder/swabb.h:(__builtin_constant_p((__u32)(x)) ? \ include/linux/byteorder/swabb.h:(__builtin_constant_p((__u32)(x)) ? \ include/linux/byteorder/swab.h:(__builtin_constant_p((__u16)(x)) ? \ include/linux/byteorder/swab.h:(__builtin_constant_p((__u32)(x)) ? \ include/linux/byteorder/swab.h:(__builtin_constant_p((__u64)(x)) ? \ -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] aacraid: big endian issues
On Jan 8, 2008 1:37 PM, Salyzyn, Mark <[EMAIL PROTECTED]> wrote: > I've always assumed that byte swapping of constants would be optimized where > a variable would not :-) definitely...I understood that. It just seemed odd usage. > > I have confirmed in assembler output of the compiler that constant > merely become byte reversed constants optimized or no in at least > one architectural case. Thanks! And apologies, I didn't mean to cause you additional work since I was just curious. > I have *not* confirmed that a variable byte reversal requires processing > overhead, as one could possibly expect the compiler to instead optimize by > byte reversing the constant when comparing. However, I will guarantee you > that if optimization is turned off in the compiler that such an optimization > will not take place... :) > > I do not think this turns into a readability issue in either case > and view this as a simple cosmetic coding precaution much > like likely()/unlikely() offers hints to the compiler on code intent... Agreed - it's not a readability issue. thanks, grant > > Sincerely -- Mark Salyzyn > > > > -Original Message- > > From: Grant Grundler [mailto:[EMAIL PROTECTED] > > Sent: Tuesday, January 08, 2008 4:17 PM > > To: Salyzyn, Mark > > Cc: linux-scsi@vger.kernel.org > > Subject: Re: [PATCH 1/1] aacraid: big endian issues > > > > On Jan 8, 2008 12:48 PM, Salyzyn, Mark > > <[EMAIL PROTECTED]> wrote: > > > Big endian systems issues discovered in the aacraid driver. > > > > ... > > --- a/drivers/scsi/aacraid/comminit.c 2008-01-08 > > 15:32:28.329810853 -0500 > > +++ b/drivers/scsi/aacraid/comminit.c 2008-01-08 > > 15:37:35.633163607 -0500 > > @@ -301,10 +301,10 @@ > > if ((!aac_adapter_sync_cmd(dev, GET_ADAPTER_PROPERTIES, > > 0, 0, 0, 0, 0, 0, status+0, status+1, > > status+2, NULL, NULL)) && > > (status[0] == 0x0001)) { > > - if (status[1] & AAC_OPT_NEW_COMM_64) > > + if (status[1] & le32_to_cpu(AAC_OPT_NEW_COMM_64)) > > ... > > > > Why apply le32_to_cpu() to the constant instead of the variable? > > On systems were le32_to_cpu() is doing something, can gcc or > > preprocessor optimize the constant? > > I've always assumed it could not but that might be wrong. > > > > thanks, > > grant > - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html