On 10/14/2011 06:30 PM, Larry Finger wrote:
> On 10/14/2011 10:11 AM, Pavel Roskin wrote:
>> On Sat, 08 Oct 2011 17:28:42 -0500
>> Larry Finger<Larry.Finger@xxxxxxxxxxxx> wrote:
>>
>>> +static inline void htol16_buf(u16 *buf, unsigned int size)
>>> +{
>>> + size /= 2;
>>> + while (size--)
>>> + *(__le16 *)(buf + size) = cpu_to_le16(*(buf + size));
>>> }
>>
>> I'm not not sure compilers would optimize it out on little-endian
>> systems. Perhaps you want a define that uses this code on
>> big-endian systems and does nothing on little endian systems.
>>
>> Also, it would be nice to have a compile-time check that size is even.
>> Or maybe size should be the number of 16-bit words, but then it would be
>> better to call the argument "count" or something like that.
>
> The patch was dropped. Even so, as this routine is found in brcmsmac, your
> comments warrant further discussion.
Following the thread over here ;-)
> I am pretty sure that the compiler would optimize out the entire htol16_buf
> routine. After substitution for cpu_to_le16() on a little-endian system, the
> statement in the while loop becomes '*(buf + size) = *(buf + size)', which is
> certainly optimized away, as will the now empty while loop. The entire routine
> is reduced to 'size /= 2'. As this will have no effect on the external world, it
> will also be dropped leaving an empty htol16_buf(). I don't think any "#ifdef
> __BIG_ENDIAN ... #endif" statements are needed.
Agree.
> Your suggestion that the argument be renamed is good, but there is no need to
> check for an even number as the data in question come from 16-bit reads of the
> SPROM on the b43 device. That number of 16-bit quantities was multiplied by 2 to
> get the byte count before calling this routine. Of course, the routine should
> have been passed the number of 16-bit words, not the byte count. My second
> version would have done this.
>
> Larry
Feedback on the renaming is indeed valid. Passing the word count is
better here. For brcmsmac I plan to fill the buffer using 8-bit reads
from SPROM, verify the crc8, and perform the endianess conversion from
le16 to cpu when crc is ok (actually under review internally).
Gr. AvS
--
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
