Re: [PATCH] ssb: Convert to use crc8 code in kernel library

["Arend van Spriel" <arend@broadcom.com>]

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@lwfinger.net>  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